diff --git a/clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h b/clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h index 9d99b6771f71e..5cf52ad3d7223 100644 --- a/clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h +++ b/clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h @@ -334,6 +334,31 @@ class Environment { /// in the environment. AggregateStorageLocation *getThisPointeeStorageLocation() const; + /// Returns the location of the result object for a record-type prvalue. + /// + /// In C++, prvalues of record type serve only a limited purpose: They can + /// only be used to initialize a result object (e.g. a variable or a + /// temporary). This function returns the location of that result object. + /// + /// When creating a prvalue of record type, we already need the storage + /// 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. + AggregateStorageLocation &getResultObjectLocation(const Expr &RecordPRValue); + /// Returns the return value of the current function. This can be null if: /// - The function has a void return type /// - No return value could be determined for the function, for example @@ -385,10 +410,16 @@ class Environment { PointerValue &getOrCreateNullPointerValue(QualType PointeeType); /// Creates a value appropriate for `Type`, if `Type` is supported, otherwise - /// return null. If `Type` is a pointer or reference type, creates all the - /// necessary storage locations and values for indirections until it finds a + /// returns null. + /// + /// If `Type` is a pointer or reference type, creates all the necessary + /// storage locations and values for indirections until it finds a /// non-pointer/non-reference type. /// + /// If `Type` is a class, struct, or union type, calls `setValue()` to + /// associate the `StructValue` with its storage location + /// (`StructValue::getAggregateLoc()`). + /// /// If `Type` is one of the following types, this function will always return /// a non-null pointer: /// - `bool` @@ -430,7 +461,7 @@ class Environment { void setValue(const StorageLocation &Loc, Value &Val); /// Clears any association between `Loc` and a value in the environment. - void clearValue(const StorageLocation &Loc); + void clearValue(const StorageLocation &Loc) { LocToVal.erase(&Loc); } /// Assigns `Val` as the value of the prvalue `E` in the environment. /// @@ -449,6 +480,10 @@ class Environment { /// /// `E` must be a prvalue /// `Val` must not be a `ReferenceValue` + /// If `Val` is a `StructValue`, its `AggregateStorageLocation` must be the + /// same as that of any `StructValue` that has already been associated with + /// `E`. This is to guarantee that the result object initialized by a prvalue + /// `StructValue` has a durable storage location. void setValueStrict(const Expr &E, Value &Val); /// Returns the value assigned to `Loc` in the environment or null if `Loc` @@ -624,6 +659,14 @@ class Environment { llvm::DenseSet &Visited, int Depth, int &CreatedValuesCount); + /// Creates a storage location for `Ty`. Also creates and associates a value + /// with the storage location, unless values of this type are not supported or + /// we hit one of the limits at which we stop producing values (controlled by + /// `Visited`, `Depth`, and `CreatedValuesCount`). + StorageLocation &createLocAndMaybeValue(QualType Ty, + llvm::DenseSet &Visited, + int Depth, int &CreatedValuesCount); + /// Shared implementation of `createObject()` overloads. /// `D` and `InitExpr` may be null. StorageLocation &createObjectInternal(const VarDecl *D, QualType Ty, @@ -672,12 +715,6 @@ class Environment { // deterministic sequence. This in turn produces deterministic SAT formulas. llvm::MapVector LocToVal; - // Maps locations of struct members to symbolic values of the structs that own - // them and the decls of the struct members. - llvm::DenseMap> - MemberLocToStruct; - Atom FlowConditionToken; }; diff --git a/clang/include/clang/Analysis/FlowSensitive/StorageLocation.h b/clang/include/clang/Analysis/FlowSensitive/StorageLocation.h index 453f1e13362b7..62e3d5e59c659 100644 --- a/clang/include/clang/Analysis/FlowSensitive/StorageLocation.h +++ b/clang/include/clang/Analysis/FlowSensitive/StorageLocation.h @@ -18,6 +18,7 @@ #include "clang/AST/Type.h" #include "llvm/ADT/DenseMap.h" #include "llvm/Support/Debug.h" +#include #define DEBUG_TYPE "dataflow" @@ -32,7 +33,9 @@ class StorageLocation { public: enum class Kind { Scalar, Aggregate }; - StorageLocation(Kind LocKind, QualType Type) : LocKind(LocKind), Type(Type) {} + StorageLocation(Kind LocKind, QualType Type) : LocKind(LocKind), Type(Type) { + assert(Type.isNull() || !Type->isReferenceType()); + } // Non-copyable because addresses of storage locations are used as their // identities throughout framework and user code. The framework is responsible @@ -68,6 +71,14 @@ class ScalarStorageLocation final : public StorageLocation { /// struct with public members. The child map is flat, so when used for a struct /// or class type, all accessible members of base struct and class types are /// directly accesible as children of this location. +/// +/// The storage location for a field of reference type may be null. This +/// typically occurs in one of two situations: +/// - The record has not been fully initialized. +/// - The maximum depth for modelling a self-referential data structure has been +/// reached. +/// Storage locations for fields of all other types must be non-null. +/// /// FIXME: Currently, the storage location of unions is modelled the same way as /// that of structs or classes. Eventually, we need to change this modelling so /// that all of the members of a given union have the same storage location. @@ -78,15 +89,32 @@ class AggregateStorageLocation final : public StorageLocation { explicit AggregateStorageLocation(QualType Type) : AggregateStorageLocation(Type, FieldToLoc()) {} - AggregateStorageLocation(QualType Type, FieldToLoc Children) - : StorageLocation(Kind::Aggregate, Type), Children(std::move(Children)) {} + AggregateStorageLocation(QualType Type, FieldToLoc TheChildren) + : StorageLocation(Kind::Aggregate, Type), + Children(std::move(TheChildren)) { + assert(!Type.isNull()); + assert(Type->isRecordType()); + assert([this] { + for (auto [Field, Loc] : Children) { + if (!Field->getType()->isReferenceType() && Loc == nullptr) + return false; + } + return true; + }()); + } static bool classof(const StorageLocation *Loc) { return Loc->getKind() == Kind::Aggregate; } /// Returns the child storage location for `D`. - StorageLocation &getChild(const ValueDecl &D) const { + /// + /// May return null if `D` has reference type; guaranteed to return non-null + /// in all other cases. + /// + /// Note that it is an error to call this with a field that does not exist. + /// The function does not return null in this case. + StorageLocation *getChild(const ValueDecl &D) const { auto It = Children.find(&D); LLVM_DEBUG({ if (It == Children.end()) { @@ -100,7 +128,19 @@ class AggregateStorageLocation final : public StorageLocation { } }); assert(It != Children.end()); - return *It->second; + return It->second; + } + + /// Changes the child storage location for a field `D` of reference type. + /// All other fields cannot change their storage location and always retain + /// the storage location passed to the `AggregateStorageLocation` constructor. + /// + /// Requirements: + /// + /// `D` must have reference type. + void setChild(const ValueDecl &D, StorageLocation *Loc) { + assert(D.getType()->isReferenceType()); + Children[&D] = Loc; } llvm::iterator_range children() const { diff --git a/clang/include/clang/Analysis/FlowSensitive/Value.h b/clang/include/clang/Analysis/FlowSensitive/Value.h index 1d4f8008d1930..7d9a7b7d28254 100644 --- a/clang/include/clang/Analysis/FlowSensitive/Value.h +++ b/clang/include/clang/Analysis/FlowSensitive/Value.h @@ -198,38 +198,59 @@ class PointerValue final : public Value { StorageLocation &PointeeLoc; }; -/// Models a value of `struct` or `class` type, with a flat map of fields to -/// child storage locations, containing all accessible members of base struct -/// and class types. +/// Models a value of `struct` or `class` type. +/// In C++, prvalues of class type serve only a limited purpose: They can only +/// be used to initialize a result object. It is not possible to access member +/// variables or call member functions on a prvalue of class type. +/// Correspondingly, `StructValue` also serves only two limited purposes: +/// - It conveys a prvalue of class type from the place where the object is +/// constructed to the result object that it initializes. +/// +/// When creating a prvalue of class type, we already need a storage location +/// for `this`, even though prvalues are otherwise not associated with storage +/// locations. `StructValue` is therefore essentially a wrapper for a storage +/// location, which is then used to set the storage location for the result +/// object when we process the AST node for that result object. +/// +/// For example: +/// MyStruct S = MyStruct(3); +/// +/// In this example, `MyStruct(3) is a prvalue, which is modeled as a +/// `StructValue` that wraps an `AbstractStorageLocation`. This +// `AbstractStorageLocation` is then used as the storage location for `S`. +/// +/// - It allows properties to be associated with an object of class type. +/// Note that when doing so, you should avoid mutating the properties of an +/// existing `StructValue` in place, as these changes would be visible to +/// other `Environment`s that share the same `StructValue`. Instead, associate +/// a new `StructValue` with the `AggregateStorageLocation` and set the +/// properties on this new `StructValue`. (See also `refreshStructValue()` in +/// DataflowEnvironment.h, which makes this easy.) +/// Note also that this implies that it is common for the same +/// `AggregateStorageLocation` to be associated with different `StructValue`s +/// in different environments. +/// Over time, we may eliminate `StructValue` entirely. See also the discussion +/// here: https://reviews.llvm.org/D155204#inline-1503204 class StructValue final : public Value { public: - StructValue() : StructValue(llvm::DenseMap()) {} - - explicit StructValue(llvm::DenseMap Children) - : Value(Kind::Struct), Children(std::move(Children)) {} + explicit StructValue(AggregateStorageLocation &Loc) + : Value(Kind::Struct), Loc(Loc) {} static bool classof(const Value *Val) { return Val->getKind() == Kind::Struct; } - /// Returns the child value that is assigned for `D` or null if the child is - /// not initialized. - Value *getChild(const ValueDecl &D) const { return Children.lookup(&D); } - - /// Assigns `Val` as the child value for `D`. - void setChild(const ValueDecl &D, Value &Val) { Children[&D] = &Val; } - - /// Clears any value assigned as the child value for `D`. - void clearChild(const ValueDecl &D) { Children.erase(&D); } + /// Returns the storage location that this `StructValue` is associated with. + AggregateStorageLocation &getAggregateLoc() const { return Loc; } - llvm::iterator_range< - llvm::DenseMap::const_iterator> - children() const { - return {Children.begin(), Children.end()}; + /// Convenience function that returns the child storage location for `Field`. + /// See also the documentation for `AggregateStorageLocation::getChild()`. + StorageLocation *getChild(const ValueDecl &Field) const { + return Loc.getChild(Field); } private: - llvm::DenseMap Children; + AggregateStorageLocation &Loc; }; raw_ostream &operator<<(raw_ostream &OS, const Value &Val); diff --git a/clang/lib/Analysis/FlowSensitive/DataflowAnalysisContext.cpp b/clang/lib/Analysis/FlowSensitive/DataflowAnalysisContext.cpp index bb1c2c7a1de10..9f72dc8f6ab38 100644 --- a/clang/lib/Analysis/FlowSensitive/DataflowAnalysisContext.cpp +++ b/clang/lib/Analysis/FlowSensitive/DataflowAnalysisContext.cpp @@ -62,7 +62,11 @@ StorageLocation &DataflowAnalysisContext::createStorageLocation(QualType Type) { if (!Type.isNull() && Type->isRecordType()) { llvm::DenseMap FieldLocs; for (const FieldDecl *Field : getModeledFields(Type)) - FieldLocs.insert({Field, &createStorageLocation(Field->getType())}); + if (Field->getType()->isReferenceType()) + FieldLocs.insert({Field, nullptr}); + else + FieldLocs.insert({Field, &createStorageLocation( + Field->getType().getNonReferenceType())}); return arena().create(Type, std::move(FieldLocs)); } return arena().create(Type); diff --git a/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp b/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp index 54632f5662b63..5514d25df6cc6 100644 --- a/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp +++ b/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp @@ -120,6 +120,24 @@ static Value *mergeDistinctValues(QualType Type, Value &Val1, return &A.makeBoolValue(MergedVal); } + Value *MergedVal = nullptr; + if (auto *StructVal1 = dyn_cast(&Val1)) { + auto *StructVal2 = cast(&Val2); + + // Values to be merged are always associated with the same location in + // `LocToVal`. The location stored in `StructVal` should therefore also + // be the same. + assert(&StructVal1->getAggregateLoc() == &StructVal2->getAggregateLoc()); + + // `StructVal1` and `StructVal2` may have different properties associated + // with them. Create a new `StructValue` without any properties so that we + // soundly approximate both values. If a particular analysis needs to merge + // properties, it should do so in `DataflowAnalysis::merge()`. + MergedVal = &MergedEnv.create(StructVal1->getAggregateLoc()); + } else { + MergedVal = MergedEnv.createValue(Type); + } + // FIXME: Consider destroying `MergedValue` immediately if `ValueModel::merge` // returns false to avoid storing unneeded values in `DACtx`. // FIXME: Creating the value based on the type alone creates misshapen values @@ -127,7 +145,7 @@ static Value *mergeDistinctValues(QualType Type, Value &Val1, // This issue will be resolved when `ReferenceValue` is eliminated as part // of the ongoing migration to strict handling of value categories (see // https://discourse.llvm.org/t/70086 for details). - if (Value *MergedVal = MergedEnv.createValue(Type)) + if (MergedVal) if (Model.merge(Type, Val1, Env1, Val2, Env2, *MergedVal, MergedEnv)) return MergedVal; @@ -307,10 +325,8 @@ Environment::Environment(DataflowAnalysisContext &DACtx, // FIXME: Initialize the ThisPointeeLoc of lambdas too. if (MethodDecl && !MethodDecl->isStatic()) { QualType ThisPointeeType = MethodDecl->getThisObjectType(); - ThisPointeeLoc = &cast( - createStorageLocation(ThisPointeeType)); - if (Value *ThisPointeeVal = createValue(ThisPointeeType)) - setValue(*ThisPointeeLoc, *ThisPointeeVal); + ThisPointeeLoc = + &cast(createValue(ThisPointeeType))->getAggregateLoc(); } } } @@ -342,10 +358,7 @@ Environment Environment::pushCall(const CallExpr *Call) const { Environment Environment::pushCall(const CXXConstructExpr *Call) const { Environment Env(*this); - Env.ThisPointeeLoc = &cast( - Env.createStorageLocation(Call->getType())); - if (Value *Val = Env.createValue(Call->getType())) - Env.setValue(*Env.ThisPointeeLoc, *Val); + Env.ThisPointeeLoc = &Env.getResultObjectLocation(*Call); Env.pushCallInternal(Call->getConstructor(), llvm::ArrayRef(Call->getArgs(), Call->getNumArgs())); @@ -385,7 +398,6 @@ void Environment::popCall(const CallExpr *Call, const Environment &CalleeEnv) { // these maps capture information from the local scope, so when popping that // scope, we do not propagate the maps. this->LocToVal = std::move(CalleeEnv.LocToVal); - this->MemberLocToStruct = std::move(CalleeEnv.MemberLocToStruct); this->FlowConditionToken = std::move(CalleeEnv.FlowConditionToken); if (Call->isGLValue()) { @@ -401,7 +413,6 @@ void Environment::popCall(const CXXConstructExpr *Call, const Environment &CalleeEnv) { // See also comment in `popCall(const CallExpr *, const Environment &)` above. this->LocToVal = std::move(CalleeEnv.LocToVal); - this->MemberLocToStruct = std::move(CalleeEnv.MemberLocToStruct); this->FlowConditionToken = std::move(CalleeEnv.FlowConditionToken); if (Value *Val = CalleeEnv.getValue(*CalleeEnv.ThisPointeeLoc)) { @@ -466,15 +477,8 @@ LatticeJoinEffect Environment::widen(const Environment &PrevEnv, // block. For `DeclToLoc` and `ExprToLoc`, join guarantees that these maps are // subsets of the maps in `PrevEnv`. So, as long as we maintain this property // here, we don't need change their current values to widen. - // - // FIXME: `MemberLocToStruct` does not share the above property, because - // `join` can cause the map size to increase (when we add fresh data in places - // of conflict). Once this issue with join is resolved, re-enable the - // assertion below or replace with something that captures the desired - // invariant. assert(DeclToLoc.size() <= PrevEnv.DeclToLoc.size()); assert(ExprToLoc.size() <= PrevEnv.ExprToLoc.size()); - // assert(MemberLocToStruct.size() <= PrevEnv.MemberLocToStruct.size()); llvm::MapVector WidenedLocToVal; for (auto &Entry : LocToVal) { @@ -501,12 +505,9 @@ LatticeJoinEffect Environment::widen(const Environment &PrevEnv, Effect = LatticeJoinEffect::Changed; } LocToVal = std::move(WidenedLocToVal); - // FIXME: update the equivalence calculation for `MemberLocToStruct`, once we - // have a systematic way of soundly comparing this map. if (DeclToLoc.size() != PrevEnv.DeclToLoc.size() || ExprToLoc.size() != PrevEnv.ExprToLoc.size() || - LocToVal.size() != PrevEnv.LocToVal.size() || - MemberLocToStruct.size() != PrevEnv.MemberLocToStruct.size()) + LocToVal.size() != PrevEnv.LocToVal.size()) Effect = LatticeJoinEffect::Changed; return Effect; @@ -559,9 +560,6 @@ Environment Environment::join(const Environment &EnvA, const Environment &EnvB, JoinedEnv.ExprToLoc = intersectDenseMaps(EnvA.ExprToLoc, EnvB.ExprToLoc); - JoinedEnv.MemberLocToStruct = - intersectDenseMaps(EnvA.MemberLocToStruct, EnvB.MemberLocToStruct); - // FIXME: update join to detect backedges and simplify the flow condition // accordingly. JoinedEnv.FlowConditionToken = EnvA.DACtx->joinFlowConditions( @@ -671,64 +669,46 @@ AggregateStorageLocation *Environment::getThisPointeeStorageLocation() const { return ThisPointeeLoc; } +AggregateStorageLocation & +Environment::getResultObjectLocation(const Expr &RecordPRValue) { + assert(RecordPRValue.getType()->isRecordType()); + assert(RecordPRValue.isPRValue()); + + if (StorageLocation *ExistingLoc = + getStorageLocation(RecordPRValue, SkipPast::None)) + return *cast(ExistingLoc); + auto &Loc = cast( + DACtx->getStableStorageLocation(RecordPRValue)); + setStorageLocation(RecordPRValue, Loc); + return Loc; +} + PointerValue &Environment::getOrCreateNullPointerValue(QualType PointeeType) { return DACtx->getOrCreateNullPointerValue(PointeeType); } void Environment::setValue(const StorageLocation &Loc, Value &Val) { - LocToVal[&Loc] = &Val; - - if (auto *StructVal = dyn_cast(&Val)) { - auto &AggregateLoc = *cast(&Loc); - - const QualType Type = AggregateLoc.getType(); - assert(Type->isRecordType()); - - for (const FieldDecl *Field : DACtx->getModeledFields(Type)) { - assert(Field != nullptr); - StorageLocation &FieldLoc = AggregateLoc.getChild(*Field); - MemberLocToStruct[&FieldLoc] = std::make_pair(StructVal, Field); - if (auto *FieldVal = StructVal->getChild(*Field)) - setValue(FieldLoc, *FieldVal); - } - } - - auto It = MemberLocToStruct.find(&Loc); - if (It != MemberLocToStruct.end()) { - // `Loc` is the location of a struct member so we need to also update the - // value of the member in the corresponding `StructValue`. - - assert(It->second.first != nullptr); - StructValue &StructVal = *It->second.first; - - assert(It->second.second != nullptr); - const ValueDecl &Member = *It->second.second; - - StructVal.setChild(Member, Val); - } -} - -void Environment::clearValue(const StorageLocation &Loc) { - LocToVal.erase(&Loc); - - if (auto It = MemberLocToStruct.find(&Loc); It != MemberLocToStruct.end()) { - // `Loc` is the location of a struct member so we need to also clear the - // member in the corresponding `StructValue`. - - assert(It->second.first != nullptr); - StructValue &StructVal = *It->second.first; + assert(!isa(&Val) || + &cast(&Val)->getAggregateLoc() == &Loc); - assert(It->second.second != nullptr); - const ValueDecl &Member = *It->second.second; - - StructVal.clearChild(Member); - } + LocToVal[&Loc] = &Val; } void Environment::setValueStrict(const Expr &E, Value &Val) { assert(E.isPRValue()); assert(!isa(Val)); + if (auto *StructVal = dyn_cast(&Val)) { + if (auto *ExistingVal = cast_or_null(getValueStrict(E))) + assert(&ExistingVal->getAggregateLoc() == &StructVal->getAggregateLoc()); + if (StorageLocation *ExistingLoc = getStorageLocation(E, SkipPast::None)) + assert(ExistingLoc == &StructVal->getAggregateLoc()); + else + setStorageLocation(E, StructVal->getAggregateLoc()); + setValue(StructVal->getAggregateLoc(), Val); + return; + } + StorageLocation *Loc = getStorageLocation(E, SkipPast::None); if (Loc == nullptr) { Loc = &createStorageLocation(E); @@ -802,16 +782,8 @@ Value *Environment::createValueUnlessSelfReferential( if (Type->isReferenceType() || Type->isPointerType()) { CreatedValuesCount++; QualType PointeeType = Type->getPointeeType(); - auto &PointeeLoc = createStorageLocation(PointeeType); - - if (Visited.insert(PointeeType.getCanonicalType()).second) { - Value *PointeeVal = createValueUnlessSelfReferential( - PointeeType, Visited, Depth, CreatedValuesCount); - Visited.erase(PointeeType.getCanonicalType()); - - if (PointeeVal != nullptr) - setValue(PointeeLoc, *PointeeVal); - } + StorageLocation &PointeeLoc = + createLocAndMaybeValue(PointeeType, Visited, Depth, CreatedValuesCount); if (Type->isReferenceType()) return &arena().create(PointeeLoc); @@ -821,27 +793,54 @@ Value *Environment::createValueUnlessSelfReferential( if (Type->isRecordType()) { CreatedValuesCount++; - llvm::DenseMap FieldValues; + llvm::DenseMap FieldLocs; for (const FieldDecl *Field : DACtx->getModeledFields(Type)) { assert(Field != nullptr); QualType FieldType = Field->getType(); - if (Visited.contains(FieldType.getCanonicalType())) - continue; - - Visited.insert(FieldType.getCanonicalType()); - if (auto *FieldValue = createValueUnlessSelfReferential( - FieldType, Visited, Depth + 1, CreatedValuesCount)) - FieldValues.insert({Field, FieldValue}); - Visited.erase(FieldType.getCanonicalType()); + + FieldLocs.insert( + {Field, &createLocAndMaybeValue(FieldType, Visited, Depth + 1, + CreatedValuesCount)}); } - return &arena().create(std::move(FieldValues)); + AggregateStorageLocation &Loc = + arena().create(Type, std::move(FieldLocs)); + StructValue &StructVal = create(Loc); + + // As we already have a storage location for the `StructValue`, we can and + // should associate them in the environment. + setValue(Loc, StructVal); + + return &StructVal; } return nullptr; } +StorageLocation & +Environment::createLocAndMaybeValue(QualType Ty, + llvm::DenseSet &Visited, + int Depth, int &CreatedValuesCount) { + if (!Visited.insert(Ty.getCanonicalType()).second) + return createStorageLocation(Ty.getNonReferenceType()); + Value *Val = createValueUnlessSelfReferential( + Ty.getNonReferenceType(), Visited, Depth, CreatedValuesCount); + Visited.erase(Ty.getCanonicalType()); + + Ty = Ty.getNonReferenceType(); + + if (Val == nullptr) + return createStorageLocation(Ty); + + if (Ty->isRecordType()) + return cast(Val)->getAggregateLoc(); + + StorageLocation &Loc = createStorageLocation(Ty); + setValue(Loc, *Val); + return Loc; +} + StorageLocation &Environment::createObjectInternal(const VarDecl *D, QualType Ty, const Expr *InitExpr) { @@ -881,6 +880,9 @@ StorageLocation &Environment::createObjectInternal(const VarDecl *D, if (!Val) Val = createValue(Ty); + if (Ty->isRecordType()) + return cast(Val)->getAggregateLoc(); + StorageLocation &Loc = D ? createStorageLocation(*D) : createStorageLocation(Ty); @@ -989,7 +991,7 @@ std::vector getFieldsForInitListExpr(const RecordDecl *RD) { StructValue &refreshStructValue(AggregateStorageLocation &Loc, Environment &Env) { - auto &NewVal = *cast(Env.createValue(Loc.getType())); + auto &NewVal = Env.create(Loc); Env.setValue(Loc, NewVal); return NewVal; } @@ -997,19 +999,28 @@ StructValue &refreshStructValue(AggregateStorageLocation &Loc, StructValue &refreshStructValue(const Expr &Expr, Environment &Env) { assert(Expr.getType()->isRecordType()); - auto &NewVal = *cast(Env.createValue(Expr.getType())); - if (Expr.isPRValue()) { - Env.setValueStrict(Expr, NewVal); - } else { - StorageLocation *Loc = Env.getStorageLocationStrict(Expr); - if (Loc == nullptr) { - Loc = &Env.createStorageLocation(Expr); - Env.setStorageLocation(Expr, *Loc); + if (auto *ExistingVal = + cast_or_null(Env.getValueStrict(Expr))) { + auto &NewVal = Env.create(ExistingVal->getAggregateLoc()); + Env.setValueStrict(Expr, NewVal); + return NewVal; } + + auto &NewVal = *cast(Env.createValue(Expr.getType())); + Env.setValueStrict(Expr, NewVal); + return NewVal; + } + + if (auto *Loc = cast_or_null( + Env.getStorageLocationStrict(Expr))) { + auto &NewVal = Env.create(*Loc); Env.setValue(*Loc, NewVal); + return NewVal; } + auto &NewVal = *cast(Env.createValue(Expr.getType())); + Env.setStorageLocationStrict(Expr, NewVal.getAggregateLoc()); return NewVal; } diff --git a/clang/lib/Analysis/FlowSensitive/HTMLLogger.cpp b/clang/lib/Analysis/FlowSensitive/HTMLLogger.cpp index 0c4e838e667be..ee89e074f8467 100644 --- a/clang/lib/Analysis/FlowSensitive/HTMLLogger.cpp +++ b/clang/lib/Analysis/FlowSensitive/HTMLLogger.cpp @@ -108,9 +108,13 @@ class ModelDumper { "pointee", [&] { dump(cast(V).getPointeeLoc()); }); break; case Value::Kind::Struct: - for (const auto &Child : cast(V).children()) - JOS.attributeObject("f:" + Child.first->getNameAsString(), - [&] { dump(*Child.second); }); + for (const auto &Child : + cast(V).getAggregateLoc().children()) + JOS.attributeObject("f:" + Child.first->getNameAsString(), [&] { + if (Child.second) + if (Value *Val = Env.getValue(*Child.second)) + dump(*Val); + }); break; } diff --git a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp index a39aa2240c4fc..1db255dd81f3f 100644 --- a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp +++ b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp @@ -254,21 +254,14 @@ void setHasValue(Value &OptionalVal, BoolValue &HasValueVal) { OptionalVal.setProperty("has_value", HasValueVal); } -/// Creates a symbolic value for an `optional` value using `HasValueVal` as the -/// symbolic value of its "has_value" property. -StructValue &createOptionalValue(BoolValue &HasValueVal, Environment &Env) { - auto &OptionalVal = Env.create(); - setHasValue(OptionalVal, HasValueVal); - return OptionalVal; -} - /// Creates a symbolic value for an `optional` value at an existing storage /// location. Uses `HasValueVal` as the symbolic value of the "has_value" /// property. StructValue &createOptionalValue(AggregateStorageLocation &Loc, BoolValue &HasValueVal, Environment &Env) { - StructValue &OptionalVal = createOptionalValue(HasValueVal, Env); + auto &OptionalVal = Env.create(Loc); Env.setValue(Loc, OptionalVal); + setHasValue(OptionalVal, HasValueVal); return OptionalVal; } @@ -324,37 +317,38 @@ StorageLocation *maybeInitializeOptionalValueMember(QualType Q, if (auto *ValueProp = OptionalVal.getProperty("value")) { auto *ValuePtr = clang::cast(ValueProp); auto &ValueLoc = ValuePtr->getPointeeLoc(); - if (Env.getValue(ValueLoc) == nullptr) { - // The property was previously set, but the value has been lost. This can - // happen in various situations, for example: - // - Because of an environment merge (where the two environments mapped - // the property to different values, which resulted in them both being - // discarded). - // - When two blocks in the CFG, with neither a dominator of the other, - // visit the same optional value. (FIXME: This is something we can and - // should fix -- see also the lengthy FIXME below.) - // - Or even when a block is revisited during testing to collect - // per-statement state. - // - // FIXME: This situation means that the optional contents are not shared - // between branches and the like. Practically, this lack of sharing - // reduces the precision of the model when the contents are relevant to - // the check, like another optional or a boolean that influences control - // flow. + if (Env.getValue(ValueLoc) != nullptr) + return &ValueLoc; + + // The property was previously set, but the value has been lost. This can + // happen in various situations, for example: + // - Because of an environment merge (where the two environments mapped the + // property to different values, which resulted in them both being + // discarded). + // - When two blocks in the CFG, with neither a dominator of the other, + // visit the same optional value. (FIXME: This is something we can and + // should fix -- see also the lengthy FIXME below.) + // - Or even when a block is revisited during testing to collect + // per-statement state. + // FIXME: This situation means that the optional contents are not shared + // between branches and the like. Practically, this lack of sharing + // reduces the precision of the model when the contents are relevant to + // the check, like another optional or a boolean that influences control + // flow. + if (ValueLoc.getType()->isRecordType()) { + refreshStructValue(cast(ValueLoc), Env); + return &ValueLoc; + } else { auto *ValueVal = Env.createValue(ValueLoc.getType()); if (ValueVal == nullptr) return nullptr; Env.setValue(ValueLoc, *ValueVal); + return &ValueLoc; } - return &ValueLoc; } auto Ty = Q.getNonReferenceType(); - auto *ValueVal = Env.createValue(Ty); - if (ValueVal == nullptr) - return nullptr; - auto &ValueLoc = Env.createStorageLocation(Ty); - Env.setValue(ValueLoc, *ValueVal); + auto &ValueLoc = Env.createObject(Ty); auto &ValuePtr = Env.create(ValueLoc); // FIXME: // The change we make to the `value` property below may become visible to @@ -467,10 +461,8 @@ void transferArrowOpCall(const Expr *UnwrapExpr, const Expr *ObjectExpr, void transferMakeOptionalCall(const CallExpr *E, const MatchFinder::MatchResult &, LatticeTransferState &State) { - auto &Loc = State.Env.createStorageLocation(*E); - State.Env.setStorageLocation(*E, Loc); - State.Env.setValue( - Loc, createOptionalValue(State.Env.getBoolLiteralValue(true), State.Env)); + createOptionalValue(State.Env.getResultObjectLocation(*E), + State.Env.getBoolLiteralValue(true), State.Env); } void transferOptionalHasValueCall(const CXXMemberCallExpr *CallExpr, @@ -545,15 +537,21 @@ void transferCallReturningOptional(const CallExpr *E, if (State.Env.getStorageLocation(*E, SkipPast::None) != nullptr) return; - auto &Loc = State.Env.createStorageLocation(*E); - State.Env.setStorageLocation(*E, Loc); - State.Env.setValue( - Loc, createOptionalValue(State.Env.makeAtomicBoolValue(), State.Env)); + AggregateStorageLocation *Loc = nullptr; + if (E->isPRValue()) { + Loc = &State.Env.getResultObjectLocation(*E); + } else { + Loc = &cast(State.Env.createStorageLocation(*E)); + State.Env.setStorageLocationStrict(*E, *Loc); + } + + createOptionalValue(*Loc, State.Env.makeAtomicBoolValue(), State.Env); } void constructOptionalValue(const Expr &E, Environment &Env, BoolValue &HasValueVal) { - Env.setValueStrict(E, createOptionalValue(HasValueVal, Env)); + AggregateStorageLocation &Loc = Env.getResultObjectLocation(E); + Env.setValueStrict(E, createOptionalValue(Loc, HasValueVal, Env)); } /// Returns a symbolic value for the "has_value" property of an `optional` @@ -1032,7 +1030,8 @@ Value *UncheckedOptionalAccessModel::widen(QualType Type, Value &Prev, if (isa(CurrentHasVal)) return &Current; } - return &createOptionalValue(CurrentEnv.makeTopBoolValue(), CurrentEnv); + return &createOptionalValue(cast(Current).getAggregateLoc(), + CurrentEnv.makeTopBoolValue(), CurrentEnv); case ComparisonResult::Unknown: return nullptr; } diff --git a/clang/lib/Analysis/FlowSensitive/RecordOps.cpp b/clang/lib/Analysis/FlowSensitive/RecordOps.cpp index eac9e3d4f93f2..60144531c2518 100644 --- a/clang/lib/Analysis/FlowSensitive/RecordOps.cpp +++ b/clang/lib/Analysis/FlowSensitive/RecordOps.cpp @@ -28,32 +28,33 @@ void clang::dataflow::copyRecord(AggregateStorageLocation &Src, Src.getType().getCanonicalType().getUnqualifiedType()); for (auto [Field, SrcFieldLoc] : Src.children()) { - assert(SrcFieldLoc != nullptr); + StorageLocation *DstFieldLoc = Dst.getChild(*Field); - StorageLocation &DstFieldLoc = Dst.getChild(*Field); + assert(Field->getType()->isReferenceType() || + (SrcFieldLoc != nullptr && DstFieldLoc != nullptr)); if (Field->getType()->isRecordType()) { copyRecord(cast(*SrcFieldLoc), - cast(DstFieldLoc), Env); + cast(*DstFieldLoc), Env); + } else if (Field->getType()->isReferenceType()) { + Dst.setChild(*Field, SrcFieldLoc); } else { if (Value *Val = Env.getValue(*SrcFieldLoc)) - Env.setValue(DstFieldLoc, *Val); + Env.setValue(*DstFieldLoc, *Val); else - Env.clearValue(DstFieldLoc); + Env.clearValue(*DstFieldLoc); } } StructValue *SrcVal = cast_or_null(Env.getValue(Src)); StructValue *DstVal = cast_or_null(Env.getValue(Dst)); - if (SrcVal == nullptr || DstVal == nullptr) - return; - - auto DstChildren = DstVal->children(); - DstVal = &Env.create(llvm::DenseMap( - DstChildren.begin(), DstChildren.end())); + DstVal = &Env.create(Dst); Env.setValue(Dst, *DstVal); + if (SrcVal == nullptr) + return; + for (const auto &[Name, Value] : SrcVal->properties()) { if (Value != nullptr) DstVal->setProperty(Name, *Value); @@ -75,29 +76,20 @@ bool clang::dataflow::recordsEqual(const AggregateStorageLocation &Loc1, Loc1.getType().getCanonicalType().getUnqualifiedType()); for (auto [Field, FieldLoc1] : Loc1.children()) { - assert(FieldLoc1 != nullptr); + StorageLocation *FieldLoc2 = Loc2.getChild(*Field); - StorageLocation &FieldLoc2 = Loc2.getChild(*Field); + assert(Field->getType()->isReferenceType() || + (FieldLoc1 != nullptr && FieldLoc2 != nullptr)); if (Field->getType()->isRecordType()) { if (!recordsEqual(cast(*FieldLoc1), Env1, - cast(FieldLoc2), Env2)) + cast(*FieldLoc2), Env2)) return false; } else if (Field->getType()->isReferenceType()) { - auto *RefVal1 = cast_or_null(Env1.getValue(*FieldLoc1)); - auto *RefVal2 = cast_or_null(Env1.getValue(FieldLoc2)); - if (RefVal1 && RefVal2) { - if (&RefVal1->getReferentLoc() != &RefVal2->getReferentLoc()) - return false; - } else { - // If either of `RefVal1` and `RefVal2` is null, we only consider them - // equal if they're both null. - if (RefVal1 || RefVal2) - return false; - } - } else { - if (Env1.getValue(*FieldLoc1) != Env2.getValue(FieldLoc2)) + if (FieldLoc1 != FieldLoc2) return false; + } else if (Env1.getValue(*FieldLoc1) != Env2.getValue(*FieldLoc2)) { + return false; } } diff --git a/clang/lib/Analysis/FlowSensitive/Transfer.cpp b/clang/lib/Analysis/FlowSensitive/Transfer.cpp index f51b9f34d9863..39faeca4b45ce 100644 --- a/clang/lib/Analysis/FlowSensitive/Transfer.cpp +++ b/clang/lib/Analysis/FlowSensitive/Transfer.cpp @@ -460,29 +460,10 @@ class TransferVisitor : public ConstStmtVisitor { if (BaseLoc == nullptr) return; - auto &MemberLoc = BaseLoc->getChild(*Member); - if (MemberLoc.getType()->isReferenceType()) { - // Based on its type, `MemberLoc` must be mapped either to nothing or to a - // `ReferenceValue`. For the former, we won't set a storage location for - // this expression, so as to maintain an invariant lvalue expressions; - // namely, that their location maps to a `ReferenceValue`. In this, - // lvalues are unlike other expressions, where it is valid for their - // location to map to nothing (because they are not modeled). - // - // Note: we need this invariant for lvalues so that, when accessing a - // value, we can distinguish an rvalue from an lvalue. An alternative - // design, which takes the expression's value category into account, would - // avoid the need for this invariant. - if (auto *V = Env.getValue(MemberLoc)) { - assert(isa(V) && - "reference-typed declarations map to `ReferenceValue`s"); - Env.setStorageLocation(*S, MemberLoc); - } - } else { - auto &Loc = Env.createStorageLocation(*S); - Env.setStorageLocation(*S, Loc); - Env.setValue(Loc, Env.create(MemberLoc)); - } + auto *MemberLoc = BaseLoc->getChild(*Member); + if (MemberLoc == nullptr) + return; + Env.setStorageLocationStrict(*S, *MemberLoc); } void VisitCXXDefaultInitExpr(const CXXDefaultInitExpr *S) { @@ -510,22 +491,18 @@ class TransferVisitor : public ConstStmtVisitor { if (S->isElidable()) { Env.setStorageLocation(*S, *ArgLoc); - } else { - auto &Loc = - cast(Env.createStorageLocation(*S)); - Env.setStorageLocation(*S, Loc); - if (Value *Val = Env.createValue(S->getType())) { - Env.setValue(Loc, *Val); - copyRecord(*ArgLoc, Loc, Env); - } + } else if (auto *ArgVal = cast_or_null( + Env.getValue(*Arg, SkipPast::Reference))) { + auto &Val = *cast(Env.createValue(S->getType())); + Env.setValueStrict(*S, Val); + copyRecord(ArgVal->getAggregateLoc(), Val.getAggregateLoc(), Env); } return; } - auto &Loc = Env.createStorageLocation(*S); - Env.setStorageLocation(*S, Loc); - if (Value *Val = Env.createValue(S->getType())) - Env.setValue(Loc, *Val); + auto &InitialVal = *cast(Env.createValue(S->getType())); + copyRecord(InitialVal.getAggregateLoc(), Env.getResultObjectLocation(*S), + Env); transferInlineCall(S, ConstructorDecl); } @@ -549,21 +526,15 @@ class TransferVisitor : public ConstStmtVisitor { !Method->isMoveAssignmentOperator()) return; - auto *ObjectLoc = cast_or_null( - Env.getStorageLocation(*Arg0, SkipPast::Reference)); - if (ObjectLoc == nullptr) - return; + auto *LocSrc = cast_or_null( + Env.getStorageLocationStrict(*Arg1)); + auto *LocDst = cast_or_null( + Env.getStorageLocationStrict(*Arg0)); - auto *ArgLoc = cast_or_null( - Env.getStorageLocation(*Arg1, SkipPast::Reference)); - if (ArgLoc == nullptr) - return; - - copyRecord(*ArgLoc, *ObjectLoc, Env); - - // FIXME: Add a test for the value of the whole expression. - // Assign a storage location for the whole expression. - Env.setStorageLocation(*S, *ObjectLoc); + if (LocSrc != nullptr && LocDst != nullptr) { + copyRecord(*LocSrc, *LocDst, Env); + Env.setStorageLocationStrict(*S, *LocDst); + } } } @@ -620,9 +591,14 @@ class TransferVisitor : public ConstStmtVisitor { if (SubExprVal == nullptr) return; - auto &Loc = Env.createStorageLocation(*S); - Env.setStorageLocationStrict(*S, Loc); + if (StructValue *StructVal = dyn_cast(SubExprVal)) { + Env.setStorageLocation(*S, StructVal->getAggregateLoc()); + return; + } + + StorageLocation &Loc = Env.createStorageLocation(*S); Env.setValue(Loc, *SubExprVal); + Env.setStorageLocation(*S, Loc); } void VisitCXXBindTemporaryExpr(const CXXBindTemporaryExpr *S) { @@ -645,43 +621,43 @@ class TransferVisitor : public ConstStmtVisitor { // FIXME: Revisit this once flow conditions are added to the framework. For // `a = b ? c : d` we can add `b => a == c && !b => a == d` to the flow // condition. - if (S->isGLValue()) { - auto &Loc = Env.createStorageLocation(*S); - Env.setStorageLocationStrict(*S, Loc); - if (Value *Val = Env.createValue(S->getType())) - Env.setValue(Loc, *Val); - } else if (Value *Val = Env.createValue(S->getType())) { + if (S->isGLValue()) + Env.setStorageLocationStrict(*S, Env.createObject(S->getType())); + else if (Value *Val = Env.createValue(S->getType())) Env.setValueStrict(*S, *Val); - } } void VisitInitListExpr(const InitListExpr *S) { QualType Type = S->getType(); - auto &Loc = Env.createStorageLocation(*S); - Env.setStorageLocation(*S, Loc); + if (!Type->isStructureOrClassType()) { + if (auto *Val = Env.createValue(Type)) + Env.setValueStrict(*S, *Val); - auto *Val = Env.createValue(Type); - if (Val == nullptr) return; + } - Env.setValue(Loc, *Val); - - if (Type->isStructureOrClassType()) { - std::vector Fields = - getFieldsForInitListExpr(Type->getAsRecordDecl()); - for (auto [Field, Init] : llvm::zip(Fields, S->inits())) { - assert(Field != nullptr); - assert(Init != nullptr); - - if (Field->getType()->isReferenceType()) { - if (StorageLocation *Loc = Env.getStorageLocationStrict(*Init)) - cast(Val)->setChild(*Field, - Env.create(*Loc)); - } else if (Value *InitVal = Env.getValue(*Init, SkipPast::None)) - cast(Val)->setChild(*Field, *InitVal); - } + std::vector Fields = + getFieldsForInitListExpr(Type->getAsRecordDecl()); + llvm::DenseMap FieldLocs; + + for (auto [Field, Init] : llvm::zip(Fields, S->inits())) { + assert(Field != nullptr); + assert(Init != nullptr); + + FieldLocs.insert({Field, &Env.createObject(Field->getType(), Init)}); } + + auto &Loc = + Env.getDataflowAnalysisContext() + .arena() + .create(Type, std::move(FieldLocs)); + StructValue &StructVal = Env.create(Loc); + + Env.setValue(Loc, StructVal); + + Env.setValueStrict(*S, StructVal); + // FIXME: Implement array initialization. } diff --git a/clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp b/clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp index 1146a21b05c68..1b68d76ffc8cd 100644 --- a/clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp +++ b/clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp @@ -27,6 +27,7 @@ #include "clang/Analysis/FlowSensitive/DataflowEnvironment.h" #include "clang/Analysis/FlowSensitive/DataflowLattice.h" #include "clang/Analysis/FlowSensitive/DataflowWorklist.h" +#include "clang/Analysis/FlowSensitive/RecordOps.h" #include "clang/Analysis/FlowSensitive/Transfer.h" #include "clang/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.h" #include "clang/Analysis/FlowSensitive/Value.h" @@ -371,34 +372,53 @@ builtinTransferInitializer(const CFGInitializer &Elt, // FIXME: Handle base initialization return; - auto *InitStmt = Init->getInit(); - assert(InitStmt != nullptr); + auto *InitExpr = Init->getInit(); + assert(InitExpr != nullptr); const FieldDecl *Member = nullptr; + AggregateStorageLocation *ParentLoc = &ThisLoc; StorageLocation *MemberLoc = nullptr; if (Init->isMemberInitializer()) { Member = Init->getMember(); - MemberLoc = &ThisLoc.getChild(*Member); + MemberLoc = ThisLoc.getChild(*Member); } else { IndirectFieldDecl *IndirectField = Init->getIndirectMember(); assert(IndirectField != nullptr); MemberLoc = &ThisLoc; for (const auto *I : IndirectField->chain()) { Member = cast(I); - MemberLoc = &cast(MemberLoc)->getChild(*Member); + ParentLoc = cast(MemberLoc); + MemberLoc = ParentLoc->getChild(*Member); } } assert(Member != nullptr); assert(MemberLoc != nullptr); + // FIXME: Instead of these case distinctions, we would ideally want to be able + // to simply use `Environment::createObject()` here, the same way that we do + // this in `TransferVisitor::VisitInitListExpr()`. However, this would require + // us to be able to build a list of fields that we then use to initialize an + // `AggregateStorageLocation` -- and the problem is that, when we get here, + // the `AggregateStorageLocation` already exists. We should explore if there's + // anything that we can do to change this. if (Member->getType()->isReferenceType()) { - auto *InitStmtLoc = Env.getStorageLocationStrict(*InitStmt); - if (InitStmtLoc == nullptr) + auto *InitExprLoc = Env.getStorageLocationStrict(*InitExpr); + if (InitExprLoc == nullptr) return; - Env.setValue(*MemberLoc, Env.create(*InitStmtLoc)); - } else if (auto *InitStmtVal = Env.getValueStrict(*InitStmt)) { - Env.setValue(*MemberLoc, *InitStmtVal); + ParentLoc->setChild(*Member, InitExprLoc); + } else if (auto *InitExprVal = Env.getValueStrict(*InitExpr)) { + 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 + // `StructValue`. + copyRecord(InitValStruct->getAggregateLoc(), + *cast(MemberLoc), Env); + } else { + Env.setValue(*MemberLoc, *InitExprVal); + } } } diff --git a/clang/unittests/Analysis/FlowSensitive/RecordOpsTest.cpp b/clang/unittests/Analysis/FlowSensitive/RecordOpsTest.cpp index dc81e9594b691..2b4b64c74481f 100644 --- a/clang/unittests/Analysis/FlowSensitive/RecordOpsTest.cpp +++ b/clang/unittests/Analysis/FlowSensitive/RecordOpsTest.cpp @@ -60,13 +60,12 @@ TEST(RecordOpsTest, CopyRecord) { auto &S1 = getLocForDecl(ASTCtx, Env, "s1"); auto &S2 = getLocForDecl(ASTCtx, Env, "s2"); - auto &Inner1 = cast(S1.getChild(*InnerDecl)); - auto &Inner2 = cast(S2.getChild(*InnerDecl)); + auto &Inner1 = *cast(S1.getChild(*InnerDecl)); + auto &Inner2 = *cast(S2.getChild(*InnerDecl)); EXPECT_NE(getFieldValue(&S1, *OuterIntDecl, Env), getFieldValue(&S2, *OuterIntDecl, Env)); - EXPECT_NE(Env.getValue(S1.getChild(*RefDecl)), - Env.getValue(S2.getChild(*RefDecl))); + EXPECT_NE(S1.getChild(*RefDecl), S2.getChild(*RefDecl)); EXPECT_NE(getFieldValue(&Inner1, *InnerIntDecl, Env), getFieldValue(&Inner2, *InnerIntDecl, Env)); @@ -80,8 +79,7 @@ TEST(RecordOpsTest, CopyRecord) { EXPECT_EQ(getFieldValue(&S1, *OuterIntDecl, Env), getFieldValue(&S2, *OuterIntDecl, Env)); - EXPECT_EQ(Env.getValue(S1.getChild(*RefDecl)), - Env.getValue(S2.getChild(*RefDecl))); + EXPECT_EQ(S1.getChild(*RefDecl), S2.getChild(*RefDecl)); EXPECT_EQ(getFieldValue(&Inner1, *InnerIntDecl, Env), getFieldValue(&Inner2, *InnerIntDecl, Env)); @@ -122,7 +120,7 @@ TEST(RecordOpsTest, RecordsEqual) { auto &S1 = getLocForDecl(ASTCtx, Env, "s1"); auto &S2 = getLocForDecl(ASTCtx, Env, "s2"); - auto &Inner2 = cast(S2.getChild(*InnerDecl)); + auto &Inner2 = *cast(S2.getChild(*InnerDecl)); cast(Env.getValue(S1)) ->setProperty("prop", Env.getBoolLiteralValue(true)); @@ -139,33 +137,26 @@ TEST(RecordOpsTest, RecordsEqual) { EXPECT_TRUE(recordsEqual(S1, S2, Env)); // S2 has a different outer_int. - Env.setValue(S2.getChild(*OuterIntDecl), Env.create()); + Env.setValue(*S2.getChild(*OuterIntDecl), Env.create()); EXPECT_FALSE(recordsEqual(S1, S2, Env)); copyRecord(S1, S2, Env); EXPECT_TRUE(recordsEqual(S1, S2, Env)); // S2 doesn't have outer_int at all. - Env.clearValue(S2.getChild(*OuterIntDecl)); + Env.clearValue(*S2.getChild(*OuterIntDecl)); EXPECT_FALSE(recordsEqual(S1, S2, Env)); copyRecord(S1, S2, Env); EXPECT_TRUE(recordsEqual(S1, S2, Env)); // S2 has a different ref. - Env.setValue(S2.getChild(*RefDecl), - Env.create(Env.createStorageLocation( - RefDecl->getType().getNonReferenceType()))); - EXPECT_FALSE(recordsEqual(S1, S2, Env)); - copyRecord(S1, S2, Env); - EXPECT_TRUE(recordsEqual(S1, S2, Env)); - - // S2 doesn't have ref at all. - Env.clearValue(S2.getChild(*RefDecl)); + S2.setChild(*RefDecl, &Env.createStorageLocation( + RefDecl->getType().getNonReferenceType())); EXPECT_FALSE(recordsEqual(S1, S2, Env)); copyRecord(S1, S2, Env); EXPECT_TRUE(recordsEqual(S1, S2, Env)); // S2 as a different inner_int. - Env.setValue(Inner2.getChild(*InnerIntDecl), + Env.setValue(*Inner2.getChild(*InnerIntDecl), Env.create()); EXPECT_FALSE(recordsEqual(S1, S2, Env)); copyRecord(S1, S2, Env); diff --git a/clang/unittests/Analysis/FlowSensitive/TestingSupport.h b/clang/unittests/Analysis/FlowSensitive/TestingSupport.h index aa1a0f0e773a9..e6f0b04c43c71 100644 --- a/clang/unittests/Analysis/FlowSensitive/TestingSupport.h +++ b/clang/unittests/Analysis/FlowSensitive/TestingSupport.h @@ -457,7 +457,10 @@ inline Value *getFieldValue(const AggregateStorageLocation *Loc, const ValueDecl &Field, const Environment &Env) { if (Loc == nullptr) return nullptr; - return Env.getValue(Loc->getChild(Field)); + StorageLocation *FieldLoc = Loc->getChild(Field); + if (FieldLoc == nullptr) + return nullptr; + return Env.getValue(*FieldLoc); } /// Returns the value of a `Field` on a `Struct. @@ -470,7 +473,10 @@ inline Value *getFieldValue(const StructValue *Struct, const ValueDecl &Field, const Environment &Env) { if (Struct == nullptr) return nullptr; - return Struct->getChild(Field); + StorageLocation *FieldLoc = Struct->getChild(Field); + if (FieldLoc == nullptr) + return nullptr; + return Env.getValue(*FieldLoc); } /// Creates and owns constraints which are boolean values. diff --git a/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp b/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp index 291f8329a60fd..78f13a044ce64 100644 --- a/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp +++ b/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp @@ -27,6 +27,11 @@ #include #include +// FIXME: There are still remaining checks here that check for consistency +// between `StructValue` and `AggregateStorageLocation`. Now that the redundancy +// between these two classes has been eliminated, these checks aren't needed any +// more, so remove them. + namespace { using namespace clang; @@ -186,7 +191,7 @@ TEST(TransferTest, StructFieldUnmodeled) { void target() { D Bar; - A Foo = Bar.F1.F2.F3; + A &Foo = Bar.F1.F2.F3; int Zab = Foo.Unmodeled.X; // [[p]] } @@ -200,8 +205,9 @@ TEST(TransferTest, StructFieldUnmodeled) { const ValueDecl *FooDecl = findValueDecl(ASTCtx, "Foo"); ASSERT_THAT(FooDecl, NotNull()); - ASSERT_TRUE(FooDecl->getType()->isStructureType()); - auto FooFields = FooDecl->getType()->getAsRecordDecl()->fields(); + QualType FooReferentType = FooDecl->getType()->getPointeeType(); + ASSERT_TRUE(FooReferentType->isStructureType()); + auto FooFields = FooReferentType->getAsRecordDecl()->fields(); FieldDecl *UnmodeledDecl = nullptr; for (FieldDecl *Field : FooFields) { @@ -215,9 +221,9 @@ TEST(TransferTest, StructFieldUnmodeled) { const auto *FooLoc = cast(Env.getStorageLocation(*FooDecl)); - const auto *UnmodeledLoc = &FooLoc->getChild(*UnmodeledDecl); - ASSERT_TRUE(isa(UnmodeledLoc)); - ASSERT_THAT(Env.getValue(*UnmodeledLoc), IsNull()); + const auto *UnmodeledLoc = FooLoc->getChild(*UnmodeledDecl); + ASSERT_TRUE(isa(UnmodeledLoc)); + EXPECT_THAT(Env.getValue(*UnmodeledLoc), IsNull()); const ValueDecl *ZabDecl = findValueDecl(ASTCtx, "Zab"); ASSERT_THAT(ZabDecl, NotNull()); @@ -263,7 +269,7 @@ TEST(TransferTest, StructVarDecl) { const auto *FooLoc = cast(Env.getStorageLocation(*FooDecl)); const auto *BarLoc = - cast(&FooLoc->getChild(*BarDecl)); + cast(FooLoc->getChild(*BarDecl)); const auto *FooVal = cast(Env.getValue(*FooLoc)); const auto *BarVal = @@ -312,7 +318,7 @@ TEST(TransferTest, StructVarDeclWithInit) { const auto *FooLoc = cast(Env.getStorageLocation(*FooDecl)); const auto *BarLoc = - cast(&FooLoc->getChild(*BarDecl)); + cast(FooLoc->getChild(*BarDecl)); const auto *FooVal = cast(Env.getValue(*FooLoc)); const auto *BarVal = @@ -360,7 +366,7 @@ TEST(TransferTest, ClassVarDecl) { const auto *FooLoc = cast(Env.getStorageLocation(*FooDecl)); const auto *BarLoc = - cast(&FooLoc->getChild(*BarDecl)); + cast(FooLoc->getChild(*BarDecl)); const auto *FooVal = cast(Env.getValue(*FooLoc)); const auto *BarVal = @@ -479,34 +485,26 @@ TEST(TransferTest, SelfReferentialReferenceVarDecl) { const auto &FooLoc = *cast(Env.getStorageLocation(*FooDecl)); - const auto &FooReferentVal = *cast(Env.getValue(FooLoc)); - const auto &BarVal = - *cast(FooReferentVal.getChild(*BarDecl)); - const auto &BarReferentVal = - *cast(Env.getValue(BarVal.getReferentLoc())); + const auto &BarLoc = + *cast(FooLoc.getChild(*BarDecl)); - const auto &FooRefVal = - *cast(getFieldValue(&BarReferentVal, *FooRefDecl, Env)); const auto &FooReferentLoc = - cast(FooRefVal.getReferentLoc()); + *cast(BarLoc.getChild(*FooRefDecl)); EXPECT_THAT(Env.getValue(FooReferentLoc), NotNull()); EXPECT_THAT(getFieldValue(&FooReferentLoc, *BarDecl, Env), IsNull()); const auto &FooPtrVal = - *cast(getFieldValue(&BarReferentVal, *FooPtrDecl, Env)); + *cast(getFieldValue(&BarLoc, *FooPtrDecl, Env)); const auto &FooPtrPointeeLoc = cast(FooPtrVal.getPointeeLoc()); EXPECT_THAT(Env.getValue(FooPtrPointeeLoc), NotNull()); EXPECT_THAT(getFieldValue(&FooPtrPointeeLoc, *BarDecl, Env), IsNull()); - const auto &BazRefVal = - *cast(getFieldValue(&BarReferentVal, *BazRefDecl, Env)); - const StorageLocation &BazReferentLoc = BazRefVal.getReferentLoc(); - EXPECT_THAT(Env.getValue(BazReferentLoc), NotNull()); + EXPECT_THAT(getFieldValue(&BarLoc, *BazRefDecl, Env), NotNull()); const auto &BazPtrVal = - *cast(getFieldValue(&BarReferentVal, *BazPtrDecl, Env)); + *cast(getFieldValue(&BarLoc, *BazPtrDecl, Env)); const StorageLocation &BazPtrPointeeLoc = BazPtrVal.getPointeeLoc(); EXPECT_THAT(Env.getValue(BazPtrPointeeLoc), NotNull()); }); @@ -648,10 +646,7 @@ TEST(TransferTest, SelfReferentialPointerVarDecl) { const auto &BarPointeeVal = *cast(Env.getValue(BarVal.getPointeeLoc())); - const auto &FooRefVal = *cast( - getFieldValue(&BarPointeeVal, *FooRefDecl, Env)); - const StorageLocation &FooReferentLoc = FooRefVal.getReferentLoc(); - EXPECT_THAT(Env.getValue(FooReferentLoc), IsNull()); + EXPECT_THAT(getFieldValue(&BarPointeeVal, *FooRefDecl, Env), NotNull()); const auto &FooPtrVal = *cast( getFieldValue(&BarPointeeVal, *FooPtrDecl, Env)); @@ -659,10 +654,7 @@ TEST(TransferTest, SelfReferentialPointerVarDecl) { cast(FooPtrVal.getPointeeLoc()); EXPECT_THAT(Env.getValue(FooPtrPointeeLoc), IsNull()); - const auto &BazRefVal = *cast( - getFieldValue(&BarPointeeVal, *BazRefDecl, Env)); - const StorageLocation &BazReferentLoc = BazRefVal.getReferentLoc(); - EXPECT_THAT(Env.getValue(BazReferentLoc), NotNull()); + EXPECT_THAT(getFieldValue(&BarPointeeVal, *BazRefDecl, Env), NotNull()); const auto &BazPtrVal = *cast( getFieldValue(&BarPointeeVal, *BazPtrDecl, Env)); @@ -689,9 +681,7 @@ TEST(TransferTest, DirectlySelfReferentialReference) { const ValueDecl *SelfDecl = findValueDecl(ASTCtx, "self"); auto *ThisLoc = Env.getThisPointeeStorageLocation(); - auto *RefVal = - cast(Env.getValue(ThisLoc->getChild(*SelfDecl))); - ASSERT_EQ(&RefVal->getReferentLoc(), ThisLoc); + ASSERT_EQ(ThisLoc->getChild(*SelfDecl), ThisLoc); }); } @@ -1057,7 +1047,7 @@ TEST(TransferTest, StructParamDecl) { const auto *FooLoc = cast(Env.getStorageLocation(*FooDecl)); const auto *BarLoc = - cast(&FooLoc->getChild(*BarDecl)); + cast(FooLoc->getChild(*BarDecl)); const auto *FooVal = cast(Env.getValue(*FooLoc)); const auto *BarVal = @@ -1290,36 +1280,8 @@ TEST(TransferTest, DerivedBaseMemberClass) { ASSERT_THAT(APrivateDecl, NotNull()); ASSERT_THAT(APublicDecl, NotNull()); - const auto &FooLoc = - *cast(Env.getStorageLocation(*FooDecl)); - const auto &FooVal = *cast(Env.getValue(FooLoc)); - - // Note: we can't test presence of children in `FooLoc`, because - // `getChild` requires its argument be present (or fails an assert). So, - // we limit to testing presence in `FooVal` and coherence between the - // two. - - // Base-class fields. - EXPECT_THAT(FooVal.getChild(*ADefaultDecl), NotNull()); - EXPECT_THAT(FooVal.getChild(*APrivateDecl), NotNull()); - - EXPECT_THAT(FooVal.getChild(*AProtectedDecl), NotNull()); - EXPECT_EQ(Env.getValue(FooLoc.getChild(*APublicDecl)), - FooVal.getChild(*APublicDecl)); - EXPECT_THAT(FooVal.getChild(*APublicDecl), NotNull()); - EXPECT_EQ(Env.getValue(FooLoc.getChild(*AProtectedDecl)), - FooVal.getChild(*AProtectedDecl)); - - // Derived-class fields. - EXPECT_THAT(FooVal.getChild(*BDefaultDecl), NotNull()); - EXPECT_EQ(Env.getValue(FooLoc.getChild(*BDefaultDecl)), - FooVal.getChild(*BDefaultDecl)); - EXPECT_THAT(FooVal.getChild(*BProtectedDecl), NotNull()); - EXPECT_EQ(Env.getValue(FooLoc.getChild(*BProtectedDecl)), - FooVal.getChild(*BProtectedDecl)); - EXPECT_THAT(FooVal.getChild(*BPrivateDecl), NotNull()); - EXPECT_EQ(Env.getValue(FooLoc.getChild(*BPrivateDecl)), - FooVal.getChild(*BPrivateDecl)); + ASSERT_TRUE( + isa(Env.getStorageLocation(*FooDecl))); }); } @@ -1352,8 +1314,7 @@ static void derivedBaseMemberExpectations( const auto &FooLoc = *cast(Env.getStorageLocation(*FooDecl)); const auto &FooVal = *cast(Env.getValue(FooLoc)); - EXPECT_THAT(FooVal.getChild(*BarDecl), NotNull()); - EXPECT_EQ(Env.getValue(FooLoc.getChild(*BarDecl)), FooVal.getChild(*BarDecl)); + EXPECT_EQ(&FooVal.getAggregateLoc(), &FooLoc); } TEST(TransferTest, DerivedBaseMemberStructDefault) { @@ -1522,10 +1483,8 @@ TEST(TransferTest, ReferenceMember) { const auto *FooLoc = cast(Env.getStorageLocation(*FooDecl)); - const auto *FooVal = cast(Env.getValue(*FooLoc)); - const auto *BarVal = cast(FooVal->getChild(*BarDecl)); const auto *BarReferentVal = - cast(Env.getValue(BarVal->getReferentLoc())); + cast(getFieldValue(FooLoc, *BarDecl, Env)); const ValueDecl *BazDecl = findValueDecl(ASTCtx, "Baz"); ASSERT_THAT(BazDecl, NotNull()); @@ -1566,7 +1525,7 @@ TEST(TransferTest, StructThisMember) { ASSERT_THAT(BarDecl, NotNull()); const auto *BarLoc = - cast(&ThisLoc->getChild(*BarDecl)); + cast(ThisLoc->getChild(*BarDecl)); ASSERT_TRUE(isa_and_nonnull(BarLoc)); const Value *BarVal = Env.getValue(*BarLoc); @@ -1593,12 +1552,12 @@ TEST(TransferTest, StructThisMember) { ASSERT_THAT(BazDecl, NotNull()); const auto *QuxLoc = - cast(&ThisLoc->getChild(*QuxDecl)); + cast(ThisLoc->getChild(*QuxDecl)); const auto *QuxVal = dyn_cast(Env.getValue(*QuxLoc)); ASSERT_THAT(QuxVal, NotNull()); const auto *BazLoc = - cast(&QuxLoc->getChild(*BazDecl)); + cast(QuxLoc->getChild(*BazDecl)); const auto *BazVal = cast(getFieldValue(QuxVal, *BazDecl, Env)); EXPECT_EQ(Env.getValue(*BazLoc), BazVal); @@ -1641,7 +1600,7 @@ TEST(TransferTest, ClassThisMember) { ASSERT_THAT(BarDecl, NotNull()); const auto *BarLoc = - cast(&ThisLoc->getChild(*BarDecl)); + cast(ThisLoc->getChild(*BarDecl)); ASSERT_TRUE(isa_and_nonnull(BarLoc)); const Value *BarVal = Env.getValue(*BarLoc); @@ -1668,12 +1627,12 @@ TEST(TransferTest, ClassThisMember) { ASSERT_THAT(BazDecl, NotNull()); const auto *QuxLoc = - cast(&ThisLoc->getChild(*QuxDecl)); + cast(ThisLoc->getChild(*QuxDecl)); const auto *QuxVal = dyn_cast(Env.getValue(*QuxLoc)); ASSERT_THAT(QuxVal, NotNull()); const auto *BazLoc = - cast(&QuxLoc->getChild(*BazDecl)); + cast(QuxLoc->getChild(*BazDecl)); const auto *BazVal = cast(getFieldValue(QuxVal, *BazDecl, Env)); EXPECT_EQ(Env.getValue(*BazLoc), BazVal); @@ -1713,7 +1672,7 @@ TEST(TransferTest, UnionThisMember) { ASSERT_THAT(FooDecl, NotNull()); const auto *FooLoc = - cast(&ThisLoc->getChild(*FooDecl)); + cast(ThisLoc->getChild(*FooDecl)); ASSERT_TRUE(isa_and_nonnull(FooLoc)); const Value *FooVal = Env.getValue(*FooLoc); @@ -1723,7 +1682,7 @@ TEST(TransferTest, UnionThisMember) { ASSERT_THAT(BarDecl, NotNull()); const auto *BarLoc = - cast(&ThisLoc->getChild(*BarDecl)); + cast(ThisLoc->getChild(*BarDecl)); ASSERT_TRUE(isa_and_nonnull(BarLoc)); const Value *BarVal = Env.getValue(*BarLoc); @@ -1758,7 +1717,7 @@ TEST(TransferTest, StructThisInLambda) { ASSERT_THAT(BarDecl, NotNull()); const auto *BarLoc = - cast(&ThisLoc->getChild(*BarDecl)); + cast(ThisLoc->getChild(*BarDecl)); ASSERT_TRUE(isa_and_nonnull(BarLoc)); const Value *BarVal = Env.getValue(*BarLoc); @@ -1796,7 +1755,7 @@ TEST(TransferTest, StructThisInLambda) { ASSERT_THAT(BarDecl, NotNull()); const auto *BarLoc = - cast(&ThisLoc->getChild(*BarDecl)); + cast(ThisLoc->getChild(*BarDecl)); ASSERT_TRUE(isa_and_nonnull(BarLoc)); const Value *BarVal = Env.getValue(*BarLoc); @@ -1957,7 +1916,7 @@ TEST(TransferTest, TemporaryObject) { const auto *FooLoc = cast(Env.getStorageLocation(*FooDecl)); const auto *BarLoc = - cast(&FooLoc->getChild(*BarDecl)); + cast(FooLoc->getChild(*BarDecl)); const auto *FooVal = cast(Env.getValue(*FooLoc)); const auto *BarVal = @@ -1996,7 +1955,7 @@ TEST(TransferTest, ElidableConstructor) { const auto *FooLoc = cast(Env.getStorageLocation(*FooDecl)); const auto *BarLoc = - cast(&FooLoc->getChild(*BarDecl)); + cast(FooLoc->getChild(*BarDecl)); const auto *FooVal = cast(Env.getValue(*FooLoc)); const auto *BarVal = @@ -2341,12 +2300,12 @@ TEST(TransferTest, MoveConstructor) { const auto *BarLoc1 = cast(Env1.getStorageLocation(*BarDecl)); + EXPECT_FALSE(recordsEqual(*FooLoc1, *BarLoc1, Env1)); + const auto *FooVal1 = cast(Env1.getValue(*FooLoc1)); const auto *BarVal1 = cast(Env1.getValue(*BarLoc1)); EXPECT_NE(FooVal1, BarVal1); - EXPECT_FALSE(recordsEqual(*FooLoc1, *BarLoc1, Env1)); - const auto *FooBazVal1 = cast(getFieldValue(FooLoc1, *BazDecl, Env1)); const auto *BarBazVal1 = @@ -2938,9 +2897,7 @@ TEST(TransferTest, AggregateInitializationReferenceField) { auto &ILoc = getLocForDecl(ASTCtx, Env, "i"); auto &SLoc = getLocForDecl(ASTCtx, Env, "s"); - auto &RefValue = - *cast(getFieldValue(&SLoc, *RefFieldDecl, Env)); - EXPECT_EQ(&RefValue.getReferentLoc(), &ILoc); + EXPECT_EQ(SLoc.getChild(*RefFieldDecl), &ILoc); }); } @@ -5512,8 +5469,8 @@ TEST(TransferTest, NewExpressions_Structs) { auto &OuterLoc = cast(P.getPointeeLoc()); auto &OuterFieldLoc = - cast(OuterLoc.getChild(*OuterField)); - auto &InnerFieldLoc = OuterFieldLoc.getChild(*InnerField); + *cast(OuterLoc.getChild(*OuterField)); + auto &InnerFieldLoc = *OuterFieldLoc.getChild(*InnerField); // Values for the struct and all fields exist after the new. EXPECT_THAT(Env.getValue(OuterLoc), NotNull()); @@ -5620,7 +5577,7 @@ TEST(TransferTest, AnonymousStruct) { auto *S = cast(Env.getStorageLocation(*SDecl)); - auto &AnonStruct = cast( + auto &AnonStruct = *cast( S->getChild(*cast(IndirectField->chain().front()))); auto *B = cast(getFieldValue(&AnonStruct, *BDecl, Env)); @@ -5651,7 +5608,7 @@ TEST(TransferTest, AnonymousStructWithInitializer) { auto *ThisLoc = cast(Env.getThisPointeeStorageLocation()); - auto &AnonStruct = cast(ThisLoc->getChild( + auto &AnonStruct = *cast(ThisLoc->getChild( *cast(IndirectField->chain().front()))); auto *B = cast(getFieldValue(&AnonStruct, *BDecl, Env)); @@ -5684,13 +5641,10 @@ TEST(TransferTest, AnonymousStructWithReferenceField) { auto *ThisLoc = cast(Env.getThisPointeeStorageLocation()); - auto &AnonStruct = cast(ThisLoc->getChild( + auto &AnonStruct = *cast(ThisLoc->getChild( *cast(IndirectField->chain().front()))); - auto *RefVal = - cast(Env.getValue(AnonStruct.getChild(*IDecl))); - - ASSERT_EQ(&RefVal->getReferentLoc(), + ASSERT_EQ(AnonStruct.getChild(*IDecl), Env.getStorageLocation(*GlobalIDecl)); }); }