[PATCH] [StaticAnalyzer] Add 'clang_analyzer_warnIfReached'
Jordan Rose
jordan_rose at apple.com
Mon Sep 16 18:25:52 PDT 2013
[+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
More information about the cfe-commits
mailing list