1

I am interested to know if the MSVC compiler has any options that might have shown up the following bug the moment it was first written (over a year ago). In that time, the bug has been copied and pasted to several other places in the code. By rights that should have caused crashes every few seconds. By luck, it only crashes every few hours instead, and it usually leaves very little evidence to debug. The essence of the buggy code was something like:

int someDataSubjectToMultiThreadAccess;
CCriticalSection someSyncObject;
...
int SomeFunction( void )
{
  CSingleLock( someSyncObject, TRUE );
  return someDataSubjectToMultiThreadAccess;
}

I'm not going to highlight the error - the Visual Studio compiler certainly doesn't! If you can't see the error, create a test file, compile it, and look at the .cod file to see the assembler generated. With my optimisation settings, it calls the CSingleLock constructor to create a temporary object, then immediately calls the corresponding destructor, which (through optimisation?) is the CSingleLock::Unlock() function. Finally, the shared data is accessed without actually being locked.

So this is a cautionary tale in how to create a completely ineffective locking mechanism by omitting the variable that would last throughout the scope of the function - CSingleLock vitalLockVariable( someLock, TRUE );. It surprised me how many eyes had seen this code and not noticed the bug.

I'll save you some of you the effort of typing this advice: "use the boost synchronisation primitives instead". Yes - thank you - one day I probably will. But this kind of slip probably applies to them equally well.

What I really want to know is what tools are available to catch this sort of thing, preferably the moment it is typed, but via any means other than code reviews or trying to analyse baffling crashes.

omatai
  • 3,448
  • 5
  • 47
  • 74
  • 3
    Returning an `int` from a function with `void` return type is a little weird too. ;-] – ildjarn Feb 19 '13 at 00:38
  • 2
    In general, calling a constructor without assigning it anywhere is valid, if perhaps a little odd. So it would probably be wrong for the compiler to warn about it, in general. – Oliver Charlesworth Feb 19 '13 at 00:40
  • There are plenty of things that will compile yet still result in problems. That's why we make the distinction between syntax and runtime errors. Toolsmiths are expected to know how to use their tools :-) – paxdiablo Feb 19 '13 at 00:42
  • 3
    I believe all of the text above can be reduced to 2 sentences. – mfontanini Feb 19 '13 at 00:42
  • :) Oops - let me edit that... – omatai Feb 19 '13 at 00:43
  • 1
    The bug is that he meant `CSingleLock l( someSyncObject, TRUE );` so the lock would be held through the scope. – David Schwartz Feb 19 '13 at 00:45
  • 1
    possible duplicate of [Why is there no gcc/g++ warning for unused temporaries?](http://stackoverflow.com/questions/6518337/why-is-there-no-gcc-g-warning-for-unused-temporaries) – rob mayoff Feb 19 '13 at 00:49
  • @mfontanini - try reducing it to 2 sentences, and see which (if any) of the 7 different interpretations actually answer the question you had in mind :-) – omatai Feb 19 '13 at 00:53
  • 1
    -1 correct me if i am wrong, but this doesn't violate c++ syntax. it's dumb, but it's legit code. there is no way for the compiler to tell that you didn't mean to do what you typed. – thang Feb 19 '13 at 01:00
  • I don't know of anything that will "catch this sort of thing the moment it is typed". Perhaps a static analysis tool would figure this out, but you run those after you're finished typing. The one I'm most familiar with is Coverity, but the last time I used it (about four years ago) I don't think it would. – Bob Murphy Feb 19 '13 at 01:05
  • 2
    You could just post that code snippet and ask "Why doesn't my compiler warn me that I'm creating a temporary object that gets destroyed right after construction?" – mfontanini Feb 19 '13 at 01:21
  • ... except that was not the question. Only @BobMurphy has addressed the actual question. Just like the bug, the question is something different to what it first looks like :-) – omatai Feb 19 '13 at 02:24
  • 1
    But why is the question different than what it first looks like? That's just going to annoy people trying to answer your question. Also, what exactly is your question? Including the title, you mention three different questions ("Why does this compile?","Can MSVC catch this?", and "Are there other tools that can catch this?"). Can you perhaps fix the title and shorten/clarify your question? – David Brown Feb 19 '13 at 02:56
  • 1
    @omatai Ok, replace "Why doesn't my compier" with "Is there any tool that". The fact that only one person could address your question means you're not asking it properly. – mfontanini Feb 19 '13 at 03:17

0 Answers0