CLang 2.1 doesn't catch assert within if

I've been looking around for an analysis I could add to clang, but I
used Xcode 4.1's clang to test this, which only has CLang 2.1. Could
someone please check whether the current SVN catches this? I would
check myself but I am having some trouble getting the current SVN to
build. It's not the source code but some arcane problems with the
configure shell scripts. If I can't figure out my configure problem
soon I will ask about it here.

If the current SVN does not catch this, I'd like to do the work to add
the analysis myself. I've been following the list for a couple weeks
and understand now what would be expected of my code for it to be
accepted. I will open a Bugzilla bug to track my progress.

You NEVER EVER want to do the following:

if ( condition )
  assert( must_be_true );

following_line();

If NDEBUG is defined as for a release or profile build, you'll get the
following in your preprocessed code, which will be very hard to debug:

if ( condition )
   following_line();

If you really must insist on putting an assert as the only line within
an if statement, you MUST place it in a Basic Block:

if ( condition ){
  assert( must_be_true );
}

Even better is to make the if statement part of the assert:

assert( condition && must_be_true );

Back in the day I wrote a new library to replace my client's old one.
To test that my new library got the same results as his old one, he
asked me to link his executable with both his old and my new library,
and to compare the results of all the calls to each library. I don't
recall why anymore but I actually made the mistake, then spent all day
long tracking down my bug, as the executables and both libraries were
very complex.

I'm running Mac OS X Snow Leopard 10.6.8 on a MacBook Pro, Model
Identifier MacBookPro1,1 (32-bit Core Duo, NOT 64-bit Core 2 Duo).

$ clang --version
Apple clang version 2.1 (tags/Apple/clang-163.7.1) (based on LLVM 3.0svn)
Target: i386-apple-darwin10.8.0
Thread model: posix

$ gcc --version
i686-apple-darwin10-llvm-gcc-4.2 (GCC) 4.2.1 (Based on Apple Inc.
build 5658) (LLVM build 2335.15.00)

$ gcc --pedantic -Wall -c assert_within_if.c

This compile succeeds without any warnings.

$ clang --analyze assert_within_if.c

CLang doesn't print anything to the terminal. assert_within_if.plist
doesn't contain any diagnostics.

====== Cut Here ======
/* assert_within_if.c */
#include <stdlib.h>
#include <assert.h>

int assert_within_if( int flag, int *pointer );

int assert_within_if( int flag, int *pointer )
{
  int result = 0;

  if ( flag )
    assert( NULL != pointer );

  result = *pointer;

  return result;
}

====== Cut Here =====

Ever Faithful,

Don Quixote

Many analyses are missing in tools/clang/lib/Analysis/

2011/10/7 Don Quixote de la Mancha <quixote@dulcineatech.com>

The analyzer doesn't find this, but it would be an interesting check to write. Conditional code for branches are just asking for trouble.

You NEVER EVER want to do the following:

if ( condition )
assert( must_be_true );

following_line();

If NDEBUG is defined as for a release or profile build, you'll get the
following in your preprocessed code, which will be very hard to debug:

if ( condition )
  following_line();

No it won't. It will look like this:
if ( condition )
  (void)0;

following_line();

That's because, when NDEBUG is defined, assert(3) is defined like so:

#define assert(cond) (void)0

Even better is to make the if statement part of the assert:

assert( condition && must_be_true );

Actually, that's worse. Now if the condition is false, instead of skipping the assert, the assertion will now fail. What you really meant is:

assert( !(condition) || (must_be_true) );

Now the assert won't fail if the condition is false. If it is true, it will attempt to evaluate (must_be_true) like before. (I put it in parentheses because 'must_be_true' might be an expression like 'this && that', which won't work quite right without them.)

Chip

Awesome! Sort of.

The analyzer doesn't find this, but it would be an interesting check to write. Conditional code for branches are just asking for trouble.

It would be best if I didn't have to explicitly depend on the advance
knowledge that assert() is a macro that can be defined to NULL.

Better would be to preprocess a sample with NDEBUG defined and without
NDEBUG defined, then check that every line of code that is still
present in both cases is in the same basic block in both cases, while
allowing some of the lines to just disappear.

