[PATCH] Division by zero

[Anders Rönnholm]

Ping
________________________________________
Från: Anders Rönnholm
Skickat: den 16 maj 2014 13:15
Till: Jordan Rose
Cc: cfe-commits at cs.uiuc.edu; Daniel Marjamäki
Ämne: Re: [PATCH] Division by zero

< 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.
Added as value, couldn't get it to work as key.


< +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).

<  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.

Ok, only blockid exposed now


<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.)

I have added a couple of inline functions.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: divzero2.diff
Type: text/x-patch
Size: 16881 bytes
Desc: divzero2.diff
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140523/8a9efb81/attachment.bin>