<div dir="ltr">On Wed, Jul 10, 2013 at 10:51 AM, Jordan Rose <span dir="ltr"><<a href="mailto:jordan_rose@apple.com" target="_blank">jordan_rose@apple.com</a>></span> wrote:<br><div class="gmail_extra"><div class="gmail_quote">
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div style="word-wrap:break-word"><div>Hi, Matthew. Thanks for working on this -- these are definitely useful checks! One concern I have, though, is that plenty of people assume that comparisons between different objects are not just undefined or unspecified but actually consistent.</div>
</div></blockquote><div><br></div><div>Well, they can assume consistent pointer comparisons just like they can assume signed integer overflow wrapping, but that's not what the language specification says. :)</div><div>
<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div style="word-wrap:break-word"><div> It might be nice to have a separate checker flag for the comparison checks, though the implementation can still be shared in one Checker class. You can see how this works in CStringChecker or IvarInvalidationChecker.</div>
</div></blockquote><div><br></div><div>Okay, I'll look into that.</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div style="word-wrap:break-word"><div><div><div class="h5"><blockquote type="cite">-  // When doing pointer subtraction, if the two pointers do not point to the<br>-  // same memory chunk, emit a warning.<br>-  if (B->getOpcode() != BO_Sub)<br>
+  // When doing pointer subtraction or relational comparisons, if the two<br>+  // pointers do not point to the same memory chunk, emit a warning.<br>+  if (B->getOpcode() != BO_Sub &&<br>+      B->getOpcode() != BO_LT && B->getOpcode() != BO_GT &&<br>
+      B->getOpcode() != BO_LE && B->getOpcode() != BO_GE)<br>     return;<br></blockquote><div><br></div></div></div><div>You can simplify this with B->isRelationalOp().</div></div></div></blockquote><div>
<br></div><div>Thanks, will do.</div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div style="word-wrap:break-word">
<div><div class="im"><blockquote type="cite">+    DefinedOrUnknownSVal NullityMatched = SVB.evalEQ(state,<br>+      SVB.evalEQ(state, *DLV, Null), SVB.evalEQ(state, *DRV, Null));<br></blockquote><div><br></div></div><div>
It would be wonderful if this worked, and indeed it will appear to work for your test cases involving a literal null. However, in the general case evalEQ just constructs an expression "a == b", and can't evaluate that on its own. The constraint manager's what actually does the work.</div>
</div></div></blockquote><div><br></div><div>Yeah.  I found writing the constraint as ((lhs == null) && (rhs == null)) || ((lhs != null) && (rhs != null)) worked better with the constraint manager, but just looked uglier.  I was going to look into if evalEQ like this could be improved, but in a pinch rewriting it would work too.</div>
<div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div style="word-wrap:break-word"><div><div>
A better way to do this would be to use ProgramState::isNull on each value, and checking if one value is constrained to true while the other is false. However, it looks like isNull is actually going to give you the wrong answer for <i>non-</i>symbolic regions, so you'd have to fix that first. Barring that, you should assume() each condition individually and compare the results.<br>
</div></div></div></blockquote><div><br></div><div>I originally assume()'d each variable separately, but I couldn't figure out how to later add an addTransition() to represent that we now know (!lhs == !rhs), because I ended up with two separate program states: one for (lhs && rhs) and one for (!lhs && !rhs).</div>
<div><br></div><div>I suppose it's not the end of the world to lost that information though.</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div style="word-wrap:break-word"><div><div></div><div class="im"><blockquote type="cite">+    ProgramStateRef StMatch = state->assume(NullityMatched, true);<br>+    if (!StMatch) {<br>+      if (ExplodedNode *N = C.addTransition()) {<br>
+        BugReport *R;<br>+        if (B->getOpcode() == BO_Sub) {<br>+          if (!BT_nullsub)<br>+            BT_nullsub.reset(new BuiltinBug("Pointer subtraction", <br>+                                "Subtraction between a null and non-null pointer "<br>
+                                "may cause incorrect result."));<br>+          R = new BugReport(*BT_nullsub, BT_nullsub->getDescription(), N);<br>+        } else {<br>+          if (!BT_nullcmp)<br>+            BT_nullcmp.reset(new BuiltinBug("Pointer comparison", <br>
+                                "Comparison between a null and non-null pointer "<br>+                                "may cause incorrect result."));<br>+          R = new BugReport(*BT_nullcmp, BT_nullcmp->getDescription(), N);<br>
+        }<br>+        R->addRange(B->getSourceRange());<br>+        C.emitReport(R);<br>+      }<br>+      return;<br>+    }<br></blockquote><br><blockquote type="cite">+    if (B->getOpcode() == BO_Sub || !C.getLangOpts().CPlusPlus) {<br>
+      C.addTransition(StMatch);<br></blockquote><div><br></div></div><div>Since you have more work to do, that's actually not a good idea! Each addTransition splits the state space, and we don't actually want to do that. I suggest just updating your 'state' variable, so that the final addTransition operates on the right state.</div>
<div><br></div><div>Basically, you want to make sure we don't actually bifurcate the path as a result of these checks, and at the same time you want to make sure that each report happens after at least one addTransition (so they don't look they're the previous statement's fault).</div>
</div></div></blockquote><div><br></div><div>Sorry, not sure I follow you here.  There aren't any other non-sink transitions that get added by PointerSubChecker, so how does this bifurcate the path?  Can you sketch out how you think this should be done instead and/or point me to another checker to look at for comparison?</div>
<div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div style="word-wrap:break-word"><div><div>
You also are checking for C++ in determining whether or not to record the assumption, but not in whether or not to warn. As tempting as it is, I don't think we can warn in C++ mode for something that's merely unspecified.<br>
</div></div></div></blockquote><div><br></div><div>If clang analyzer's warning conventions are to only warn about undefined behavior and never to warn about merely unspecified behavior, I'm happy to abide that.</div>
<div><br></div><div>My rationale was just we should warn because the result has no specified value in C++, so users are likely to be caught off guard if/when compilers start exploiting that.  E.g., it's conforming for a C++ compiler to optimize this code</div>
<div><br></div><div>    int a, b;</div><div>    return (&a < &b) || (&a == &b) || (&a > &b);</div><div><br></div><div>to merely "return false", but I think users would still be pretty shocked if this happened and find it tedious to track down themselves.</div>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div style="word-wrap:break-word"><div><div><div class="h5">
<blockquote type="cite">+  if (RV.isZeroConstant() || RV.isConstant(1))<br>+    return;<br></blockquote><div><br></div></div></div><div>I think this one should be a separate patch. I see how it's useful (for pretending a single variable is an array), but it's not really part of the first set of checks, and you should get the addition vs. subtraction thing right anyway. (Your TODOs in the test file.)</div>
</div></div></blockquote><div><br></div><div>Will do.</div><div></div></div><br></div><div class="gmail_extra">Thanks for the review!</div></div>