hbr

One bug, and one better solution to a previous bug.

Local static state is local, static

int Foo() {
  static int timeout = kTimeoutLimit;
  bool polled_condition = CheckExternalCondition();
  if( !polled_condition )
    --timeout;
  if( timeout == 0 ) { 
    printf("condition not set before timeout");
    return kTaskFailed;
  }
  if( polled_condition )
    return kTaskComplete;
  else
    return kTaskIncomplete;
}
void Bar() { 
  while( Foo() == kTaskIncomplete );
}

Sometimes a local static variable is a useful thing — conveniently quick to add to any piece of code, and localized to a particular scope. You know everything about what can happen to it during its entire lifetime — assuming you know about how the scope it is contained within operates. (and I’m disregarding the returning of references to local static variables from a function. That’s a whole different bag of fun…)

Quickly rewriting some code today, I added a local static counter — counting down each time a desired condition was not met, and triggering a printf() should the count ever reach zero. I was using this to diagnose that something unexpected had happened, and that some event had not taken place.

The above code should work adequately well — if Bar() is only ever run once (and only from one thread at a time — but that’s not the issue here). timeout is never reset, so unless CheckExternalCondition() returns true, first time, every time, the value in timeout will continue to decrease with each run of Bar(). If it drops below zero, there will no longer be a limit on the on how long Foo() can loop.

A fix here is to make sure timeout is reset each time Foo() “completes” i.e. when it returns kTaskComplete or kTaskFailed.

More generally, this does not seem like a good use of a static local variable — relevant context for the function is not readily apparent within the function, so it seems better to store the timeout value outside of Foo().

(The actual code is more convoluted than the example above, so it’s not quite as simple as adding a local timeout var in Bar(), but only just)

Redux: Changed virtual function prototype results in correct function not being called

This was a bug where I changed the prototype for a virtual function, but that classes with the old prototype would not be detected at compile time and would silently do nothing should the virtual function be called. Read the details via the link above.

I had added an assert to catch any classes that didn’t have the new version of the function, but that only worked at runtime, and only when the function was actually called. What I really wanted for this was a compile-time check that was more reliable than the inheriting programmer remembering to use override. I discovered that there is a way to do this with C++11: final.

class Base {
public:
  virtual void Foo(int some_param, WhatsitType some_other_param)  {  }
private:
  virtual void Foo(int) final { }
};

With this, anything that inherits from Base and attempts to define its own void Foo(int) method will result in a compile error. I’m much happier with this, and making this change revealed some extra instances of the old function that I’d missed in my previous sweep. Huzzah.

Initialized, but not in the right order

One runtime bug today, being fallout from an attempted fix for Friday’s Unexpected use of class bug.

To recap/elaborate: I changed something like this

struct Foo : public Baz {
  Bar* m_Bar;
  virtual void Destroy() override { }
};

to be something more like this

struct Foo : public Baz {
  Bar* m_Bar;
  virtual void Destroy() override { m_Bar->Destroy(); }
};

and I had assumed this would be OK because I [thought that I] knew the only place that Foos would be created, which would always safely initialize m_Bar.

Unfortunately, there is another way to construct these objects with some unexpected (bad) input data that would not initialize m_Bar. In such a case, Foo::Destroy() would crash because m_Bar pointed at who-knows-what.

So I implemented something like

struct Foo : public Baz {
  Bar* m_Bar;
  virtual void Init() override { m_Bar = NULL; }
  virtual void Destroy() override {
    if( m_Bar != NULL)
      m_Bar->Destroy();
    else
      HARD_TO_MISS_NOTIFICATION( USEFUL_WARNING_37 );
  }
};

“Hooray,” I thought. “That will catch those pesky bad datas.”

In practice what I got was a crash from the first thing that tried to use m_Bar.

It turns out that the construction path for Foos sets up m_Bar before calling Foo::Init(), so now our correctly constructed Foo object has it’s valid data overwritten by an overzealous initialization function.

