Let's go with the single word, then.
Jordan
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 Jordan has just pointed out that I got the +/- backwards below. I suggest this: BugReport *R = new BugReport(*BT, "REACHABLE", N);
Cheers, Anna. 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.
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.
Jared
[+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
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
|
patch-warnIfReached.diff