This revision was automatically updated to reflect the committed changes.
Closed by commit rGab5823867c4a: [analyzer] Find better description for tracked
symbolic values (authored by vsavchenko).
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D101041/new/
https://reviews.llvm.org/D101041
Files:
clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
clang/test/Analysis/Inputs/expected-plists/retain-release-path-notes.m.plist
clang/test/Analysis/diagnostics/deref-track-symbolic-region.cpp
clang/test/Analysis/osobject-retain-release.cpp
clang/test/Analysis/retain-release-path-notes.m
clang/test/Analysis/uninit-const.c
clang/test/Analysis/uninit-const.cpp
Index: clang/test/Analysis/uninit-const.cpp
===================================================================
--- clang/test/Analysis/uninit-const.cpp
+++ clang/test/Analysis/uninit-const.cpp
@@ -63,7 +63,7 @@
}
void f6_1(void) {
- int t; // expected-note{{'t' declared without an initial value}}
+ int t; // expected-note{{'t' declared without an initial value}}
int p = f6_1_sub(t); //expected-warning {{Assigned value is garbage or undefined}}
//expected-note@-1 {{Passing value via 1st parameter 'p'}}
//expected-note@-2 {{Calling 'f6_1_sub'}}
@@ -76,8 +76,8 @@
void f6_2(void) {
int t; //expected-note {{'t' declared without an initial value}}
int &p = t; //expected-note {{'p' initialized here}}
- int &s = p; //expected-note {{'s' initialized here}}
- int &q = s; //expected-note {{'q' initialized here}}
+ int &s = p; //expected-note {{'s' initialized to the value of 'p'}}
+ int &q = s; //expected-note {{'q' initialized to the value of 's'}}
doStuff6(q); //expected-warning {{1st function call argument is an uninitialized value}}
//expected-note@-1 {{1st function call argument is an uninitialized value}}
}
Index: clang/test/Analysis/uninit-const.c
===================================================================
--- clang/test/Analysis/uninit-const.c
+++ clang/test/Analysis/uninit-const.c
@@ -37,7 +37,7 @@
void f_1_1(void) {
int t; // expected-note {{'t' declared without an initial value}}
int *tp1 = &t; // expected-note {{'tp1' initialized here}}
- int* tp2 = tp1; // expected-note {{'tp2' initialized here}}
+ int *tp2 = tp1; // expected-note {{'tp2' initialized to the value of 'tp1'}}
doStuff_pointerToConstInt(tp2); // expected-warning {{1st function call argument is a pointer to uninitialized value}}
// expected-note@-1 {{1st function call argument is a pointer to uninitialized value}}
}
@@ -53,7 +53,7 @@
// expected-note@-1{{Calling 'f_2_sub'}}
// expected-note@-2{{Returning from 'f_2_sub'}}
// expected-note@-3{{'p' initialized here}}
- int* tp = p; // expected-note {{'tp' initialized here}}
+ int *tp = p; // expected-note {{'tp' initialized to the value of 'p'}}
doStuff_pointerToConstInt(tp); // expected-warning {{1st function call argument is a pointer to uninitialized value}}
// expected-note@-1 {{1st function call argument is a pointer to uninitialized value}}
}
@@ -70,7 +70,7 @@
void f_5(void) {
int ta[5]; // expected-note {{'ta' initialized here}}
- int* tp = ta; // expected-note {{'tp' initialized here}}
+ int *tp = ta; // expected-note {{'tp' initialized here}}
doStuff_pointerToConstInt(tp); // expected-warning {{1st function call argument is a pointer to uninitialized value}}
// expected-note@-1 {{1st function call argument is a pointer to uninitialized value}}
}
Index: clang/test/Analysis/retain-release-path-notes.m
===================================================================
--- clang/test/Analysis/retain-release-path-notes.m
+++ clang/test/Analysis/retain-release-path-notes.m
@@ -339,7 +339,7 @@
// expected-note@216 {{Returning pointer (loaded from 'self')}}
// expected-note@-3 {{Returning from 'initY'}}
// expected-note@-4 {{'Original' initialized here}}
- id New = Original; // expected-note {{'New' initialized here}}
+ id New = Original; // expected-note {{'New' initialized to the value of 'Original'}}
Original = [[MyObj alloc] initZ];
(void)New;
[Original release]; // expected-warning {{Potential leak of an object stored into 'New'}}
@@ -352,8 +352,8 @@
// expected-note@216 {{Returning pointer (loaded from 'self')}}
// expected-note@-3 {{Returning from 'initY'}}
// expected-note@-4 {{'Original' initialized here}}
- id Intermediate = Original; // expected-note {{'Intermediate' initialized here}}
- id New = Intermediate; // expected-note {{'New' initialized here}}
+ id Intermediate = Original; // expected-note {{'Intermediate' initialized to the value of 'Original'}}
+ id New = Intermediate; // expected-note {{'New' initialized to the value of 'Intermediate'}}
Original = [[MyObj alloc] initZ];
(void)New;
[Original release]; // expected-warning {{Potential leak of an object stored into 'New'}}
@@ -380,7 +380,7 @@
// expected-note@-3 {{Returning from 'initY'}}
// expected-note@-4 {{'Original' initialized here}}
id New = 0;
- New = Original; // expected-note {{Value assigned to 'New'}}
+ New = Original; // expected-note {{The value of 'Original' is assigned to 'New'}}
Original = [[MyObj alloc] initZ];
[self log:New with:[self calculate]];
[Original release]; // expected-warning {{Potential leak of an object stored into 'New'}}
Index: clang/test/Analysis/osobject-retain-release.cpp
===================================================================
--- clang/test/Analysis/osobject-retain-release.cpp
+++ clang/test/Analysis/osobject-retain-release.cpp
@@ -536,7 +536,7 @@
void check_dynamic_cast_alias() {
OSObject *originalPtr = OSObject::generateObject(1); // expected-note {{Call to method 'OSObject::generateObject' returns an OSObject}}
- OSArray *newPtr = OSDynamicCast(OSArray, originalPtr); // expected-note {{'newPtr' initialized here}}
+ OSArray *newPtr = OSDynamicCast(OSArray, originalPtr); // expected-note {{'newPtr' initialized to the value of 'originalPtr'}}
if (newPtr) { // expected-note {{'newPtr' is non-null}}
// expected-note@-1 {{Taking true branch}}
originalPtr = OSObject::generateObject(42);
@@ -549,7 +549,7 @@
void check_dynamic_cast_alias_cond() {
OSObject *originalPtr = OSObject::generateObject(1); // expected-note {{Call to method 'OSObject::generateObject' returns an OSObject}}
OSArray *newPtr = 0;
- if ((newPtr = OSDynamicCast(OSArray, originalPtr))) { // expected-note {{Value assigned to 'newPtr'}}
+ if ((newPtr = OSDynamicCast(OSArray, originalPtr))) { // expected-note {{The value of 'originalPtr' is assigned to 'newPtr'}}
// expected-note@-1 {{'newPtr' is non-null}}
// expected-note@-2 {{Taking true branch}}
originalPtr = OSObject::generateObject(42);
@@ -563,7 +563,7 @@
OSObject *originalPtr = OSObject::generateObject(1); // expected-note {{Call to method 'OSObject::generateObject' returns an OSObject of type 'OSObject' with a +1 retain count}}
OSObject *intermediate = originalPtr; // TODO: add note here as well
OSArray *newPtr = 0;
- if ((newPtr = OSDynamicCast(OSArray, intermediate))) { // expected-note {{Value assigned to 'newPtr'}}
+ if ((newPtr = OSDynamicCast(OSArray, intermediate))) { // expected-note {{The value of 'intermediate' is assigned to 'newPtr'}}
// expected-note@-1 {{'newPtr' is non-null}}
// expected-note@-2 {{Taking true branch}}
intermediate = OSObject::generateObject(42);
@@ -573,6 +573,21 @@
// expected-note@-1 {{Object leaked: object allocated and stored into 'newPtr' is not referenced later in this execution path and has a retain count of +1}}
}
+void check_dynamic_cast_alias_intermediate_2() {
+ OSObject *originalPtr = OSObject::generateObject(1); // expected-note {{Call to method 'OSObject::generateObject' returns an OSObject of type 'OSObject' with a +1 retain count}}
+ OSObject *intermediate = originalPtr; // TODO: add note here as well
+ OSArray *newPtr = 0;
+ if ((newPtr = OSDynamicCast(OSArray, intermediate))) { // expected-note {{Value assigned to 'newPtr'}}
+ // expected-note@-1 {{'newPtr' is non-null}}
+ // expected-note@-2 {{Taking true branch}}
+ intermediate = OSObject::generateObject(42);
+ (void)originalPtr;
+ }
+ (void)newPtr;
+ intermediate->release(); // expected-warning {{Potential leak of an object stored into 'newPtr'}}
+ // expected-note@-1 {{Object leaked: object allocated and stored into 'newPtr' is not referenced later in this execution path and has a retain count of +1}}
+}
+
void use_after_release() {
OSArray *arr = OSArray::withCapacity(10); // expected-note{{Call to method 'OSArray::withCapacity' returns an OSObject of type 'OSArray' with a +1 retain count}}
arr->release(); // expected-note{{Object released}}
Index: clang/test/Analysis/diagnostics/deref-track-symbolic-region.cpp
===================================================================
--- clang/test/Analysis/diagnostics/deref-track-symbolic-region.cpp
+++ clang/test/Analysis/diagnostics/deref-track-symbolic-region.cpp
@@ -28,16 +28,16 @@
}
int testRefToNullPtr() {
- int *p = 0; // expected-note {{'p' initialized to a null pointer value}}
+ int *p = 0; // expected-note {{'p' initialized to a null pointer value}}
int *const &p2 = p; // expected-note{{'p2' initialized here}}
- int *p3 = p2; // expected-note {{'p3' initialized to a null pointer value}}
- return *p3; // expected-warning {{Dereference of null pointer}}
- // expected-note@-1{{Dereference of null pointer}}
+ int *p3 = p2; // expected-note {{'p3' initialized to a null pointer value}}
+ return *p3; // expected-warning {{Dereference of null pointer}}
+ // expected-note@-1{{Dereference of null pointer}}
}
int testRefToNullPtr2() {
- int *p = 0; // expected-note {{'p' initialized to a null pointer value}}
- int *const &p2 = p;// expected-note{{'p2' initialized here}}
- return *p2; //expected-warning {{Dereference of null pointer}}
- // expected-note@-1{{Dereference of null pointer}}
-}
\ No newline at end of file
+ int *p = 0; // expected-note {{'p' initialized to a null pointer value}}
+ int *const &p2 = p; // expected-note{{'p2' initialized here}}
+ return *p2; //expected-warning {{Dereference of null pointer}}
+ // expected-note@-1{{Dereference of null pointer}}
+}
Index: clang/test/Analysis/Inputs/expected-plists/retain-release-path-notes.m.plist
===================================================================
--- clang/test/Analysis/Inputs/expected-plists/retain-release-path-notes.m.plist
+++ clang/test/Analysis/Inputs/expected-plists/retain-release-path-notes.m.plist
@@ -5398,9 +5398,9 @@
</array>
<key>depth</key><integer>0</integer>
<key>extended_message</key>
- <string>'New' initialized here</string>
+ <string>'New' initialized to the value of 'Original'</string>
<key>message</key>
- <string>'New' initialized here</string>
+ <string>'New' initialized to the value of 'Original'</string>
</dict>
<dict>
<key>kind</key><string>control</string>
@@ -5886,9 +5886,9 @@
</array>
<key>depth</key><integer>0</integer>
<key>extended_message</key>
- <string>'Intermediate' initialized here</string>
+ <string>'Intermediate' initialized to the value of 'Original'</string>
<key>message</key>
- <string>'Intermediate' initialized here</string>
+ <string>'Intermediate' initialized to the value of 'Original'</string>
</dict>
<dict>
<key>kind</key><string>control</string>
@@ -5949,9 +5949,9 @@
</array>
<key>depth</key><integer>0</integer>
<key>extended_message</key>
- <string>'New' initialized here</string>
+ <string>'New' initialized to the value of 'Intermediate'</string>
<key>message</key>
- <string>'New' initialized here</string>
+ <string>'New' initialized to the value of 'Intermediate'</string>
</dict>
<dict>
<key>kind</key><string>control</string>
@@ -6438,9 +6438,9 @@
</array>
<key>depth</key><integer>0</integer>
<key>extended_message</key>
- <string>Value assigned to 'New'</string>
+ <string>The value of 'Original' is assigned to 'New'</string>
<key>message</key>
- <string>Value assigned to 'New'</string>
+ <string>The value of 'Original' is assigned to 'New'</string>
</dict>
<dict>
<key>kind</key><string>control</string>
Index: clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
===================================================================
--- clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
+++ clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
@@ -153,6 +153,28 @@
return E;
}
+static const MemRegion *
+getLocationRegionIfReference(const Expr *E, const ExplodedNode *N,
+ bool LookingForReference = true) {
+ if (const auto *DR = dyn_cast<DeclRefExpr>(E)) {
+ if (const auto *VD = dyn_cast<VarDecl>(DR->getDecl())) {
+ if (LookingForReference && !VD->getType()->isReferenceType())
+ return nullptr;
+ return N->getState()
+ ->getLValue(VD, N->getLocationContext())
+ .getAsRegion();
+ }
+ }
+
+ // FIXME: This does not handle other kinds of null references,
+ // for example, references from FieldRegions:
+ // struct Wrapper { int &ref; };
+ // Wrapper w = { *(int *)0 };
+ // w.ref = 1;
+
+ return nullptr;
+}
+
/// Comparing internal representations of symbolic values (via
/// SVal::operator==()) is a valid way to check if the value was updated,
/// unless it's a LazyCompoundVal that may have a different internal
@@ -1241,16 +1263,17 @@
/// Show diagnostics for initializing or declaring a region \p R with a bad value.
static void showBRDiagnostics(const char *action, llvm::raw_svector_ostream &os,
- const MemRegion *R, SVal V, const DeclStmt *DS) {
- if (R->canPrintPretty()) {
- R->printPretty(os);
+ const MemRegion *NewR, SVal V,
+ const MemRegion *OldR, const DeclStmt *DS) {
+ if (NewR->canPrintPretty()) {
+ NewR->printPretty(os);
os << " ";
}
if (V.getAs<loc::ConcreteInt>()) {
bool b = false;
- if (R->isBoundable()) {
- if (const auto *TR = dyn_cast<TypedValueRegion>(R)) {
+ if (NewR->isBoundable()) {
+ if (const auto *TR = dyn_cast<TypedValueRegion>(NewR)) {
if (TR->getValueType()->isObjCObjectPointerType()) {
os << action << "nil";
b = true;
@@ -1262,29 +1285,31 @@
} else if (auto CVal = V.getAs<nonloc::ConcreteInt>()) {
os << action << CVal->getValue();
+ } else if (OldR && OldR->canPrintPretty()) {
+ os << action << "the value of ";
+ OldR->printPretty(os);
} else if (DS) {
if (V.isUndef()) {
- if (isa<VarRegion>(R)) {
+ if (isa<VarRegion>(NewR)) {
const auto *VD = cast<VarDecl>(DS->getSingleDecl());
if (VD->getInit()) {
- os << (R->canPrintPretty() ? "initialized" : "Initializing")
- << " to a garbage value";
+ os << (NewR->canPrintPretty() ? "initialized" : "Initializing")
+ << " to a garbage value";
} else {
- os << (R->canPrintPretty() ? "declared" : "Declaring")
- << " without an initial value";
+ os << (NewR->canPrintPretty() ? "declared" : "Declaring")
+ << " without an initial value";
}
}
} else {
- os << (R->canPrintPretty() ? "initialized" : "Initialized")
- << " here";
+ os << (NewR->canPrintPretty() ? "initialized" : "Initialized") << " here";
}
}
}
/// Display diagnostics for passing bad region as a parameter.
-static void showBRParamDiagnostics(llvm::raw_svector_ostream& os,
- const VarRegion *VR,
- SVal V) {
+static void showBRParamDiagnostics(llvm::raw_svector_ostream &os,
+ const VarRegion *VR, SVal V,
+ const MemRegion *ValueR) {
const auto *Param = cast<ParmVarDecl>(VR->getDecl());
os << "Passing ";
@@ -1298,6 +1323,8 @@
os << "uninitialized value";
} else if (auto CI = V.getAs<nonloc::ConcreteInt>()) {
os << "the value " << CI->getValue();
+ } else if (ValueR && ValueR->canPrintPretty()) {
+ ValueR->printPretty(os);
} else {
os << "value";
}
@@ -1313,11 +1340,12 @@
/// Show default diagnostics for storing bad region.
static void showBRDefaultDiagnostics(llvm::raw_svector_ostream &os,
- const MemRegion *R, SVal V) {
+ const MemRegion *NewR, SVal V,
+ const MemRegion *OldR) {
if (V.getAs<loc::ConcreteInt>()) {
bool b = false;
- if (R->isBoundable()) {
- if (const auto *TR = dyn_cast<TypedValueRegion>(R)) {
+ if (NewR->isBoundable()) {
+ if (const auto *TR = dyn_cast<TypedValueRegion>(NewR)) {
if (TR->getValueType()->isObjCObjectPointerType()) {
os << "nil object reference stored";
b = true;
@@ -1325,34 +1353,44 @@
}
}
if (!b) {
- if (R->canPrintPretty())
+ if (NewR->canPrintPretty())
os << "Null pointer value stored";
else
os << "Storing null pointer value";
}
} else if (V.isUndef()) {
- if (R->canPrintPretty())
+ if (NewR->canPrintPretty())
os << "Uninitialized value stored";
else
os << "Storing uninitialized value";
} else if (auto CV = V.getAs<nonloc::ConcreteInt>()) {
- if (R->canPrintPretty())
+ if (NewR->canPrintPretty())
os << "The value " << CV->getValue() << " is assigned";
else
os << "Assigning " << CV->getValue();
+ } else if (OldR && OldR->canPrintPretty()) {
+ if (NewR->canPrintPretty()) {
+ os << "The value of ";
+ OldR->printPretty(os);
+ os << " is assigned";
+ } else {
+ os << "Assigning the value of ";
+ OldR->printPretty(os);
+ }
+
} else {
- if (R->canPrintPretty())
+ if (NewR->canPrintPretty())
os << "Value assigned";
else
os << "Assigning value";
}
- if (R->canPrintPretty()) {
+ if (NewR->canPrintPretty()) {
os << " to ";
- R->printPretty(os);
+ NewR->printPretty(os);
}
}
@@ -1451,8 +1489,76 @@
if (!IsParam)
InitE = InitE->IgnoreParenCasts();
- bugreporter::trackExpressionValue(
- StoreSite, InitE, BR, TKind, EnableNullFPSuppression);
+ bugreporter::trackExpressionValue(StoreSite, InitE, BR, TKind,
+ EnableNullFPSuppression);
+ }
+
+ // Let's try to find the region where the value came from.
+ const MemRegion *OldRegion = nullptr;
+
+ // If we have init expression, it might be simply a reference
+ // to a variable, so we can use it.
+ if (InitE) {
+ // That region might still be not exactly what we are looking for.
+ // In situations like `int &ref = val;`, we can't say that
+ // `ref` is initialized with `val`, rather refers to `val`.
+ //
+ // In order, to mitigate situations like this, we check if the last
+ // stored value in that region is the value that we track.
+ //
+ // TODO: support other situations better.
+ if (const MemRegion *Candidate =
+ getLocationRegionIfReference(InitE, Succ, false)) {
+ const StoreManager &SM = BRC.getStateManager().getStoreManager();
+
+ // Here we traverse the graph up to find the last node where the
+ // candidate region is still in the store.
+ for (const ExplodedNode *N = StoreSite; N; N = N->getFirstPred()) {
+ if (SM.includedInBindings(N->getState()->getStore(), Candidate)) {
+ // And if it was bound to the target value, we can use it.
+ if (N->getState()->getSVal(Candidate) == V) {
+ OldRegion = Candidate;
+ }
+ break;
+ }
+ }
+ }
+ }
+
+ // Otherwise, if the current region does indeed contain the value
+ // we are looking for, we can look for a region where this value
+ // was before.
+ //
+ // It can be useful for situations like:
+ // new = identity(old)
+ // where the analyzer knows that 'identity' returns the value of its
+ // first argument.
+ //
+ // NOTE: If the region R is not a simple var region, it can contain
+ // V in one of its subregions.
+ if (!OldRegion && StoreSite->getState()->getSVal(R) == V) {
+ // Let's go up the graph to find the node where the region is
+ // bound to V.
+ const ExplodedNode *NodeWithoutBinding = StoreSite->getFirstPred();
+ for (;
+ NodeWithoutBinding && NodeWithoutBinding->getState()->getSVal(R) == V;
+ NodeWithoutBinding = NodeWithoutBinding->getFirstPred()) {
+ }
+
+ if (NodeWithoutBinding) {
+ // Let's try to find a unique binding for the value in that node.
+ // We want to use this to find unique bindings because of the following
+ // situations:
+ // b = a;
+ // c = identity(b);
+ //
+ // Telling the user that the value of 'a' is assigned to 'c', while
+ // correct, can be confusing.
+ StoreManager::FindUniqueBinding FB(V.getAsLocSymbol());
+ BRC.getStateManager().iterBindings(NodeWithoutBinding->getState(), FB);
+ if (FB)
+ OldRegion = FB.getRegion();
+ }
}
if (TKind == TrackingKind::Condition &&
@@ -1490,15 +1596,15 @@
}
}
if (action)
- showBRDiagnostics(action, os, R, V, DS);
+ showBRDiagnostics(action, os, R, V, OldRegion, DS);
} else if (StoreSite->getLocation().getAs<CallEnter>()) {
if (const auto *VR = dyn_cast<VarRegion>(R))
- showBRParamDiagnostics(os, VR, V);
+ showBRParamDiagnostics(os, VR, V, OldRegion);
}
if (os.str().empty())
- showBRDefaultDiagnostics(os, R, V);
+ showBRDefaultDiagnostics(os, R, V, OldRegion);
if (TKind == bugreporter::TrackingKind::Condition)
os << WillBeUsedForACondition;
@@ -1825,27 +1931,6 @@
// Implementation of trackExpressionValue.
//===----------------------------------------------------------------------===//
-static const MemRegion *getLocationRegionIfReference(const Expr *E,
- const ExplodedNode *N) {
- if (const auto *DR = dyn_cast<DeclRefExpr>(E)) {
- if (const auto *VD = dyn_cast<VarDecl>(DR->getDecl())) {
- if (!VD->getType()->isReferenceType())
- return nullptr;
- ProgramStateManager &StateMgr = N->getState()->getStateManager();
- MemRegionManager &MRMgr = StateMgr.getRegionManager();
- return MRMgr.getVarRegion(VD, N->getLocationContext());
- }
- }
-
- // FIXME: This does not handle other kinds of null references,
- // for example, references from FieldRegions:
- // struct Wrapper { int &ref; };
- // Wrapper w = { *(int *)0 };
- // w.ref = 1;
-
- return nullptr;
-}
-
/// \return A subexpression of @c Ex which represents the
/// expression-of-interest.
static const Expr *peelOffOuterExpr(const Expr *Ex,
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
