NoQ created this revision.
NoQ added reviewers: dcoughlin, xazax.hun, a_sidorin, rnkovacs, Szelethus,
baloghadamsoftware, Charusso.
Herald added subscribers: cfe-commits, dkrupp, donat.nagy, mikhail.ramalho,
a.sidorin, szepet.
Herald added a project: clang.
Got myself distracted with a tiny fix for the inlined defensive check
suppression.
When `bugreporter::trackExpressionValue()` is invoked on a `DeclRefExpr`, it
tries to do most of its computations over the node in which this `DeclRefExpr`
is computed, rather than on the error node (or whatever node is stuffed into
it). I'm quite confused about the idea behind it and i highly doubt that it
actually works correctly, but one reason why we can't simply use the error node
may be that the binding to that variable might have already disappeared from
the state by the time the bug is found.
This time i noticed that for the inlined defensive checks visitor the
`DeclRefExpr` node is in fact sometimes too //early//: the call in which the
inlined defensive check has happened might have not been entered yet.
Therefore i change the visitor to be fine with tracking dead symbols (which it
is totally capable of - the collapse point for the symbol is still
well-defined), and fire it up directly on the error node. I still use
"`LVState`" to find out which value should we be tracking, so there shouldn't
be any problems with accidentally loading an ill-formed value from a dead
variable.
I hope it's one tiny piece of understanding that'll bring us to a better
architecture of this code.
Repository:
rC Clang
https://reviews.llvm.org/D67932
Files:
clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
clang/test/Analysis/NSContainers.m
Index: clang/test/Analysis/NSContainers.m
===================================================================
--- clang/test/Analysis/NSContainers.m
+++ clang/test/Analysis/NSContainers.m
@@ -2,6 +2,8 @@
void clang_analyzer_eval(int);
+#define nil ((id)0)
+
typedef unsigned long NSUInteger;
typedef signed char BOOL;
typedef struct _NSZone NSZone;
@@ -310,3 +312,14 @@
// here either.
[subviews addObject:view]; // no-warning
}
+
+NSString *getStringFromString(NSString *string) {
+ if (!string)
+ return nil;
+ return @"New String";
+}
+void testInlinedDefensiveCheck(NSMutableDictionary *dict, id obj) {
+ // The check in getStringFromString() is not a good indication
+ // that 'obj' can be nil in this context.
+ dict[obj] = getStringFromString(obj); // no-warning
+}
Index: clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
===================================================================
--- clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
+++ clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
@@ -1601,9 +1601,6 @@
AnalyzerOptions &Options = N->getState()->getAnalysisManager().options;
if (!Options.ShouldSuppressInlinedDefensiveChecks)
IsSatisfied = true;
-
- assert(N->getState()->isNull(V).isConstrainedTrue() &&
- "The visitor only tracks the cases where V is constrained to 0");
}
void SuppressInlineDefensiveChecksVisitor::Profile(
@@ -1634,13 +1631,12 @@
// Check if in the previous state it was feasible for this value
// to *not* be null.
- if (!Pred->getState()->isNull(V).isConstrainedTrue()) {
+ if (!Pred->getState()->isNull(V).isConstrainedTrue() &&
+ Succ->getState()->isNull(V).isConstrainedTrue()) {
IsSatisfied = true;
- assert(Succ->getState()->isNull(V).isConstrainedTrue());
-
// Check if this is inlined defensive checks.
- const LocationContext *CurLC =Succ->getLocationContext();
+ const LocationContext *CurLC = Succ->getLocationContext();
const LocationContext *ReportLC = BR.getErrorNode()->getLocationContext();
if (CurLC != ReportLC && !CurLC->isParentOf(ReportLC)) {
BR.markInvalid("Suppress IDC", CurLC);
@@ -2007,11 +2003,16 @@
// Add visitor, which will suppress inline defensive checks.
if (auto DV = V.getAs<DefinedSVal>())
- if (!DV->isZeroConstant() && LVState->isNull(*DV).isConstrainedTrue()
&&
- EnableNullFPSuppression)
+ if (!DV->isZeroConstant() && EnableNullFPSuppression) {
+ // Note that LVNode may be too late; the lvalue may have been
computed
+ // before the inlined call was evaluated. InputNode may as well be
+ // too late, because the symbol is already dead; this, however,
+ // is fine because we can still find the node in which it
+ // collapsed to null previously.
report.addVisitor(
- std::make_unique<SuppressInlineDefensiveChecksVisitor>(*DV,
- LVNode));
+ std::make_unique<SuppressInlineDefensiveChecksVisitor>(
+ *DV, InputNode));
+ }
if (auto KV = V.getAs<KnownSVal>())
report.addVisitor(std::make_unique<FindLastStoreBRVisitor>(
Index: clang/test/Analysis/NSContainers.m
===================================================================
--- clang/test/Analysis/NSContainers.m
+++ clang/test/Analysis/NSContainers.m
@@ -2,6 +2,8 @@
void clang_analyzer_eval(int);
+#define nil ((id)0)
+
typedef unsigned long NSUInteger;
typedef signed char BOOL;
typedef struct _NSZone NSZone;
@@ -310,3 +312,14 @@
// here either.
[subviews addObject:view]; // no-warning
}
+
+NSString *getStringFromString(NSString *string) {
+ if (!string)
+ return nil;
+ return @"New String";
+}
+void testInlinedDefensiveCheck(NSMutableDictionary *dict, id obj) {
+ // The check in getStringFromString() is not a good indication
+ // that 'obj' can be nil in this context.
+ dict[obj] = getStringFromString(obj); // no-warning
+}
Index: clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
===================================================================
--- clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
+++ clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
@@ -1601,9 +1601,6 @@
AnalyzerOptions &Options = N->getState()->getAnalysisManager().options;
if (!Options.ShouldSuppressInlinedDefensiveChecks)
IsSatisfied = true;
-
- assert(N->getState()->isNull(V).isConstrainedTrue() &&
- "The visitor only tracks the cases where V is constrained to 0");
}
void SuppressInlineDefensiveChecksVisitor::Profile(
@@ -1634,13 +1631,12 @@
// Check if in the previous state it was feasible for this value
// to *not* be null.
- if (!Pred->getState()->isNull(V).isConstrainedTrue()) {
+ if (!Pred->getState()->isNull(V).isConstrainedTrue() &&
+ Succ->getState()->isNull(V).isConstrainedTrue()) {
IsSatisfied = true;
- assert(Succ->getState()->isNull(V).isConstrainedTrue());
-
// Check if this is inlined defensive checks.
- const LocationContext *CurLC =Succ->getLocationContext();
+ const LocationContext *CurLC = Succ->getLocationContext();
const LocationContext *ReportLC = BR.getErrorNode()->getLocationContext();
if (CurLC != ReportLC && !CurLC->isParentOf(ReportLC)) {
BR.markInvalid("Suppress IDC", CurLC);
@@ -2007,11 +2003,16 @@
// Add visitor, which will suppress inline defensive checks.
if (auto DV = V.getAs<DefinedSVal>())
- if (!DV->isZeroConstant() && LVState->isNull(*DV).isConstrainedTrue() &&
- EnableNullFPSuppression)
+ if (!DV->isZeroConstant() && EnableNullFPSuppression) {
+ // Note that LVNode may be too late; the lvalue may have been computed
+ // before the inlined call was evaluated. InputNode may as well be
+ // too late, because the symbol is already dead; this, however,
+ // is fine because we can still find the node in which it
+ // collapsed to null previously.
report.addVisitor(
- std::make_unique<SuppressInlineDefensiveChecksVisitor>(*DV,
- LVNode));
+ std::make_unique<SuppressInlineDefensiveChecksVisitor>(
+ *DV, InputNode));
+ }
if (auto KV = V.getAs<KnownSVal>())
report.addVisitor(std::make_unique<FindLastStoreBRVisitor>(
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
