vsavchenko created this revision.
vsavchenko added reviewers: NoQ, martong, steakhal, xazax.hun.
Herald added subscribers: ASDenysPetrov, Charusso, dkrupp, donat.nagy,
Szelethus, mikhail.ramalho, a.sidorin, rnkovacs, szepet, baloghadamsoftware.
vsavchenko requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.
When searching for stores and creating corresponding notes, the
analyzer is more specific about the target region of the store
as opposed to the stored value. While this description was tweaked
for constant and undefined values, it lacked in the most general
case of symbolic values.
This patch tries to find a memory region, where this value is stored,
to use it as a better alias for the value.
rdar://76645710
Repository:
rG LLVM Github Monorepo
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/placement-new.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,9 +63,9 @@
}
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@-1 {{Passing 't' via 1st parameter 'p'}}
//expected-note@-2 {{Calling 'f6_1_sub'}}
//expected-note@-3 {{Returning from 'f6_1_sub'}}
//expected-note@-4 {{Assigned value is garbage or undefined}}
@@ -75,9 +75,9 @@
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 &p = t; //expected-note {{'p' initialized to the value of 't'}}
+ 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 to the value of 'ta'}}
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}}
}
@@ -107,7 +107,7 @@
void f_9(void) {
int a[6]; // expected-note {{'a' initialized here}}
- int const *ptau = a; // expected-note {{'ptau' initialized here}}
+ int const *ptau = a; // expected-note {{'ptau' initialized to the value of 'a'}}
doStuff_arrayOfConstInt(ptau); // 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/placement-new.cpp
===================================================================
--- clang/test/Analysis/placement-new.cpp
+++ clang/test/Analysis/placement-new.cpp
@@ -79,7 +79,7 @@
void f() {
//char *st = new char [8];
char buf[3]; // expected-note {{'buf' initialized here}}
- void *st = buf; // expected-note {{'st' initialized here}}
+ void *st = buf; // expected-note {{'st' initialized to the value of 'buf'}}
long *lp = ::new (st) long; // expected-warning{{Storage provided to placement new is only 3 bytes, whereas the allocated type requires 8 bytes}} expected-note 1 {{}}
(void)lp;
}
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);
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 *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 *p = 0; // expected-note {{'p' initialized to a null pointer value}}
+ int *const &p2 = p; // expected-note{{'p2' initialized to the value of 'p'}}
+ 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 to the value of 'p'}}
+ 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;
+ 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;
+}
+
/// 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,52 @@
if (!IsParam)
InitE = InitE->IgnoreParenCasts();
- bugreporter::trackExpressionValue(
- StoreSite, InitE, BR, TKind, EnableNullFPSuppression);
+ bugreporter::trackExpressionValue(StoreSite, InitE, BR, TKind,
+ EnableNullFPSuppression);
+ }
+
+ // Let's try to find there 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)
+ OldRegion = getLocationRegionIfReference(InitE, Succ, false);
+
+ // 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 = foo(old)
+ // where the analyzer knows that 'foo' 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 = foo(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 +1572,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 +1907,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
