Page 1 of 2
need some help with a printf function
Posted: Sat Jun 09, 2007 11:20 am
by 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?
Posted: Sat Jun 09, 2007 11:38 am
by 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
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.
Posted: Sat Jun 09, 2007 11:43 am
by 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.)
Posted: Sat Jun 09, 2007 11:58 am
by 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)
Posted: Sat Jun 09, 2007 12:02 pm
by mathematician
Have you changed your itoa function?
Also, in case the compiler doesn't do it for you, you should probably have
before entering the while loop so that you have got a null string that you can subsequently append things to.
Posted: Sat Jun 09, 2007 12:07 pm
by Pyrofan1
I have
Posted: Sat Jun 09, 2007 12:18 pm
by 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).
Posted: Sat Jun 09, 2007 12:23 pm
by 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.
Posted: Sat Jun 09, 2007 12:25 pm
by 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
Posted: Sat Jun 09, 2007 12:28 pm
by nick8325
Posted: Sat Jun 09, 2007 12:30 pm
by jnc100
You can't just do this:
Pyrofan1 wrote: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.
Posted: Sat Jun 09, 2007 12:32 pm
by mathematician
nick8325 wrote: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.
Posted: Sat Jun 09, 2007 12:34 pm
by nick8325
mathematician wrote:nick8325 wrote: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.
Posted: Sat Jun 09, 2007 12:59 pm
by 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)
Posted: Sat Jun 09, 2007 1:08 pm
by jnc100
You need to increment buf after each strcat, and terminate buf with a null.
Regards,
John.