Foo::Init() is called from both the preferred and not-preferred construction paths, so my hope had been to fix it there and catch use of the not-preferred path, but this didn’t work. My next thought was to reorder the setup of m_Bar and the call to Init(), but that seemed far too risky — there are a lot of subclasses of Foo, and many of them have non-trivial Init() overrides of their own, so there is a high probability that something else will break if m_Bar is not set up before the call to Init().

The winner this time seems to be to initialize the member to zero using: a constructor. Yes, it’s probably the obvious option, but for much of this codebase explicit Init() functions are used instead of constructors. Fortunately, for Foo and its friends (sub classes, parent class) we call the constructor (via placement new) to set up the vtable for us, so we can use it to also initialize the m_Bar pointer and consequently help catch occurrences of bad data in a less fatal fashion.

 

Terrible day — between an unfamiliar system, the unshelving of the remains of half a changelist and a whole lot of carelessness, I made a lot of mistakes and got to do a lot of full recompiles. A day to learn from.

So before I summarize that disaster, I’ll start with a could of other bugs.

Unexpected use of class

A couple of days ago, I checked in some changes to a class whereby one of its member classes could dynamically accrue allocations during its lifespan — it keeps track of certain memory allocations to reduce the number of allocations needed during its lifespan.

I subsequently added a Destroy() method for that member so that it could be cleaned up properly at the end of its life, expecting that the member would be correctly initialized so that Destroy() would Do The Right Thing.

One crash callstack later…

There exists two ways to create objects of the class type. One — used to construct objects in a particular family — will perform the correct initialization. The other — a more general construction path used to construct a wide variety of other more-or-less-related object types — will not correctly initialize this member type.

I was of the understanding that, for various architectural reasons, nothing would attempt to construct an object of this type via the more general approach. It should be no surprise that I was wrong.

I am of the opinion that anyone using the “general” construction path is Doing Something Wrong, and so I think it will be appropriate to add an assert to catch this kind of thing [a little awkwardly] in future. I do need confirmation from from other users of this system that we agree that what has happened here is undesired/unexpected, and that it can be assumed to be Wrong in future.

The “general” construction path is used to dynamically construct object based on class data passed into the program. It is likely possible to prevent this kind of “bad” data from entering the program by more strongly protecting it at the data-construction stage, rather than trying to prevent it at runtime. I’m not sure what protection is in place presently and need to investigate that.

The absolute minimum fix is to prevent this code from crashing should it get the wrong/unexpected data…

One of the issues leading to this problem is that the type being used here derives from a common base class. This does offer a great deal of convenience in practice within this codebase, but it also leads to this kind of complexity (source of bugs) where Bar isMostlyButNotAlwaysA Foo.

Hard-coded array size, no checking

This one arrived as a crash callstack in some code I had a lot to do with a while back, that had remained largely nearly touched for a number of months. The code is heavily optimized and is reasonably complex — and quite awkward to modify. As a result, having to locate and fix bugs in this code is a somewhat daunting task, not least because of how much use it has received (every frame…), and has been running without problems for a long time.

Fortunately (HAHAHAHA), it seems the problem was really quite simple: this code uses some fixed-size static arrays for storage of intermediate data. At no point is there a check that the data being processed does not exceed the size of these arrays. As is the way of these things, the size of the data being processed has grown over time, resulting in a silent writing past the end of one of these arrays. Crashilarity ensues.

Fix: increase the size of the arrays, add an assert to make sure we have enough space.

(Removed the comment on one of the arrays asking why it was so small in the first place :\ )

General disaster

