NoQ updated this revision to Diff 181961.
NoQ marked 12 inline comments as done.
NoQ added a comment.
Fix stuff.
In D56632#1356163 <https://reviews.llvm.org/D56632#1356163>,
@baloghadamsoftware wrote:
> Did you measure decrease in the false-positive rate or an increase in the
> true-positive rate on real code? I expect some.
In progress :)
In D56632#1356249 <https://reviews.llvm.org/D56632#1356249>, @xazax.hun wrote:
> > When a memory region is live, all its sub-regions and super-regions are
> > automatically live (and vice versa), so it is only necessary to track
> > liveness of base regions.
>
>
>
> I think this is non-obvious. If we had all the information it would make
> perfect sense to have a dead field in an alive struct. But since the liveness
> analysis is intraprocedural and we cannot know what happens to a struct when
> it escapes we have no choice but keep the field alive. A more sophisticated
> analysis (which is also likely to be more expensive) could have dead fields
> in an alive struct in case the struct never escapes.
Great point indeed! I vaguely remember that some other tools do actually work
that way. If a field is not referenced anywhere in the path and there's no
weird pointer arithmetic going on upon the variable, we can actually diagnose a
leak of the value within the field *before* the variable itself dies. Even
intraprocedurally, we can just handle escapes and still do slightly better than
we do now. This gets really valuable when the variable is, say, a non-escaping
static (local or global). The variable never dies, but there may still be no
deallocation for the field anywhere in the code and we may be able to see this
within the translation unit. This doesn't have to have anything to do with
fields though, the variable itself may carry the leaking pointer. Same with
private fields regardless of storage. Neat food for thought.
Added a comment. Is there anything else worth documenting here, other than "the
whole damn thing"?
>> The answer is yes, it does work correctly, because scanReachableSymbols
>> always scans the whole super-region chain of every region. Which means that
>> every time the Environment or the Store marks a region as live, all of its
>> super-regions are added to RegionRoots. However, i would still like to add
>> conversion to the base region into markLive(), because this behavior is
>> something that should be guaranteed by SymbolReaper rather implied by
>> callers manually, even if the callers have to do that anyway.
>
> I did not really follow, but probably my understanding of how memory regions
> work is not correct. If we work with base regions, why do we still need to
> scan the whole super-region chain?
It's just that `scanReachableSymbols` is written that way, and it's used
everywhere for these kind of purposes. I.e., we (i.e., `SymbolReaper`) don't
(i.e., doesn't) need to scan the whole chain of regions (apart from the
`markElementIndicesLive()` thing... wait a minute, does it work in the opposite
direction? - i.e., if an array-typed region is live, does it automatically mean
that all index symbols in all element regions within it are actually treated as
live everywhere in the program state? - need to check), but this behavior is
re-used from other users `scanReachableSymbols` (wait a minute, do any of the
other users actually need that? - i'm not immediately seeing any user actually
need that, it seems that *everybody* operates on base regions only - but
probably it's anyway not that much slower than jumping to the base region
directly - need to check).
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D56632/new/
https://reviews.llvm.org/D56632
Files:
include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
lib/StaticAnalyzer/Core/ExprEngine.cpp
lib/StaticAnalyzer/Core/SymbolManager.cpp
test/Analysis/diagnostics/dtors.cpp
test/Analysis/symbol-reaper.cpp
unittests/StaticAnalyzer/CMakeLists.txt
unittests/StaticAnalyzer/SymbolReaperTest.cpp
Index: unittests/StaticAnalyzer/SymbolReaperTest.cpp
===================================================================
--- /dev/null
+++ unittests/StaticAnalyzer/SymbolReaperTest.cpp
@@ -0,0 +1,121 @@
+//===- unittests/StaticAnalyzer/SymbolReaperTest.cpp ----------------------===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/Frontend/CompilerInstance.h"
+#include "clang/StaticAnalyzer/Core/BugReporter/BugReporter.h"
+#include "clang/StaticAnalyzer/Core/BugReporter/BugType.h"
+#include "clang/CrossTU/CrossTranslationUnit.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h"
+#include "clang/StaticAnalyzer/Frontend/AnalysisConsumer.h"
+#include "clang/Tooling/Tooling.h"
+#include "gtest/gtest.h"
+
+namespace clang {
+namespace ento {
+namespace {
+
+using namespace ast_matchers;
+
+// A re-usable consumer that constructs ExprEngine out of CompilerInvocation.
+// TODO: Actually re-use it when we write our second test.
+class ExprEngineConsumer : public ASTConsumer {
+protected:
+ CompilerInstance &C;
+
+private:
+ // We need to construct all of these in order to construct ExprEngine.
+ CheckerManager ChkMgr;
+ cross_tu::CrossTranslationUnitContext CTU;
+ PathDiagnosticConsumers Consumers;
+ AnalysisManager AMgr;
+ SetOfConstDecls VisitedCallees;
+ FunctionSummariesTy FS;
+
+protected:
+ ExprEngine Eng;
+
+ // Find a declaration in the current AST by name. This has nothing to do
+ // with ExprEngine but turns out to be handy.
+ // TODO: There's probably a better place for it.
+ template <typename T>
+ const T *findDeclByName(const Decl *Where, StringRef Name) {
+ auto Matcher = decl(hasDescendant(namedDecl(hasName(Name)).bind("d")));
+ auto Matches = match(Matcher, *Where, Eng.getContext());
+ assert(Matches.size() == 1 && "Ambiguous name!");
+ const T *Node = selectFirst<T>("d", Matches);
+ assert(Node && "Name not found!");
+ return Node;
+ }
+
+public:
+ ExprEngineConsumer(CompilerInstance &C)
+ : C(C), ChkMgr(C.getASTContext(), *C.getAnalyzerOpts()), CTU(C),
+ Consumers(),
+ AMgr(C.getASTContext(), C.getDiagnostics(), Consumers,
+ CreateRegionStoreManager, CreateRangeConstraintManager, &ChkMgr,
+ *C.getAnalyzerOpts()),
+ VisitedCallees(), FS(),
+ Eng(CTU, AMgr, &VisitedCallees, &FS, ExprEngine::Inline_Regular) {}
+};
+
+class SuperRegionLivenessConsumer : public ExprEngineConsumer {
+ void performTest(const Decl *D) {
+ const auto *FD = findDeclByName<FieldDecl>(D, "x");
+ const auto *VD = findDeclByName<VarDecl>(D, "s");
+ assert(FD && VD);
+
+ // The variable must belong to a stack frame,
+ // otherwise SymbolReaper would think it's a global.
+ const StackFrameContext *SFC =
+ Eng.getAnalysisDeclContextManager().getStackFrame(D);
+
+ // Create regions for 's' and 's.x'.
+ const VarRegion *VR = Eng.getRegionManager().getVarRegion(VD, SFC);
+ const FieldRegion *FR = Eng.getRegionManager().getFieldRegion(FD, VR);
+
+ // Pass a null location context to the SymbolReaper so that
+ // it was thinking that the variable is dead.
+ SymbolReaper SymReaper((StackFrameContext *)nullptr, (Stmt *)nullptr,
+ Eng.getSymbolManager(), Eng.getStoreManager());
+
+ SymReaper.markLive(FR);
+ EXPECT_TRUE(SymReaper.isLiveRegion(VR));
+ }
+
+public:
+ SuperRegionLivenessConsumer(CompilerInstance &C) : ExprEngineConsumer(C) {}
+ ~SuperRegionLivenessConsumer() override {}
+
+ bool HandleTopLevelDecl(DeclGroupRef DG) override {
+ for (const auto *D : DG)
+ performTest(D);
+ return true;
+ }
+};
+
+class SuperRegionLivenessAction: public ASTFrontendAction {
+public:
+ SuperRegionLivenessAction() {}
+ std::unique_ptr<ASTConsumer> CreateASTConsumer(CompilerInstance &Compiler,
+ StringRef File) override {
+ auto Consumer = llvm::make_unique<SuperRegionLivenessConsumer>(Compiler);
+ return Consumer;
+ }
+};
+
+// Test that marking s.x as live would also make s live.
+TEST(SymbolReaper, SuperRegionLiveness) {
+ EXPECT_TRUE(tooling::runToolOnCode(new SuperRegionLivenessAction,
+ "void foo() { struct S { int x; } s; }"));
+}
+
+} // namespace
+} // namespace ento
+} // namespace clang
Index: unittests/StaticAnalyzer/CMakeLists.txt
===================================================================
--- unittests/StaticAnalyzer/CMakeLists.txt
+++ unittests/StaticAnalyzer/CMakeLists.txt
@@ -5,6 +5,7 @@
add_clang_unittest(StaticAnalysisTests
AnalyzerOptionsTest.cpp
RegisterCustomCheckersTest.cpp
+ SymbolReaperTest.cpp
)
target_link_libraries(StaticAnalysisTests
Index: test/Analysis/symbol-reaper.cpp
===================================================================
--- /dev/null
+++ test/Analysis/symbol-reaper.cpp
@@ -0,0 +1,60 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -verify %s
+
+void clang_analyzer_eval(int);
+void clang_analyzer_warnOnDeadSymbol(int);
+
+namespace test_dead_region_with_live_subregion_in_environment {
+int glob;
+
+struct A {
+ int x;
+
+ void foo() {
+ // FIXME: Maybe just let clang_analyzer_eval() work within callees already?
+ // The glob variable shouldn't keep our symbol alive because
+ // 'x != 0' is concrete 'true'.
+ glob = (x != 0);
+ }
+};
+
+void test_A(A a) {
+ if (a.x == 0)
+ return;
+
+ clang_analyzer_warnOnDeadSymbol(a.x);
+
+ // What we're testing is that a.x is alive until foo() exits.
+ a.foo(); // no-warning // (i.e., no 'SYMBOL DEAD' yet)
+
+ // Let's see if constraints on a.x were known within foo().
+ clang_analyzer_eval(glob); // expected-warning{{TRUE}}
+ // expected-warning@-1{{SYMBOL DEAD}}
+}
+
+struct B {
+ A a;
+ int y;
+};
+
+A &noop(A &a) {
+ // This function ensures that the 'b' expression within its argument
+ // would be cleaned up before its call, so that only 'b.a' remains
+ // in the Environment.
+ return a;
+}
+
+
+void test_B(B b) {
+ if (b.a.x == 0)
+ return;
+
+ clang_analyzer_warnOnDeadSymbol(b.a.x);
+
+ // What we're testing is that b.a.x is alive until foo() exits.
+ noop(b.a).foo(); // no-warning // (i.e., no 'SYMBOL DEAD' yet)
+
+ // Let's see if constraints on a.x were known within foo().
+ clang_analyzer_eval(glob); // expected-warning{{TRUE}}
+ // expected-warning@-1{{SYMBOL DEAD}}
+}
+} // namespace test_dead_region_with_live_subregion_in_environment
Index: test/Analysis/diagnostics/dtors.cpp
===================================================================
--- test/Analysis/diagnostics/dtors.cpp
+++ test/Analysis/diagnostics/dtors.cpp
@@ -1,9 +1,11 @@
-// RUN: %clang_analyze_cc1 -w -analyzer-checker=core,cplusplus -verify %s
-
-// expected-no-diagnostics
+// RUN: %clang_analyze_cc1 -w -analyzer-checker=core,cplusplus -analyzer-output=text -verify %s
namespace no_crash_on_delete_dtor {
-// We were crashing when producing diagnostics for this code.
+// We were crashing when producing diagnostics for this code, but not for the
+// report that it currently emits. Instead, Static Analyzer was thinking that
+// p.get()->foo() is a null dereference because it was dropping
+// constraints over x too early and took a different branch next time
+// we call .get().
struct S {
void foo();
~S();
@@ -14,12 +16,15 @@
S *s;
smart_ptr(S *);
S *get() {
- return (x || 0) ? nullptr : s;
+ return (x || 0) ? nullptr : s; // expected-note{{Left side of '||' is false}}
+ // expected-note@-1{{'?' condition is false}}
+ // expected-warning@-2{{Use of memory after it is freed}}
+ // expected-note@-3{{Use of memory after it is freed}}
}
};
void bar(smart_ptr p) {
- delete p.get();
- p.get()->foo();
+ delete p.get(); // expected-note{{Memory is released}}
+ p.get()->foo(); // expected-note{{Calling 'smart_ptr::get'}}
}
} // namespace no_crash_on_delete_dtor
Index: lib/StaticAnalyzer/Core/SymbolManager.cpp
===================================================================
--- lib/StaticAnalyzer/Core/SymbolManager.cpp
+++ lib/StaticAnalyzer/Core/SymbolManager.cpp
@@ -405,7 +405,7 @@
}
void SymbolReaper::markLive(const MemRegion *region) {
- RegionRoots.insert(region);
+ RegionRoots.insert(region->getBaseRegion());
markElementIndicesLive(region);
}
@@ -426,11 +426,15 @@
}
bool SymbolReaper::isLiveRegion(const MemRegion *MR) {
+ // TODO: For now, liveness of a memory region is equivalent to liveness of its
+ // base region. In fact we can do a bit better: say, if a particular FieldDecl
+ // is not used later in the path, we can diagnose a leak of a value within
+ // that field earlier than, say, the variable that contains the field dies.
+ MR = MR->getBaseRegion();
+
if (RegionRoots.count(MR))
return true;
- MR = MR->getBaseRegion();
-
if (const auto *SR = dyn_cast<SymbolicRegion>(MR))
return isLive(SR->getSymbol());
Index: lib/StaticAnalyzer/Core/ExprEngine.cpp
===================================================================
--- lib/StaticAnalyzer/Core/ExprEngine.cpp
+++ lib/StaticAnalyzer/Core/ExprEngine.cpp
@@ -198,7 +198,9 @@
mgr.getConstraintManagerCreator(), G.getAllocator(),
this),
SymMgr(StateMgr.getSymbolManager()),
- svalBuilder(StateMgr.getSValBuilder()), ObjCNoRet(mgr.getASTContext()),
+ MRMgr(StateMgr.getRegionManager()),
+ svalBuilder(StateMgr.getSValBuilder()),
+ ObjCNoRet(mgr.getASTContext()),
BR(mgr, *this),
VisitedCallees(VisitedCalleesIn), HowToInline(HowToInlineIn) {
unsigned TrimInterval = mgr.options.GraphTrimInterval;
Index: include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
===================================================================
--- include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
+++ include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
@@ -131,6 +131,9 @@
/// SymMgr - Object that manages the symbol information.
SymbolManager &SymMgr;
+ /// MRMgr - MemRegionManager object that creates memory regions.
+ MemRegionManager &MRMgr;
+
/// svalBuilder - SValBuilder object that creates SVals from expressions.
SValBuilder &svalBuilder;
@@ -180,6 +183,10 @@
AnalysisManager &getAnalysisManager() override { return AMgr; }
+ AnalysisDeclContextManager &getAnalysisDeclContextManager() {
+ return AMgr.getAnalysisDeclContextManager();
+ }
+
CheckerManager &getCheckerManager() const {
return *AMgr.getCheckerManager();
}
@@ -387,9 +394,9 @@
return StateMgr.getBasicVals();
}
- // FIXME: Remove when we migrate over to just using ValueManager.
SymbolManager &getSymbolManager() { return SymMgr; }
- const SymbolManager &getSymbolManager() const { return SymMgr; }
+ MemRegionManager &getRegionManager() { return MRMgr; }
+
// Functions for external checking of whether we have unfinished work
bool wasBlocksExhausted() const { return Engine.wasBlocksExhausted(); }
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
