Szelethus created this revision.
Szelethus added reviewers: dkrupp, NoQ, steakhal, gamesh411, martong, balazske.
Szelethus added a project: clang.
Herald added subscribers: manas, ASDenysPetrov, donat.nagy, mikhail.ramalho,
a.sidorin, rnkovacs, szepet, baloghadamsoftware, kristof.beyls, xazax.hun.
Szelethus requested review of this revision.
Herald added a subscriber: cfe-commits.
I recently evaluated ~150 of bug reports on open source projects relating to my
GSoC'19 project <https://szelethus.github.io/gsoc2019/>, which was about
tracking control dependencies that were relevant to a bug report.
Here is what I found: when the condition is a function call, the extra notes
were almost always unimportant, and often times intrusive:
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;
}
It almost always boiled down to a few "Returning null pointer, which
participates in a condition later", or similar notes. I struggled to find a
single case where the notes revealed anything interesting or some previously
hidden correlation, which is kind of the point of condition tracking.
This patch checks whether the condition is a function call, and if so, bails
out.
The argument against the patch is the popular feedback we hear from some of our
users, namely that they can never have too much information. I was specifically
fishing for examples that display best that my contribution did more good than
harm, so admittedly I set the bar high, and one can argue that there can be
non-trivial trickery inside functions, and function names may not be that
descriptive.
My argument for the patch is all those reports that got longer without any
notable improvement in the report intelligibility. I think the few exceptional
cases where this patch would remove notable information are an acceptable
sacrifice in favor of more reports being leaner.
Repository:
rG LLVM Github Monorepo
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,9 @@
// 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{{$}}}}
+ // debug-note-re@-1{{{{^}}Tracking condition '!conjurePointer()'{{$}}}}
+ // expected-note-re@-2{{{{^}}Assuming the condition is true{{$}}}}
+ // expected-note-re@-3{{{{^}}Taking true branch{{$}}}}
*ptr = 5; // expected-warning{{Dereference of null pointer}}
// expected-note@-1{{Dereference of null pointer}}
}
@@ -226,9 +222,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 +241,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}}
@@ -278,11 +272,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 +282,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 +291,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{{$}}}}
+ // expected-note-re@-1{{{{^}}'value' is 0{{$}}}}
+ // expected-note-re@-2{{{{^}}Taking false branch{{$}}}}
+ 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 +992,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:
@@ -1918,11 +1922,30 @@
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;
+ // }
+ 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);
@@ -1937,10 +1960,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
@@ -5909,6 +5909,10 @@
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
