Tag Archive: PGPlus


Breaking stuff to look tough

Yesterday we started tidying up the code by first cleaning up ADDSTACK and ENDSTACK in shortcut.c, before replacing them with macro definitions in proto.h. Sadly for us those macro’s don’t fix the hundreds of lines of:
sprintf(stack, "something");
stack = strchr(stack, 0);

For that we’re going to have to manually track down every sprintf and edit. But before we do, lets consider another case we’d also like to tidy up.
strcpy(stack, "some text");
stack = strchr(stack, 0);

Unlike the sprintf example, we can’t attribute this to sloppy coding. The return value for strcpy is a pointer to its destination, which in the above example is stack. Now we could replace those with sprintf, except it would be a silly idea. sprintf is an expensive function to call just for a string copy, if anything we want to convert sprintf to strcpy when its not doing any text formatting. A naive solution to this problem would look like:
char *my_strcpy(char *dest, char *src)
{
char *work;
strcpy(dest, src);
/* wordpress doesn't render the usual null terminator correctly, so I'll use 0 even though thats wrong to show my point */
work = strchr(dest, 0);
return work;
}

Which would make the code look pretty, but replaces 2 function calls with 3, and makes the code slightly slower. So onto a better example:
char *my_strcpy(char *dest, const char *src)
{
char *d = dest;
const char *s = src;
do
*d++ = *s;
while (*s++ != 0);

return d - 1;
}

However GNU has a library function named stpcpy which does exactly what we want. Ideally we want to use that function instead of our own when its available. To do so we first need to change our makefile. Heading to src/configure/makefiles we edit the file named linux, changing:
MF_OS_CFLAGS="-DLINUX"
To now read:
MF_OS_CFLAGS="-DLINUX -D_GNU_SOURCE"
Then back in our src directory, we need to “make newos” for those changes to take effect. If we now try to compile the talker, the compiler will immediately produce errors, so lets work through those.
In file included from admin2.c:24:0:
include/proto.h:439:19: error: conflicting types for strcasestr
/usr/include/string.h:371:14: note: previous declaration of strcasestr was here

GNU has a strcasestr function which now conflicts with the utility function in xstring.c. We can fix those compiler errors simply by wrapping the prototype and the code inside:
#ifndef _GNU_SOURCE
/* conflicting code is here */
#endif

One thing to note whilst we’re doing this, the PGPlus strcasestr function has a subtle bug:

Return Value

Upon successful completion, strcasestr() shall return a pointer to the located string or a null pointer if the string is not found. If s2 points to a string with zero length, the function shall return s1.

Whilst PGPlus’s way of returning a null pointer is saner, any code that relies upon that will break. So we’ll change PGPlus’s default function to do the right thing. Now we check every call to strcasestr to ensure its passing a valid string, and modify it if its not.

After making those changes, our next compiler error is:
In file included from aliases.c:19:0:
include/fix.h:23:17: error: conflicting types for getitimer
/usr/include/sys/time.h:127:12: note: previous declaration of getitimer was here

And just as with the strcasestr prototype, we’ll wrap the contents of fix.h in an #ifndef…#endif block.
And one last compiler warning:
ttt.c: In function ttt_print_board:
ttt.c:94:5: warning: implicit declaration of function sprintf
ttt.c:94:5: warning: incompatible implicit declaration of built-in function sprintf

That ones our fault. When we changed ADDSTACK and ENDSTACK to be macro forms of sprintf we didn’t check every file includes the correct header. So we just need to add an include for stdio.h and our code is back to compiling cleanly, and we can return to cleaning up the code.

Advertisements

Its always important when starting a new project to ensure the foundations are solid. With PGPlus that invariably means tidying up the code and ensuring the layout is consistent.

Consider the following 2 functions:

void ADDSTACK(char *format,...)
{
va_list argum;
va_start(argum, format);
vsprintf(stack, format, argum);
va_end(argum);
stack = strchr(stack, 0);
}
void ENDSTACK(char *format,...)
{
va_list argum;
va_start(argum, format);
vsprintf(stack, format, argum);
va_end(argum);
stack = end_string(stack);
}

Both functions copy text at the position pointed to by stack, and then move that pointer forwards. However they both do so sub-optimally. We can  immediately improve this and remove the call to strchr by taking advantage of the fact that vsprintf returns the number of characters written.