• Double-init of system — whereas yesterday I was failing to init the system I was working with, today I managed to call the init function twice. Added a flag and check to detect this.
• Double-shutdown of system — Later on in the day (after I’d got past most of my other bugs), I discovered I was calling the shutdown function for the system twice, in exactly the same way as I’d been double-calling the init function. I should have checked for this when I fixed the double-init bug (where there’s smoke…)
• Failure to flush — started writing a prefix token to a buffer with an incorrect initialization order. Data didn’t get flushed, record of memory blocks was confused, things failed.
• Need to reset state after flush — once the buffer is flushed to disk, reset all the variables that tracked the un-flushed state.
• Flush [too] early and flush [too] often — put calls to various Flush() methods from a range of class destructors. Got a little carried away. Let it mellow.
• Calling functions on the right objects — sometimes it’s nice to be able to construct things from other things, and to be able to rely on object lifetimes to encode certain behaviors. Even so, it will usually matter which object’s method gets called…
• Case matters. Sometimes. — When interacting with an external unfamiliar system, it’s worth making sure you know whether it is case sensitive. In this instance, it was, and I wasn’t. I worked this out almost by accident, which is regrettable.

Post-disaster-mortem: working through this code was highly reactive — I spent far too little time reading the code and working out what it was/should be doing, and far too much compiling, running the app, eyeballing output, manually feeding it into the next system, and trying to work out what I’d done wrong each time. I have no doubt that manually tracing through [a relatively small amount of] code, and validating its function up front would have saved me a lot of time.

Note to self: know when you’ve fallen into the reactive compile/test/modify cycle and step back to look at the bigger picture.

Failed to initialize instance-specific member variable correctly because I was calling the wrong constructor

struct Foo {
  bool m_B;
  Foo(Bar& bar) : m_B(true) { /*do something else with bar*/ }
};
struct Baz : public Foo {
  Baz(Foo& foo):Foo(foo) {}
};

Thinking that Baz::Baz(Foo&) was calling the defined construct-from-reference-to-bar constructor that would initialize m_B to a particular state when in fact I was calling the compiler-generated copy constructor that would not.

Yet another case of pay attention-to-what-you’re-doing. The thing that got me this time was that this codebase doesn’t have a lot of construct-from-reference, so led to the trap of if it compiles, it’s probably OK.

Quite possibly this was fallout from yesterday’s the slicing-inheritance bug where I changed inheritance to a pointer-member, but didn’t change the construction call.

Use of uninitialized system

Calling functions in a particular system without having called the appropriate System::Init() function. The results aren’t pretty.

This was caused by carelessness when un-shelving a partial rework of the init and use of the particular system.

Keeping track of where particular code is being called requires extra effort when managing multiple versions of the same files. Clearly, I’m too easy to confuse and need to be more careful, and to work with fewer variations.

In general, checks/asserts in the external interfaces to a system are probably worthwhile to catch this kind of thing a little sooner.

Yet another uninitialized member variable crash

In a rework of a rework of a rework of some code I lost a couple of struct member initializers.

Init to zero, all works as expected. Sigh.

End of day reflection:

Making changes to a previously-working system, it would have been profitable to spend some time to capture the known-good output (and a record of the known-extant bugs) at the start of the process. I spent some time reverting all my changes and re-building to confirm my analysis of these things, which was not the most efficient approach.

Uninitialized class member

The problem:

struct Foo {
  int m_A;
  int m_B;
  int m_C;
  Foo():m_A(0), m_B(0) {}
  void Bar() {
    DoSomethingWith(m_C); // fail it
  }
};

My defense is that this was spread across a header and a .cpp file, which is to say I was careless. (My preference really is to put all code inline in one place — .h and .cpp is repetitive and costs time and effort to keep in sync)

This was some existing code that I reworked a couple of weeks ago and hadn’t got to the point of running/testing until today. I’m really not sure how I managed to remove the initializer here — it existed in the original version.

The fix:

Initialize to zero, which was not just known-value, but correct-starting-value.

Prevention:

Always initialize everything. Keep class definitions in sync with constructors and Init() methods.

Sliced inheritance

The problem:

class Bar {
  virtual void Baz() {/*nothing interesting*/}
};

class BarExtender: public Bar {
  virtual void Baz() {DoSomethingInteresting();}
};

class Foo : public Bar {
  Foo(Bar& bar): Bar(bar) {}
  void Yolo() { Baz(); }
};

BarExtender b;
Foo f(b);
f.Yolo(); // intended that this would DoSomethingInteresting(). Actually does nothing interesting

The fix:

