Not so C++



  • The last week I finally managed to fix a bug that had been haunting me all year. The bug would cause a crash in some of our licensed code.
    Unfortunately, the crash occurs about once every 10 hours of use, and only seems to occur in release mode.

    Fortunately, I was eventually able to store enough of the applications crashed state that I could reproduce the bug. Sortof.


    Anyway.
    I thought I'd post some snippets and quotes about the source file where the bug occurred:


    Memory allocation:

    All arrays, for some odd reason, were allocated so they are indexed from [1,n]. Not [0,n-1] as you might expect. So 0 is an invalid index.
    All matrices are allocated in the same way, [1,n] on both columns and rows. To make it more fun, they are double pointered, and if you do happen to access [0][0], you somehow end up overwriting the original allocation pointer (I think...).
    Malloc is only used, no new(), no std::vector, etc.
    To save allocations, the allocations are done only when needed, so each function has function static pointers to store it's allocations. The values are only allocated/reallocated when an input value to the functions (which itself is accessed as a pointer) is greater than zero.
    There is no bounds checking, and the size of the original allocations of these vectors/arrays is not stored.

    Comments:

    The functions themselves 'implement the levenberg marquard algorithm'.
    Thats pretty much it for the comments. There is a short description of what the algorithm tries to do.
    In the actual code, there is only one comment:
    "//basically a hack - keeps trying to invert"

    There are no whitespace lines, at all. The programmer used first line {'s, which I really, really don't like. Not all {} blocks are indented.

    And don't forget the global variables.


    Now, onto the code.

    I'm not going to post it all, just parts.

    Here are the function declarations (I've renamed parts to anonymize it)


    [code]

    void lin(float *chiSq,matrix *param,int dataL,void (*func) (matrix *, matrix *, void *),void *ptr, int maxIter, float incfd)

    int levMarq(float sigma[], int datan, float a[], int ia[], int ma, float **varcor, float **gamma, float *sqchi,void (*func)(matrix *, matrix *, matrix *, void *), float *mdaala, void *ptr)

    int optimLevMarq(float sigma[], int datan, float a[], int ia[], int ma, float **varcor, float **gamma, float *sqchi,void (*func)(matrix *, matrix *, void *), float *mdaala, void *ptr, float incfd)

    void cof(float sigma[], int datan, float a[], int ia[], int ma, float **gamma, float omega[], float *sqchi,void (*func)(matrix *, matrix *,matrix *, void *),void *ptr)

    void optimCof(float sigma[], int datan, float a[], int ia[], int ma, float **gamma, float omega[], float *sqchi,void (*func)(matrix *, matrix *, void *),void *ptr,float incfd)

    [/code]


    At one point there is a 6 level deep set of nested for loops.

    The limited error reporting is through printf, and occasionally exit().

    There is no checking for nulls, and after freeing the memory, the pointers are not zero'd. The next run will require the odd input value to be greater than zero to reallocate.


    As for the bug?
    It came down to an array access. Because the arrays are [1,n]. At one point, an index variable was being used without being initialised (once every 10 hours remember), so it was zero. Somehow, this would screw the original pointer to the allocation, which would then somehow screw the input function pointers on the stack, which would blow up spectacularly when they were invoked.
    Ohh, and the error didn't occur in debug mode.

    So the fix was:

    change:

    int col;

    to...

    int col=1;


    As you might imagine, it's been trying.

    What I will say; the person/people who wrote this code are very smart. Their mathematical ability is a quantum leap beyond mine, it's just they were too smart for their own good in places - which has resulted in the easily 100+ hours I've put into fixing it's many bugs.




  • Sounds like whoever wrote it used to be a FORTRAN programmer. Arrays started from 1 in FORTRAN, and back then nobody ever thought about maintainability.



  • I am absolutely astounded that this error was not caught by noticing a 'possbile use of uninitialised variable' warning.

    Ah - I mean the real WTF is....



  • what is "first line {'s"?



  • Well the loop that set the variable had calculations, each would have a threshold, and if under, it would set the value. It's the 'once in 10 hours' thing when it would not set the value.
    Because it was a mix of C/C++, none of the variables were initalised at all. Ever, in the entire application. (all declared at the top of the function too, yet classes were used, all .cpp files, etc).
    Even so, it wasn't at all clear what the variable did (it wasn't called 'col', was actually 'ic'... it was used for multiple things, including as a column index), so logically you'd put an =0 on the end if you did get a warning. It wouldn't matter, as it needed to be =1 because of the stupid array decision.

    the first line '{'s I mean:

    if (...) {
    }

    syntax instead of

    if (...)
    {
        ...
    }

    Don't know the proper term.
    I find the former very hard to read, especially with no other whitespace lines, and the level of complexity in this code.
    Some files in this code has no indenting at all.

    Arrays would also sometimes be allocated with a dynamic range. So [a,b], with a/b possibly being negative too.



  • they make code formatters that will reformat your code and move the braces around.  BTW, I like the first method better.  there was a huge thread a couple months ago arguing over which one is better.   let's just say that that style is NOT a WTF.



  • [quote user="Graham"]Well the loop that set the variable had calculations, each would have a threshold, and if under, it would set the value. It's the 'once in 10 hours' thing when it would not set the value.
    Because it was a mix of C/C++, none of the variables were initalised at all. Ever, in the entire application. (all declared at the top of the function too, yet classes were used, all .cpp files, etc).
    Even so, it wasn't at all clear what the variable did (it wasn't called 'col', was actually 'ic'... it was used for multiple things, including as a column index), so logically you'd put an =0 on the end if you did get a warning. It wouldn't matter, as it needed to be =1 because of the stupid array decision.

    the first line '{'s I mean:

    if (...) {
    }

    syntax instead of

    if (...)
    {
        ...
    }

    Don't know the proper term.
    I find the former very hard to read, especially with no other whitespace lines, and the level of complexity in this code.
    Some files in this code has no indenting at all.

    Arrays would also sometimes be allocated with a dynamic range. So [a,b], with a/b possibly being negative too.
    [/quote]


    The one brace style is known as K&R, and quite a few of us here use it. I find Allanman style (the one you use) anonying, and GNU style to nightmare. With proper indentation, I find K&R 10 times easier to read then Allenman, because I don't have to move my eyes up to find the statement, instead, I see the statement and the brace.



  • The real question is, did he write

    } else {

    or

    }
    else {



  • Neither. He probably used:

     if (condition) {
    //do something
    }
    if (!condition) {
    //do other thing
    }
     



  • [quote user="Graham"]All arrays, for some odd reason, were allocated so they are indexed from [1,n]. Not [0,n-1] as you might expect. So 0 is an invalid index.[/quote]

    Ok, I haven't done too much C/C++ but how was this done? Are you saying that arrays were allocated like

    [code]malloc( size_i_want + 1 );[/code]?

     



  • [quote user="hetas"]

    [quote user="Graham"]All arrays, for some odd reason, were allocated so they are indexed from [1,n]. Not [0,n-1] as you might expect. So 0 is an invalid index.[/quote]

    Ok, I haven't done too much C/C++ but how was this done? Are you saying that arrays were allocated like

    [code]malloc( size_i_want + 1 );[/code]?

    [/quote]

    Probably more like

    [code]malloc( size_i_want ) - 1[/code]

    shudders 



  • Wow, what a nightmare. I'll never understand why some people stick with C and refuse to take advantage of C++. Templates is what makes C++ bearable.<font face="Lucida Console" size="2">
    </font>



  • No, classes are what makes C++ bearable.  Templates are what makes C++ suck.



  • [quote user="Graham"]Ohh, and the error didn't occur in debug mode.[/quote]

    You need to use better debugging tools.  Even VS 2003.NET debug mode would have caught an uninitialized variable at runtime, if you turn on /RTCsu .  Failing that, there are lots of runtime code analyzers (Rational, lint, DevPartner) that can help track down these problems.

     
    I do feel your pain though.  The current code base I'm working on is full of crap like that.  If you are using the MS IDE, remember that CTRL-a, CTRL-k, CTRL-f is your friend...



  • I find the "if (...) {" style horrific to deal with. 

    I use this (which was very difficult to type because of the horrible editor of this forum software, which kept adding blank lines when I hit <ENTER>):

    if (...)
        {
        // do whatever
        }

    Much, much easier to keep track of both the contents of a block, and the condition that owns it.  And that's not even mentioning nested blocks, where the "if (...) {" style is an absolute nightmare.

    Of course, all styles can be made difficult when an inconsistent mix of tab characters and spaces are used for indentation.  Using an editor with a different tab stop really ruins formatting.

    Which is another point of contention.  What's the best size for a tab stop?  I say four, because two is too small an indentation to see clearly at a glance, and eight sucks up too much room.  I've seen one and three as well (don't like them), but curiously, nothing between four and eight.

     




     



  • WRT to the whole indentation thing,

    I personally use K&R with 8 space tab stops, with the exception that namespaces, function, classes, etc get their own line for the opening brace.

    <rant mode="on">
    When the closing brace has comments on it this i find it very annoying ( that is pulled from a slide used in yesterday's computer science lecture. Note the three ways of writing Comparable ), maybe for #defines that cover the whole file it would be justifed, but when the opening brace is *two* lines above i dont really see that reason for it.
    </rant>



  • [quote user="Thany"]Which is another point of contention.  What's the best size for a tab stop?  I say four, because two is too small an indentation to see clearly at a glance, and eight sucks up too much room.  I've seen one and three as well (don't like them), but curiously, nothing between four and eight.[/quote]

    Emacs uses 5 spaces per tab in K&R mode, although you can override it, I think it's because that's what K&R themselves used (I may be wrong though).  I guess it is a little weird, but I've never really cared enough to change it, and no-one else ever sees my code.  (I do try to stick to the existing style when modifying other people's code, not that I do a great deal of that either.)

    [quote user="paranoidgeek"]I personally use K&R with 8 space tab stops, with the exception that namespaces, function, classes, etc get their own line for the opening brace.[/quote]

    Isn't that normal for K&R style anyway?  At least for functions; classes and namespace would be a logical extension. 



  • [quote user="Thanny"]

    Of course, all styles can be made difficult
    when an inconsistent mix of tab characters and spaces are used for
    indentation.  Using an editor with a different tab stop really
    ruins formatting.

    Which is another point of contention. 
    What's the best size for a tab stop?  I say four, because two is
    too small an indentation to see clearly at a glance, and eight sucks up
    too much room.  I've seen one and three as well (don't like them),
    but curiously, nothing between four and eight.[/quote]

    I once worked on code that used all of the following for the first level of indentation:

    *One space,
    *two spaces,
    *three spaces,
    *four spaces,
    *two spaces and a tab,
    *a tab and a space,
    *five spaces,
    *a tab and two spaces
    *a tab, a space, and a tab,
    *seven spaces,
    *eight spaces,
    *two tabs,
    *ten spaces,
    *or three tabs.

    frequently with a half-dozen or so mixed within a single function.  Keep in mind that this code was written by one person, using the same IDE the whole time.  The second thing I did upon recieving that code was to run it through a pretty-printer to set things up with my preferred level of indentation: four spaces per level.



  • [quote user="mrprogguy"]No, classes are what makes C++ bearable.  Templates are what makes C++ suck.[/quote]

    Why would anybody choose to use C++ without templates? Without it I would never want to get anywhere near C++. Templates provide some of the great things that make other languages so much easier to use.

    Smart pointers like std::auto_ptr and boost::shared_ptr makes memory management as easy as in Java. Without std::list, std::vector, and std::map you'd just be dipping into a big bucket of bits with no safety checks. Converting to and from textual representations would be much messier without boost::lexical_cast. Function pointers have a syntax worse than Perl, but boost::function makes that a non-issue, and passing callbacks to member functions would be practically impossible without templates.

    If you aren't taking advantage of C++ templates, you're not being nearly as efficient as you could be. Just take a look at Boost to see how powerful they are.



  • [quote user="HeroreV"]... you'd just be dipping into a big bucket of bits with no safety checks.[/quote]

    But that's what C++ is good for! The big bucket of bits without the safety checks is what it's all about! Absence of {raw bit buckets, unchecked casts, pointer arithmetics, ambiguous grammar, (2^k; k much larger than 1) ambiguous and unintuitive precedence rules} makes Jack a dull boy, you know.



  • Sorry for the late reply, I'll try and answer some of the questions:


    When I said 'it didn't happen in debug' I meant that literally. Somewhere else in the application, the values being used were computed differently, so the 'once in 10 hours' case would not occur, the value would be initialised, and the array access would work. So debug mode wasn't any help. I don't know why this was the case, I tried all sorts of compiler settings, matching them to release mode.



    Probably more like

    malloc( size_i_want ) - 1

    shudders


    It was actually worse than that :-)

    All the arrays were 2D arrays, although 1D arrays were simply treated as nx1 arrays passed through the same 2D function.

    How it worked was as such:

    data = malloc(col * row * sizeof(double) + row * sizeof(double*));

    The first colrow8 bytes were the matrix of doubles,
    the next row4/8 bytes were the pointers to each row

    Also remember columns range between c1 and c2, and rows between r1 and r2.

    Usually c1 = 1 and r1 = 1.

    So, the pointers were setup like so:

    doubles = data;
    pointers = data + col
    row8;

    for each row,
    pointers[n] = &doubles[col
    n - c1];

    return pointers - r1;


    What this meant was accessing array[0][0] = x; would:

    array[0] would be the last 4 bytes in the matrix data (when n1 = 1), which was usually garbage because it was floating point. Bang, access violation.
    Does that make sense?


    There were other WTFs in the code. Some utterly ridiculous.I managed to cut out so much unused or useless code from the library it went down from ~500kb to around 150. This included a full custom threading library that had clearly never been either used or tested.


    Possibly the most ridiculous 'bug' was a pattern matching subsystem (that was absolutely critical to the programs operation). Given X patterns in memory, it tried to match those patterns to an input. However, it did like so:

    Take the first known pattern in memory.
    Loop through all potential patterns in the input,
    find the closest correspondence confidence, match these two. Remove the pattern from the input,
    continue to the next known pattern.

    However this had the 'slight' issue was that if the input data only had (say) 1 potential pattern, it would always match to the first loaded known pattern (even if there were 10s or 100s). Always. No matter what it was, even if the confidence was 0.0. They had probably never tested with more than 1 pattern.




Log in to reply