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.)
Jordan On Oct 1, 2013, at 9:10 , Anna Zaks <[email protected]> wrote: > I think the following would be the most consistent with the rest of the debug > checkers: > - BugReport *R = new BugReport(*BT, "REACHABLE", N); > + BugReport *R = new BugReport(*BT, "Analyzer reached this line > (WARN-REACHED)", N); > > Cheers, > Anna. > > On Sep 30, 2013, at 8:40 PM, Jared Grubb <[email protected]> wrote: > >> Sorry for the delay. My free time has been limited lately. >> >> From your comments, the only change you suggested was the capitalization: >> - clang_analyzer_warnIfReached(); // expected-warning{{reached}} >> + clang_analyzer_warnIfReached(); // expected-warning{{REACHED}} >> >> Attached are the minor changes required to make that work. Unit tests >> continue to all pass. >> >> <patch-warnIfReached.diff> >> >> >> Jared >> >> On Sep 16, 2013, at 18:25, Jordan Rose <[email protected]> wrote: >> >>> [+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