class Foo : public Bar {
  Foo(Bar& bar): m_Bar(bar) {}
  void Yolo() { m_Bar.Baz(); }
  Bar& m_Bar;
};

Prevention:

Know the language well enough to know when you’re writing code that doesn’t say what you mean.

Undesired printf output

The problem:

This is the first “functional” bug — something that didn’t work quite the way I’d intended.

For a data serialization class, I had hoped to replace several functions of the form:

WriteInt32(int32_t i) { snprintf(..., "%d", i); }
WriteDouble(double d) { snprintf(..., "%f", d); }

to a single function:

WriteNumber(double d) { snprintf(..., "%f", d);  }

Unfortunately, the %f format specifier will, by default, always write a decimal point and 6 figures thereafter, which is undesirable for integers-types-cast-to-double written by this function — for particular use cases it doubles (hah!) the size of the data written.

The fix:

I’ve not tested this yet, but it looks like %g (or probably %.ng will provide the desired behaviour by not writing the surplus zeroes. I will need to determine if the output for floating point input and large integers will be acceptable.

Prevention:

I still have a lot to learn about printf format specifiers, though I’ve found http://www.cplusplus.com/reference/cstdio/printf/ useful when considering/trying to to understand the available options. This does make me wonder if a printf-format-string constructor exists analogous to http://cdecl.org …

Update:

Rather than trying to find a single format that works for all input or testing the number before printing it, I’ve opted to overload the function by type. This way we can use knowledge of the type to do something appropriate and more efficient.

I have overloads for int, unsigned int, double, long and unsigned long, and a comment as to why there’s no implementation for unsigned long long (the loader treats all numbers as doubles, which means we could end up writing out numbers that we can’t read back without loss of precision along the way). I’ve also added static asserts in case this code gets compiled on a system where sizeof(long) > sizeof(int). There are separate functions for writing out bool and pointer values.

Changed virtual function prototype results in correct function not being called

This one wasn’t a bug that landed immediately from my change, but happened when a subsequent code branch was integrated. I’m going to claim this as my bug as it could have been prevented (or at least made more obvious) with some extra though.

The problem:

We have a virtual function, something like

class Base {
public:
  virtual void Foo(int some_param)  {  }
};

Which I modified a few weeks back by adding an extra parameter, so foo() became

  virtual void foo(int some_param, WhatsitType some_other_param) { }

and I added the same parameter to every one of the classes that inherited from class Base. There were quite a few.

Sometime later,  code was integrated from another branch where the old prototype was still in use, including something like this:

class NewInheritor : public Base {
public:
  virtual void Foo(int some_param) { // oh noes - it's the old prototype!
    DoAmazingThings();
  }
};

The new code compiled without warning, but at runtime, NewInheritor::Foo() wasn’t being called, so the amazing things were failing to be done. Debugging this was somewhat inefficient — the process for calling Foo() methods includes a number of steps, and I failed to notice the missing parameter for quite a while.

The fix:

Update NewInheritor::Foo() to match the parent class.

Preventing these in future:

Remember that virtual inheritance is a horribly fragile beast.

One thing that may have worked is to make Foo() pure (compile error) — or undefined (link error) — in the base class (i.e. void Foo() = 0;). This isn’t a great match in this case as inheriting classes don’t need to have their own implementation of Foo() to be valid, and requiring it makes for some additional tedious boilerplate code in every inheritor (of which there are many).

The override keyword will help detect this at compile time, but it relies on the implementer of NewInheritor to have used it :\ The action here is to recommend that people use override on every virtual function override.

In this case, what I do know about this code is that it is never valid to call Foo() on a class that doesn’t have its own implementation. We can put an assert() in Base::Foo() and let the writers of inheriting types (or me when next I get to debug one of these) have a more obvious clue about what’s happening. Adequately functional.

memcpy() into an uninitialized pointer variable

The problem:

I had changed the type of a variable from being a static array to being a pointer, to match a change I’d made to a particular function. What I hadn’t paid attention to was that this array was being used as the destination address for a call to memcpy(). An uninitialized pointer on the stack is not a good place to try to copy things.

The call to memcpy() did not cause the program to crash. The program did crash shortly thereafter, though, which distracted me for a short while from the cause.

The fix:

Explicitly allocate some space for the copy.

Preventing in the future:

The pointer on the stack had not been set. A null pointer would have caused the crash to happen at the call to memcpy(), making the cause more obvious.

Bad loop counting

The problem:

int max = m_Max;
int i = 0;
for( ; i < max; ++max )
  if( a[i] == bar )
    break;

++max? Yeesh.

m_Max is initialized to zero, which means that this was a no-op rather than an infinite loop.

The fix:

Increment the counter, not the upper bound.

Preventing in the future:

Don’t be a goose.

Failing to store updated state

The problem:

Immediately following the code above, in the case where we failed to find the desired bar in the array, increase the size of the array. And then fail to store the new max size into m_Max.

I even got this wrong twice: my first attempt was to replace a use of max+1 with ++max.

The fix:

Write the necessary state back to the object: ++m_Max.

Preventing in the future:

Pay attention to where the state should go. Don’t be a goose.

I like to try and fix as many of the bugs that I write as early as possible, before they’re deployed to the rest of the team — I hate inconveniencing others with my own defect-laden code. As such, my preference is to do what I can to catch the bugs I write as early as I can.

I rely very heavily on the compiler to catch particular classes of bugs for me, to the point that (unlike my early computer science lecturers) I don’t look at a compile error to be a bug: the compiler is very good at reminding me about parts of the code I’m writing that I haven’t finished changing yet, and does so quickly and with minimal fuss. If I change the assumptions or guarantees of a particular function, deliberately changing the function so that callers will no longer compile is — for some projects — not a terrible way to start tracing through parts of a codebase that might be affected. This can include changing arguments to a different order, more args, fewer args, changing the function name, #if 0/#endif around the function, etc.

There are various scenarios where this may not be so useful — very widely used functions, virtual functions, changes to a library used by a number of disparate programs, etc — but I find that it can be more efficient than grepping through a large number of files, particularly when there’s some ambiguity in the symbol name you’re searching for.

Some other ways to have the compiler error for you include leaving symbols undefined so that the compiler error list becomes something of a todo list, making class members private, removing methods from a base class. There are probably others I use. Many of these changes can be applied as a short-lived change to learn more about potential consequences of a change, and are typically easily revertable (assuming the use of any basic version control system).

Compiler warnings are also well worth considering and, if you operate with warnings-as-errors, impossible to avoid. I’ve found plenty of bugs that would have be horrible to diagnose from their symptoms thanks to the warnings of a good compiler.

Compile-time/static asserts are also a good thing. You can never have too many of those.

After the compile-time stuff, next is linker errors. If it all compiles but won’t link, the undefined symbol list can again become that todo list of functions that you haven’t yet got around to implementing. The downside with link errors is that for a larger program it takes a lot longer to compile and attempt linking the whole thing before you find out about the work yet to be done. There are other tricks that can be done with the linker to deliberately break the build, but that’s beyond what I want to write about right now.

When it all compiles and links, finding the bugs is a matter for the runtime. You need to run the code and find out if it will work or not (for the purpose of this article, I’m going to treat “running a test suite” as “running the code”). Any defects present at this stage typically take more time to identify, and more time to fix — even if it is something blatantly obvious, the edit, compile, link, run cycle can be much longer than just edit, compile.

It’s at this point I get to my intended purpose of this blog post: I want to write fewer bugs. Various classes of bugs are prevented for me by the compiler and linker, but if it’s something that gets all the way to failing at runtime (whatever that means), I’m left disappointed. What to be done about it?

What I’m going to try today and, hopefully, continue to do after today, is keep track of bugs I wrote that survived to the point of being run. I’ll give some thought to how they got there and consider if there was something I could have done (and can do in the future) to prevent that kind of bug. And maybe I’ll find out if I keep making the same kinds mistakes over and over again, or if I can learn to write fewer bugs.

I shall attempt to begin with today’s bugs in another post.