need some help with a printf function

[Pyrofan1]

I have this

Code: Select all

void Printf(unsigned char forecolor,unsigned char backcolor,char *fmt,...)
{
	va_list args;
	char buffer[50];

	va_start(args,fmt);
	Sprintf(buffer,fmt,args);
	Puts(buffer,forecolor,backcolor);
}

and here's my Sprintf function

Code: Select all

void Sprintf(char *buf,char *fmt,...)
{
	va_list args;

	va_start(args,fmt);
	while(1)
	{
		if(*fmt=='\0')
		{
			break;
		}
		else
		{
			if(*fmt=='%' && (*fmt++)=='d')
			{
				strcpy(buf,(char*)itoa(va_arg(args,int)));
			}
			else
			if(*fmt=='%' && (*fmt++)=='c')
			{
				strcpy(buf,(char*)va_arg(args,int));
			}
			else
			if(*fmt=='%' && (*fmt++)=='s')
			{
				strcpy(buf,va_arg(args,char*));
			}
			else
			if(*fmt=='%' && (*fmt++)=='%')
			{
				strcpy(buf,"%");
			}
			else
			{
				strcpy(buf,*fmt);
			}
			fmt++;
		}
	}
}

and my itoa function

Code: Select all

char* itoa(int n)
{
	int i=0;
	int j;
	char s[10];
	char u[10];

	do
	{
		s[i++]=(char)( n%10+48 );
		n-=n%10;
	}
	while((n/=10)>0);

	for (j=0;j<i;j++)
	{
		u[i-1-j]=s[j];
	}

	u[j]='\0';
	return u;
}

and my main.c code

Code: Select all

#include "kernel/video.h"

void _main( void* mbd, unsigned int magic )
{
	int num=5;

	clrscr();
	Puts("Hello world\n",WHITE,BLACK);
	Printf(WHITE,BLACK,"Num is %d",num);
}

Now when I compile and run this I get

Hello world
(two weird characters here)

How do I fix my Printf/Sprintf functions?

[nick8325]

Two mistakes I can see: first of all, your if statements are wrong. What will happen for the format string "%c"? The line

Code: Select all

if(*fmt == '%' && (*fmt++) == 'd')

will test *fmt == '%', which will be true. Then it will check *fmt == 'd', which will be false, then increment fmt (I think). Now *fmt will be 'c', and all the other tests will fail.

Instead, you should write something like

Code: Select all

if(*fmt == '%') {
    fmt++;
    switch(*fmt) {
        case 'd': ...
        case 'c': ...
    }
}

Second, the line

Code: Select all

strcpy(buf,*fmt);

is wrong. I think this is what is causing your problem. You are reading *fmt, which is a character, and then passing that as a char* to strcpy. So the line of code will read some address in memory and copy a string from there. Not what you want! Instead, just write *buf++ = *fmt. Your case for '%c' is wrong similarly. You should also use strcat() instead of strcpy(), so as to append the output to the format string instead of overwriting it.

[mathematician]

The u string is being stored on the stack, so when itoa returns it stands a good chance of being overwritten by something else. You should declare it as a static variable so that it is stored in a data segment. (The same applies to any variable you want to retain its value after a function has returned.)

[Pyrofan1]

here is my new Sprintf function

Code: Select all

void Sprintf(char *buf,char *fmt,...)
{
	va_list args;

	va_start(args,fmt);
	while(1)
	{
		if(*fmt=='\0')
		{
			break;
		}
		else
		{
			if(*fmt=='%')
			{
				fmt++;
				switch(*fmt)
				{
					case 'd':
					strcat(buf,itoa(va_arg(args,int)));
					break;

					case 'c':
					strcat(buf,va_arg(args,int));
					break;

					case 's':
					strcat(buf,va_arg(args,char*));
					break;

					default:
					strcat(buf,*fmt);
					break;
				}
			}
			else
			{
				strcat(buf,*fmt);
				fmt++;
			}
		}
	}
}

and when I try my os now it looks like this

Hello world
(bunch of weird characters and some numbers)

[mathematician]

Have you changed your itoa function?

Also, in case the compiler doesn't do it for you, you should probably have

Code: Select all

buf[0] = '\0'

before entering the while loop so that you have got a null string that you can subsequently append things to.