I will need some hand-holding here as I don't have the first clue yet
as to how to actually write an analyzer.

I've been trying to build LLVM on Ubuntu 11.04 by bootstrapping it
from scratch, without using a prebuild llvm-gcc. I think it would
work better for this purpose if I built the SVN on Mac OS X without
trying to bootstrap it, but to instead use the llvm-gcc that comes
with Xcode 4.1. I'll give that a try right now.

Don Quixote

You wrote that as "assert(3)" which suggests you are referring to a
UNIX man page. When I got bit in the ass by putting assert() in a
conditional, it was with Microsoft Visual C++ 7 or maybe 7.1.

Redefining assert() to (void)0 is clearly the right thing to do but I
am quite certain that whatever version of VC++ I was using didn't do
that, because a release build had definitely reproducible, subtly
different results than a debug build that got fixed when I put my
assert() in a Basic Block.

Besides NDEBUG, what other common preprocessor flags and compiler
settings are supposed to guarantee that the program's error-free does
not change?

Ever Faithful,

Don Quixote

I can’t speak for old versions, but at least VS 2010 Express defines assert in non-debug as:

#define assert(_Expression) ((void)0)

so it looks OK.

Side note: if you put a semicolon at the end of your assert, you should also be fine even in the case of empty assert definitions:

if (x)
assert(y);
z();

would become:

if (x)
;
z();

Yes, you are completely correct. I ALWAYS put semicolons after
asserts, to make them look like any other function call.

But I am COMPLETELY CERTAIN that putting an assert() inside of an if
statement without also placing it in a Basic Block totally broke my
VC++ code. The program I was integrating my library with was a
financial package that produced graphs. If I graphed the output from
both my library and the client's old library, with my assert inside
the if, those two graphs subtly diverged. When I fixed my code to use
a Basic Block instead, the two graphs coincided perfectly.

I still have the Visual Studio 7.0 installer. I'm not sure but I
think 7.1 is an update one can download from Microsoft's site. I
could install them both then just have a look at <assert.h> to see how
it is defined.

Even if all modern compilers do the right thing, I assert (!) it would
still be useful to write the analysis, as it would catch portability
problems. There are lots of coders who still use broken old
compilers. I still hear about people using Visual Studio 6!

Ever Faithful,

Don Quixote

Yes, you are completely correct. I ALWAYS put semicolons after
asserts, to make them look like any other function call.

Indeed, even for the correct definition of non-debug assert ((void)0) you would need a semicolon after it to ensure the next function call was correctly parsed. (eg, this: “((void)0) func();” should fail to compile).

But I am COMPLETELY CERTAIN that putting an assert() inside of an if
statement without also placing it in a Basic Block totally broke my
VC++ code. The program I was integrating my library with was a
financial package that produced graphs. If I graphed the output from
both my library and the client’s old library, with my assert inside
the if, those two graphs subtly diverged. When I fixed my code to use
a Basic Block instead, the two graphs coincided perfectly.

It’d be really interesting to see the repro so we could see what we’re actually dealing with/trying to address.

I still have the Visual Studio 7.0 installer. I’m not sure but I
think 7.1 is an update one can download from Microsoft’s site. I
could install them both then just have a look at <assert.h> to see how
it is defined.

I’d love to see what was going on.

Even if all modern compilers do the right thing, I assert (!) it would
still be useful to write the analysis, as it would catch portability
problems. There are lots of coders who still use broken old
compilers. I still hear about people using Visual Studio 6!

Certainly they are, though it might be a lesser priority for clang to try to address such broken standard libraries.

As for the general problem of non-debug builds possibly changing control flow that could be an interesting puzzle (& even in the presence of well behaved standard libraries it could still be a problem:

if (foo)
#ifndef NDEBUG
printDebugInfo();
#endif
doImportantProductionWork();

) for the static analyzer to possibly play with, but I do wonder if it might produce a lot of false positives on code that might be legitimately trying to change behavior under debug builds in some way or another. Might require a fair few heuristics to reduce that noise - though that’s not necessarily a reason not to try it out & see what it looks like.

  • David