From 7549b45825a05fc24fcdbacf006461165aa042cb Mon Sep 17 00:00:00 2001 From: martinboehme Date: Wed, 10 Apr 2024 21:27:10 +0200 Subject: Revert "[clang][dataflow] Propagate locations from result objects to initializers." (#88315) Reverts llvm/llvm-project#87320 This is causing buildbots to fail because `isOriginalRecordConstructor()` is now unused. --- .../Analysis/FlowSensitive/DataflowEnvironment.h | 64 +--- .../Analysis/FlowSensitive/DataflowEnvironment.cpp | 405 +++++---------------- clang/lib/Analysis/FlowSensitive/Transfer.cpp | 176 +++++---- .../FlowSensitive/TypeErasedDataflowAnalysis.cpp | 13 +- .../FlowSensitive/DataflowEnvironmentTest.cpp | 43 --- .../Analysis/FlowSensitive/TransferTest.cpp | 172 +++------ 6 files changed, 283 insertions(+), 590 deletions(-) diff --git a/clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h b/clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h index 706664d..9a65f76 100644 --- a/clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h +++ b/clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h @@ -30,7 +30,6 @@ #include "llvm/ADT/MapVector.h" #include "llvm/Support/Compiler.h" #include "llvm/Support/ErrorHandling.h" -#include #include #include @@ -345,6 +344,17 @@ public: /// 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 & @@ -452,13 +462,7 @@ public: /// 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. - /// 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()); - } + void initializeFieldsWithValues(RecordStorageLocation &Loc); /// Assigns `Val` as the value of `Loc` in the environment. void setValue(const StorageLocation &Loc, Value &Val); @@ -649,9 +653,6 @@ public: LLVM_DUMP_METHOD void dump(raw_ostream &OS) const; private: - using PrValueToResultObject = - llvm::DenseMap; - // The copy-constructor is for use in fork() only. Environment(const Environment &) = default; @@ -681,10 +682,8 @@ private: /// 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`). If `Type` is different from - /// `Loc.getType()`, initializes only those fields that are modeled for - /// `Type`. - void initializeFieldsWithValues(RecordStorageLocation &Loc, QualType Type, + /// `Depth`, and `CreatedValuesCount`). + void initializeFieldsWithValues(RecordStorageLocation &Loc, llvm::DenseSet &Visited, int Depth, int &CreatedValuesCount); @@ -703,45 +702,22 @@ private: /// 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`, `ResultObjectMap`, `ReturnVal`, - // `ReturnLoc` and `ThisPointeeLoc` into a separate call-context object, - // shared between environments in the same call. + // FIXME: move the fields `CallStack`, `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 CallStack; - // 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 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 returned by the function (if it has non-reference return type). Value *ReturnVal = nullptr; - // - If the return type is a reference: Storage location of the reference - // returned by the function. + // Storage location of the reference returned by the function (if it has + // reference return type). 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 6c796b4..1bfa7eb 100644 --- a/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp +++ b/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp @@ -15,7 +15,6 @@ #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" @@ -27,8 +26,6 @@ #include #include -#define DEBUG_TYPE "dataflow" - namespace clang { namespace dataflow { @@ -357,8 +354,6 @@ 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(&S)) - getFieldsGlobalsAndFuncs(*DefaultArg->getExpr(), Fields, Vars, Funcs); if (const auto *DefaultInit = dyn_cast(&S)) getFieldsGlobalsAndFuncs(*DefaultInit->getExpr(), Fields, Vars, Funcs); @@ -391,186 +386,6 @@ 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 { -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 &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( - 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(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::TraverseBindingDecl(BD); - } - - bool VisitVarDecl(VarDecl *VD) { - if (VD->getType()->isRecordType() && VD->hasInit()) - PropagateResultObject( - VD->getInit(), - &cast(DACtx.getStableStorageLocation(*VD))); - return true; - } - - bool VisitMaterializeTemporaryExpr(MaterializeTemporaryExpr *MTE) { - if (MTE->getType()->isRecordType()) - PropagateResultObject( - MTE->getSubExpr(), - &cast(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(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(E) || isa(E) || isa(E) || - isa(E) || isa(E) || - isa(E)) { - return; - } - - if (auto *InitList = dyn_cast(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(Loc->getChild(*Field))); - } - return; - } - - if (auto *Op = dyn_cast(E); Op && Op->isCommaOp()) { - PropagateResultObject(Op->getRHS(), Loc); - return; - } - - if (auto *Cond = dyn_cast(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 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(S), Loc); - } - -private: - llvm::DenseMap &ResultObjectMap; - RecordStorageLocation *LocForRecordReturnVal; - DataflowAnalysisContext &DACtx; -}; - -} // namespace - Environment::Environment(DataflowAnalysisContext &DACtx) : DACtx(&DACtx), FlowConditionToken(DACtx.arena().makeFlowConditionToken()) {} @@ -586,23 +401,17 @@ void Environment::initialize() { if (DeclCtx == nullptr) return; - const auto *FuncDecl = dyn_cast(DeclCtx); - if (FuncDecl == nullptr) - return; - - assert(FuncDecl->doesThisDeclarationHaveABody()); + if (const auto *FuncDecl = dyn_cast(DeclCtx)) { + 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( - createStorageLocation(FuncDecl->getReturnType())); - if (const auto *MethodDecl = dyn_cast(DeclCtx)) { auto *Parent = MethodDecl->getParent(); assert(Parent != nullptr); @@ -635,12 +444,6 @@ 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( - buildResultObjectMap(DACtx, FuncDecl, getThisPointeeStorageLocation(), - LocForRecordReturnVal)); } // FIXME: Add support for resetting globals after function calls to enable @@ -681,18 +484,13 @@ void Environment::initFieldsGlobalsAndFuncs(const FunctionDecl *FuncDecl) { if (getStorageLocation(*D) != nullptr) continue; - // 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)); + setStorageLocation(*D, createObject(*D)); } for (const FunctionDecl *FD : Funcs) { if (getStorageLocation(*FD) != nullptr) continue; - auto &Loc = createStorageLocation(*FD); + auto &Loc = createStorageLocation(FD->getType()); setStorageLocation(*FD, Loc); } } @@ -721,9 +519,6 @@ 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())); @@ -734,7 +529,6 @@ 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())); @@ -763,10 +557,6 @@ void Environment::pushCallInternal(const FunctionDecl *FuncDecl, const VarDecl *Param = *ParamIt; setStorageLocation(*Param, createObject(*Param, Args[ArgIndex])); } - - ResultObjectMap = std::make_shared( - buildResultObjectMap(DACtx, FuncDecl, getThisPointeeStorageLocation(), - LocForRecordReturnVal)); } void Environment::popCall(const CallExpr *Call, const Environment &CalleeEnv) { @@ -810,9 +600,6 @@ bool Environment::equivalentTo(const Environment &Other, if (ReturnLoc != Other.ReturnLoc) return false; - if (LocForRecordReturnVal != Other.LocForRecordReturnVal) - return false; - if (ThisPointeeLoc != Other.ThisPointeeLoc) return false; @@ -836,10 +623,8 @@ 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; @@ -871,16 +656,12 @@ 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) { @@ -949,12 +730,6 @@ 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(D) || - &Loc == &createStorageLocation(D)); DeclToLoc[&D] = &Loc; } @@ -1016,29 +791,50 @@ Environment::getResultObjectLocation(const Expr &RecordPRValue) const { assert(RecordPRValue.getType()->isRecordType()); assert(RecordPRValue.isPRValue()); - 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) + // Returns a storage location that we can use if assertions fail. + auto FallbackForAssertFailure = + [this, &RecordPRValue]() -> RecordStorageLocation & { return cast( DACtx->getStableStorageLocation(RecordPRValue)); - return *Loc; + }; + + if (isOriginalRecordConstructor(RecordPRValue)) { + auto *Val = cast_or_null(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(&RecordPRValue); + Op && Op->isCommaOp()) { + return getResultObjectLocation(*Op->getRHS()); + } + + // All other expression nodes that propagate a record prvalue should have + // exactly one child. + llvm::SmallVector children(RecordPRValue.child_begin(), + RecordPRValue.child_end()); + assert(children.size() == 1); + if (children.empty()) + return FallbackForAssertFailure(); + + return getResultObjectLocation(*cast(children[0])); } PointerValue &Environment::getOrCreateNullPointerValue(QualType PointeeType) { return DACtx->getOrCreateNullPointerValue(PointeeType); } -void Environment::initializeFieldsWithValues(RecordStorageLocation &Loc, - QualType Type) { +void Environment::initializeFieldsWithValues(RecordStorageLocation &Loc) { llvm::DenseSet Visited; int CreatedValuesCount = 0; - initializeFieldsWithValues(Loc, Type, Visited, 0, CreatedValuesCount); + initializeFieldsWithValues(Loc, Visited, 0, CreatedValuesCount); if (CreatedValuesCount > MaxCompositeValueSize) { - llvm::errs() << "Attempting to initialize a huge value of type: " << Type - << '\n'; + llvm::errs() << "Attempting to initialize a huge value of type: " + << Loc.getType() << '\n'; } } @@ -1052,7 +848,8 @@ void Environment::setValue(const Expr &E, Value &Val) { const Expr &CanonE = ignoreCFGOmittedNodes(E); if (auto *RecordVal = dyn_cast(&Val)) { - assert(&RecordVal->getLoc() == &getResultObjectLocation(CanonE)); + assert(isOriginalRecordConstructor(CanonE) || + &RecordVal->getLoc() == &getResultObjectLocation(CanonE)); (void)RecordVal; } @@ -1131,8 +928,7 @@ Value *Environment::createValueUnlessSelfReferential( if (Type->isRecordType()) { CreatedValuesCount++; auto &Loc = cast(createStorageLocation(Type)); - initializeFieldsWithValues(Loc, Loc.getType(), Visited, Depth, - CreatedValuesCount); + initializeFieldsWithValues(Loc, Visited, Depth, CreatedValuesCount); return &refreshRecordValue(Loc, *this); } @@ -1164,7 +960,6 @@ Environment::createLocAndMaybeValue(QualType Ty, } void Environment::initializeFieldsWithValues(RecordStorageLocation &Loc, - QualType Type, llvm::DenseSet &Visited, int Depth, int &CreatedValuesCount) { @@ -1172,8 +967,8 @@ void Environment::initializeFieldsWithValues(RecordStorageLocation &Loc, if (FieldType->isRecordType()) { auto &FieldRecordLoc = cast(FieldLoc); setValue(FieldRecordLoc, create(FieldRecordLoc)); - initializeFieldsWithValues(FieldRecordLoc, FieldRecordLoc.getType(), - Visited, Depth + 1, CreatedValuesCount); + initializeFieldsWithValues(FieldRecordLoc, Visited, Depth + 1, + CreatedValuesCount); } else { if (!Visited.insert(FieldType.getCanonicalType()).second) return; @@ -1184,7 +979,7 @@ void Environment::initializeFieldsWithValues(RecordStorageLocation &Loc, } }; - for (const FieldDecl *Field : DACtx->getModeledFields(Type)) { + for (const auto &[Field, FieldLoc] : Loc.children()) { assert(Field != nullptr); QualType FieldType = Field->getType(); @@ -1193,12 +988,14 @@ 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, FieldType] : DACtx->getSyntheticFields(Type)) { + for (const auto &[FieldName, FieldLoc] : Loc.synthetic_fields()) { + assert(FieldLoc != nullptr); + QualType FieldType = FieldLoc->getType(); + // Synthetic fields cannot have reference type, so we don't need to deal // with this case. assert(!FieldType->isReferenceType()); @@ -1225,36 +1022,38 @@ 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(InitExpr) && + InitExpr->getType()->isPointerType()) + Val = &getOrCreateNullPointerValue(InitExpr->getType()->getPointeeType()); + } + if (!Val) + Val = createValue(Ty); + + if (Ty->isRecordType()) + return cast(Val)->getLoc(); + StorageLocation &Loc = D ? createStorageLocation(*D) : createStorageLocation(Ty); - if (Ty->isRecordType()) { - auto &RecordLoc = cast(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); - } + if (Val) + setValue(Loc, *Val); return Loc; } @@ -1273,8 +1072,6 @@ bool Environment::allows(const Formula &F) const { void Environment::dump(raw_ostream &OS) const { llvm::DenseMap LocToName; - if (LocForRecordReturnVal != nullptr) - LocToName[LocForRecordReturnVal] = "(returned record)"; if (ThisPointeeLoc != nullptr) LocToName[ThisPointeeLoc] = "this"; @@ -1305,9 +1102,6 @@ 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(Func)) { - OS << "LocForRecordReturnVal: " << LocForRecordReturnVal << "\n"; } else if (!Func->getReturnType()->isVoidType()) { if (ReturnVal == nullptr) OS << "ReturnVal: nullptr\n"; @@ -1328,22 +1122,6 @@ 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(FuncDecl)) - Visitor.TraverseConstructorInits(Ctor, ThisPointeeLoc); - Visitor.TraverseStmt(FuncDecl->getBody()); - - return Map; -} - RecordStorageLocation *getImplicitObjectLocation(const CXXMemberCallExpr &MCE, const Environment &Env) { Expr *ImplicitObject = MCE.getImplicitObjectArgument(); @@ -1438,11 +1216,24 @@ RecordValue &refreshRecordValue(RecordStorageLocation &Loc, Environment &Env) { RecordValue &refreshRecordValue(const Expr &Expr, Environment &Env) { assert(Expr.getType()->isRecordType()); - if (Expr.isPRValue()) - refreshRecordValue(Env.getResultObjectLocation(Expr), Env); + if (Expr.isPRValue()) { + if (auto *ExistingVal = Env.get(Expr)) { + auto &NewVal = Env.create(ExistingVal->getLoc()); + Env.setValue(Expr, NewVal); + Env.setValue(NewVal.getLoc(), NewVal); + return NewVal; + } - if (auto *Loc = Env.get(Expr)) - refreshRecordValue(*Loc, Env); + auto &NewVal = *cast(Env.createValue(Expr.getType())); + Env.setValue(Expr, NewVal); + return NewVal; + } + + if (auto *Loc = Env.get(Expr)) { + auto &NewVal = Env.create(*Loc); + Env.setValue(*Loc, NewVal); + return NewVal; + } auto &NewVal = *cast(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 88a9c0e..0a2e836 100644 --- a/clang/lib/Analysis/FlowSensitive/Transfer.cpp +++ b/clang/lib/Analysis/FlowSensitive/Transfer.cpp @@ -460,9 +460,11 @@ public: // 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) { - auto &Loc = Env.getResultObjectLocation(*S); - Env.initializeFieldsWithValues(Loc); - refreshRecordValue(Loc, Env); + 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); } } } @@ -470,13 +472,6 @@ public: 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); } @@ -484,17 +479,6 @@ public: 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. @@ -507,14 +491,24 @@ public: if (ArgLoc == nullptr) return; - // 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); + if (S->isElidable()) { + if (Value *Val = Env.getValue(*ArgLoc)) + Env.setValue(*S, *Val); + } else { + auto &Val = *cast(Env.createValue(S->getType())); + Env.setValue(*S, Val); + copyRecord(*ArgLoc, Val.getLoc(), Env); + } return; } - Env.initializeFieldsWithValues(Loc, S->getType()); + // `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(Env.createValue(S->getType())); + Env.setValue(*S, InitialVal); + } transferInlineCall(S, ConstructorDecl); } @@ -557,15 +551,19 @@ public: if (S->isGLValue()) { Env.setStorageLocation(*S, *LocDst); } else if (S->getType()->isRecordType()) { - // Assume that the assignment returns the assigned value. - copyRecord(*LocDst, Env.getResultObjectLocation(*S), Env); + // 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); } return; } - // `CXXOperatorCallExpr` can be a prvalue. Call `VisitCallExpr`() to - // initialize the prvalue's fields with values. + // CXXOperatorCallExpr can be prvalues. Call `VisitCallExpr`() to create + // a `RecordValue` for them so that `Environment::getResultObjectLocation()` + // can return a value. VisitCallExpr(S); } @@ -582,6 +580,11 @@ public: } } + 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 @@ -609,14 +612,13 @@ public: } else if (const FunctionDecl *F = S->getDirectCallee()) { transferInlineCall(S, F); - // If this call produces a prvalue of record type, initialize its fields - // with values. + // 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 (S->getType()->isRecordType() && S->isPRValue()) - if (Env.getValue(*S) == nullptr) { - RecordStorageLocation &Loc = Env.getResultObjectLocation(*S); - Env.initializeFieldsWithValues(Loc); - Env.setValue(*S, refreshRecordValue(Loc, Env)); - } + if (Env.getValue(*S) == nullptr) + refreshRecordValue(*S, Env); } } @@ -664,10 +666,8 @@ public: // `getLogicOperatorSubExprValue()`. if (S->isGLValue()) Env.setStorageLocation(*S, Env.createObject(S->getType())); - else if (!S->getType()->isRecordType()) { - if (Value *Val = Env.createValue(S->getType())) - Env.setValue(*S, *Val); - } + else if (Value *Val = Env.createValue(S->getType())) + Env.setValue(*S, *Val); } void VisitInitListExpr(const InitListExpr *S) { @@ -688,51 +688,71 @@ public: return; } - RecordStorageLocation &Loc = Env.getResultObjectLocation(*S); - Env.setValue(*S, refreshRecordValue(Loc, Env)); - - // 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. - + llvm::DenseMap FieldLocs; RecordInitListHelper InitListHelper(S); + for (auto [Base, Init] : InitListHelper.base_inits()) { + assert(Base->getType().getCanonicalType() == + Init->getType().getCanonicalType()); + auto *BaseVal = Env.get(*Init); + if (!BaseVal) + BaseVal = cast(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()); + } + 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(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); + 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}); } - for (const auto &[FieldName, FieldLoc] : Loc.synthetic_fields()) { - QualType FieldType = FieldLoc->getType(); - if (FieldType->isRecordType()) { - Env.initializeFieldsWithValues(*cast(FieldLoc)); - } else { - if (Value *Val = Env.createValue(FieldType)) - Env.setValue(*FieldLoc, *Val); + // 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()); } } + // 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())}); + } + + auto &Loc = Env.getDataflowAnalysisContext().createRecordStorageLocation( + Type, std::move(FieldLocs), std::move(SyntheticFieldLocs)); + RecordValue &RecordVal = Env.create(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 1b73c5d..595f70f 100644 --- a/clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp +++ b/clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp @@ -369,10 +369,17 @@ builtinTransferInitializer(const CFGInitializer &Elt, ParentLoc->setChild(*Member, InitExprLoc); } else if (auto *InitExprVal = Env.getValue(*InitExpr)) { assert(MemberLoc != nullptr); - // Record-type initializers construct themselves directly into the result - // object, so there is no need to handle them here. - if (!Member->getType()->isRecordType()) + if (Member->getType()->isRecordType()) { + auto *InitValStruct = cast(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(MemberLoc), Env); + } else { Env.setValue(*MemberLoc, *InitExprVal); + } } } diff --git a/clang/unittests/Analysis/FlowSensitive/DataflowEnvironmentTest.cpp b/clang/unittests/Analysis/FlowSensitive/DataflowEnvironmentTest.cpp index cc20623..465a8e2 100644 --- a/clang/unittests/Analysis/FlowSensitive/DataflowEnvironmentTest.cpp +++ b/clang/unittests/Analysis/FlowSensitive/DataflowEnvironmentTest.cpp @@ -24,7 +24,6 @@ namespace { using namespace clang; using namespace dataflow; -using ::clang::dataflow::test::findValueDecl; using ::clang::dataflow::test::getFieldValue; using ::testing::Contains; using ::testing::IsNull; @@ -200,48 +199,6 @@ 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 00dafb2..ca055a4 100644 --- a/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp +++ b/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp @@ -1582,9 +1582,10 @@ TEST(TransferTest, FieldsDontHaveValuesInConstructorWithBaseClass) { [](const llvm::StringMap> &Results, ASTContext &ASTCtx) { const Environment &Env = getEnvironmentAtAnnotation(Results, "p"); - // The field of the base class should already have been initialized with - // a value by the base constructor. - EXPECT_NE(getFieldValue(Env.getThisPointeeStorageLocation(), "BaseVal", + // 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", ASTCtx, Env), nullptr); EXPECT_EQ(getFieldValue(Env.getThisPointeeStorageLocation(), "Val", @@ -2997,12 +2998,8 @@ TEST(TransferTest, ResultObjectLocation) { TEST(TransferTest, ResultObjectLocationForDefaultArgExpr) { std::string Code = R"( - struct Inner {}; - struct Outer { - Inner I = {}; - }; - - void funcWithDefaultArg(Outer O = {}); + struct S {}; + void funcWithDefaultArg(S s = S()); void target() { funcWithDefaultArg(); // [[p]] @@ -3061,7 +3058,13 @@ TEST(TransferTest, ResultObjectLocationForDefaultInitExpr) { RecordStorageLocation &Loc = Env.getResultObjectLocation(*DefaultInit); - EXPECT_EQ(&Loc, Env.getThisPointeeStorageLocation()->getChild(*SField)); + // 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)); }); } @@ -3098,79 +3101,6 @@ TEST(TransferTest, ResultObjectLocationForCXXOperatorCallExpr) { }); } -TEST(TransferTest, ResultObjectLocationForStdInitializerListExpr) { - std::string Code = R"( - namespace std { - template - struct initializer_list {}; - } // namespace std - - void target() { - std::initializer_list list = {1}; - // [[p]] - } - )"; - - using ast_matchers::cxxStdInitializerListExpr; - using ast_matchers::match; - using ast_matchers::selectFirst; - runDataflow( - Code, - [](const llvm::StringMap> &Results, - ASTContext &ASTCtx) { - const Environment &Env = getEnvironmentAtAnnotation(Results, "p"); - - auto *StdInitList = selectFirst( - "std_init_list", - match(cxxStdInitializerListExpr().bind("std_init_list"), ASTCtx)); - ASSERT_NE(StdInitList, nullptr); - - EXPECT_EQ(&Env.getResultObjectLocation(*StdInitList), - &getLocForDecl(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> &Results, - ASTContext &ASTCtx) { - const Environment &Env = getEnvironmentAtAnnotation(Results, "p"); - - auto *ConstructExpr0 = selectFirst( - "construct", - match(cxxConstructExpr(hasArgument(0, integerLiteral(equals(0)))) - .bind("construct"), - ASTCtx)); - auto *ConstructExpr1 = selectFirst( - "construct", - match(cxxConstructExpr(hasArgument(0, integerLiteral(equals(1)))) - .bind("construct"), - ASTCtx)); - - auto &ALoc = getLocForDecl(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) { @@ -5956,38 +5886,6 @@ 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> &Results, - ASTContext &ASTCtx) { - const Environment &Env = getEnvironmentAtAnnotation(Results, "p"); - - auto &MySLoc = getLocForDecl(ASTCtx, Env, "MyS"); - - auto *SelfVal = - cast(getFieldValue(&MySLoc, "self", ASTCtx, Env)); - EXPECT_EQ(&SelfVal->getPointeeLoc(), &MySLoc); - }, - {BuiltinOptions{ContextSensitiveOptions{}}}); -} - TEST(TransferTest, ContextSensitiveMethodLiteral) { std::string Code = R"( class MyClass { @@ -6932,6 +6830,50 @@ 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 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> &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) { -- cgit v1.1