[PATCH] Division by zero

[Jordan Rose]

I don't understand why the DivZeroMap uses a MemRegion as a key at all. In this patch you never look things up by MemRegion. I would expect it to be a map from SymbolRef to block ID.

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

I'm not sure why you only update the DivZeroMap with the first BlockID that gets used. If I do a division operation, and then I do another one in another block, and then I do a check of the variable, shouldn't that still count as a reversed check?

Smaller comments:

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

+  if (ExplodedNode *N = C.generateSink(C.getState())) {
+    if (!BB)
+      BB.reset(new BuiltinBug(this, "Division by zero"));

Before this checker goes non-alpha, we definitely need a visitor that says where the most recent division or remainder operation happened.

+      if (IntLiteral && IntLiteral->getValue() != 0)
+        return;

This check should be "if (!IntLiteral || IntLiteral->getValue() != 0)".

+        reportBug("Comparison is useless, "
+                  "or there is division by zero previously",

Now that the checker is restricted to a guaranteed check-after-division, we can probably improve the error message to be more precise. "Value being compared against zero has already been used for division" or something.

Finally, please include some test cases with inlining so that the clear-at-end-of-function logic is exercised.

Thanks for working on this, and sorry for the slow responses.
Jordan


On Apr 24, 2014, at 7:50 , Anders Rönnholm <Anders.Ronnholm at evidente.se> wrote:

> Hi Jordan,
> 
> I have changed the patch to store $foo instead of the variable. I also make sure that the divison and check is in the same block by comparing the blockid. This handles your second example.
> 
> //Anders
> 
> ________________________________________
> Från: Jordan Rose [jordan_rose at apple.com]
> Skickat: den 5 april 2014 04:03
> Till: Anders Rönnholm
> Cc: cfe-commits at cs.uiuc.edu; Daniel Marjamäki
> Ämne: Re: [PATCH] Division by zero
> 
> On Apr 3, 2014, at 7:18 , Anders Rönnholm <Anders.Ronnholm at evidente.se<mailto:Anders.Ronnholm at evidente.se>> wrote:
> 
> Hi Jordan, see below for comments and questions.
> 
> < Hi, Anders. I have some high-level concerns about this checker as it's structured now. First (and most mundanely), the checker is conflating values and locations. A particular symbol in the analyzer can  <never change, so there's no reason to ever have to remove something from the DivZeroMap because of invalidation.
> <
> < int x = foo(); // The value stored in 'x' is some symbol '$foo'
> < int y = 5 / x; // use $foo
> < x = bar(); // The value stored in 'x' is now some symbol '$bar', but '$foo' hasn't changed.
> 
> < On the flip side, I'm concerned that values never leave the map. You could clean up some of them with a checkDeadSymbols callback, but that's still a large amount of state for values that may have been < checked quite a while ago. In particular, this will result in false positives if a value is used in one function and then checked in another.
> 
> Why don't we have to remove it? When x changes as you say we need to remove it or flag it so we don't check x against zero anymore since x has changed after the division.
> 
> I guess we could remove all items in DivZeroMap when we leave the function, do you think that would work?
> 
> "When 'x' changes" is the interesting part. '$foo' can't change, and that's what you're inserting into the map. The value stored in 'x' can change, and that's why 'x' is being removed from the map. But 'x' should never be in the map in the first place; it's '$foo', the value, that's important.
> 
> Here's another example:
> 
> int x = foo();
> int y = x;
> 
> use(5 / x);
> if (y == 0) // warn!
> 
> If we track the variable 'x', then we won't catch this case, even though it's pretty much equivalent to what you had before.
> 
> Here's another problem with doing things this way: doing this as a path-sensitive check says that there's a spurious comparison along one path, but you really need to know if it happens along all paths:
> 
> void foo() {
> bool isPositive;
> int x = getValue(&isPositive);
> if (isPositive) {
> use(5 / x);
> }
> 
> if (x == 0) {
> log("x is zero");
> }
> }
> 
> In this case, there exists a path where 'x' checked against 0 after being used as a denominator, but it's not actually a problem. You can argue that the test should be moved up before the use, or that an assertion should be added, but I don't know if we want to tackle that. OTOH, you would have to add an assertion if you did move the test before the use.
> 
> I like the idea of clearing the map/set on function exit, because it's somewhat reasonable to add asserts within a function...but you'd basically also have to clear it on function entrance too. Which means you have one set per LocationContext. I haven't fully thought that through yet.
> 
> 
> 
> <
> < (The opposite order hit us so much when dealing with C++ support that we coined a term for it: "inlined defensive checks". This occurs when one function accepts null pointers and so does a check, the   <caller knows the value is never null but doesn't actually assert it, and then the value is used. We ended up having to silence all bugs where the first null check came from an inlined function, which  <definitely through out some true positives with the false ones.)
> <
> < What do you think?
> < Jordan
> <
> < P.S. If your map entries always map to "true", you might as well use a set. ;-)
> 
> Is it possible to get a specific entry using set or do you have to iterate through all items to find the one your looking for?
> 
> With map you can do this,:
> const bool D = State->get<DivZeroMap>(MR);
> 
> but with set i'm only able to get them all.
> DivZeroMapTy DivZeroes = State->get<DivZeroMap>();
> 
> For future reference, set traits get a State->contains<DivZeroMap>().
> 
> Jordan
> <divzero2.diff>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140428/fb5991af/attachment.html>