[PATCH] [StaticAnalyzer] Add 'clang_analyzer_warnIfReached'
Jordan Rose
jordan_rose at apple.com
Thu Oct 3 10:00:31 PDT 2013
Thanks, Jared! Committed in r191909.
On Oct 2, 2013, at 23:40 , Jared Grubb <jared.grubb at gmail.com> wrote:
> Sounds good. I changed the report to use just "REACHABLE", as well as the changes in the unit test. Tests pass for me still.
>
> Thanks,
> Jared
>
>
>
> On Oct 2, 2013, at 9:03, Jordan Rose <jordan_rose at apple.com> wrote:
>
>> Let's go with the single word, then.
>>
>> Jordan
>>
>>
>> On Oct 1, 2013, at 20:25 , Jared Grubb <jared.grubb at gmail.com> wrote:
>>
>>> I agree they should all be consistent.
>>>
>>> Whether this one should be a single word, or whether we fill out phrases for the others, I dont have any strong opinion. I'll defer to you both on whatever you both prefer and am happy to adjust the patch as you like.
>>>
>>> Jared
>>>
>>> On Oct 1, 2013, at 9:29, Anna Zaks <ganna at apple.com> wrote:
>>>
>>>> Jordan has just pointed out that I got the +/- backwards below. I suggest this:
>>>>
>>>> BugReport *R = new BugReport(*BT, "REACHABLE", N);
>>>>
>>>> Cheers,
>>>> Anna.
>>>> On Oct 1, 2013, at 9:10 AM, 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
>>>>>>>
>>>>>>
>>>>>
>>>>> _______________________________________________
>>>>> cfe-commits mailing list
>>>>> cfe-commits at cs.uiuc.edu
>>>>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>>>>
>>>
>>> _______________________________________________
>>> 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/20131003/6978a35e/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: patch-warnIfReached.diff
Type: application/octet-stream
Size: 8234 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20131003/6978a35e/attachment.obj>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20131003/6978a35e/attachment-0001.html>
More information about the cfe-commits
mailing list