[+Anna for opinions] Two comments:
- I think we should keep the ExprInspection output in all caps, to make it clear that it's not a usual analyzer warning - 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. I think you're right that it's better to be explicit, even though it's functionally equivalent to constructs we have already. Anna? Jordan On Sep 16, 2013, at 9:07 , Jared Grubb <[email protected]> wrote: > 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. > > Looking through the unit test, this kind of check is done in various ways: > * Intentional deref of null pointer (the most popular, apparently) > * Intentional divide by zero (tests in "cast.c") > * "clang_analyzer_eval(true); // expected-warning{{TRUE}}" > > 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. > > Unit tests continue to pass. The direct patch to the analyzer is very minimal. > > Jared > > > diff --git a/docs/analyzer/DebugChecks.rst b/docs/analyzer/DebugChecks.rst > index a0f2a07..b0983e8 100644 > --- a/docs/analyzer/DebugChecks.rst > +++ b/docs/analyzer/DebugChecks.rst > @@ -125,6 +125,19 @@ ExprInspection checks > clang_analyzer_eval(value == 42); // expected-warning{{TRUE}} > } > > +- void clang_analyzer_warnIfReached(); > + > + Generate a warning if this line of code gets reached by the analyzer. > + > + Example usage:: > + > + if (true) { > + clang_analyzer_warnIfReached(); // expected-warning{{reached}} > + } > + else { > + clang_analyzer_warnIfReached(); // no-warning > + } > + > > Statistics > ========== > diff --git a/lib/StaticAnalyzer/Checkers/ExprInspectionChecker.cpp > b/lib/StaticAnalyzer/Checkers/ExprInspectionChecker.cpp > index 9522d1d..53f7c3d 100644 > --- a/lib/StaticAnalyzer/Checkers/ExprInspectionChecker.cpp > +++ b/lib/StaticAnalyzer/Checkers/ExprInspectionChecker.cpp > @@ -22,6 +22,7 @@ class ExprInspectionChecker : public Checker< eval::Call > { > > void analyzerEval(const CallExpr *CE, CheckerContext &C) const; > void analyzerCheckInlined(const CallExpr *CE, CheckerContext &C) const; > + void analyzerWarnIfReached(const CallExpr *CE, CheckerContext &C) const; > void analyzerCrash(const CallExpr *CE, CheckerContext &C) const; > > typedef void (ExprInspectionChecker::*FnCheck)(const CallExpr *, > @@ -41,6 +42,7 @@ bool ExprInspectionChecker::evalCall(const CallExpr *CE, > .Case("clang_analyzer_checkInlined", > &ExprInspectionChecker::analyzerCheckInlined) > .Case("clang_analyzer_crash", &ExprInspectionChecker::analyzerCrash) > + .Case("clang_analyzer_warnIfReached", > &ExprInspectionChecker::analyzerWarnIfReached) > .Default(0); > > if (!Handler) > @@ -99,6 +101,17 @@ void ExprInspectionChecker::analyzerEval(const CallExpr > *CE, > C.emitReport(R); > } > > +void ExprInspectionChecker::analyzerWarnIfReached(const CallExpr *CE, > + CheckerContext &C) const { > + ExplodedNode *N = C.getPredecessor(); > + > + if (!BT) > + BT.reset(new BugType("Checking analyzer assumptions", "debug")); > + > + BugReport *R = new BugReport(*BT, "Analyzer reached this line", N); > + C.emitReport(R); > +} > + > void ExprInspectionChecker::analyzerCheckInlined(const CallExpr *CE, > CheckerContext &C) const { > ExplodedNode *N = C.getPredecessor(); > diff --git a/test/Analysis/misc-ps-region-store.cpp > b/test/Analysis/misc-ps-region-store.cpp > index 902a5e5..c00d357 100644 > --- a/test/Analysis/misc-ps-region-store.cpp > +++ b/test/Analysis/misc-ps-region-store.cpp > @@ -1,5 +1,7 @@ > -// 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 > -// 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 > +// 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 > +// 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 > + > +void clang_analyzer_warnIfReached(); > > // Test basic handling of references. > char &test1_aux(); > @@ -54,9 +56,7 @@ int test_init_in_condition_switch() { > if (x == 2) > return 0; > else { > - // Unreachable. > - int *p = 0; > - *p = 0xDEADBEEF; // no-warning > + clang_analyzer_warnIfReached(); // unreachable > } > default: > break; > @@ -74,8 +74,7 @@ int test_init_in_condition_while() { > if (z == 2) > return 0; > > - int *p = 0; > - *p = 0xDEADBEEF; // no-warning > + clang_analyzer_warnIfReached(); // unreachable > return 0; > } > > @@ -89,8 +88,7 @@ int test_init_in_condition_for() { > if (z == 1) > return 0; > > - int *p = 0; > - *p = 0xDEADBEEF; // no-warning > + clang_analyzer_warnIfReached(); // unreachable > return 0; > } > > @@ -117,8 +115,7 @@ int TestHandleThis::null_deref_negative() { > if (x == 10) { > return 1; > } > - int *p = 0; > - *p = 0xDEADBEEF; // no-warning > + clang_analyzer_warnIfReached(); // unreachable > return 0; > } > > @@ -127,8 +124,7 @@ int TestHandleThis::null_deref_positive() { > if (x == 9) { > return 1; > } > - int *p = 0; > - *p = 0xDEADBEEF; // expected-warning{{null pointer}} > + clang_analyzer_warnIfReached(); // expected-warning{{reached}} > return 0; > } > > @@ -143,9 +139,9 @@ void pr7675_test() { > pr7675(10); > pr7675('c'); > pr7675_i(4.0i); > - // Add null deref to ensure we are analyzing the code up to this point. > - int *p = 0; > - *p = 0xDEADBEEF; // expected-warning{{null pointer}} > + > + // Add check to ensure we are analyzing the code up to this point. > + clang_analyzer_warnIfReached(); // expected-warning{{reached}} > } > > // <rdar://problem/8375510> - CFGBuilder should handle temporaries. > @@ -328,26 +324,23 @@ class RDar9267815 { > }; > > void RDar9267815::test_pos() { > - int *p = 0; > if (x == 42) > return; > - *p = 0xDEADBEEF; // expected-warning {{null}} > + clang_analyzer_warnIfReached(); // expected-warning{{reached}} > } > void RDar9267815::test() { > - int *p = 0; > if (x == 42) > return; > if (x == 42) > - *p = 0xDEADBEEF; // no-warning > + clang_analyzer_warnIfReached(); // no-warning > } > > void RDar9267815::test2() { > - int *p = 0; > if (x == 42) > return; > invalidate(); > if (x == 42) > - *p = 0xDEADBEEF; // expected-warning {{null}} > + clang_analyzer_warnIfReached(); // expected-warning{{reached}} > } > > // Test reference parameters. > @@ -440,8 +433,7 @@ int rdar9948787_negative() { > unsigned value = classWithStatic.value; > if (value == 1) > return 1; > - int *p = 0; > - *p = 0xDEADBEEF; // no-warning > + clang_analyzer_warnIfReached(); // no-warning > return 0; > } > > @@ -450,8 +442,7 @@ int rdar9948787_positive() { > unsigned value = classWithStatic.value; > if (value == 0) > return 1; > - int *p = 0; > - *p = 0xDEADBEEF; // expected-warning {{null}} > + clang_analyzer_warnIfReached(); // expected-warning{{reached}} > return 0; > } > > @@ -467,8 +458,7 @@ void rdar10202899_test1() { > void rdar10202899_test2() { > if (val == rdar10202899_ValTA) > return; > - int *p = 0; > - *p = 0xDEADBEEF; > + clang_analyzer_warnIfReached(); // no-warning > } > > void rdar10202899_test3() { > @@ -476,8 +466,7 @@ void rdar10202899_test3() { > case rdar10202899_ValTA: return; > default: ; > }; > - int *p = 0; > - *p = 0xDEADBEEF; > + clang_analyzer_warnIfReached(); // no-warning > } > > // This used to crash the analyzer because of the unnamed bitfield. > @@ -489,13 +478,12 @@ void PR11249() > char f2[1]; > char f3; > } V = { 1, {2}, 3 }; > - int *p = 0; > if (V.f1 != 1) > - *p = 0xDEADBEEF; // no-warning > + clang_analyzer_warnIfReached(); // no-warning > if (V.f2[0] != 2) > - *p = 0xDEADBEEF; // no-warning > + clang_analyzer_warnIfReached(); // no-warning > if (V.f3 != 3) > - *p = 0xDEADBEEF; // no-warning > + clang_analyzer_warnIfReached(); // no-warning > } > > // Handle doing a load from the memory associated with the code for > @@ -599,12 +587,10 @@ void rdar10924675(unsigned short x[], int index, int > index2) { > void rdar11401827() { > int x = int(); > if (!x) { > - int *p = 0; > - *p = 0xDEADBEEF; // expected-warning {{null pointer}} > + clang_analyzer_warnIfReached(); // expected-warning{{reached}} > } > else { > - int *p = 0; > - *p = 0xDEADBEEF; > + clang_analyzer_warnIfReached(); // no-warning > } > } > > @@ -701,8 +687,7 @@ const Rdar12755044_foo *radar12755044() { > void rdar12759044() { > int flag = 512; > if (!(flag & 512)) { > - int *p = 0; > - *p = 0xDEADBEEF; // no-warning > + clang_analyzer_warnIfReached(); // no-warning > } > } > > > _______________________________________________ > cfe-commits mailing list > [email protected] > http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
