[clang] reland 87320 (PR #88316)
via cfe-commits
cfe-commits at lists.llvm.org
Wed Apr 10 12:43:50 PDT 2024
https://github.com/martinboehme created https://github.com/llvm/llvm-project/pull/88316
- **Reapply "[clang][dataflow] Propagate locations from result objects to initializers." (#88315)**
- **Remove now-unused function `isOriginalRecordConstructor()`.**
>From 26c36aa39d34f25ca8f787525ba4470f59263184 Mon Sep 17 00:00:00 2001
From: Martin Braenne <mboehme at google.com>
Date: Wed, 10 Apr 2024 19:32:47 +0000
Subject: [PATCH 1/2] Reapply "[clang][dataflow] Propagate locations from
result objects to initializers." (#88315)
This reverts commit 7549b45825a05fc24fcdbacf006461165aa042cb.
---
.../FlowSensitive/DataflowEnvironment.h | 64 ++-
.../FlowSensitive/DataflowEnvironment.cpp | 405 +++++++++++++-----
clang/lib/Analysis/FlowSensitive/Transfer.cpp | 176 ++++----
.../TypeErasedDataflowAnalysis.cpp | 13 +-
.../FlowSensitive/DataflowEnvironmentTest.cpp | 43 ++
.../Analysis/FlowSensitive/TransferTest.cpp | 172 +++++---
6 files changed, 590 insertions(+), 283 deletions(-)
diff --git a/clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h b/clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h
index 9a65f76cdf56bc..706664d7db1c25 100644
--- a/clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h
+++ b/clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h
@@ -30,6 +30,7 @@
#include "llvm/ADT/MapVector.h"
#include "llvm/Support/Compiler.h"
#include "llvm/Support/ErrorHandling.h"
+#include <memory>
#include <type_traits>
#include <utility>
@@ -344,17 +345,6 @@ class Environment {
/// location of the result object to pass in `this`, even though prvalues are
/// otherwise not associated with storage locations.
///
- /// FIXME: Currently, this simply returns a stable storage location for `E`,
- /// but this doesn't do the right thing in scenarios like the following:
- /// ```
- /// MyClass c = some_condition()? MyClass(foo) : MyClass(bar);
- /// ```
- /// Here, `MyClass(foo)` and `MyClass(bar)` will have two different storage
- /// locations, when in fact their storage locations should be the same.
- /// Eventually, we want to propagate storage locations from result objects
- /// down to the prvalues that initialize them, similar to the way that this is
- /// done in Clang's CodeGen.
- ///
/// Requirements:
/// `E` must be a prvalue of record type.
RecordStorageLocation &
@@ -462,7 +452,13 @@ class Environment {
/// Initializes the fields (including synthetic fields) of `Loc` with values,
/// unless values of the field type are not supported or we hit one of the
/// limits at which we stop producing values.
- void initializeFieldsWithValues(RecordStorageLocation &Loc);
+ /// If `Type` is provided, initializes only those fields that are modeled for
+ /// `Type`; this is intended for use in cases where `Loc` is a derived type
+ /// and we only want to initialize the fields of a base type.
+ void initializeFieldsWithValues(RecordStorageLocation &Loc, QualType Type);
+ void initializeFieldsWithValues(RecordStorageLocation &Loc) {
+ initializeFieldsWithValues(Loc, Loc.getType());
+ }
/// Assigns `Val` as the value of `Loc` in the environment.
void setValue(const StorageLocation &Loc, Value &Val);
@@ -653,6 +649,9 @@ class Environment {
LLVM_DUMP_METHOD void dump(raw_ostream &OS) const;
private:
+ using PrValueToResultObject =
+ llvm::DenseMap<const Expr *, RecordStorageLocation *>;
+
// The copy-constructor is for use in fork() only.
Environment(const Environment &) = default;
@@ -682,8 +681,10 @@ class Environment {
/// Initializes the fields (including synthetic fields) of `Loc` with values,
/// unless values of the field type are not supported or we hit one of the
/// limits at which we stop producing values (controlled by `Visited`,
- /// `Depth`, and `CreatedValuesCount`).
- void initializeFieldsWithValues(RecordStorageLocation &Loc,
+ /// `Depth`, and `CreatedValuesCount`). If `Type` is different from
+ /// `Loc.getType()`, initializes only those fields that are modeled for
+ /// `Type`.
+ void initializeFieldsWithValues(RecordStorageLocation &Loc, QualType Type,
llvm::DenseSet<QualType> &Visited, int Depth,
int &CreatedValuesCount);
@@ -702,22 +703,45 @@ class Environment {
/// and functions referenced in `FuncDecl`. `FuncDecl` must have a body.
void initFieldsGlobalsAndFuncs(const FunctionDecl *FuncDecl);
+ static PrValueToResultObject
+ buildResultObjectMap(DataflowAnalysisContext *DACtx,
+ const FunctionDecl *FuncDecl,
+ RecordStorageLocation *ThisPointeeLoc,
+ RecordStorageLocation *LocForRecordReturnVal);
+
// `DACtx` is not null and not owned by this object.
DataflowAnalysisContext *DACtx;
- // FIXME: move the fields `CallStack`, `ReturnVal`, `ReturnLoc` and
- // `ThisPointeeLoc` into a separate call-context object, shared between
- // environments in the same call.
+ // FIXME: move the fields `CallStack`, `ResultObjectMap`, `ReturnVal`,
+ // `ReturnLoc` and `ThisPointeeLoc` into a separate call-context object,
+ // shared between environments in the same call.
// https://github.com/llvm/llvm-project/issues/59005
// `DeclContext` of the block being analysed if provided.
std::vector<const DeclContext *> CallStack;
- // Value returned by the function (if it has non-reference return type).
+ // Maps from prvalues of record type to their result objects. Shared between
+ // all environments for the same function.
+ // FIXME: It's somewhat unsatisfactory that we have to use a `shared_ptr`
+ // here, though the cost is acceptable: The overhead of a `shared_ptr` is
+ // incurred when it is copied, and this happens only relatively rarely (when
+ // we fork the environment). The need for a `shared_ptr` will go away once we
+ // introduce a shared call-context object (see above).
+ std::shared_ptr<PrValueToResultObject> ResultObjectMap;
+
+ // The following three member variables handle various different types of
+ // return values.
+ // - If the return type is not a reference and not a record: Value returned
+ // by the function.
Value *ReturnVal = nullptr;
- // Storage location of the reference returned by the function (if it has
- // reference return type).
+ // - If the return type is a reference: Storage location of the reference
+ // returned by the function.
StorageLocation *ReturnLoc = nullptr;
+ // - If the return type is a record or the function being analyzed is a
+ // constructor: Storage location into which the return value should be
+ // constructed.
+ RecordStorageLocation *LocForRecordReturnVal = nullptr;
+
// The storage location of the `this` pointee. Should only be null if the
// function being analyzed is only a function and not a method.
RecordStorageLocation *ThisPointeeLoc = nullptr;
diff --git a/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp b/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
index 1bfa7ebcfd50c9..6c796b4ad923e8 100644
--- a/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
+++ b/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
@@ -15,6 +15,7 @@
#include "clang/Analysis/FlowSensitive/DataflowEnvironment.h"
#include "clang/AST/Decl.h"
#include "clang/AST/DeclCXX.h"
+#include "clang/AST/RecursiveASTVisitor.h"
#include "clang/AST/Type.h"
#include "clang/Analysis/FlowSensitive/DataflowLattice.h"
#include "clang/Analysis/FlowSensitive/Value.h"
@@ -26,6 +27,8 @@
#include <cassert>
#include <utility>
+#define DEBUG_TYPE "dataflow"
+
namespace clang {
namespace dataflow {
@@ -354,6 +357,8 @@ getFieldsGlobalsAndFuncs(const Stmt &S, FieldSet &Fields,
for (auto *Child : S.children())
if (Child != nullptr)
getFieldsGlobalsAndFuncs(*Child, Fields, Vars, Funcs);
+ if (const auto *DefaultArg = dyn_cast<CXXDefaultArgExpr>(&S))
+ getFieldsGlobalsAndFuncs(*DefaultArg->getExpr(), Fields, Vars, Funcs);
if (const auto *DefaultInit = dyn_cast<CXXDefaultInitExpr>(&S))
getFieldsGlobalsAndFuncs(*DefaultInit->getExpr(), Fields, Vars, Funcs);
@@ -386,6 +391,186 @@ getFieldsGlobalsAndFuncs(const Stmt &S, FieldSet &Fields,
}
}
+namespace {
+
+// Visitor that builds a map from record prvalues to result objects.
+// This traverses the body of the function to be analyzed; for each result
+// object that it encounters, it propagates the storage location of the result
+// object to all record prvalues that can initialize it.
+class ResultObjectVisitor : public RecursiveASTVisitor<ResultObjectVisitor> {
+public:
+ // `ResultObjectMap` will be filled with a map from record prvalues to result
+ // object. If the function being analyzed returns a record by value,
+ // `LocForRecordReturnVal` is the location to which this record should be
+ // written; otherwise, it is null.
+ explicit ResultObjectVisitor(
+ llvm::DenseMap<const Expr *, RecordStorageLocation *> &ResultObjectMap,
+ RecordStorageLocation *LocForRecordReturnVal,
+ DataflowAnalysisContext &DACtx)
+ : ResultObjectMap(ResultObjectMap),
+ LocForRecordReturnVal(LocForRecordReturnVal), DACtx(DACtx) {}
+
+ bool shouldVisitImplicitCode() { return true; }
+
+ bool shouldVisitLambdaBody() const { return false; }
+
+ // Traverse all member and base initializers of `Ctor`. This function is not
+ // called by `RecursiveASTVisitor`; it should be called manually if we are
+ // analyzing a constructor. `ThisPointeeLoc` is the storage location that
+ // `this` points to.
+ void TraverseConstructorInits(const CXXConstructorDecl *Ctor,
+ RecordStorageLocation *ThisPointeeLoc) {
+ assert(ThisPointeeLoc != nullptr);
+ for (const CXXCtorInitializer *Init : Ctor->inits()) {
+ Expr *InitExpr = Init->getInit();
+ if (FieldDecl *Field = Init->getMember();
+ Field != nullptr && Field->getType()->isRecordType()) {
+ PropagateResultObject(InitExpr, cast<RecordStorageLocation>(
+ ThisPointeeLoc->getChild(*Field)));
+ } else if (Init->getBaseClass()) {
+ PropagateResultObject(InitExpr, ThisPointeeLoc);
+ }
+
+ // Ensure that any result objects within `InitExpr` (e.g. temporaries)
+ // are also propagated to the prvalues that initialize them.
+ TraverseStmt(InitExpr);
+
+ // If this is a `CXXDefaultInitExpr`, also propagate any result objects
+ // within the default expression.
+ if (auto *DefaultInit = dyn_cast<CXXDefaultInitExpr>(InitExpr))
+ TraverseStmt(DefaultInit->getExpr());
+ }
+ }
+
+ bool TraverseBindingDecl(BindingDecl *BD) {
+ // `RecursiveASTVisitor` doesn't traverse holding variables for
+ // `BindingDecl`s by itself, so we need to tell it to.
+ if (VarDecl *HoldingVar = BD->getHoldingVar())
+ TraverseDecl(HoldingVar);
+ return RecursiveASTVisitor<ResultObjectVisitor>::TraverseBindingDecl(BD);
+ }
+
+ bool VisitVarDecl(VarDecl *VD) {
+ if (VD->getType()->isRecordType() && VD->hasInit())
+ PropagateResultObject(
+ VD->getInit(),
+ &cast<RecordStorageLocation>(DACtx.getStableStorageLocation(*VD)));
+ return true;
+ }
+
+ bool VisitMaterializeTemporaryExpr(MaterializeTemporaryExpr *MTE) {
+ if (MTE->getType()->isRecordType())
+ PropagateResultObject(
+ MTE->getSubExpr(),
+ &cast<RecordStorageLocation>(DACtx.getStableStorageLocation(*MTE)));
+ return true;
+ }
+
+ bool VisitReturnStmt(ReturnStmt *Return) {
+ Expr *RetValue = Return->getRetValue();
+ if (RetValue != nullptr && RetValue->getType()->isRecordType() &&
+ RetValue->isPRValue())
+ PropagateResultObject(RetValue, LocForRecordReturnVal);
+ return true;
+ }
+
+ bool VisitExpr(Expr *E) {
+ // Clang's AST can have record-type prvalues without a result object -- for
+ // example as full-expressions contained in a compound statement or as
+ // arguments of call expressions. We notice this if we get here and a
+ // storage location has not yet been associated with `E`. In this case,
+ // treat this as if it was a `MaterializeTemporaryExpr`.
+ if (E->isPRValue() && E->getType()->isRecordType() &&
+ !ResultObjectMap.contains(E))
+ PropagateResultObject(
+ E, &cast<RecordStorageLocation>(DACtx.getStableStorageLocation(*E)));
+ return true;
+ }
+
+ // Assigns `Loc` as the result object location of `E`, then propagates the
+ // location to all lower-level prvalues that initialize the same object as
+ // `E` (or one of its base classes or member variables).
+ void PropagateResultObject(Expr *E, RecordStorageLocation *Loc) {
+ if (!E->isPRValue() || !E->getType()->isRecordType()) {
+ assert(false);
+ // Ensure we don't propagate the result object if we hit this in a
+ // release build.
+ return;
+ }
+
+ ResultObjectMap[E] = Loc;
+
+ // The following AST node kinds are "original initializers": They are the
+ // lowest-level AST node that initializes a given object, and nothing
+ // below them can initialize the same object (or part of it).
+ if (isa<CXXConstructExpr>(E) || isa<CallExpr>(E) || isa<LambdaExpr>(E) ||
+ isa<CXXDefaultArgExpr>(E) || isa<CXXDefaultInitExpr>(E) ||
+ isa<CXXStdInitializerListExpr>(E)) {
+ return;
+ }
+
+ if (auto *InitList = dyn_cast<InitListExpr>(E)) {
+ if (!InitList->isSemanticForm())
+ return;
+ if (InitList->isTransparent()) {
+ PropagateResultObject(InitList->getInit(0), Loc);
+ return;
+ }
+
+ RecordInitListHelper InitListHelper(InitList);
+
+ for (auto [Base, Init] : InitListHelper.base_inits()) {
+ assert(Base->getType().getCanonicalType() ==
+ Init->getType().getCanonicalType());
+
+ // Storage location for the base class is the same as that of the
+ // derived class because we "flatten" the object hierarchy and put all
+ // fields in `RecordStorageLocation` of the derived class.
+ PropagateResultObject(Init, Loc);
+ }
+
+ for (auto [Field, Init] : InitListHelper.field_inits()) {
+ // Fields of non-record type are handled in
+ // `TransferVisitor::VisitInitListExpr()`.
+ if (!Field->getType()->isRecordType())
+ continue;
+ PropagateResultObject(
+ Init, cast<RecordStorageLocation>(Loc->getChild(*Field)));
+ }
+ return;
+ }
+
+ if (auto *Op = dyn_cast<BinaryOperator>(E); Op && Op->isCommaOp()) {
+ PropagateResultObject(Op->getRHS(), Loc);
+ return;
+ }
+
+ if (auto *Cond = dyn_cast<AbstractConditionalOperator>(E)) {
+ PropagateResultObject(Cond->getTrueExpr(), Loc);
+ PropagateResultObject(Cond->getFalseExpr(), Loc);
+ return;
+ }
+
+ // All other expression nodes that propagate a record prvalue should have
+ // exactly one child.
+ SmallVector<Stmt *, 1> Children(E->child_begin(), E->child_end());
+ LLVM_DEBUG({
+ if (Children.size() != 1)
+ E->dump();
+ });
+ assert(Children.size() == 1);
+ for (Stmt *S : Children)
+ PropagateResultObject(cast<Expr>(S), Loc);
+ }
+
+private:
+ llvm::DenseMap<const Expr *, RecordStorageLocation *> &ResultObjectMap;
+ RecordStorageLocation *LocForRecordReturnVal;
+ DataflowAnalysisContext &DACtx;
+};
+
+} // namespace
+
Environment::Environment(DataflowAnalysisContext &DACtx)
: DACtx(&DACtx),
FlowConditionToken(DACtx.arena().makeFlowConditionToken()) {}
@@ -401,17 +586,23 @@ void Environment::initialize() {
if (DeclCtx == nullptr)
return;
- if (const auto *FuncDecl = dyn_cast<FunctionDecl>(DeclCtx)) {
- assert(FuncDecl->doesThisDeclarationHaveABody());
+ const auto *FuncDecl = dyn_cast<FunctionDecl>(DeclCtx);
+ if (FuncDecl == nullptr)
+ return;
+
+ assert(FuncDecl->doesThisDeclarationHaveABody());
- initFieldsGlobalsAndFuncs(FuncDecl);
+ initFieldsGlobalsAndFuncs(FuncDecl);
- for (const auto *ParamDecl : FuncDecl->parameters()) {
- assert(ParamDecl != nullptr);
- setStorageLocation(*ParamDecl, createObject(*ParamDecl, nullptr));
- }
+ for (const auto *ParamDecl : FuncDecl->parameters()) {
+ assert(ParamDecl != nullptr);
+ setStorageLocation(*ParamDecl, createObject(*ParamDecl, nullptr));
}
+ if (FuncDecl->getReturnType()->isRecordType())
+ LocForRecordReturnVal = &cast<RecordStorageLocation>(
+ createStorageLocation(FuncDecl->getReturnType()));
+
if (const auto *MethodDecl = dyn_cast<CXXMethodDecl>(DeclCtx)) {
auto *Parent = MethodDecl->getParent();
assert(Parent != nullptr);
@@ -444,6 +635,12 @@ void Environment::initialize() {
initializeFieldsWithValues(ThisLoc);
}
}
+
+ // We do this below the handling of `CXXMethodDecl` above so that we can
+ // be sure that the storage location for `this` has been set.
+ ResultObjectMap = std::make_shared<PrValueToResultObject>(
+ buildResultObjectMap(DACtx, FuncDecl, getThisPointeeStorageLocation(),
+ LocForRecordReturnVal));
}
// FIXME: Add support for resetting globals after function calls to enable
@@ -484,13 +681,18 @@ void Environment::initFieldsGlobalsAndFuncs(const FunctionDecl *FuncDecl) {
if (getStorageLocation(*D) != nullptr)
continue;
- setStorageLocation(*D, createObject(*D));
+ // We don't run transfer functions on the initializers of global variables,
+ // so they won't be associated with a value or storage location. We
+ // therefore intentionally don't pass an initializer to `createObject()`;
+ // in particular, this ensures that `createObject()` will initialize the
+ // fields of record-type variables with values.
+ setStorageLocation(*D, createObject(*D, nullptr));
}
for (const FunctionDecl *FD : Funcs) {
if (getStorageLocation(*FD) != nullptr)
continue;
- auto &Loc = createStorageLocation(FD->getType());
+ auto &Loc = createStorageLocation(*FD);
setStorageLocation(*FD, Loc);
}
}
@@ -519,6 +721,9 @@ Environment Environment::pushCall(const CallExpr *Call) const {
}
}
+ if (Call->getType()->isRecordType() && Call->isPRValue())
+ Env.LocForRecordReturnVal = &Env.getResultObjectLocation(*Call);
+
Env.pushCallInternal(Call->getDirectCallee(),
llvm::ArrayRef(Call->getArgs(), Call->getNumArgs()));
@@ -529,6 +734,7 @@ Environment Environment::pushCall(const CXXConstructExpr *Call) const {
Environment Env(*this);
Env.ThisPointeeLoc = &Env.getResultObjectLocation(*Call);
+ Env.LocForRecordReturnVal = &Env.getResultObjectLocation(*Call);
Env.pushCallInternal(Call->getConstructor(),
llvm::ArrayRef(Call->getArgs(), Call->getNumArgs()));
@@ -557,6 +763,10 @@ void Environment::pushCallInternal(const FunctionDecl *FuncDecl,
const VarDecl *Param = *ParamIt;
setStorageLocation(*Param, createObject(*Param, Args[ArgIndex]));
}
+
+ ResultObjectMap = std::make_shared<PrValueToResultObject>(
+ buildResultObjectMap(DACtx, FuncDecl, getThisPointeeStorageLocation(),
+ LocForRecordReturnVal));
}
void Environment::popCall(const CallExpr *Call, const Environment &CalleeEnv) {
@@ -600,6 +810,9 @@ bool Environment::equivalentTo(const Environment &Other,
if (ReturnLoc != Other.ReturnLoc)
return false;
+ if (LocForRecordReturnVal != Other.LocForRecordReturnVal)
+ return false;
+
if (ThisPointeeLoc != Other.ThisPointeeLoc)
return false;
@@ -623,8 +836,10 @@ LatticeEffect Environment::widen(const Environment &PrevEnv,
assert(DACtx == PrevEnv.DACtx);
assert(ReturnVal == PrevEnv.ReturnVal);
assert(ReturnLoc == PrevEnv.ReturnLoc);
+ assert(LocForRecordReturnVal == PrevEnv.LocForRecordReturnVal);
assert(ThisPointeeLoc == PrevEnv.ThisPointeeLoc);
assert(CallStack == PrevEnv.CallStack);
+ assert(ResultObjectMap == PrevEnv.ResultObjectMap);
auto Effect = LatticeEffect::Unchanged;
@@ -656,12 +871,16 @@ Environment Environment::join(const Environment &EnvA, const Environment &EnvB,
Environment::ValueModel &Model,
ExprJoinBehavior ExprBehavior) {
assert(EnvA.DACtx == EnvB.DACtx);
+ assert(EnvA.LocForRecordReturnVal == EnvB.LocForRecordReturnVal);
assert(EnvA.ThisPointeeLoc == EnvB.ThisPointeeLoc);
assert(EnvA.CallStack == EnvB.CallStack);
+ assert(EnvA.ResultObjectMap == EnvB.ResultObjectMap);
Environment JoinedEnv(*EnvA.DACtx);
JoinedEnv.CallStack = EnvA.CallStack;
+ JoinedEnv.ResultObjectMap = EnvA.ResultObjectMap;
+ JoinedEnv.LocForRecordReturnVal = EnvA.LocForRecordReturnVal;
JoinedEnv.ThisPointeeLoc = EnvA.ThisPointeeLoc;
if (EnvA.ReturnVal == nullptr || EnvB.ReturnVal == nullptr) {
@@ -730,6 +949,12 @@ StorageLocation &Environment::createStorageLocation(const Expr &E) {
void Environment::setStorageLocation(const ValueDecl &D, StorageLocation &Loc) {
assert(!DeclToLoc.contains(&D));
+ // The only kinds of declarations that may have a "variable" storage location
+ // are declarations of reference type and `BindingDecl`. For all other
+ // declaration, the storage location should be the stable storage location
+ // returned by `createStorageLocation()`.
+ assert(D.getType()->isReferenceType() || isa<BindingDecl>(D) ||
+ &Loc == &createStorageLocation(D));
DeclToLoc[&D] = &Loc;
}
@@ -791,50 +1016,29 @@ Environment::getResultObjectLocation(const Expr &RecordPRValue) const {
assert(RecordPRValue.getType()->isRecordType());
assert(RecordPRValue.isPRValue());
- // Returns a storage location that we can use if assertions fail.
- auto FallbackForAssertFailure =
- [this, &RecordPRValue]() -> RecordStorageLocation & {
+ assert(ResultObjectMap != nullptr);
+ RecordStorageLocation *Loc = ResultObjectMap->lookup(&RecordPRValue);
+ assert(Loc != nullptr);
+ // In release builds, use the "stable" storage location if the map lookup
+ // failed.
+ if (Loc == nullptr)
return cast<RecordStorageLocation>(
DACtx->getStableStorageLocation(RecordPRValue));
- };
-
- if (isOriginalRecordConstructor(RecordPRValue)) {
- auto *Val = cast_or_null<RecordValue>(getValue(RecordPRValue));
- // The builtin transfer function should have created a `RecordValue` for all
- // original record constructors.
- assert(Val);
- if (!Val)
- return FallbackForAssertFailure();
- return Val->getLoc();
- }
-
- if (auto *Op = dyn_cast<BinaryOperator>(&RecordPRValue);
- Op && Op->isCommaOp()) {
- return getResultObjectLocation(*Op->getRHS());
- }
-
- // All other expression nodes that propagate a record prvalue should have
- // exactly one child.
- llvm::SmallVector<const Stmt *> children(RecordPRValue.child_begin(),
- RecordPRValue.child_end());
- assert(children.size() == 1);
- if (children.empty())
- return FallbackForAssertFailure();
-
- return getResultObjectLocation(*cast<Expr>(children[0]));
+ return *Loc;
}
PointerValue &Environment::getOrCreateNullPointerValue(QualType PointeeType) {
return DACtx->getOrCreateNullPointerValue(PointeeType);
}
-void Environment::initializeFieldsWithValues(RecordStorageLocation &Loc) {
+void Environment::initializeFieldsWithValues(RecordStorageLocation &Loc,
+ QualType Type) {
llvm::DenseSet<QualType> Visited;
int CreatedValuesCount = 0;
- initializeFieldsWithValues(Loc, Visited, 0, CreatedValuesCount);
+ initializeFieldsWithValues(Loc, Type, Visited, 0, CreatedValuesCount);
if (CreatedValuesCount > MaxCompositeValueSize) {
- llvm::errs() << "Attempting to initialize a huge value of type: "
- << Loc.getType() << '\n';
+ llvm::errs() << "Attempting to initialize a huge value of type: " << Type
+ << '\n';
}
}
@@ -848,8 +1052,7 @@ void Environment::setValue(const Expr &E, Value &Val) {
const Expr &CanonE = ignoreCFGOmittedNodes(E);
if (auto *RecordVal = dyn_cast<RecordValue>(&Val)) {
- assert(isOriginalRecordConstructor(CanonE) ||
- &RecordVal->getLoc() == &getResultObjectLocation(CanonE));
+ assert(&RecordVal->getLoc() == &getResultObjectLocation(CanonE));
(void)RecordVal;
}
@@ -928,7 +1131,8 @@ Value *Environment::createValueUnlessSelfReferential(
if (Type->isRecordType()) {
CreatedValuesCount++;
auto &Loc = cast<RecordStorageLocation>(createStorageLocation(Type));
- initializeFieldsWithValues(Loc, Visited, Depth, CreatedValuesCount);
+ initializeFieldsWithValues(Loc, Loc.getType(), Visited, Depth,
+ CreatedValuesCount);
return &refreshRecordValue(Loc, *this);
}
@@ -960,6 +1164,7 @@ Environment::createLocAndMaybeValue(QualType Ty,
}
void Environment::initializeFieldsWithValues(RecordStorageLocation &Loc,
+ QualType Type,
llvm::DenseSet<QualType> &Visited,
int Depth,
int &CreatedValuesCount) {
@@ -967,8 +1172,8 @@ void Environment::initializeFieldsWithValues(RecordStorageLocation &Loc,
if (FieldType->isRecordType()) {
auto &FieldRecordLoc = cast<RecordStorageLocation>(FieldLoc);
setValue(FieldRecordLoc, create<RecordValue>(FieldRecordLoc));
- initializeFieldsWithValues(FieldRecordLoc, Visited, Depth + 1,
- CreatedValuesCount);
+ initializeFieldsWithValues(FieldRecordLoc, FieldRecordLoc.getType(),
+ Visited, Depth + 1, CreatedValuesCount);
} else {
if (!Visited.insert(FieldType.getCanonicalType()).second)
return;
@@ -979,7 +1184,7 @@ void Environment::initializeFieldsWithValues(RecordStorageLocation &Loc,
}
};
- for (const auto &[Field, FieldLoc] : Loc.children()) {
+ for (const FieldDecl *Field : DACtx->getModeledFields(Type)) {
assert(Field != nullptr);
QualType FieldType = Field->getType();
@@ -988,14 +1193,12 @@ void Environment::initializeFieldsWithValues(RecordStorageLocation &Loc,
&createLocAndMaybeValue(FieldType, Visited, Depth + 1,
CreatedValuesCount));
} else {
+ StorageLocation *FieldLoc = Loc.getChild(*Field);
assert(FieldLoc != nullptr);
initField(FieldType, *FieldLoc);
}
}
- for (const auto &[FieldName, FieldLoc] : Loc.synthetic_fields()) {
- assert(FieldLoc != nullptr);
- QualType FieldType = FieldLoc->getType();
-
+ for (const auto &[FieldName, FieldType] : DACtx->getSyntheticFields(Type)) {
// Synthetic fields cannot have reference type, so we don't need to deal
// with this case.
assert(!FieldType->isReferenceType());
@@ -1022,38 +1225,36 @@ StorageLocation &Environment::createObjectInternal(const ValueDecl *D,
return createObjectInternal(D, Ty.getNonReferenceType(), nullptr);
}
- Value *Val = nullptr;
- if (InitExpr) {
- // In the (few) cases where an expression is intentionally
- // "uninterpreted", `InitExpr` is not associated with a value. There are
- // two ways to handle this situation: propagate the status, so that
- // uninterpreted initializers result in uninterpreted variables, or
- // provide a default value. We choose the latter so that later refinements
- // of the variable can be used for reasoning about the surrounding code.
- // For this reason, we let this case be handled by the `createValue()`
- // call below.
- //
- // FIXME. If and when we interpret all language cases, change this to
- // assert that `InitExpr` is interpreted, rather than supplying a
- // default value (assuming we don't update the environment API to return
- // references).
- Val = getValue(*InitExpr);
-
- if (!Val && isa<ImplicitValueInitExpr>(InitExpr) &&
- InitExpr->getType()->isPointerType())
- Val = &getOrCreateNullPointerValue(InitExpr->getType()->getPointeeType());
- }
- if (!Val)
- Val = createValue(Ty);
-
- if (Ty->isRecordType())
- return cast<RecordValue>(Val)->getLoc();
-
StorageLocation &Loc =
D ? createStorageLocation(*D) : createStorageLocation(Ty);
- if (Val)
- setValue(Loc, *Val);
+ if (Ty->isRecordType()) {
+ auto &RecordLoc = cast<RecordStorageLocation>(Loc);
+ if (!InitExpr)
+ initializeFieldsWithValues(RecordLoc);
+ refreshRecordValue(RecordLoc, *this);
+ } else {
+ Value *Val = nullptr;
+ if (InitExpr)
+ // In the (few) cases where an expression is intentionally
+ // "uninterpreted", `InitExpr` is not associated with a value. There are
+ // two ways to handle this situation: propagate the status, so that
+ // uninterpreted initializers result in uninterpreted variables, or
+ // provide a default value. We choose the latter so that later refinements
+ // of the variable can be used for reasoning about the surrounding code.
+ // For this reason, we let this case be handled by the `createValue()`
+ // call below.
+ //
+ // FIXME. If and when we interpret all language cases, change this to
+ // assert that `InitExpr` is interpreted, rather than supplying a
+ // default value (assuming we don't update the environment API to return
+ // references).
+ Val = getValue(*InitExpr);
+ if (!Val)
+ Val = createValue(Ty);
+ if (Val)
+ setValue(Loc, *Val);
+ }
return Loc;
}
@@ -1072,6 +1273,8 @@ bool Environment::allows(const Formula &F) const {
void Environment::dump(raw_ostream &OS) const {
llvm::DenseMap<const StorageLocation *, std::string> LocToName;
+ if (LocForRecordReturnVal != nullptr)
+ LocToName[LocForRecordReturnVal] = "(returned record)";
if (ThisPointeeLoc != nullptr)
LocToName[ThisPointeeLoc] = "this";
@@ -1102,6 +1305,9 @@ void Environment::dump(raw_ostream &OS) const {
if (auto Iter = LocToName.find(ReturnLoc); Iter != LocToName.end())
OS << " (" << Iter->second << ")";
OS << "\n";
+ } else if (Func->getReturnType()->isRecordType() ||
+ isa<CXXConstructorDecl>(Func)) {
+ OS << "LocForRecordReturnVal: " << LocForRecordReturnVal << "\n";
} else if (!Func->getReturnType()->isVoidType()) {
if (ReturnVal == nullptr)
OS << "ReturnVal: nullptr\n";
@@ -1122,6 +1328,22 @@ void Environment::dump() const {
dump(llvm::dbgs());
}
+Environment::PrValueToResultObject Environment::buildResultObjectMap(
+ DataflowAnalysisContext *DACtx, const FunctionDecl *FuncDecl,
+ RecordStorageLocation *ThisPointeeLoc,
+ RecordStorageLocation *LocForRecordReturnVal) {
+ assert(FuncDecl->doesThisDeclarationHaveABody());
+
+ PrValueToResultObject Map;
+
+ ResultObjectVisitor Visitor(Map, LocForRecordReturnVal, *DACtx);
+ if (const auto *Ctor = dyn_cast<CXXConstructorDecl>(FuncDecl))
+ Visitor.TraverseConstructorInits(Ctor, ThisPointeeLoc);
+ Visitor.TraverseStmt(FuncDecl->getBody());
+
+ return Map;
+}
+
RecordStorageLocation *getImplicitObjectLocation(const CXXMemberCallExpr &MCE,
const Environment &Env) {
Expr *ImplicitObject = MCE.getImplicitObjectArgument();
@@ -1216,24 +1438,11 @@ RecordValue &refreshRecordValue(RecordStorageLocation &Loc, Environment &Env) {
RecordValue &refreshRecordValue(const Expr &Expr, Environment &Env) {
assert(Expr.getType()->isRecordType());
- if (Expr.isPRValue()) {
- if (auto *ExistingVal = Env.get<RecordValue>(Expr)) {
- auto &NewVal = Env.create<RecordValue>(ExistingVal->getLoc());
- Env.setValue(Expr, NewVal);
- Env.setValue(NewVal.getLoc(), NewVal);
- return NewVal;
- }
+ if (Expr.isPRValue())
+ refreshRecordValue(Env.getResultObjectLocation(Expr), Env);
- auto &NewVal = *cast<RecordValue>(Env.createValue(Expr.getType()));
- Env.setValue(Expr, NewVal);
- return NewVal;
- }
-
- if (auto *Loc = Env.get<RecordStorageLocation>(Expr)) {
- auto &NewVal = Env.create<RecordValue>(*Loc);
- Env.setValue(*Loc, NewVal);
- return NewVal;
- }
+ if (auto *Loc = Env.get<RecordStorageLocation>(Expr))
+ refreshRecordValue(*Loc, Env);
auto &NewVal = *cast<RecordValue>(Env.createValue(Expr.getType()));
Env.setStorageLocation(Expr, NewVal.getLoc());
diff --git a/clang/lib/Analysis/FlowSensitive/Transfer.cpp b/clang/lib/Analysis/FlowSensitive/Transfer.cpp
index 0a2e8368d541dd..88a9c0eccbebc0 100644
--- a/clang/lib/Analysis/FlowSensitive/Transfer.cpp
+++ b/clang/lib/Analysis/FlowSensitive/Transfer.cpp
@@ -460,11 +460,9 @@ class TransferVisitor : public ConstStmtVisitor<TransferVisitor> {
// So make sure we have a value if we didn't propagate one above.
if (S->isPRValue() && S->getType()->isRecordType()) {
if (Env.getValue(*S) == nullptr) {
- Value *Val = Env.createValue(S->getType());
- // We're guaranteed to always be able to create a value for record
- // types.
- assert(Val != nullptr);
- Env.setValue(*S, *Val);
+ auto &Loc = Env.getResultObjectLocation(*S);
+ Env.initializeFieldsWithValues(Loc);
+ refreshRecordValue(Loc, Env);
}
}
}
@@ -472,6 +470,13 @@ class TransferVisitor : public ConstStmtVisitor<TransferVisitor> {
void VisitCXXDefaultInitExpr(const CXXDefaultInitExpr *S) {
const Expr *InitExpr = S->getExpr();
assert(InitExpr != nullptr);
+
+ // If this is a prvalue of record type, the handler for `*InitExpr` (if one
+ // exists) will initialize the result object; there is no value to propgate
+ // here.
+ if (S->getType()->isRecordType() && S->isPRValue())
+ return;
+
propagateValueOrStorageLocation(*InitExpr, *S, Env);
}
@@ -479,6 +484,17 @@ class TransferVisitor : public ConstStmtVisitor<TransferVisitor> {
const CXXConstructorDecl *ConstructorDecl = S->getConstructor();
assert(ConstructorDecl != nullptr);
+ // `CXXConstructExpr` can have array type if default-initializing an array
+ // of records. We don't handle this specifically beyond potentially inlining
+ // the call.
+ if (!S->getType()->isRecordType()) {
+ transferInlineCall(S, ConstructorDecl);
+ return;
+ }
+
+ RecordStorageLocation &Loc = Env.getResultObjectLocation(*S);
+ Env.setValue(*S, refreshRecordValue(Loc, Env));
+
if (ConstructorDecl->isCopyOrMoveConstructor()) {
// It is permissible for a copy/move constructor to have additional
// parameters as long as they have default arguments defined for them.
@@ -491,24 +507,14 @@ class TransferVisitor : public ConstStmtVisitor<TransferVisitor> {
if (ArgLoc == nullptr)
return;
- if (S->isElidable()) {
- if (Value *Val = Env.getValue(*ArgLoc))
- Env.setValue(*S, *Val);
- } else {
- auto &Val = *cast<RecordValue>(Env.createValue(S->getType()));
- Env.setValue(*S, Val);
- copyRecord(*ArgLoc, Val.getLoc(), Env);
- }
+ // Even if the copy/move constructor call is elidable, we choose to copy
+ // the record in all cases (which isn't wrong, just potentially not
+ // optimal).
+ copyRecord(*ArgLoc, Loc, Env);
return;
}
- // `CXXConstructExpr` can have array type if default-initializing an array
- // of records, and we currently can't create values for arrays. So check if
- // we've got a record type.
- if (S->getType()->isRecordType()) {
- auto &InitialVal = *cast<RecordValue>(Env.createValue(S->getType()));
- Env.setValue(*S, InitialVal);
- }
+ Env.initializeFieldsWithValues(Loc, S->getType());
transferInlineCall(S, ConstructorDecl);
}
@@ -551,19 +557,15 @@ class TransferVisitor : public ConstStmtVisitor<TransferVisitor> {
if (S->isGLValue()) {
Env.setStorageLocation(*S, *LocDst);
} else if (S->getType()->isRecordType()) {
- // Make sure that we have a `RecordValue` for this expression so that
- // `Environment::getResultObjectLocation()` is able to return a location
- // for it.
- if (Env.getValue(*S) == nullptr)
- refreshRecordValue(*S, Env);
+ // Assume that the assignment returns the assigned value.
+ copyRecord(*LocDst, Env.getResultObjectLocation(*S), Env);
}
return;
}
- // CXXOperatorCallExpr can be prvalues. Call `VisitCallExpr`() to create
- // a `RecordValue` for them so that `Environment::getResultObjectLocation()`
- // can return a value.
+ // `CXXOperatorCallExpr` can be a prvalue. Call `VisitCallExpr`() to
+ // initialize the prvalue's fields with values.
VisitCallExpr(S);
}
@@ -580,11 +582,6 @@ class TransferVisitor : public ConstStmtVisitor<TransferVisitor> {
}
}
- void VisitCXXTemporaryObjectExpr(const CXXTemporaryObjectExpr *S) {
- if (Value *Val = Env.createValue(S->getType()))
- Env.setValue(*S, *Val);
- }
-
void VisitCallExpr(const CallExpr *S) {
// Of clang's builtins, only `__builtin_expect` is handled explicitly, since
// others (like trap, debugtrap, and unreachable) are handled by CFG
@@ -612,13 +609,14 @@ class TransferVisitor : public ConstStmtVisitor<TransferVisitor> {
} else if (const FunctionDecl *F = S->getDirectCallee()) {
transferInlineCall(S, F);
- // If this call produces a prvalue of record type, make sure that we have
- // a `RecordValue` for it. This is required so that
- // `Environment::getResultObjectLocation()` is able to return a location
- // for this `CallExpr`.
+ // If this call produces a prvalue of record type, initialize its fields
+ // with values.
if (S->getType()->isRecordType() && S->isPRValue())
- if (Env.getValue(*S) == nullptr)
- refreshRecordValue(*S, Env);
+ if (Env.getValue(*S) == nullptr) {
+ RecordStorageLocation &Loc = Env.getResultObjectLocation(*S);
+ Env.initializeFieldsWithValues(Loc);
+ Env.setValue(*S, refreshRecordValue(Loc, Env));
+ }
}
}
@@ -666,8 +664,10 @@ class TransferVisitor : public ConstStmtVisitor<TransferVisitor> {
// `getLogicOperatorSubExprValue()`.
if (S->isGLValue())
Env.setStorageLocation(*S, Env.createObject(S->getType()));
- else if (Value *Val = Env.createValue(S->getType()))
- Env.setValue(*S, *Val);
+ else if (!S->getType()->isRecordType()) {
+ if (Value *Val = Env.createValue(S->getType()))
+ Env.setValue(*S, *Val);
+ }
}
void VisitInitListExpr(const InitListExpr *S) {
@@ -688,71 +688,51 @@ class TransferVisitor : public ConstStmtVisitor<TransferVisitor> {
return;
}
- llvm::DenseMap<const ValueDecl *, StorageLocation *> FieldLocs;
- RecordInitListHelper InitListHelper(S);
+ RecordStorageLocation &Loc = Env.getResultObjectLocation(*S);
+ Env.setValue(*S, refreshRecordValue(Loc, Env));
- for (auto [Base, Init] : InitListHelper.base_inits()) {
- assert(Base->getType().getCanonicalType() ==
- Init->getType().getCanonicalType());
- auto *BaseVal = Env.get<RecordValue>(*Init);
- if (!BaseVal)
- BaseVal = cast<RecordValue>(Env.createValue(Init->getType()));
- // Take ownership of the fields of the `RecordValue` for the base class
- // and incorporate them into the "flattened" set of fields for the
- // derived class.
- auto Children = BaseVal->getLoc().children();
- FieldLocs.insert(Children.begin(), Children.end());
- }
+ // Initialization of base classes and fields of record type happens when we
+ // visit the nested `CXXConstructExpr` or `InitListExpr` for that base class
+ // or field. We therefore only need to deal with fields of non-record type
+ // here.
- for (auto [Field, Init] : InitListHelper.field_inits()) {
- assert(
- // The types are same, or
- Field->getType().getCanonicalType().getUnqualifiedType() ==
- Init->getType().getCanonicalType().getUnqualifiedType() ||
- // The field's type is T&, and initializer is T
- (Field->getType()->isReferenceType() &&
- Field->getType().getCanonicalType()->getPointeeType() ==
- Init->getType().getCanonicalType()));
- auto& Loc = Env.createObject(Field->getType(), Init);
- FieldLocs.insert({Field, &Loc});
- }
+ RecordInitListHelper InitListHelper(S);
- // In the case of a union, we don't in general have initializers for all
- // of the fields. Create storage locations for the remaining fields (but
- // don't associate them with values).
- if (Type->isUnionType()) {
- for (const FieldDecl *Field :
- Env.getDataflowAnalysisContext().getModeledFields(Type)) {
- if (auto [it, inserted] = FieldLocs.insert({Field, nullptr}); inserted)
- it->second = &Env.createStorageLocation(Field->getType());
+ for (auto [Field, Init] : InitListHelper.field_inits()) {
+ if (Field->getType()->isRecordType())
+ continue;
+ if (Field->getType()->isReferenceType()) {
+ assert(Field->getType().getCanonicalType()->getPointeeType() ==
+ Init->getType().getCanonicalType());
+ Loc.setChild(*Field, &Env.createObject(Field->getType(), Init));
+ continue;
}
+ assert(Field->getType().getCanonicalType().getUnqualifiedType() ==
+ Init->getType().getCanonicalType().getUnqualifiedType());
+ StorageLocation *FieldLoc = Loc.getChild(*Field);
+ // Locations for non-reference fields must always be non-null.
+ assert(FieldLoc != nullptr);
+ Value *Val = Env.getValue(*Init);
+ if (Val == nullptr && isa<ImplicitValueInitExpr>(Init) &&
+ Init->getType()->isPointerType())
+ Val =
+ &Env.getOrCreateNullPointerValue(Init->getType()->getPointeeType());
+ if (Val == nullptr)
+ Val = Env.createValue(Field->getType());
+ if (Val != nullptr)
+ Env.setValue(*FieldLoc, *Val);
}
- // Check that we satisfy the invariant that a `RecordStorageLoation`
- // contains exactly the set of modeled fields for that type.
- // `ModeledFields` includes fields from all the bases, but only the
- // modeled ones. However, if a class type is initialized with an
- // `InitListExpr`, all fields in the class, including those from base
- // classes, are included in the set of modeled fields. The code above
- // should therefore populate exactly the modeled fields.
- assert(containsSameFields(
- Env.getDataflowAnalysisContext().getModeledFields(Type), FieldLocs));
-
- RecordStorageLocation::SyntheticFieldMap SyntheticFieldLocs;
- for (const auto &Entry :
- Env.getDataflowAnalysisContext().getSyntheticFields(Type)) {
- SyntheticFieldLocs.insert(
- {Entry.getKey(), &Env.createObject(Entry.getValue())});
+ for (const auto &[FieldName, FieldLoc] : Loc.synthetic_fields()) {
+ QualType FieldType = FieldLoc->getType();
+ if (FieldType->isRecordType()) {
+ Env.initializeFieldsWithValues(*cast<RecordStorageLocation>(FieldLoc));
+ } else {
+ if (Value *Val = Env.createValue(FieldType))
+ Env.setValue(*FieldLoc, *Val);
+ }
}
- auto &Loc = Env.getDataflowAnalysisContext().createRecordStorageLocation(
- Type, std::move(FieldLocs), std::move(SyntheticFieldLocs));
- RecordValue &RecordVal = Env.create<RecordValue>(Loc);
-
- Env.setValue(Loc, RecordVal);
-
- Env.setValue(*S, RecordVal);
-
// FIXME: Implement array initialization.
}
diff --git a/clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp b/clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp
index 595f70f819ddb5..1b73c5d6830161 100644
--- a/clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp
+++ b/clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp
@@ -369,17 +369,10 @@ builtinTransferInitializer(const CFGInitializer &Elt,
ParentLoc->setChild(*Member, InitExprLoc);
} else if (auto *InitExprVal = Env.getValue(*InitExpr)) {
assert(MemberLoc != nullptr);
- if (Member->getType()->isRecordType()) {
- auto *InitValStruct = cast<RecordValue>(InitExprVal);
- // FIXME: Rather than performing a copy here, we should really be
- // initializing the field in place. This would require us to propagate the
- // storage location of the field to the AST node that creates the
- // `RecordValue`.
- copyRecord(InitValStruct->getLoc(),
- *cast<RecordStorageLocation>(MemberLoc), Env);
- } else {
+ // Record-type initializers construct themselves directly into the result
+ // object, so there is no need to handle them here.
+ if (!Member->getType()->isRecordType())
Env.setValue(*MemberLoc, *InitExprVal);
- }
}
}
diff --git a/clang/unittests/Analysis/FlowSensitive/DataflowEnvironmentTest.cpp b/clang/unittests/Analysis/FlowSensitive/DataflowEnvironmentTest.cpp
index 465a8e21690c4a..cc20623f881ff4 100644
--- a/clang/unittests/Analysis/FlowSensitive/DataflowEnvironmentTest.cpp
+++ b/clang/unittests/Analysis/FlowSensitive/DataflowEnvironmentTest.cpp
@@ -24,6 +24,7 @@ namespace {
using namespace clang;
using namespace dataflow;
+using ::clang::dataflow::test::findValueDecl;
using ::clang::dataflow::test::getFieldValue;
using ::testing::Contains;
using ::testing::IsNull;
@@ -199,6 +200,48 @@ TEST_F(EnvironmentTest, JoinRecords) {
}
}
+TEST_F(EnvironmentTest, DifferentReferenceLocInJoin) {
+ // This tests the case where the storage location for a reference-type
+ // variable is different for two states being joined. We used to believe this
+ // could not happen and therefore had an assertion disallowing this; this test
+ // exists to demonstrate that we can handle this condition without a failing
+ // assertion. See also the discussion here:
+ // https://discourse.llvm.org/t/70086/6
+
+ using namespace ast_matchers;
+
+ std::string Code = R"cc(
+ void f(int &ref) {}
+ )cc";
+
+ auto Unit =
+ tooling::buildASTFromCodeWithArgs(Code, {"-fsyntax-only", "-std=c++11"});
+ auto &Context = Unit->getASTContext();
+
+ ASSERT_EQ(Context.getDiagnostics().getClient()->getNumErrors(), 0U);
+
+ const ValueDecl *Ref = findValueDecl(Context, "ref");
+
+ Environment Env1(DAContext);
+ StorageLocation &Loc1 = Env1.createStorageLocation(Context.IntTy);
+ Env1.setStorageLocation(*Ref, Loc1);
+
+ Environment Env2(DAContext);
+ StorageLocation &Loc2 = Env2.createStorageLocation(Context.IntTy);
+ Env2.setStorageLocation(*Ref, Loc2);
+
+ EXPECT_NE(&Loc1, &Loc2);
+
+ Environment::ValueModel Model;
+ Environment EnvJoined =
+ Environment::join(Env1, Env2, Model, Environment::DiscardExprState);
+
+ // Joining environments with different storage locations for the same
+ // declaration results in the declaration being removed from the joined
+ // environment.
+ EXPECT_EQ(EnvJoined.getStorageLocation(*Ref), nullptr);
+}
+
TEST_F(EnvironmentTest, InitGlobalVarsFun) {
using namespace ast_matchers;
diff --git a/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp b/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
index ca055a462a2866..00dafb2988c690 100644
--- a/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
+++ b/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
@@ -1582,10 +1582,9 @@ TEST(TransferTest, FieldsDontHaveValuesInConstructorWithBaseClass) {
[](const llvm::StringMap<DataflowAnalysisState<NoopLattice>> &Results,
ASTContext &ASTCtx) {
const Environment &Env = getEnvironmentAtAnnotation(Results, "p");
- // FIXME: The field of the base class should already have been
- // initialized with a value by the base constructor. This test documents
- // the current buggy behavior.
- EXPECT_EQ(getFieldValue(Env.getThisPointeeStorageLocation(), "BaseVal",
+ // The field of the base class should already have been initialized with
+ // a value by the base constructor.
+ EXPECT_NE(getFieldValue(Env.getThisPointeeStorageLocation(), "BaseVal",
ASTCtx, Env),
nullptr);
EXPECT_EQ(getFieldValue(Env.getThisPointeeStorageLocation(), "Val",
@@ -2998,8 +2997,12 @@ TEST(TransferTest, ResultObjectLocation) {
TEST(TransferTest, ResultObjectLocationForDefaultArgExpr) {
std::string Code = R"(
- struct S {};
- void funcWithDefaultArg(S s = S());
+ struct Inner {};
+ struct Outer {
+ Inner I = {};
+ };
+
+ void funcWithDefaultArg(Outer O = {});
void target() {
funcWithDefaultArg();
// [[p]]
@@ -3058,13 +3061,7 @@ TEST(TransferTest, ResultObjectLocationForDefaultInitExpr) {
RecordStorageLocation &Loc = Env.getResultObjectLocation(*DefaultInit);
- // FIXME: The result object location for the `CXXDefaultInitExpr` should
- // be the location of the member variable being initialized, but we
- // don't do this correctly yet; see also comments in
- // `builtinTransferInitializer()`.
- // For the time being, we just document the current erroneous behavior
- // here (this should be `EXPECT_EQ` when the behavior is fixed).
- EXPECT_NE(&Loc, Env.getThisPointeeStorageLocation()->getChild(*SField));
+ EXPECT_EQ(&Loc, Env.getThisPointeeStorageLocation()->getChild(*SField));
});
}
@@ -3101,6 +3098,79 @@ TEST(TransferTest, ResultObjectLocationForCXXOperatorCallExpr) {
});
}
+TEST(TransferTest, ResultObjectLocationForStdInitializerListExpr) {
+ std::string Code = R"(
+ namespace std {
+ template <typename T>
+ struct initializer_list {};
+ } // namespace std
+
+ void target() {
+ std::initializer_list<int> list = {1};
+ // [[p]]
+ }
+ )";
+
+ using ast_matchers::cxxStdInitializerListExpr;
+ using ast_matchers::match;
+ using ast_matchers::selectFirst;
+ runDataflow(
+ Code,
+ [](const llvm::StringMap<DataflowAnalysisState<NoopLattice>> &Results,
+ ASTContext &ASTCtx) {
+ const Environment &Env = getEnvironmentAtAnnotation(Results, "p");
+
+ auto *StdInitList = selectFirst<CXXStdInitializerListExpr>(
+ "std_init_list",
+ match(cxxStdInitializerListExpr().bind("std_init_list"), ASTCtx));
+ ASSERT_NE(StdInitList, nullptr);
+
+ EXPECT_EQ(&Env.getResultObjectLocation(*StdInitList),
+ &getLocForDecl<RecordStorageLocation>(ASTCtx, Env, "list"));
+ });
+}
+
+TEST(TransferTest, ResultObjectLocationPropagatesThroughConditionalOperator) {
+ std::string Code = R"(
+ struct A {
+ A(int);
+ };
+
+ void target(bool b) {
+ A a = b ? A(0) : A(1);
+ (void)0; // [[p]]
+ }
+ )";
+ using ast_matchers::cxxConstructExpr;
+ using ast_matchers::equals;
+ using ast_matchers::hasArgument;
+ using ast_matchers::integerLiteral;
+ using ast_matchers::match;
+ using ast_matchers::selectFirst;
+ using ast_matchers::traverse;
+ runDataflow(
+ Code,
+ [](const llvm::StringMap<DataflowAnalysisState<NoopLattice>> &Results,
+ ASTContext &ASTCtx) {
+ const Environment &Env = getEnvironmentAtAnnotation(Results, "p");
+
+ auto *ConstructExpr0 = selectFirst<CXXConstructExpr>(
+ "construct",
+ match(cxxConstructExpr(hasArgument(0, integerLiteral(equals(0))))
+ .bind("construct"),
+ ASTCtx));
+ auto *ConstructExpr1 = selectFirst<CXXConstructExpr>(
+ "construct",
+ match(cxxConstructExpr(hasArgument(0, integerLiteral(equals(1))))
+ .bind("construct"),
+ ASTCtx));
+
+ auto &ALoc = getLocForDecl<StorageLocation>(ASTCtx, Env, "a");
+ EXPECT_EQ(&Env.getResultObjectLocation(*ConstructExpr0), &ALoc);
+ EXPECT_EQ(&Env.getResultObjectLocation(*ConstructExpr1), &ALoc);
+ });
+}
+
TEST(TransferTest, StaticCast) {
std::string Code = R"(
void target(int Foo) {
@@ -5886,6 +5956,38 @@ TEST(TransferTest, ContextSensitiveReturnRecord) {
{BuiltinOptions{ContextSensitiveOptions{}}});
}
+TEST(TransferTest, ContextSensitiveReturnSelfReferentialRecord) {
+ std::string Code = R"(
+ struct S {
+ S() { self = this; }
+ S *self;
+ };
+
+ S makeS() {
+ // RVO guarantees that this will be constructed directly into `MyS`.
+ return S();
+ }
+
+ void target() {
+ S MyS = makeS();
+ // [[p]]
+ }
+ )";
+ runDataflow(
+ Code,
+ [](const llvm::StringMap<DataflowAnalysisState<NoopLattice>> &Results,
+ ASTContext &ASTCtx) {
+ const Environment &Env = getEnvironmentAtAnnotation(Results, "p");
+
+ auto &MySLoc = getLocForDecl<RecordStorageLocation>(ASTCtx, Env, "MyS");
+
+ auto *SelfVal =
+ cast<PointerValue>(getFieldValue(&MySLoc, "self", ASTCtx, Env));
+ EXPECT_EQ(&SelfVal->getPointeeLoc(), &MySLoc);
+ },
+ {BuiltinOptions{ContextSensitiveOptions{}}});
+}
+
TEST(TransferTest, ContextSensitiveMethodLiteral) {
std::string Code = R"(
class MyClass {
@@ -6830,50 +6932,6 @@ TEST(TransferTest, LambdaCaptureThis) {
});
}
-TEST(TransferTest, DifferentReferenceLocInJoin) {
- // This test triggers a case where the storage location for a reference-type
- // variable is different for two states being joined. We used to believe this
- // could not happen and therefore had an assertion disallowing this; this test
- // exists to demonstrate that we can handle this condition without a failing
- // assertion. See also the discussion here:
- // https://discourse.llvm.org/t/70086/6
- std::string Code = R"(
- namespace std {
- template <class T> struct initializer_list {
- const T* begin();
- const T* end();
- };
- }
-
- void target(char* p, char* end) {
- while (p != end) {
- if (*p == ' ') {
- p++;
- continue;
- }
-
- auto && range = {1, 2};
- for (auto b = range.begin(), e = range.end(); b != e; ++b) {
- }
- (void)0;
- // [[p]]
- }
- }
- )";
- runDataflow(
- Code,
- [](const llvm::StringMap<DataflowAnalysisState<NoopLattice>> &Results,
- ASTContext &ASTCtx) {
- const Environment &Env = getEnvironmentAtAnnotation(Results, "p");
-
- // Joining environments with different storage locations for the same
- // declaration results in the declaration being removed from the joined
- // environment.
- const ValueDecl *VD = findValueDecl(ASTCtx, "range");
- ASSERT_EQ(Env.getStorageLocation(*VD), nullptr);
- });
-}
-
// This test verifies correct modeling of a relational dependency that goes
// through unmodeled functions (the simple `cond()` in this case).
TEST(TransferTest, ConditionalRelation) {
>From 53f76a925de925135e8bf546d67386faf24e680f Mon Sep 17 00:00:00 2001
From: Martin Braenne <mboehme at google.com>
Date: Wed, 10 Apr 2024 19:42:21 +0000
Subject: [PATCH 2/2] Remove now-unused function
`isOriginalRecordConstructor()`.
---
.../FlowSensitive/DataflowEnvironment.cpp | 22 -------------------
1 file changed, 22 deletions(-)
diff --git a/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp b/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
index 6c796b4ad923e8..bea15ce9bd24d1 100644
--- a/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
+++ b/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
@@ -989,28 +989,6 @@ StorageLocation *Environment::getStorageLocation(const Expr &E) const {
return It == ExprToLoc.end() ? nullptr : &*It->second;
}
-// Returns whether a prvalue of record type is the one that originally
-// constructs the object (i.e. it doesn't propagate it from one of its
-// children).
-static bool isOriginalRecordConstructor(const Expr &RecordPRValue) {
- if (auto *Init = dyn_cast<InitListExpr>(&RecordPRValue))
- return !Init->isSemanticForm() || !Init->isTransparent();
- return isa<CXXConstructExpr>(RecordPRValue) || isa<CallExpr>(RecordPRValue) ||
- isa<LambdaExpr>(RecordPRValue) ||
- isa<CXXDefaultArgExpr>(RecordPRValue) ||
- isa<CXXDefaultInitExpr>(RecordPRValue) ||
- // The framework currently does not propagate the objects created in
- // the two branches of a `ConditionalOperator` because there is no way
- // to reconcile their storage locations, which are different. We
- // therefore claim that the `ConditionalOperator` is the expression
- // that originally constructs the object.
- // Ultimately, this will be fixed by propagating locations down from
- // the result object, rather than up from the original constructor as
- // we do now (see also the FIXME in the documentation for
- // `getResultObjectLocation()`).
- isa<ConditionalOperator>(RecordPRValue);
-}
-
RecordStorageLocation &
Environment::getResultObjectLocation(const Expr &RecordPRValue) const {
assert(RecordPRValue.getType()->isRecordType());
More information about the cfe-commits
mailing list