[cfe-commits] New analyzer checker: CStringChecker

[Jordy Rose]

On Tue, 29 Jun 2010 17:54:13 -0700, Ted Kremenek <kremenek at apple.com>
wrote:
> Hi Jordy,
> 
> Overall this looks great.  Couple comments:
> 
> +    // FIXME: It would be nice to eventually make this diagnostic more
> clear,
> +    // e.g., by referencing the original declaration or by saying *why*
> this
> +    // reference is outside the range.
> 
> Generally we try and enhance bug reports by reporting the details along
> the path instead of summarizing them all at the end.  The apparatus for
> doing this right now isn't great, but the idea is to use a
> BugReporterVisitor class that acts as a callback from BugReporter when
> BugReporter has selected the path to present to the user.

This was copied verbatim from the original method in ArrayBoundChecker. In
the case of memcpy() and friends it's actually less useful, since the
argument might be in bounds but the last element out of bounds. Is it worth
trying to pull a DeclRefExpr out of an expression and show where it came
from, or perhaps highlight where the ElementRegion's base region came from?


> You also made the following comment:
> 
> +// FIXME: This was originally copied from ArrayBoundChecker.cpp.
Refactor?
> 
> I absolutely agree, although I'm fine with a check going in like this
> first and then refactoring.  There is a bunch of logic here with
tremendous
> overlap with ArrayBoundChecker.  Perhaps we should build a more generic
> "out-of-bounds detection" API, and make ArrayBoundChecker a
"meta-checker".
> Not sure.

The main issue is that the bounds check shouldn't really emit the bug on
its own. But that leaves it in an awkward place, interface-wise. Does it
return a pair of StInBound and StOutBound, and force the caller to do the
feasibility check? Does it return a bool, and supply the necessary state by
reference? It's too bad we need a state whether the check fails or not.

I wish we could use blocks; I'd just pass in a "bug-reporting" block that
takes an ExplodedNode. Maybe it should take a function pointer?

Maybe we should leave it as is? *grin*