Wednesday, September 14, 2011

Header Files and Named Parameters

If you're as much of a geek as I am you may hang out in IRC channels chatting it up with your other coder friends. Occasionally, you may also get into a heated debate about one topic or another.

About a week ago I got into just such a debate (and the geekness of it is a little embarrassing). We were debating whether you should name your parameters in header files or not. As a "software engineer" it's my belief that you should make the actual act of coding as easy as possible. There are enough ways to shoot yourself in the foot and it's always best to minimize complexity and leave that intellectual horsepower to the actual problem at hand.

I find that most seasoned "software engineers" feel this way as well. I've run into a few "programming is just hard" coders, but I believe a real "engineer" would care a little more about their users and also the interesting and unexpected problems that can arise from using other software packages. Given all this, of course I was on the "please, for the love of God, name those parameters" side of the discussion. Well, turns out I was wrong. My decision to include named parameters can actually make programming a LOT harder. Here's why:

Suppose I have 3 files, defined here:

// lib.h
#ifndef __LIB_H__
#define __LIB_H__

long sum (long int_arg, long int_arg2);

#endif
// lib.c
#include "lib.h"

long sum (long int_arg, long int_arg2)
{
    return (int_arg + int_arg2);
}
// main.c
#include <stdio.h>
#include <stdlib.h>
#include "lib.h"

int main (int argc, char *argv[])
{
    fprintf (stdout, "%d\n", sum (10, 10));
    return 0;
}


This all seems like it's going to work perfectly. If I compile and run this, I get "20" for my output. Everything is perfect.

Now then, suppose someone changes my main.c like so:

// main.c
#define int_arg long

#include <stdio.h>
#include <stdlib.h>
#include "lib.h"

int main (int argc, char *argv[])
{
    fprintf (stdout, "%d\n", sum (10, 10));
    return 0;
}


Now my output is the most unexpected: "10"

I know you may wonder why exactly someone would define int_arg to be something like long and I agree this is somewhat contrived. But, it can happen. This was the simplest plausible case I could come up with. Perhaps the coder is using a library that allows you to change the certain behavior by defining what an "int_arg" should be. Perhaps the library is using the "int_arg" type to indicate what type of integers it should read from a file. I know all of this is a stretch, but the important thing to remember is that this can happen. As all good engineers know, if it can happen it most likely will happen eventually.

Hmmm, so what happened here? Let's have a look at some disassembly to see.

// outputs "20"
_main:
  004113E0: 55                 push        ebp
  004113E1: 8B EC              mov         ebp,esp
  004113E3: 81 EC C0 00 00 00  sub         esp,0C0h
  004113E9: 53                 push        ebx
  004113EA: 56                 push        esi
  004113EB: 57                 push        edi
  004113EC: 8D BD 40 FF FF FF  lea         edi,[ebp-0C0h]
  004113F2: B9 30 00 00 00     mov         ecx,30h
  004113F7: B8 CC CC CC CC     mov         eax,0CCCCCCCCh
  004113FC: F3 AB              rep stos    dword ptr es:[edi]
  004113FE: 6A 0A              push        0Ah
  00411400: 6A 0A              push        0Ah
  00411402: E8 EE FC FF FF     call        @ILT+240(_sum)
  00411407: 83 C4 08           add         esp,8
  0041140A: 8B F4              mov         esi,esp

// outputs "10"
_main:
  004113E0: 55                 push        ebp
  004113E1: 8B EC              mov         ebp,esp
  004113E3: 81 EC C0 00 00 00  sub         esp,0C0h
  004113E9: 53                 push        ebx
  004113EA: 56                 push        esi
  004113EB: 57                 push        edi
  004113EC: 8D BD 40 FF FF FF  lea         edi,[ebp-0C0h]
  004113F2: B9 30 00 00 00     mov         ecx,30h
  004113F7: B8 CC CC CC CC     mov         eax,0CCCCCCCCh
  004113FC: F3 AB              rep stos    dword ptr es:[edi]
  004113FE: 6A 0A              push        0Ah
  00411400: 6A 00              push        0
  00411402: 6A 0A              push        0Ah
  00411404: E8 EC FC FF FF     call        @ILT+240(_sum)
  00411409: 83 C4 0C           add         esp,0Ch
  0041140C: 8B F4              mov         esi,esp


Huh, so that's weird. Looks like if int_arg is defined as a long then the compiler seems to think it should pass an extra 32-bits to the "sum" function. You can also see this in the "stack cleanup" code directly after the function call -- there's an extra 4 bytes being pulled off the stack. The extra 4 bytes are coming from that extra push onto the stack before the function call. So why is this?

Well, the compiler is converting long int_arg to be long long and then assuming it's an unnamed parameter. Later on, the actual implementation doesn't have the define and is compiled as expecting a long, not a long long. Well, that's odd, but you may ask, "It's going to throw some linker warnings, right?"

Well, no. The reason this whole thing compiles and links without warnings is because these are C functions. There is no name decoration. The linker is simply looking for a function called sum that it can use. It finds one and no linker-time type-checking is performed. The only reason C++ has any sort of name declaration is because functions can be overloaded with different parameter types. In C++ this would be a linker error but in C it's just a horrible horrible bug.

So there's a case where I was advocating for making things easier only to possibly introduce some really hard-to-find bugs. How would I fix this? I'd add some nice comments to these functions in the header file, which is a common convention. Additionally, I know understand why this convention is used.

// lib.h
#ifndef __LIB_H__
#define __LIB_H__

long sum (long /* int_arg */, long /* int_arg2 */);

#endif


Thanks!

No comments:

Post a Comment