[Pyrofan1]

I have

[nick8325]

You should replace strcat(buf, *fmt) with *buf = *fmt. You should also replace strcat(buf, va_arg(args, int)) with *buf = va_arg(args, char).

[mathematician]

I guess most compilers will fill an array with zeros when it's allocated, but, in case yours isn't one of them, see amended post above.

[Pyrofan1]

You should also replace strcat(buf, va_arg(args, int)) with *buf = va_arg(args, char).

When I do that I get

kernel/../includes/printf.h:30: warning: ‘char’ is promoted to ‘int’ when passed through ‘...’
kernel/../includes/printf.h:30: warning: (so you should pass ‘int’ not ‘char’ to ‘va_arg’)
kernel/../includes/printf.h:30: note: if this code is reached, the program will abort

[nick8325]

OK, sorry, try

Code: Select all

*buf = (char)va_arg(args, int)

instead.

[jnc100]

You can't just do this:

Code: Select all

void Sprintf(char *buf,char *fmt,...);

void Printf(unsigned char forecolor,unsigned char backcolor,char *fmt,...)
{
   va_list args;
   char buffer[50];

   va_start(args,fmt);
   Sprintf(buffer,fmt,args);

args is of type va_list, it cannot just be passed as the ... line in sprintf.

Also, you're calling va_start twice on it, once in printf and once in sprintf.

IMHO the best way to do it is to put all your formatting in a function called vsprintf(char *s, const char *format, va_list ap). In vsprintf you don't call va_start or va_end, just va_arg to get each argument. Then, for each of the various formatted out functions (sprintf, fprintf, printf) you do something like:

Code: Select all

printf(char *format, ...) {
  char *buf = (char *)malloc(MAX_BUF);
  
  va_args ap;
  int ret;

  va_start(ap, format);
  ret = vsprintf(buf, format, ap);
  va_end(ap);

  puts_no_new_line_at_end(buf);

  free(buf);

  return ret;
} 

Plus all the other fixes noted above.

Regards,
John.

[mathematician]

You should replace strcat(buf, *fmt) with *buf = *fmt. You should also replace strcat(buf, va_arg(args, int)) with *buf = va_arg(args, char).

Except that he hasn't got an index into buf.

[nick8325]

You should replace strcat(buf, *fmt) with *buf = *fmt. You should also replace strcat(buf, va_arg(args, int)) with *buf = va_arg(args, char).

Except that he hasn't got an index into buf.

Oh! Whoops. That makes things a little fiddlier.

[Pyrofan1]

here's my Vsprintf function

Code: Select all

void Vsprintf(char *buf,char *fmt,va_list args)
{
	while(1)
	{
		if(*fmt=='\0')
		{
			break;
		}
		else
		{
			if(*fmt=='%')
			{
				fmt++;
				switch(*fmt)
				{
					case 'd':
					strcat(buf,itoa(va_arg(args,int)));
					break;

					case 'c':
					strcat(buf,(char)va_arg(args,int));
					break;

					case 's':
					strcat(buf,va_arg(args,char*));
					break;

					default:
					*buf=*fmt;
					buf++;
					break;
				}
			}
			else
			{
				*buf=*fmt;
				buf++;
				fmt++;
			}
		}
	}
}

Printf function

Code: Select all

void Printf(unsigned char forecolor,unsigned char backcolor,char *fmt,...)
{
	va_list args;
	char buffer[50];

	va_start(args,fmt);
	Vsprintf(buffer,fmt,args);
	Puts(buffer,forecolor,backcolor);
}

and main.c

Code: Select all

#include "kernel/video.h"

void _main( void* mbd, unsigned int magic )
{
	char c='a';
	char string[]="Microsoft sucks";
	int num=5;

	clrscr();
	Puts("Hello world\n",WHITE,BLACK);
	Printf(WHITE,BLACK,"num is %d",num);
	Printf(WHITE,BLACK,"c is %c\n",c);
	Printf(WHITE,BLACK,"%s",string);
}

when I run it iget

Hello world
num is d(weird symbol)5 (weird symbols)
(then I'm apparently reading from a file on the hard drive)

[jnc100]

You need to increment buf after each strcat, and terminate buf with a null.

Regards,
John.