[PATCH] [StaticAnalyzer] Add 'clang_analyzer_warnIfReached'
Anna Zaks
ganna at apple.com
Tue Oct 1 09:26:35 PDT 2013
On Oct 1, 2013, at 9:13 AM, Jordan Rose <jordan_rose at apple.com> wrote:
> 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.)
>
I like keeping them short - one word if possible. It's easy to inspect the output and consistent with what we are testing in the tests.
Saying "INLINED" instead of "TRUE" would be an improvement of cause :)
Anna.
> Jordan
>
>
> On Oct 1, 2013, at 9:10 , Anna Zaks <ganna at apple.com> 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 <jared.grubb at gmail.com> 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 <jordan_rose at apple.com> 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 <jared.grubb at gmail.com> 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
>>>>> cfe-commits at cs.uiuc.edu
>>>>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>>>>
>>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20131001/81010eb1/attachment.html>
More information about the cfe-commits
mailing list