AGH AAGH EVIL CODE

[talarubi]

Brownie points to anyone who can spot the dire problem:

typedef AsmParserRec* AsmParser;

int NewAsmParser(AsmParser* Parser, char* Input, int Length) {
  // Note: New() is like Pascal New, e.g. it modifies the supplied pointer.
  //   Hence pass-by-reference.
  if(New(Parser, sizeof(AsmParser))) {
    (*Parser)->Input     = Input;
    (*Parser)->Length    = Length;
    (*Parser)->Line      = 1;
    (*Parser)->Column    = 1;
  ...
}

Hey, I know. Almost nobody ever comments in here, so reply with the right guess, and I'll get you an LJ icon or something? 8)

This otherwise innocous snippet took up my morning bluescreening Win9x!! If I didn't have access to a box with a real OS I don't know what I'd be doing right now... But fortunately, GDB under WinXP correctly traps the problem even though it runs "fine" from the prompt. Code is happy now.

Is it surprising that the dumbest goofs are the most devastating? Eesh. x..X

Comments

[soreth.livejournal.com]

Missing right (closing) curly-brace?

[draci.livejournal.com]

*points at the ellipses*
Nice try, but no. :)

[soreth.livejournal.com]

Bah! Not like us non-code-beasts can check for more than that. Ah, well. n..n

[coderlemming.livejournal.com]

Looks like Parser is a local variable to the function, assuming this is C++ as it seems to be. This means that, while you're definitely setting Parser to point to the newly allocated memory with New, the pointer isn't making it back to whoever called NewAsmParser.

That wasn't too clear, let me try again.

Say someone calls NewAsmParser. They're going to pass a pointer variable that they expect to then be filled with a pointer to a new AsmParser, right? But C++ passes by value. You're essentially passing in whatever garbage value was in the pointer already. The Parser variable in the function is actually a copy of whatever that garbage variable was, and it's local to the function. Fill it with a pointer in New, return, expect to use the pointer, and bam. Probably a NULL pointer exception.

You want to pass that pointer by reference, ie

int NewAsmParser(AsmParser* &Parser, char* Input, int Length) {

I think that's how you pass a pointer variable by reference. You could do this the C way:

int NewAsmParser(AsmParser **Parser, char* Input, int Length) {
// Note: New() is like Pascal New, e.g. it modifies the supplied pointer.
// Hence pass-by-reference.
if(New(*Parser, sizeof(AsmParser))) {
(**Parser).Input = Input;
(**Parser).Length = Length;
(**Parser).Line = 1;
(**Parser).Column = 1;
...
}

Call it by passing it a pointer to a pointer variable:


AsmParser *somePointer;

NewAsmParser(&somePointer,...);


Whoa. I just spotted the even bigger bug. (*Parser)->Input, you're dereferencing the pointer twice. That's probably the bug you found. Do I get a cookie for finding your OTHER bug?

[coderlemming.livejournal.com]

Oops. Missed the typedef. Dammit.

[coderlemming.livejournal.com]

Ok, your bug is that you should be doing New(*Parser,sizeof(AsmParserRec)), I think. Grr. Typedefs for pointer "types" really confuse matters.

[draci.livejournal.com]

*tosses homemade oatmeal-raisins at you*
Woot! You're mostly right, it needed to be AsmParserRec. :) Future methods were scribbling on the parser and smashing the heap, since pointers are only four bytes. Drove me nuts, I wasn't looking anywhere inside a silly little constructor!

New(Parser, ...) is still correct because New takes (void**). A normal top-level invocation goes New*(&object, size). New*() and Dispose*() are the only ones like this, and it's only because C lacks automatic pass-by-ref. AsmNextChar, et al, simply takes AsmParser.

Thing is, I would have caught the mistype if NewAsmParser had been written after the typedef. The intention was to emphasize them as opaque types. I'm not sure how that will pan out, but I admit it's heavily influenced by Pascal. (I really, really didn't miss C. My Pascal code rarely burns this badly! I'm only writing it to be portable...)

I suppose it's fortunate that this problem only showed up in a test case...

[coderlemming.livejournal.com]

I dunno... I still stand by what I said: typedefing a pointer "type" will lead to confusion and bugs more than it will help prevent them. After all, typedef is just syntactic sugar designed to make programming easier. In this case, it required extra thought that you forgot about.

Anyway, if you want opaque types and passing by reference, use C++ ;) Not that I'd normally be a strong proponent of C++, but it seems like the right tool for the job here.

[draci.livejournal.com]

Well... thing is, at least I understand C. C++ (or maybe just what some people write with it) scares me, and I've had mixed experiences with it. This isn't going to be a learn-new-language project. :)

I think typedef is going, though because it just looks nice, and doesn't really gain anything after all.