[PATCH] Division by zero

[Jordan Rose]

Okay, this definitely looks promising! I still see some issues, though.

+inline void inline_func3(int x) {
+  var = 77 / x;
+}
+void ok_inline(int x) {
+  var = 77 / x;
+  inline_func3(x);
+  if (x==0){}
+}

This is an example where we should still be warning. Why aren't we?

+// RUN: %clang_cc1 -analyze -analyzer-checker=core.DivideZero2 -analyzer-output=text -verify %s

We should never be running path-sensitive checks without the rest of the core checkers.


On the code itself (mostly small things):

+struct ZeroState {
+private:

Small style note: this should probably be a class. The LLVM coding standards prefer to use 'struct' only when all members are public. http://llvm.org/docs/CodingStandards.html#use-of-class-and-struct-keywords


+  unsigned B;

I'd appreciate this having a more descriptive name, like a full "BlockID".


+class DivisionBRVisitor : public BugReporterVisitorImpl<DivisionBRVisitor> {
+protected:

Why "protected"?


+  DivisionBRVisitor(KnownSVal V) : V(V), Satisfied(false) {}

Now that we're using C++11, it probably makes more sense to move the initialization of "Satisfied" into the member declaration itself. The constructor should also be marked explicit.

(But good job making the visitor at all. This makes the error report much more helpful!)


+  mutable std::unique_ptr<BuiltinBug> BB;

The name of this variable should describe the bug, not just be the type.


+bool DivZeroChecker2::hasDivZeroMapSval(SVal Var,
+                                        const CheckerContext &C) const {

The "SVal" part of this really isn't important, so I would drop it from the name. But...

+  const ZeroState *ZS = C.getState()->get<DivZeroMap>(SR);
+  return (ZS && *ZS == ZeroState(C.getBlockID(), C.getStackFrame()));

...now I'm wondering about the wisdom of using a map for this. What if the same symbol is constrained in two stack frames? Would it make more sense to keep around a set of symbol-block-frame triples, rather than a map from symbols to block/frame pairs?

Or can this not happen because a value can only be used once along a certain path before it is constrained to be non-zero?

+  } else if (const ImplicitCastExpr *IE =
+                 dyn_cast<ImplicitCastExpr>(Condition)) {
+    SVal Val = C.getSVal(IE->getSubExpr());
+
+    if (hasDivZeroMapSval(Val, C))
+      reportBug("Value being compared against zero has already been used for "
+                "division",
+                Val, C);
+  }

I'm not sure there's always an ImplicitCastExpr. There is in C++, of course, but I don't think C uses an integer-to-boolean conversion for if-statements. I suggest renaming the test to div-zero2.c, and then having two RUN lines, with the second including "-x c++".


+def DivZeroChecker2 : Checker<"DivideZero2">,
+  HelpText<"Check for division by variable that is later compared against 0. Either the comparison is useless or there is division by zero.">,
+  DescFile<"DivZeroChecker2.cpp">;
+  

I'm not sure this is really a core checker, since it's safe to run the analyzer without it. But we don't have a category for "useful C checks that should be on by default" yet. Anton was looking at reorganizing the categories, but the problem is that everything outside of "alpha" is something people expect to be able to rely on when using scan-build.

Any ideas for this particular checker, or for the general issue?

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