void ADDSTACK(char *format,...)
{
va_list argum;va_start(argum, format);
stack += vsprintf(stack, format, argum);
va_end(argum);
}
void ENDSTACK(char *format,...)
{
va_list argum;va_start(argum, format);
stack += vsprintf(stack, format, argum);
va_end(argum);
stack = end_string(stack);
}

Looking at ENDSTACK we can see that it positions the stack pointer 1 place past the string terminator, so we can replace the call to end_string with a simple statement, like so:

void ENDSTACK(char *format,...)
{
va_list argum;
va_start(argum, format);
stack += vsprintf(stack, format, argum);
va_end(argum);
*stack++ = '';
}

And what we’ve ended up with is a variable argument call wrapping another variable argument call. Obviously thats a little silly, so instead of modifying the functions themselves, lets replace them with some preprocessor magic.

#define ADDSTACK(a, ...) stack += sprintf(stack, a, ## __VA_ARGS__)
#define ENDSTACK(a, ...) stack += sprintf(stack, a, ## __VA_ARGS__); *stack++ = ''

And now the compiler is doing a search and replace for us. However if we try to compile with that change, we’re going to immediately hit a compiler error:

channel.c: In function zc:
channel.c:637:3: error: else without a previous if
make: *** [../junk/channel.o] Error 1

Looking at the code in question reveals immediately what went wrong:

if (newchan)
ENDSTACK("<(%s)> ++ %s creates and joins this channel ++\n", p->zchannel, p->name);
else
ENDSTACK("<(%s)> ++ %s joins this channel ++\n", p->zchannel, p->name);

Our ENDSTACK macro creates 2 statements, and that breaks any blocks that arn’t wrapped in a pair of braces. Luckily theres an easy way to fix this which doesn’t involve editing lots of code. ENDSTACK moves the stack pointer 1 position past the end of the string its just written, so if we change the macro like so:

#define ENDSTACK(a, ...) stack += sprintf(stack, a, ## __VA_ARGS__) + 1

And with that our code compiles correctly without us needing to do any more work.

Getting it installed

So you’ve downloaded PG+ from the website, unpacked it, typed make followed by make install and watched as your screen filled with compile errors.

So todays task is to get PG+ to compile without any warnings.

Now compiling .... admin2.c 0 adm lines
In file included from admin2.c:24:0:
include/proto.h:235:13: warning: conflicting types for built-in function log

Back when PG+ was first written, math functions were contained in their own header file, todays compilers however are aware of them, hence the warning message. Its not really a problem as we’re not linking with the math library so the PG+ log function is the one that will be used. We’ll solve this warning by renaming PG+’s function logF (which makes sense as theres a companion PG+ function called LOGF).

On a 64bit operating the next error we encounter looks like this.

Now compiling .... aliases.c alia lines
aliases.c: In function tmp_comp_alias:
aliases.c:77:24: warning: cast from pointer to integer of different size
aliases.c:77:38: warning: cast from pointer to integer of different size

Looking at the line immediately reveals the problem:

store_int(oldstack, ((int) stack - (int) oldstack));

An 8 byte pointer is being assigned to a 4 byte integer. We can easily fix that warning by removing the forced casts and allowing the compiler to do the necessary type conversions. The rest of those errors all have the same underlying cause, if not the same solution.

Now compiling .... glue.c 3 glu lines
glue.c: In function main:
glue.c:1778:7: warning: format %d expects type int, but argument 3 has type long int

And the offending line looks like:
sprintf(stack_start, "Lost stack reclaimed %d bytes\n", stack - stack_start);
We can fix this one by either changing the format specifier to handle a long int, or casting the the pointer math to an int, both are valid fixes in this instance.

intercom_glue.c: In function start_intercom:
intercom_glue.c:138:7: warning: missing sentinel in function call

Which is usually caused by a variable number of arguments not being terminated by a NULL value, heres the line in question.
execlp ("bin/intercom", intercom_name, 0);
And heres the fix.
execlp ("bin/intercom", intercom_name, (char *)NULL);

And the last 2 errors from that file.
intercom_glue.c:152:3: warning: implicit declaration of function time
intercom_glue.c:152:3: warning: nested extern declaration of time

Time functions are spread across time.h and sys/time.h, so its just a matter of ensuring both are included.

items.c:969:32: warning: trigraph ??! ignored, use -trigraphs to enable

And for that one we just need to edit the text to ensure the preprocessor isn’t confused.

And after all that work, we’re finally ready to boot our talker.