maxjhandsome
/
llvm-project
forked from OSchip/llvm-project
1
0
Fork 0
Code Issues Pull Requests Packages Releases Wiki Activity

[analyzer] When creating a temporary object, properly copy the value into it. 

Adjustments should be considered properly; we should copy the unadjusted object
over the whole temporary base region. If the unadjusted object is no longer
available in the Environment, invalidate the temporary base region, and then
copy the adjusted object into the adjusted sub-region of the temporary region.

This fixes a regression introduced by r288263, that caused various
false positives, due to copying only adjusted object into the adjusted region;
the rest of the base region therefore remained undefined.
Before r288263, the adjusted value was copied over the unadjusted region,
which is incorrect, but accidentally worked better due to how region store
disregards compound value bindings to non-base regions.

An additional test machinery is introduced to make sure that despite making
two binds, we only notify checkers once for both of them, without exposing
the partially copied objects.

This fix is a hack over a hack. The proper fix would be to model C++ temporaries
in the CFG, and after that dealing with adjustments would no longer be
necessary, and the values we need would no longer disappear from the
Environment.

rdar://problem/30658168

Differential Revision: https://reviews.llvm.org/D30534

llvm-svn: 298924
This commit is contained in:
Artem Dergachev 2017-03-28 15:43:26 +00:00
parent 5b414ebe63
commit 647804a84a
5 changed files with 160 additions and 30 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

14
clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
View File

@ -621,16 +621,16 @@ private:
void performTrivialCopy(NodeBuilder &Bldr, ExplodedNode *Pred,
const CallEvent &Call);
/// If the value of the given expression is a NonLoc, copy it into a new
/// temporary object region, and replace the value of the expression with
/// that.
/// If the value of the given expression \p InitWithAdjustments is a NonLoc,
/// copy it into a new temporary object region, and replace the value of the
/// expression with that.
///
/// If \p ResultE is provided, the new region will be bound to this expression
/// instead of \p E.
/// If \p Result is provided, the new region will be bound to this expression
/// instead of \p InitWithAdjustments.
ProgramStateRef createTemporaryRegionIfNeeded(ProgramStateRef State,
const LocationContext *LC,
const Expr *E,
const Expr *ResultE = nullptr);
const Expr *InitWithAdjustments,
const Expr *Result = nullptr);
/// For a DeclStmt or CXXInitCtorInitializer, walk backward in the current CFG
/// block to find the constructor expression that directly constructed into

45
clang/lib/StaticAnalyzer/Checkers/AnalysisOrderChecker.cpp
View File

