diff --git a/clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h b/clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h index d7e39ab2616fd9..5943af50b6ad8f 100644 --- a/clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h +++ b/clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h @@ -325,7 +325,8 @@ class Environment { /// /// Requirements: /// `E` must be a prvalue of record type. - RecordStorageLocation &getResultObjectLocation(const Expr &RecordPRValue); + RecordStorageLocation & + getResultObjectLocation(const Expr &RecordPRValue) const; /// Returns the return value of the current function. This can be null if: /// - The function has a void return type @@ -434,24 +435,14 @@ class Environment { /// Assigns `Val` as the value of the prvalue `E` in the environment. /// - /// If `E` is not yet associated with a storage location, associates it with - /// a newly created storage location. In any case, associates the storage - /// location of `E` with `Val`. - /// - /// Once the migration to strict handling of value categories is complete - /// (see https://discourse.llvm.org/t/70086), this function will be renamed to - /// `setValue()`. At this point, prvalue expressions will be associated - /// directly with `Value`s, and the legacy behavior of associating prvalue - /// expressions with storage locations (as described above) will be - /// eliminated. - /// /// Requirements: /// - /// `E` must be a prvalue - /// If `Val` is a `RecordValue`, its `RecordStorageLocation` must be the - /// same as that of any `RecordValue` that has already been associated with - /// `E`. This is to guarantee that the result object initialized by a prvalue - /// `RecordValue` has a durable storage location. + /// - `E` must be a prvalue + /// - If `Val` is a `RecordValue`, its `RecordStorageLocation` must be + /// `getResultObjectLocation(E)`. An exception to this is if `E` is an + /// expression that originally creates a `RecordValue` (such as a + /// `CXXConstructExpr` or `CallExpr`), as these establish the location of + /// the result object in the first place. void setValue(const Expr &E, Value &Val); /// Returns the value assigned to `Loc` in the environment or null if `Loc` @@ -608,14 +599,6 @@ class Environment { // The copy-constructor is for use in fork() only. Environment(const Environment &) = default; - /// Internal version of `setStorageLocation()` that doesn't check if the - /// expression is a prvalue. - void setStorageLocationInternal(const Expr &E, StorageLocation &Loc); - - /// Internal version of `getStorageLocation()` that doesn't check if the - /// expression is a prvalue. - StorageLocation *getStorageLocationInternal(const Expr &E) const; - /// Creates a value appropriate for `Type`, if `Type` is supported, otherwise /// return null. /// diff --git a/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp b/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp index b98037b7364522..93919cd0243d0c 100644 --- a/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp +++ b/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp @@ -726,27 +726,70 @@ void Environment::setStorageLocation(const Expr &E, StorageLocation &Loc) { // so allow these as an exception. assert(E.isGLValue() || E.getType()->isSpecificBuiltinType(BuiltinType::BuiltinFn)); - setStorageLocationInternal(E, Loc); + const Expr &CanonE = ignoreCFGOmittedNodes(E); + assert(!ExprToLoc.contains(&CanonE)); + ExprToLoc[&CanonE] = &Loc; } StorageLocation *Environment::getStorageLocation(const Expr &E) const { // See comment in `setStorageLocation()`. assert(E.isGLValue() || E.getType()->isSpecificBuiltinType(BuiltinType::BuiltinFn)); - return getStorageLocationInternal(E); + auto It = ExprToLoc.find(&ignoreCFGOmittedNodes(E)); + 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(&RecordPRValue)) + return !Init->isSemanticForm() || !Init->isTransparent(); + return isa(RecordPRValue) || isa(RecordPRValue) || + isa(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(RecordPRValue); } RecordStorageLocation & -Environment::getResultObjectLocation(const Expr &RecordPRValue) { +Environment::getResultObjectLocation(const Expr &RecordPRValue) const { assert(RecordPRValue.getType()->isRecordType()); assert(RecordPRValue.isPRValue()); - if (StorageLocation *ExistingLoc = getStorageLocationInternal(RecordPRValue)) - return *cast(ExistingLoc); - auto &Loc = cast( - DACtx->getStableStorageLocation(RecordPRValue)); - setStorageLocationInternal(RecordPRValue, Loc); - return Loc; + // Returns a storage location that we can use if assertions fail. + auto FallbackForAssertFailure = + [this, &RecordPRValue]() -> RecordStorageLocation & { + return cast( + DACtx->getStableStorageLocation(RecordPRValue)); + }; + + 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(); + } + + // 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) { @@ -760,6 +803,11 @@ void Environment::setValue(const StorageLocation &Loc, Value &Val) { } void Environment::setValue(const Expr &E, Value &Val) { + if (auto *RecordVal = dyn_cast(&Val)) { + assert(isOriginalRecordConstructor(E) || + &RecordVal->getLoc() == &getResultObjectLocation(E)); + } + assert(E.isPRValue()); ExprToVal[&E] = &Val; } @@ -799,18 +847,6 @@ Value *Environment::createValue(QualType Type) { return Val; } -void Environment::setStorageLocationInternal(const Expr &E, - StorageLocation &Loc) { - const Expr &CanonE = ignoreCFGOmittedNodes(E); - assert(!ExprToLoc.contains(&CanonE)); - ExprToLoc[&CanonE] = &Loc; -} - -StorageLocation *Environment::getStorageLocationInternal(const Expr &E) const { - auto It = ExprToLoc.find(&ignoreCFGOmittedNodes(E)); - return It == ExprToLoc.end() ? nullptr : &*It->second; -} - Value *Environment::createValueUnlessSelfReferential( QualType Type, llvm::DenseSet &Visited, int Depth, int &CreatedValuesCount) { @@ -1044,6 +1080,7 @@ RecordValue &refreshRecordValue(const Expr &Expr, Environment &Env) { if (auto *ExistingVal = cast_or_null(Env.getValue(Expr))) { auto &NewVal = Env.create(ExistingVal->getLoc()); Env.setValue(Expr, NewVal); + Env.setValue(NewVal.getLoc(), NewVal); return NewVal; } diff --git a/clang/lib/Analysis/FlowSensitive/Transfer.cpp b/clang/lib/Analysis/FlowSensitive/Transfer.cpp index bbf5f12359bc70..346469660662e0 100644 --- a/clang/lib/Analysis/FlowSensitive/Transfer.cpp +++ b/clang/lib/Analysis/FlowSensitive/Transfer.cpp @@ -489,7 +489,6 @@ class TransferVisitor : public ConstStmtVisitor { if (S->getType()->isRecordType()) { auto &InitialVal = *cast(Env.createValue(S->getType())); Env.setValue(*S, InitialVal); - copyRecord(InitialVal.getLoc(), Env.getResultObjectLocation(*S), Env); } transferInlineCall(S, ConstructorDecl); @@ -582,6 +581,14 @@ class TransferVisitor : public ConstStmtVisitor { Env.setValue(*S, *ArgVal); } 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 (S->getType()->isRecordType() && S->isPRValue()) + if (Env.getValue(*S) == nullptr) + refreshRecordValue(*S, Env); } } diff --git a/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp b/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp index 8da55953a32986..056c4f3383d832 100644 --- a/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp +++ b/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp @@ -2635,6 +2635,55 @@ TEST(TransferTest, BindTemporary) { }); } +TEST(TransferTest, ResultObjectLocation) { + std::string Code = R"( + struct A { + virtual ~A() = default; + }; + + void target() { + A(); + (void)0; // [[p]] + } + )"; + using ast_matchers::cxxBindTemporaryExpr; + using ast_matchers::cxxTemporaryObjectExpr; + using ast_matchers::exprWithCleanups; + using ast_matchers::has; + 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"); + + // The expresssion `A()` in the code above produces the following + // structure, consisting of three prvalues of record type. + // `Env.getResultObjectLocation()` should return the same location for + // all of these. + auto MatchResult = match( + traverse(TK_AsIs, + exprWithCleanups( + has(cxxBindTemporaryExpr( + has(cxxTemporaryObjectExpr().bind("toe"))) + .bind("bte"))) + .bind("ewc")), + ASTCtx); + auto *TOE = selectFirst("toe", MatchResult); + ASSERT_NE(TOE, nullptr); + auto *EWC = selectFirst("ewc", MatchResult); + ASSERT_NE(EWC, nullptr); + auto *BTE = selectFirst("bte", MatchResult); + ASSERT_NE(BTE, nullptr); + + RecordStorageLocation &Loc = Env.getResultObjectLocation(*TOE); + EXPECT_EQ(&Loc, &Env.getResultObjectLocation(*EWC)); + EXPECT_EQ(&Loc, &Env.getResultObjectLocation(*BTE)); + }); +} + TEST(TransferTest, StaticCast) { std::string Code = R"( void target(int Foo) {