<html><head><meta http-equiv="Content-Type" content="text/html charset=us-ascii"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><div>Should we change all the debug checkers to include an explanation? Right now they all just say "TRUE" or "FALSE" or "tainted". (The inlined checker saying "TRUE" is a bit funny, sure.)</div><div><br></div><div>Jordan</div><div><br></div><br><div><div>On Oct 1, 2013, at 9:10 , Anna Zaks <<a href="mailto:ganna@apple.com">ganna@apple.com</a>> wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><meta http-equiv="Content-Type" content="text/html charset=us-ascii"><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;">I think the following would be the most consistent with the rest of the debug checkers:<br><div>- BugReport *R = new BugReport(*BT, "REACHABLE", N);<div><div>+ BugReport *R = new BugReport(*BT, "Analyzer reached this line (WARN-REACHED)", N);</div><div><br></div><div>Cheers,</div><div>Anna.</div><div><br></div><div><div>On Sep 30, 2013, at 8:40 PM, Jared Grubb <<a href="mailto:jared.grubb@gmail.com">jared.grubb@gmail.com</a>> wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><meta http-equiv="Content-Type" content="text/html charset=us-ascii"><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;"><div>Sorry for the delay. My free time has been limited lately.</div><div><br></div><div>From your comments, the only change you suggested was the capitalization:</div><div><div><font face="Courier New">- clang_analyzer_warnIfReached(); // expected-warning{{reached}}</font></div><div><font face="Courier New">+ clang_analyzer_warnIfReached(); // expected-warning{{REACHED}}</font></div></div><div><br></div><div>Attached are the minor changes required to make that work. Unit tests continue to all pass.</div><div><br></div></div><span><patch-warnIfReached.diff></span><meta http-equiv="Content-Type" content="text/html charset=us-ascii"><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;"><br><div><br></div><div>Jared</div><div><br></div><div><div><div>On Sep 16, 2013, at 18:25, Jordan Rose <<a href="mailto:jordan_rose@apple.com">jordan_rose@apple.com</a>> wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite">[+Anna for opinions]<br><br>Two comments:<br><br>- I think we should keep the ExprInspection output in all caps, to make it clear that it's not a usual analyzer warning<br>- The null-pointer deref tests also halt analysis along that path. I don't think that's something we usually need to do, but there might be a few cases where it's important. We could probably just add a return in most cases, though.<br><br>I think you're right that it's better to be explicit, even though it's functionally equivalent to constructs we have already. Anna?<br><br>Jordan<br><br><br>On Sep 16, 2013, at 9:07 , Jared Grubb <<a href="mailto:jared.grubb@gmail.com">jared.grubb@gmail.com</a>> wrote:<br><br><blockquote type="cite">While working on unit tests for a checker, I really wanted a facility to detect that the analyzer has (or has not) made certain branch choices. <br><br>Looking through the unit test, this kind of check is done in various ways:<br>* Intentional deref of null pointer (the most popular, apparently)<br>* Intentional divide by zero (tests in "cast.c")<br>* "clang_analyzer_eval(true); // expected-warning{{TRUE}}"<br><br>Adding a new directive to explicitly serve this purpose helps the readability of unit tests. As proof of concept, I've modified an existing test to remove the fake null-pointer derefs.<br><br>Unit tests continue to pass. The direct patch to the analyzer is very minimal.<br><br>Jared<br><br><br>diff --git a/docs/analyzer/DebugChecks.rst b/docs/analyzer/DebugChecks.rst<br>index a0f2a07..b0983e8 100644<br>--- a/docs/analyzer/DebugChecks.rst<br>+++ b/docs/analyzer/DebugChecks.rst<br>@@ -125,6 +125,19 @@ ExprInspection checks<br> clang_analyzer_eval(value == 42); // expected-warning{{TRUE}}<br> }<br><br>+- void clang_analyzer_warnIfReached();<br>+<br>+ Generate a warning if this line of code gets reached by the analyzer.<br>+<br>+ Example usage::<br>+<br>+ if (true) {<br>+ clang_analyzer_warnIfReached(); // expected-warning{{reached}}<br>+ }<br>+ else {<br>+ clang_analyzer_warnIfReached(); // no-warning<br>+ }<br>+<br><br>Statistics<br>==========<br>diff --git a/lib/StaticAnalyzer/Checkers/ExprInspectionChecker.cpp b/lib/StaticAnalyzer/Checkers/ExprInspectionChecker.cpp<br>index 9522d1d..53f7c3d 100644<br>--- a/lib/StaticAnalyzer/Checkers/ExprInspectionChecker.cpp<br>+++ b/lib/StaticAnalyzer/Checkers/ExprInspectionChecker.cpp<br>@@ -22,6 +22,7 @@ class ExprInspectionChecker : public Checker< eval::Call > {<br><br> void analyzerEval(const CallExpr *CE, CheckerContext &C) const;<br> void analyzerCheckInlined(const CallExpr *CE, CheckerContext &C) const;<br>+ void analyzerWarnIfReached(const CallExpr *CE, CheckerContext &C) const;<br> void analyzerCrash(const CallExpr *CE, CheckerContext &C) const;<br><br> typedef void (ExprInspectionChecker::*FnCheck)(const CallExpr *,<br>@@ -41,6 +42,7 @@ bool ExprInspectionChecker::evalCall(const CallExpr *CE,<br> .Case("clang_analyzer_checkInlined",<br> &ExprInspectionChecker::analyzerCheckInlined)<br> .Case("clang_analyzer_crash", &ExprInspectionChecker::analyzerCrash)<br>+ .Case("clang_analyzer_warnIfReached", &ExprInspectionChecker::analyzerWarnIfReached)<br> .Default(0);<br><br> if (!Handler)<br>@@ -99,6 +101,17 @@ void ExprInspectionChecker::analyzerEval(const CallExpr *CE,<br> C.emitReport(R);<br>}<br><br>+void ExprInspectionChecker::analyzerWarnIfReached(const CallExpr *CE,<br>+ CheckerContext &C) const {<br>+ ExplodedNode *N = C.getPredecessor();<br>+<br>+ if (!BT)<br>+ BT.reset(new BugType("Checking analyzer assumptions", "debug"));<br>+<br>+ BugReport *R = new BugReport(*BT, "Analyzer reached this line", N);<br>+ C.emitReport(R);<br>+}<br>+<br>void ExprInspectionChecker::analyzerCheckInlined(const CallExpr *CE,<br> CheckerContext &C) const {<br> ExplodedNode *N = C.getPredecessor();<br>diff --git a/test/Analysis/misc-ps-region-store.cpp b/test/Analysis/misc-ps-region-store.cpp<br>index 902a5e5..c00d357 100644<br>--- a/test/Analysis/misc-ps-region-store.cpp<br>+++ b/test/Analysis/misc-ps-region-store.cpp<br>@@ -1,5 +1,7 @@<br>-// RUN: %clang_cc1 -triple i386-apple-darwin9 -analyze -analyzer-checker=core,alpha.core -analyzer-store=region -verify -fblocks -analyzer-opt-analyze-nested-blocks %s -fexceptions -fcxx-exceptions<br>-// RUN: %clang_cc1 -triple x86_64-apple-darwin9 -analyze -analyzer-checker=core,alpha.core -analyzer-store=region -verify -fblocks -analyzer-opt-analyze-nested-blocks %s -fexceptions -fcxx-exceptions<br>+// RUN: %clang_cc1 -triple i386-apple-darwin9 -analyze -analyzer-checker=core,alpha.core,debug.ExprInspection -analyzer-store=region -verify -fblocks -analyzer-opt-analyze-nested-blocks %s -fexceptions -fcxx-exceptions<br>+// RUN: %clang_cc1 -triple x86_64-apple-darwin9 -analyze -analyzer-checker=core,alpha.core,debug.ExprInspection -analyzer-store=region -verify -fblocks -analyzer-opt-analyze-nested-blocks %s -fexceptions -fcxx-exceptions<br>+<br>+void clang_analyzer_warnIfReached();<br><br>// Test basic handling of references.<br>char &test1_aux();<br>@@ -54,9 +56,7 @@ int test_init_in_condition_switch() {<br> if (x == 2)<br> return 0;<br> else {<br>- // Unreachable.<br>- int *p = 0;<br>- *p = 0xDEADBEEF; // no-warning<br>+ clang_analyzer_warnIfReached(); // unreachable<br> }<br> default:<br> break;<br>@@ -74,8 +74,7 @@ int test_init_in_condition_while() {<br> if (z == 2)<br> return 0;<br><br>- int *p = 0;<br>- *p = 0xDEADBEEF; // no-warning<br>+ clang_analyzer_warnIfReached(); // unreachable<br> return 0;<br>}<br><br>@@ -89,8 +88,7 @@ int test_init_in_condition_for() {<br> if (z == 1)<br> return 0;<br><br>- int *p = 0;<br>- *p = 0xDEADBEEF; // no-warning<br>+ clang_analyzer_warnIfReached(); // unreachable<br> return 0;<br>}<br><br>@@ -117,8 +115,7 @@ int TestHandleThis::null_deref_negative() {<br> if (x == 10) {<br> return 1;<br> }<br>- int *p = 0;<br>- *p = 0xDEADBEEF; // no-warning<br>+ clang_analyzer_warnIfReached(); // unreachable<br> return 0;<br>}<br><br>@@ -127,8 +124,7 @@ int TestHandleThis::null_deref_positive() {<br> if (x == 9) {<br> return 1;<br> }<br>- int *p = 0;<br>- *p = 0xDEADBEEF; // expected-warning{{null pointer}}<br>+ clang_analyzer_warnIfReached(); // expected-warning{{reached}}<br> return 0;<br>}<br><br>@@ -143,9 +139,9 @@ void pr7675_test() {<br> pr7675(10);<br> pr7675('c');<br> pr7675_i(4.0i);<br>- // Add null deref to ensure we are analyzing the code up to this point.<br>- int *p = 0;<br>- *p = 0xDEADBEEF; // expected-warning{{null pointer}}<br>+<br>+ // Add check to ensure we are analyzing the code up to this point.<br>+ clang_analyzer_warnIfReached(); // expected-warning{{reached}}<br>}<br><br>// <<a href="rdar://problem/8375510">rdar://problem/8375510</a>> - CFGBuilder should handle temporaries.<br>@@ -328,26 +324,23 @@ class RDar9267815 {<br>};<br><br>void RDar9267815::test_pos() {<br>- int *p = 0;<br> if (x == 42)<br> return;<br>- *p = 0xDEADBEEF; // expected-warning {{null}}<br>+ clang_analyzer_warnIfReached(); // expected-warning{{reached}}<br>}<br>void RDar9267815::test() {<br>- int *p = 0;<br> if (x == 42)<br> return;<br> if (x == 42)<br>- *p = 0xDEADBEEF; // no-warning<br>+ clang_analyzer_warnIfReached(); // no-warning<br>}<br><br>void RDar9267815::test2() {<br>- int *p = 0;<br> if (x == 42)<br> return;<br> invalidate();<br> if (x == 42)<br>- *p = 0xDEADBEEF; // expected-warning {{null}}<br>+ clang_analyzer_warnIfReached(); // expected-warning{{reached}}<br>}<br><br>// Test reference parameters.<br>@@ -440,8 +433,7 @@ int rdar9948787_negative() {<br> unsigned value = classWithStatic.value;<br> if (value == 1)<br> return 1;<br>- int *p = 0;<br>- *p = 0xDEADBEEF; // no-warning<br>+ clang_analyzer_warnIfReached(); // no-warning<br> return 0;<br>}<br><br>@@ -450,8 +442,7 @@ int rdar9948787_positive() {<br> unsigned value = classWithStatic.value;<br> if (value == 0)<br> return 1;<br>- int *p = 0;<br>- *p = 0xDEADBEEF; // expected-warning {{null}}<br>+ clang_analyzer_warnIfReached(); // expected-warning{{reached}}<br> return 0;<br>}<br><br>@@ -467,8 +458,7 @@ void rdar10202899_test1() {<br>void rdar10202899_test2() {<br> if (val == rdar10202899_ValTA)<br> return;<br>- int *p = 0;<br>- *p = 0xDEADBEEF;<br>+ clang_analyzer_warnIfReached(); // no-warning<br>}<br><br>void rdar10202899_test3() {<br>@@ -476,8 +466,7 @@ void rdar10202899_test3() {<br> case rdar10202899_ValTA: return;<br> default: ;<br> };<br>- int *p = 0;<br>- *p = 0xDEADBEEF;<br>+ clang_analyzer_warnIfReached(); // no-warning<br>}<br><br>// This used to crash the analyzer because of the unnamed bitfield.<br>@@ -489,13 +478,12 @@ void PR11249()<br> char f2[1];<br> char f3;<br> } V = { 1, {2}, 3 };<br>- int *p = 0;<br> if (V.f1 != 1)<br>- *p = 0xDEADBEEF; // no-warning<br>+ clang_analyzer_warnIfReached(); // no-warning<br> if (V.f2[0] != 2)<br>- *p = 0xDEADBEEF; // no-warning<br>+ clang_analyzer_warnIfReached(); // no-warning<br> if (V.f3 != 3)<br>- *p = 0xDEADBEEF; // no-warning<br>+ clang_analyzer_warnIfReached(); // no-warning<br>}<br><br>// Handle doing a load from the memory associated with the code for<br>@@ -599,12 +587,10 @@ void rdar10924675(unsigned short x[], int index, int index2) {<br>void rdar11401827() {<br> int x = int();<br> if (!x) {<br>- int *p = 0;<br>- *p = 0xDEADBEEF; // expected-warning {{null pointer}}<br>+ clang_analyzer_warnIfReached(); // expected-warning{{reached}}<br> }<br> else {<br>- int *p = 0;<br>- *p = 0xDEADBEEF;<br>+ clang_analyzer_warnIfReached(); // no-warning<br> }<br>}<br><br>@@ -701,8 +687,7 @@ const Rdar12755044_foo *radar12755044() {<br>void rdar12759044() {<br> int flag = 512;<br> if (!(flag & 512)) {<br>- int *p = 0;<br>- *p = 0xDEADBEEF; // no-warning<br>+ clang_analyzer_warnIfReached(); // no-warning<br> }<br>}<br><br><br>_______________________________________________<br>cfe-commits mailing list<br><a href="mailto:cfe-commits@cs.uiuc.edu">cfe-commits@cs.uiuc.edu</a><br><a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a><br></blockquote><br></blockquote></div><br></div></div></blockquote></div><br></div></div></div></blockquote></div><br></body></html>