@ -24,16 +24,29 @@ using namespace ento;
namespace {
class AnalysisOrderChecker : public Checker< check::PreStmt<CastExpr>,
class AnalysisOrderChecker
: public Checker<check::PreStmt<CastExpr>,
check::PostStmt<CastExpr>,
check::PreStmt<ArraySubscriptExpr>,
check::PostStmt<ArraySubscriptExpr>> {
bool isCallbackEnabled(CheckerContext &C, StringRef CallbackName) const {
AnalyzerOptions &Opts = C.getAnalysisManager().getAnalyzerOptions();
check::PostStmt<ArraySubscriptExpr>,
check::Bind,
check::RegionChanges> {
bool isCallbackEnabled(AnalyzerOptions &Opts, StringRef CallbackName) const {
return Opts.getBooleanOption("*", false, this) ||
Opts.getBooleanOption(CallbackName, false, this);
}
bool isCallbackEnabled(CheckerContext &C, StringRef CallbackName) const {
AnalyzerOptions &Opts = C.getAnalysisManager().getAnalyzerOptions();
return isCallbackEnabled(Opts, CallbackName);
}
bool isCallbackEnabled(ProgramStateRef State, StringRef CallbackName) const {
AnalyzerOptions &Opts = State->getStateManager().getOwningEngine()
->getAnalysisManager().getAnalyzerOptions();
return isCallbackEnabled(Opts, CallbackName);
}
public:
void checkPreStmt(const CastExpr *CE, CheckerContext &C) const {
if (isCallbackEnabled(C, "PreStmtCastExpr"))
@ -47,18 +60,36 @@ public:
<< ")\n";
}
void checkPreStmt(const ArraySubscriptExpr *SubExpr, CheckerContext &C) const {
void checkPreStmt(const ArraySubscriptExpr *SubExpr,
CheckerContext &C) const {
if (isCallbackEnabled(C, "PreStmtArraySubscriptExpr"))
llvm::errs() << "PreStmt<ArraySubscriptExpr>\n";
}
void checkPostStmt(const ArraySubscriptExpr *SubExpr, CheckerContext &C) const {
void checkPostStmt(const ArraySubscriptExpr *SubExpr,
CheckerContext &C) const {
if (isCallbackEnabled(C, "PostStmtArraySubscriptExpr"))
llvm::errs() << "PostStmt<ArraySubscriptExpr>\n";
}
};
void checkBind(SVal Loc, SVal Val, const Stmt *S, CheckerContext &C) const {
if (isCallbackEnabled(C, "Bind"))
llvm::errs() << "Bind\n";
}
ProgramStateRef
checkRegionChanges(ProgramStateRef State,
const InvalidatedSymbols *Invalidated,
ArrayRef<const MemRegion *> ExplicitRegions,
ArrayRef<const MemRegion *> Regions,
const LocationContext *LCtx, const CallEvent *Call) const {
if (isCallbackEnabled(State, "RegionChanges"))
llvm::errs() << "RegionChanges\n";
return State;
}
};
} // end anonymous namespace
//===----------------------------------------------------------------------===//
// Registration.
//===----------------------------------------------------------------------===//

62
clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
View File

@ -182,19 +182,25 @@ ProgramStateRef ExprEngine::getInitialState(const LocationContext *InitLoc) {
ProgramStateRef
ExprEngine::createTemporaryRegionIfNeeded(ProgramStateRef State,
const LocationContext *LC,
const Expr *Ex,
const Expr *InitWithAdjustments,
const Expr *Result) {
SVal V = State->getSVal(Ex, LC);
// FIXME: This function is a hack that works around the quirky AST
// we're often having with respect to C++ temporaries. If only we modelled
// the actual execution order of statements properly in the CFG,
// all the hassle with adjustments would not be necessary,
// and perhaps the whole function would be removed.
SVal InitValWithAdjustments = State->getSVal(InitWithAdjustments, LC);
if (!Result) {
// If we don't have an explicit result expression, we're in "if needed"
// mode. Only create a region if the current value is a NonLoc.
if (!V.getAs<NonLoc>())
if (!InitValWithAdjustments.getAs<NonLoc>())
return State;
Result = Ex;
Result = InitWithAdjustments;
} else {
// We need to create a region no matter what. For sanity, make sure we don't
// try to stuff a Loc into a non-pointer temporary region.
assert(!V.getAs<Loc>() || Loc::isLocType(Result->getType()) ||
assert(!InitValWithAdjustments.getAs<Loc>() ||
Loc::isLocType(Result->getType()) ||
Result->getType()->isMemberPointerType());
}
@ -226,7 +232,8 @@ ExprEngine::createTemporaryRegionIfNeeded(ProgramStateRef State,
SmallVector<const Expr *, 2> CommaLHSs;
SmallVector<SubobjectAdjustment, 2> Adjustments;
const Expr *Init = Ex->skipRValueSubobjectAdjustments(CommaLHSs, Adjustments);
const Expr *Init = InitWithAdjustments->skipRValueSubobjectAdjustments(
CommaLHSs, Adjustments);
const TypedValueRegion *TR = nullptr;
if (const MaterializeTemporaryExpr *MT =
@ -241,6 +248,7 @@ ExprEngine::createTemporaryRegionIfNeeded(ProgramStateRef State,
TR = MRMgr.getCXXTempObjectRegion(Init, LC);
SVal Reg = loc::MemRegionVal(TR);
SVal BaseReg = Reg;
// Make the necessary adjustments to obtain the sub-object.
for (auto I = Adjustments.rbegin(), E = Adjustments.rend(); I != E; ++I) {
@ -254,19 +262,47 @@ ExprEngine::createTemporaryRegionIfNeeded(ProgramStateRef State,
break;
case SubobjectAdjustment::MemberPointerAdjustment:
// FIXME: Unimplemented.
State->bindDefault(Reg, UnknownVal(), LC);
State = State->bindDefault(Reg, UnknownVal(), LC);
return State;
}
}
// Try to recover some path sensitivity in case we couldn't compute the value.
if (V.isUnknown())
V = getSValBuilder().conjureSymbolVal(Result, LC, TR->getValueType(),
// What remains is to copy the value of the object to the new region.
// FIXME: In other words, what we should always do is copy value of the
// Init expression (which corresponds to the bigger object) to the whole
// temporary region TR. However, this value is often no longer present
// in the Environment. If it has disappeared, we instead invalidate TR.
// Still, what we can do is assign the value of expression Ex (which
// corresponds to the sub-object) to the TR's sub-region Reg. At least,
// values inside Reg would be correct.
SVal InitVal = State->getSVal(Init, LC);
if (InitVal.isUnknown()) {
InitVal = getSValBuilder().conjureSymbolVal(Result, LC, Init->getType(),
currBldrCtx->blockCount());
// Bind the value of the expression to the sub-object region, and then bind
// the sub-object region to our expression.
State = State->bindLoc(Reg, V, LC);
State = State->bindLoc(BaseReg.castAs<Loc>(), InitVal, LC, false);
// Then we'd need to take the value that certainly exists and bind it over.
if (InitValWithAdjustments.isUnknown()) {
// Try to recover some path sensitivity in case we couldn't
// compute the value.
InitValWithAdjustments = getSValBuilder().conjureSymbolVal(
Result, LC, InitWithAdjustments->getType(),
currBldrCtx->blockCount());
}
State =
State->bindLoc(Reg.castAs<Loc>(), InitValWithAdjustments, LC, false);
} else {
State = State->bindLoc(BaseReg.castAs<Loc>(), InitVal, LC, false);
}
// The result expression would now point to the correct sub-region of the
// newly created temporary region. Do this last in order to getSVal of Init
// correctly in case (Result == Init).
State = State->BindExpr(Result, LC, Reg);
// Notify checkers once for two bindLoc()s.
State = processRegionChange(State, TR, LC);
return State;
}

36
clang/test/Analysis/temporaries-callback-order.cpp Normal file
View File

@ -0,0 +1,36 @@
// RUN: %clang_cc1 -analyze -analyzer-checker=debug.AnalysisOrder -analyzer-config debug.AnalysisOrder:Bind=true -analyzer-config debug.AnalysisOrder:RegionChanges=true %s 2>&1 | FileCheck %s
struct Super {
virtual void m();
};
struct Sub : Super {
virtual void m() {}
};
void testTemporaries() {
// This triggers RegionChanges twice:
// - Once for zero-initialization of the structure.
// - Once for creating a temporary region and copying the structure there.
// FIXME: This code shouldn't really produce the extra temporary, however
// that's how we behave for now.
Sub().m();
}
void seeIfCheckBindWorks() {
// This should trigger checkBind. The rest of the code shouldn't.
// This also triggers checkRegionChanges after that.
// Note that this function is analyzed first, so the messages would be on top.
int x = 1;
}
// seeIfCheckBindWorks():
// CHECK: Bind
// CHECK-NEXT: RegionChanges
// testTemporaries():
// CHECK-NEXT: RegionChanges
// CHECK-NEXT: RegionChanges
// Make sure there's no further output.
// CHECK-NOT: Bind
// CHECK-NOT: RegionChanges

27
clang/test/Analysis/temporaries.cpp
View File

@ -503,3 +503,30 @@ namespace PR32088 {
});
}
}
namespace CopyToTemporaryCorrectly {
class Super {
public:
void m() {
mImpl();
}
virtual void mImpl() = 0;
};
class Sub : public Super {
public:
Sub(const int &p) : j(p) {}
virtual void mImpl() override {
// Used to be undefined pointer dereference because we didn't copy
// the subclass data (j) to the temporary object properly.
(void)(j + 1); // no-warning
if (j != 22) {
clang_analyzer_warnIfReached(); // no-warning
}
}
const int &j;
};
void run() {
int i = 22;
Sub(i).m();
}
}