[PATCH] Division by zero

[Jordan Rose]

On Apr 30, 2014, at 3:54 , Anders Rönnholm <Anders.Ronnholm at evidente.se> wrote:

> < Clearing the map at the end of a function is a start, but you ought to make sure you don't clear it for the function that called this one. I suppose that's a refinement that can be made  later, since it only produces false negatives. (I'm not super happy about not being able to merge branches within the <function, though.)
> 
> No I don't like it either and would like to fix it but I haven't figured out how. Can I find out the current function somehow when the division occurs?

I would just put the current StackFrameContext in as part of the key for the map, or make a two-tiered map. Then when you hit the end of the function, you can just drop things that match the current StackFrameContext.

> 
> < +unsigned DivZeroChecker2::getBlockID(const Expr *B, CheckerContext &C) const {
> < +  const CFG &cfg = C.getPredecessor()->getCFG();
> < +  for (CFG::const_iterator I = cfg.begin(); I != cfg.end(); ++I) {
> < +    for (CFGBlock::const_iterator BI = (*I)->begin(), BE = (*I)->end();
> < +         BI != BE; ++BI) {
> < +      Optional<CFGStmt> stmt = BI->getAs<CFGStmt>();
> < +      if (stmt && stmt->getStmt() == B)
> < +        return (*I)->getBlockID();
> < +    }
> < +  }
> < +  return 0;
> < +}
> 
> < This is much too expensive. The CheckerContext has a NodeBuilder, which has a NodeBuilderContext, which has a CFGBlock. We should just expose that somewhere (probably on CheckerContext).
> 
> I have made a get function in CheckerContext to retrieve the NodeBuilder. Is there something better than blockid? Scope id would have been nice. Blockid creates false negatives like below when an if-statement that has nothing to do with the division comes in between.
> 
> void err_not(int x, int y) {
>  var = 77 / x;
> 
>  if (y)
>    int v = 0;
> 
>  if (!x){}// should warn here but don't since this is a new blockid.
> }

We don't currently track scopes, but that still wouldn't eliminate all the false positives. I guess it would be closer, though—some day if we start tracking scopes again, we can switch to that.

I don't think it makes sense to expose the whole NodeBuilder through the CheckerContext, just the block ID.


> < Finally, please include some test cases with inlining so that the clear-at-end-of-function logic is exercised.
> 
> What's the difference with inline and normal functions in this case? As I have understood it inline functions are still treated like normal functions when walking the ast.

When doing path-sensitive checks, the analyzer can choose to inline any function call that it can see the definition for. I want to make sure that we still detect test-after-uses when the test and use are both within the inlined function and when there is an inlined function call between the test and use in the top-most function. (The latter would hit the first issue I noted, that clearing the entire state at the end of a function is a bit too destructive.)

Jordan
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140501/07439d74/attachment.html>