Szelethus updated this revision to Diff 402549.
Szelethus added a comment.
Fix tests, mention that this is purely a heuristic.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D116597/new/
https://reviews.llvm.org/D116597
Files:
clang/include/clang/Analysis/CFG.h
clang/lib/Analysis/CFG.cpp
clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
clang/test/Analysis/return-value-guaranteed.cpp
clang/test/Analysis/track-control-dependency-conditions.cpp
Index: clang/test/Analysis/track-control-dependency-conditions.cpp
===================================================================
--- clang/test/Analysis/track-control-dependency-conditions.cpp
+++ clang/test/Analysis/track-control-dependency-conditions.cpp
@@ -149,14 +149,13 @@
int *x = 0; // expected-note-re{{{{^}}'x' initialized to a null pointer value{{$}}}}
if (ConvertsToBool())
- // debug-note-re@-1{{{{^}}Tracking condition 'ConvertsToBool()'{{$}}}}
- // expected-note-re@-2{{{{^}}Assuming the condition is true{{$}}}}
- // expected-note-re@-3{{{{^}}Taking true branch{{$}}}}
+ // expected-note-re@-1{{{{^}}Assuming the condition is true{{$}}}}
+ // expected-note-re@-2{{{{^}}Taking true branch{{$}}}}
*x = 5; // expected-warning{{Dereference of null pointer}}
// expected-note@-1{{Dereference of null pointer}}
}
-} // end of namespace variable_declaration_in_condition
+} // namespace conversion_to_bool
namespace note_from_different_but_not_nested_stackframe {
@@ -186,14 +185,13 @@
int *getIntPtr();
void storeValue(int **i) {
- *i = getIntPtr(); // tracking-note-re{{{{^}}Value assigned to 'i', which participates in a condition later{{$}}}}
+ *i = getIntPtr();
}
int *conjurePointer() {
int *i;
- storeValue(&i); // tracking-note-re{{{{^}}Calling 'storeValue'{{$}}}}
- // tracking-note-re@-1{{{{^}}Returning from 'storeValue'{{$}}}}
- return i; // tracking-note-re{{{{^}}Returning pointer (loaded from 'i'), which participates in a condition later{{$}}}}
+ storeValue(&i);
+ return i;
}
void f(int *ptr) {
@@ -201,11 +199,8 @@
// expected-note-re@-1{{{{^}}Taking false branch{{$}}}}
;
if (!conjurePointer())
- // tracking-note-re@-1{{{{^}}Calling 'conjurePointer'{{$}}}}
- // tracking-note-re@-2{{{{^}}Returning from 'conjurePointer'{{$}}}}
- // debug-note-re@-3{{{{^}}Tracking condition '!conjurePointer()'{{$}}}}
- // expected-note-re@-4{{{{^}}Assuming the condition is true{{$}}}}
- // expected-note-re@-5{{{{^}}Taking true branch{{$}}}}
+ // expected-note-re@-1{{{{^}}Assuming the condition is true{{$}}}}
+ // expected-note-re@-2{{{{^}}Taking true branch{{$}}}}
*ptr = 5; // expected-warning{{Dereference of null pointer}}
// expected-note@-1{{Dereference of null pointer}}
}
@@ -226,9 +221,8 @@
// expected-note-re@-1{{{{^}}Taking false branch{{$}}}}
;
if (!conjurePointer())
- // debug-note-re@-1{{{{^}}Tracking condition '!conjurePointer()'{{$}}}}
- // expected-note-re@-2{{{{^}}Assuming the condition is true{{$}}}}
- // expected-note-re@-3{{{{^}}Taking true branch{{$}}}}
+ // expected-note-re@-1{{{{^}}Assuming the condition is true{{$}}}}
+ // expected-note-re@-2{{{{^}}Taking true branch{{$}}}}
*ptr = 5; // expected-warning{{Dereference of null pointer}}
// expected-note@-1{{Dereference of null pointer}}
}
@@ -246,9 +240,8 @@
int *x = 0; // expected-note-re{{{{^}}'x' initialized to a null pointer value{{$}}}}
if (cast(conjure()))
- // debug-note-re@-1{{{{^}}Tracking condition 'cast(conjure())'{{$}}}}
- // expected-note-re@-2{{{{^}}Assuming the condition is false{{$}}}}
- // expected-note-re@-3{{{{^}}Taking false branch{{$}}}}
+ // expected-note-re@-1{{{{^}}Assuming the condition is false{{$}}}}
+ // expected-note-re@-2{{{{^}}Taking false branch{{$}}}}
return;
*x = 5; // expected-warning{{Dereference of null pointer}}
// expected-note@-1{{Dereference of null pointer}}
@@ -266,9 +259,8 @@
// expected-note-re@-1{{{{^}}Taking false branch{{$}}}}
;
if (!flipCoin())
- // debug-note-re@-1{{{{^}}Tracking condition '!flipCoin()'{{$}}}}
- // expected-note-re@-2{{{{^}}Assuming the condition is true{{$}}}}
- // expected-note-re@-3{{{{^}}Taking true branch{{$}}}}
+ // expected-note-re@-1{{{{^}}Assuming the condition is true{{$}}}}
+ // expected-note-re@-2{{{{^}}Taking true branch{{$}}}}
*ptr = 5; // expected-warning{{Dereference of null pointer}}
// expected-note@-1{{Dereference of null pointer}}
}
@@ -278,11 +270,9 @@
bool coin();
bool flipCoin() {
- if (coin()) // tracking-note-re{{{{^}}Assuming the condition is false{{$}}}}
- // tracking-note-re@-1{{{{^}}Taking false branch{{$}}}}
- // debug-note-re@-2{{{{^}}Tracking condition 'coin()'{{$}}}}
+ if (coin())
return true;
- return coin(); // tracking-note-re{{{{^}}Returning value, which participates in a condition later{{$}}}}
+ return coin();
}
void i(int *ptr) {
@@ -290,11 +280,8 @@
// expected-note-re@-1{{{{^}}Taking false branch{{$}}}}
;
if (!flipCoin())
- // tracking-note-re@-1{{{{^}}Calling 'flipCoin'{{$}}}}
- // tracking-note-re@-2{{{{^}}Returning from 'flipCoin'{{$}}}}
- // debug-note-re@-3{{{{^}}Tracking condition '!flipCoin()'{{$}}}}
- // expected-note-re@-4{{{{^}}Assuming the condition is true{{$}}}}
- // expected-note-re@-5{{{{^}}Taking true branch{{$}}}}
+ // expected-note-re@-1{{{{^}}Assuming the condition is true{{$}}}}
+ // expected-note-re@-2{{{{^}}Taking true branch{{$}}}}
*ptr = 5; // expected-warning{{Dereference of null pointer}}
// expected-note@-1{{Dereference of null pointer}}
}
@@ -302,29 +289,32 @@
namespace important_returning_value_note_in_linear_function {
bool coin();
+int flag;
struct super_complicated_template_hackery {
static constexpr bool value = false;
};
-bool flipCoin() {
+void flipCoin() {
if (super_complicated_template_hackery::value)
// tracking-note-re@-1{{{{^}}'value' is false{{$}}}}
// tracking-note-re@-2{{{{^}}Taking false branch{{$}}}}
- return true;
- return coin(); // tracking-note-re{{{{^}}Returning value, which participates in a condition later{{$}}}}
+ return;
+ flag = false; // tracking-note-re{{{{^}}The value 0 is assigned to 'flag', which participates in a condition later{{$}}}}
}
void i(int *ptr) {
+ flag = true;
if (ptr) // expected-note-re{{{{^}}Assuming 'ptr' is null{{$}}}}
// expected-note-re@-1{{{{^}}Taking false branch{{$}}}}
;
- if (!flipCoin())
- // tracking-note-re@-1{{{{^}}Calling 'flipCoin'{{$}}}}
- // tracking-note-re@-2{{{{^}}Returning from 'flipCoin'{{$}}}}
- // debug-note-re@-3{{{{^}}Tracking condition '!flipCoin()'{{$}}}}
- // expected-note-re@-4{{{{^}}Assuming the condition is true{{$}}}}
- // expected-note-re@-5{{{{^}}Taking true branch{{$}}}}
+ flipCoin();
+ // tracking-note-re@-1{{{{^}}Calling 'flipCoin'{{$}}}}
+ // tracking-note-re@-2{{{{^}}Returning from 'flipCoin'{{$}}}}
+ if (!flag)
+ // debug-note-re@-1{{{{^}}Tracking condition '!flag'{{$}}}}
+ // expected-note-re@-2{{{{^}}'flag' is 0{{$}}}}
+ // expected-note-re@-3{{{{^}}Taking true branch{{$}}}}
*ptr = 5; // expected-warning{{Dereference of null pointer}}
// expected-note@-1{{Dereference of null pointer}}
}
@@ -1000,3 +990,52 @@
}
} // end of namespace only_track_the_evaluated_condition
+
+namespace operator_call_in_condition_point {
+
+struct Error {
+ explicit operator bool() {
+ return true;
+ }
+};
+
+Error couldFail();
+
+void f(int *x) {
+ x = nullptr; // expected-note {{Null pointer value stored to 'x'}}
+ if (auto e = couldFail()) // expected-note {{Taking true branch}}
+ *x = 5; // expected-warning {{Dereference of null pointer (loaded from variable 'x') [core.NullDereference]}}
+ // expected-note@-1 {{Dereference}}
+}
+
+} // namespace operator_call_in_condition_point
+
+namespace funcion_call_in_condition_point {
+
+int alwaysTrue() {
+ return true;
+}
+
+void f(int *x) {
+ x = nullptr; // expected-note {{Null pointer value stored to 'x'}}
+ if (alwaysTrue()) // expected-note {{Taking true branch}}
+ *x = 5; // expected-warning {{Dereference of null pointer (loaded from variable 'x') [core.NullDereference]}}
+ // expected-note@-1 {{Dereference}}
+}
+
+} // namespace funcion_call_in_condition_point
+
+namespace funcion_call_negated_in_condition_point {
+
+int alwaysFalse() {
+ return false;
+}
+
+void f(int *x) {
+ x = nullptr; // expected-note {{Null pointer value stored to 'x'}}
+ if (!alwaysFalse()) // expected-note {{Taking true branch}}
+ *x = 5; // expected-warning {{Dereference of null pointer (loaded from variable 'x') [core.NullDereference]}}
+ // expected-note@-1 {{Dereference}}
+}
+
+} // namespace funcion_call_negated_in_condition_point
Index: clang/test/Analysis/return-value-guaranteed.cpp
===================================================================
--- clang/test/Analysis/return-value-guaranteed.cpp
+++ clang/test/Analysis/return-value-guaranteed.cpp
@@ -24,7 +24,6 @@
// class-note@-1 {{The value 0 is assigned to 'F.Field'}}
return !MCAsmParser::Error();
// class-note@-1 {{'MCAsmParser::Error' returns true}}
- // class-note@-2 {{Returning zero, which participates in a condition later}}
}
bool parseFile() {
@@ -58,7 +57,6 @@
struct MCAsmParser {
static bool Error() {
return false; // class-note {{'MCAsmParser::Error' returns false}}
- // class-note@-1 {{Returning zero, which participates in a condition later}}
}
};
@@ -74,7 +72,6 @@
return MCAsmParser::Error();
// class-note@-1 {{Calling 'MCAsmParser::Error'}}
// class-note@-2 {{Returning from 'MCAsmParser::Error'}}
- // class-note@-3 {{Returning zero, which participates in a condition later}}
}
bool parseFile() {
Index: clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
===================================================================
--- clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
+++ clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
@@ -82,6 +82,10 @@
return nullptr;
}
+/// \return A subexpression of @c Ex which represents the
+/// expression-of-interest.
+static const Expr *peelOffOuterExpr(const Expr *Ex, const ExplodedNode *N);
+
/// Given that expression S represents a pointer that would be dereferenced,
/// try to find a sub-expression from which the pointer came from.
/// This is used for tracking down origins of a null or undefined value:
@@ -1919,11 +1923,33 @@
return nullptr;
if (const Expr *Condition = NB->getLastCondition()) {
+
+ // If we can't retrieve a sensible condition, just bail out.
+ const Expr *InnerExpr = peelOffOuterExpr(Condition, N);
+ if (!InnerExpr)
+ return nullptr;
+
+ // If the condition was a function call, we likely won't gain much from
+ // tracking it either. Evidence suggests that it will mostly trigger in
+ // scenarios like this:
+ //
+ // void f(int *x) {
+ // x = nullptr;
+ // if (alwaysTrue()) // We don't need a whole lot of explanation
+ // // here, the function name is good enough.
+ // *x = 5;
+ // }
+ //
+ // Its easy to create a counterexample where this heuristic would make us
+ // lose valuable information, but we've never really seen one in practice.
+ if (isa<CallExpr>(InnerExpr))
+ return nullptr;
+
// Keeping track of the already tracked conditions on a visitor level
// isn't sufficient, because a new visitor is created for each tracked
// expression, hence the BugReport level set.
if (BR.addTrackedCondition(N)) {
- getParentTracker().track(Condition, N,
+ getParentTracker().track(InnerExpr, N,
{bugreporter::TrackingKind::Condition,
/*EnableNullFPSuppression=*/false});
return constructDebugPieceForTrackedCondition(Condition, N, BRC);
@@ -1938,10 +1964,8 @@
// Implementation of trackExpressionValue.
//===----------------------------------------------------------------------===//
-/// \return A subexpression of @c Ex which represents the
-/// expression-of-interest.
-static const Expr *peelOffOuterExpr(const Expr *Ex,
- const ExplodedNode *N) {
+static const Expr *peelOffOuterExpr(const Expr *Ex, const ExplodedNode *N) {
+
Ex = Ex->IgnoreParenCasts();
if (const auto *FE = dyn_cast<FullExpr>(Ex))
return peelOffOuterExpr(FE->getSubExpr(), N);
Index: clang/lib/Analysis/CFG.cpp
===================================================================
--- clang/lib/Analysis/CFG.cpp
+++ clang/lib/Analysis/CFG.cpp
@@ -5905,6 +5905,8 @@
print(llvm::errs(), LO, ShowColors);
}
+void CFG::dump(bool ShowColors) const { dump(LangOptions{}, ShowColors); }
+
/// print - A simple pretty printer of a CFG that outputs to an ostream.
void CFG::print(raw_ostream &OS, const LangOptions &LO, bool ShowColors) const {
StmtPrinterHelper Helper(this, LO);
Index: clang/include/clang/Analysis/CFG.h
===================================================================
--- clang/include/clang/Analysis/CFG.h
+++ clang/include/clang/Analysis/CFG.h
@@ -1428,6 +1428,7 @@
void viewCFG(const LangOptions &LO) const;
void print(raw_ostream &OS, const LangOptions &LO, bool ShowColors) const;
void dump(const LangOptions &LO, bool ShowColors) const;
+ void dump(bool ShowColors = true) const;
//===--------------------------------------------------------------------===//
// Internal: constructors and data.
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
