llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clang-analysis @llvm/pr-subscribers-clang Author: None (martinboehme) <details> <summary>Changes</summary> This class no longer serves any purpose; see also the discussion here: https://reviews.llvm.org/D155204#inline-1503204 A lot of existing tests in TransferTest.cpp check for the existence of `RecordValue`s. Some of these checks are now simply redundant and have been removed. In other cases, tests were checking for the existence of a `RecordValue` as a way of testing whether a record has been initialized. I have typically changed these test to instead check whether a field of the record has a value. --- Patch is 46.70 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/89052.diff 13 Files Affected: - (modified) clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h (+18-20) - (modified) clang/include/clang/Analysis/FlowSensitive/Value.h (-41) - (modified) clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp (+26-72) - (modified) clang/lib/Analysis/FlowSensitive/DebugSupport.cpp (-2) - (modified) clang/lib/Analysis/FlowSensitive/HTMLLogger.cpp (+7-6) - (modified) clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp (+15-31) - (modified) clang/lib/Analysis/FlowSensitive/RecordOps.cpp (-3) - (modified) clang/lib/Analysis/FlowSensitive/Transfer.cpp (+21-41) - (modified) clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp (+3-3) - (modified) clang/lib/Analysis/FlowSensitive/Value.cpp (-2) - (modified) clang/unittests/Analysis/FlowSensitive/DataflowEnvironmentTest.cpp (-81) - (modified) clang/unittests/Analysis/FlowSensitive/RecordOpsTest.cpp (-8) - (modified) clang/unittests/Analysis/FlowSensitive/TransferTest.cpp (+40-86) ``````````diff diff --git a/clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h b/clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h index 706664d7db1c25..c20e8f34d48d08 100644 --- a/clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h +++ b/clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h @@ -84,7 +84,7 @@ class Environment { virtual ComparisonResult compare(QualType Type, const Value &Val1, const Environment &Env1, const Value &Val2, const Environment &Env2) { - // FIXME: Consider adding QualType to RecordValue and removing the Type + // FIXME: Consider adding `QualType` to `Value` and removing the `Type` // argument here. return ComparisonResult::Unknown; } @@ -407,20 +407,15 @@ class Environment { /// storage locations and values for indirections until it finds a /// non-pointer/non-reference type. /// - /// If `Type` is a class, struct, or union type, creates values for all - /// modeled fields (including synthetic fields) and calls `setValue()` to - /// associate the `RecordValue` with its storage location - /// (`RecordValue::getLoc()`). - /// /// If `Type` is one of the following types, this function will always return /// a non-null pointer: /// - `bool` /// - Any integer type - /// - Any class, struct, or union type /// /// Requirements: /// - /// `Type` must not be null. + /// - `Type` must not be null. + /// - `Type` must not be a reference type or record type. Value *createValue(QualType Type); /// Creates an object (i.e. a storage location with an associated value) of @@ -452,6 +447,7 @@ class Environment { /// Initializes the fields (including synthetic fields) of `Loc` with values, /// unless values of the field type are not supported or we hit one of the /// limits at which we stop producing values. + /// If a field already has a value, that value is preserved. /// 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. @@ -461,6 +457,10 @@ class Environment { } /// Assigns `Val` as the value of `Loc` in the environment. + /// + /// Requirements: + /// + /// `Loc` must not be a `RecordStorageLocation`. void setValue(const StorageLocation &Loc, Value &Val); /// Clears any association between `Loc` and a value in the environment. @@ -470,20 +470,24 @@ class Environment { /// /// Requirements: /// - /// - `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. + /// - `E` must be a prvalue. + /// - `E` must not have record type. void setValue(const Expr &E, Value &Val); /// Returns the value assigned to `Loc` in the environment or null if `Loc` /// isn't assigned a value in the environment. + /// + /// Requirements: + /// + /// `Loc` must not be a `RecordStorageLocation`. Value *getValue(const StorageLocation &Loc) const; /// Equivalent to `getValue(getStorageLocation(D))` if `D` is assigned a /// storage location in the environment, otherwise returns null. + /// + /// Requirements: + /// + /// `D` must not have record type. Value *getValue(const ValueDecl &D) const; /// Equivalent to `getValue(getStorageLocation(E, SP))` if `E` is assigned a @@ -811,12 +815,6 @@ class RecordInitListHelper { std::optional<ImplicitValueInitExpr> ImplicitValueInitForUnion; }; -/// Associates a new `RecordValue` with `Loc` and returns the new value. -RecordValue &refreshRecordValue(RecordStorageLocation &Loc, Environment &Env); - -/// Associates a new `RecordValue` with `Expr` and returns the new value. -RecordValue &refreshRecordValue(const Expr &Expr, Environment &Env); - } // namespace dataflow } // namespace clang diff --git a/clang/include/clang/Analysis/FlowSensitive/Value.h b/clang/include/clang/Analysis/FlowSensitive/Value.h index be1bf9324c87b4..97efa3a93ce6d9 100644 --- a/clang/include/clang/Analysis/FlowSensitive/Value.h +++ b/clang/include/clang/Analysis/FlowSensitive/Value.h @@ -35,7 +35,6 @@ class Value { enum class Kind { Integer, Pointer, - Record, // TODO: Top values should not be need to be type-specific. TopBool, @@ -67,7 +66,6 @@ class Value { /// Properties may not be set on `RecordValue`s; use synthetic fields instead /// (for details, see documentation for `RecordStorageLocation`). void setProperty(llvm::StringRef Name, Value &Val) { - assert(getKind() != Kind::Record); Properties.insert_or_assign(Name, &Val); } @@ -184,45 +182,6 @@ class PointerValue final : public Value { StorageLocation &PointeeLoc; }; -/// 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, `RecordValue` also serves only a limited purpose: 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. `RecordValue` 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 -/// `RecordValue` that wraps a `RecordStorageLocation`. This -/// `RecordStorageLocation` is then used as the storage location for `S`. -/// -/// Over time, we may eliminate `RecordValue` entirely. See also the discussion -/// here: https://reviews.llvm.org/D155204#inline-1503204 -class RecordValue final : public Value { -public: - explicit RecordValue(RecordStorageLocation &Loc) - : Value(Kind::Record), Loc(Loc) {} - - static bool classof(const Value *Val) { - return Val->getKind() == Kind::Record; - } - - /// Returns the storage location that this `RecordValue` is associated with. - RecordStorageLocation &getLoc() const { return Loc; } - -private: - RecordStorageLocation &Loc; -}; - raw_ostream &operator<<(raw_ostream &OS, const Value &Val); } // namespace dataflow diff --git a/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp b/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp index bea15ce9bd24d1..28f4e76f9a75dc 100644 --- a/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp +++ b/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp @@ -23,6 +23,7 @@ #include "llvm/ADT/DenseSet.h" #include "llvm/ADT/MapVector.h" #include "llvm/ADT/STLExtras.h" +#include "llvm/ADT/ScopeExit.h" #include "llvm/Support/ErrorHandling.h" #include <cassert> #include <utility> @@ -79,7 +80,6 @@ static bool equateUnknownValues(Value::Kind K) { switch (K) { case Value::Kind::Integer: case Value::Kind::Pointer: - case Value::Kind::Record: return true; default: return false; @@ -144,25 +144,7 @@ static Value *joinDistinctValues(QualType Type, Value &Val1, return &A.makeBoolValue(JoinedVal); } - Value *JoinedVal = nullptr; - if (auto *RecordVal1 = dyn_cast<RecordValue>(&Val1)) { - auto *RecordVal2 = cast<RecordValue>(&Val2); - - if (&RecordVal1->getLoc() == &RecordVal2->getLoc()) - // `RecordVal1` and `RecordVal2` may have different properties associated - // with them. Create a new `RecordValue` with the same location but - // without any properties so that we soundly approximate both values. If a - // particular analysis needs to join properties, it should do so in - // `DataflowAnalysis::join()`. - JoinedVal = &JoinedEnv.create<RecordValue>(RecordVal1->getLoc()); - else - // If the locations for the two records are different, need to create a - // completely new value. - JoinedVal = JoinedEnv.createValue(Type); - } else { - JoinedVal = JoinedEnv.createValue(Type); - } - + Value *JoinedVal = JoinedEnv.createValue(Type); if (JoinedVal) Model.join(Type, Val1, Env1, Val2, Env2, *JoinedVal, JoinedEnv); @@ -627,7 +609,6 @@ void Environment::initialize() { auto &ThisLoc = cast<RecordStorageLocation>(createStorageLocation(ThisPointeeType)); setThisPointeeStorageLocation(ThisLoc); - refreshRecordValue(ThisLoc, *this); // Initialize fields of `*this` with values, but only if we're not // analyzing a constructor; after all, it's the constructor's job to do // this (and we want to be able to test that). @@ -794,10 +775,6 @@ void Environment::popCall(const CXXConstructExpr *Call, // See also comment in `popCall(const CallExpr *, const Environment &)` above. this->LocToVal = std::move(CalleeEnv.LocToVal); this->FlowConditionToken = std::move(CalleeEnv.FlowConditionToken); - - if (Value *Val = CalleeEnv.getValue(*CalleeEnv.ThisPointeeLoc)) { - setValue(*Call, *Val); - } } bool Environment::equivalentTo(const Environment &Other, @@ -1021,24 +998,23 @@ void Environment::initializeFieldsWithValues(RecordStorageLocation &Loc, } void Environment::setValue(const StorageLocation &Loc, Value &Val) { - assert(!isa<RecordValue>(&Val) || &cast<RecordValue>(&Val)->getLoc() == &Loc); - + // Records should not be associated with values. + assert(!isa<RecordStorageLocation>(Loc)); LocToVal[&Loc] = &Val; } void Environment::setValue(const Expr &E, Value &Val) { const Expr &CanonE = ignoreCFGOmittedNodes(E); - if (auto *RecordVal = dyn_cast<RecordValue>(&Val)) { - assert(&RecordVal->getLoc() == &getResultObjectLocation(CanonE)); - (void)RecordVal; - } - assert(CanonE.isPRValue()); + // Records should not be associated with values. + assert(!CanonE.getType()->isRecordType()); ExprToVal[&CanonE] = &Val; } Value *Environment::getValue(const StorageLocation &Loc) const { + // Records should not be associated with values. + assert(!isa<RecordStorageLocation>(Loc)); return LocToVal.lookup(&Loc); } @@ -1050,6 +1026,9 @@ Value *Environment::getValue(const ValueDecl &D) const { } Value *Environment::getValue(const Expr &E) const { + // Records should not be associated with values. + assert(!E.getType()->isRecordType()); + if (E.isPRValue()) { auto It = ExprToVal.find(&ignoreCFGOmittedNodes(E)); return It == ExprToVal.end() ? nullptr : It->second; @@ -1078,6 +1057,7 @@ Value *Environment::createValueUnlessSelfReferential( int &CreatedValuesCount) { assert(!Type.isNull()); assert(!Type->isReferenceType()); + assert(!Type->isRecordType()); // Allow unlimited fields at depth 1; only cap at deeper nesting levels. if ((Depth > 1 && CreatedValuesCount > MaxCompositeValueSize) || @@ -1106,15 +1086,6 @@ Value *Environment::createValueUnlessSelfReferential( return &arena().create<PointerValue>(PointeeLoc); } - if (Type->isRecordType()) { - CreatedValuesCount++; - auto &Loc = cast<RecordStorageLocation>(createStorageLocation(Type)); - initializeFieldsWithValues(Loc, Loc.getType(), Visited, Depth, - CreatedValuesCount); - - return &refreshRecordValue(Loc, *this); - } - return nullptr; } @@ -1124,20 +1095,23 @@ Environment::createLocAndMaybeValue(QualType Ty, 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()); + auto EraseVisited = llvm::make_scope_exit( + [&Visited, Ty] { Visited.erase(Ty.getCanonicalType()); }); Ty = Ty.getNonReferenceType(); - if (Val == nullptr) - return createStorageLocation(Ty); - - if (Ty->isRecordType()) - return cast<RecordValue>(Val)->getLoc(); + if (Ty->isRecordType()) { + auto &Loc = cast<RecordStorageLocation>(createStorageLocation(Ty)); + initializeFieldsWithValues(Loc, Ty, Visited, Depth, CreatedValuesCount); + return Loc; + } StorageLocation &Loc = createStorageLocation(Ty); - setValue(Loc, *Val); + + if (Value *Val = createValueUnlessSelfReferential(Ty, Visited, Depth, + CreatedValuesCount)) + setValue(Loc, *Val); + return Loc; } @@ -1149,10 +1123,11 @@ void Environment::initializeFieldsWithValues(RecordStorageLocation &Loc, auto initField = [&](QualType FieldType, StorageLocation &FieldLoc) { if (FieldType->isRecordType()) { auto &FieldRecordLoc = cast<RecordStorageLocation>(FieldLoc); - setValue(FieldRecordLoc, create<RecordValue>(FieldRecordLoc)); initializeFieldsWithValues(FieldRecordLoc, FieldRecordLoc.getType(), Visited, Depth + 1, CreatedValuesCount); } else { + if (getValue(FieldLoc) != nullptr) + return; if (!Visited.insert(FieldType.getCanonicalType()).second) return; if (Value *Val = createValueUnlessSelfReferential( @@ -1210,7 +1185,6 @@ StorageLocation &Environment::createObjectInternal(const ValueDecl *D, auto &RecordLoc = cast<RecordStorageLocation>(Loc); if (!InitExpr) initializeFieldsWithValues(RecordLoc); - refreshRecordValue(RecordLoc, *this); } else { Value *Val = nullptr; if (InitExpr) @@ -1407,25 +1381,5 @@ RecordInitListHelper::RecordInitListHelper(const InitListExpr *InitList) { } } -RecordValue &refreshRecordValue(RecordStorageLocation &Loc, Environment &Env) { - auto &NewVal = Env.create<RecordValue>(Loc); - Env.setValue(Loc, NewVal); - return NewVal; -} - -RecordValue &refreshRecordValue(const Expr &Expr, Environment &Env) { - assert(Expr.getType()->isRecordType()); - - if (Expr.isPRValue()) - refreshRecordValue(Env.getResultObjectLocation(Expr), Env); - - if (auto *Loc = Env.get<RecordStorageLocation>(Expr)) - refreshRecordValue(*Loc, Env); - - auto &NewVal = *cast<RecordValue>(Env.createValue(Expr.getType())); - Env.setStorageLocation(Expr, NewVal.getLoc()); - return NewVal; -} - } // namespace dataflow } // namespace clang diff --git a/clang/lib/Analysis/FlowSensitive/DebugSupport.cpp b/clang/lib/Analysis/FlowSensitive/DebugSupport.cpp index 573c4b1d474bf4..d40aab7a7f1035 100644 --- a/clang/lib/Analysis/FlowSensitive/DebugSupport.cpp +++ b/clang/lib/Analysis/FlowSensitive/DebugSupport.cpp @@ -28,8 +28,6 @@ llvm::StringRef debugString(Value::Kind Kind) { return "Integer"; case Value::Kind::Pointer: return "Pointer"; - case Value::Kind::Record: - return "Record"; case Value::Kind::AtomicBool: return "AtomicBool"; case Value::Kind::TopBool: diff --git a/clang/lib/Analysis/FlowSensitive/HTMLLogger.cpp b/clang/lib/Analysis/FlowSensitive/HTMLLogger.cpp index 397a8d87e114d7..a36cb41a63dfb1 100644 --- a/clang/lib/Analysis/FlowSensitive/HTMLLogger.cpp +++ b/clang/lib/Analysis/FlowSensitive/HTMLLogger.cpp @@ -95,7 +95,6 @@ class ModelDumper { switch (V.getKind()) { case Value::Kind::Integer: - case Value::Kind::Record: case Value::Kind::TopBool: case Value::Kind::AtomicBool: case Value::Kind::FormulaBool: @@ -126,8 +125,9 @@ class ModelDumper { return; JOS.attribute("type", L.getType().getAsString()); - if (auto *V = Env.getValue(L)) - dump(*V); + if (!L.getType()->isRecordType()) + if (auto *V = Env.getValue(L)) + dump(*V); if (auto *RLoc = dyn_cast<RecordStorageLocation>(&L)) { for (const auto &Child : RLoc->children()) @@ -281,9 +281,10 @@ class HTMLLogger : public Logger { Iters.back().Block->Elements[ElementIndex - 1].getAs<CFGStmt>(); if (const Expr *E = S ? llvm::dyn_cast<Expr>(S->getStmt()) : nullptr) { if (E->isPRValue()) { - if (auto *V = State.Env.getValue(*E)) - JOS.attributeObject( - "value", [&] { ModelDumper(JOS, State.Env).dump(*V); }); + if (!E->getType()->isRecordType()) + if (auto *V = State.Env.getValue(*E)) + JOS.attributeObject( + "value", [&] { ModelDumper(JOS, State.Env).dump(*V); }); } else { if (auto *Loc = State.Env.getStorageLocation(*E)) JOS.attributeObject( diff --git a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp index cadb1ceb2d8507..0707aa662e4cc2 100644 --- a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp +++ b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp @@ -339,17 +339,6 @@ void setHasValue(RecordStorageLocation &OptionalLoc, BoolValue &HasValueVal, Env.setValue(locForHasValue(OptionalLoc), HasValueVal); } -/// Creates a symbolic value for an `optional` value at an existing storage -/// location. Uses `HasValueVal` as the symbolic value of the "has_value" -/// property. -RecordValue &createOptionalValue(RecordStorageLocation &Loc, - BoolValue &HasValueVal, Environment &Env) { - auto &OptionalVal = Env.create<RecordValue>(Loc); - Env.setValue(Loc, OptionalVal); - setHasValue(Loc, HasValueVal, Env); - return OptionalVal; -} - /// Returns the symbolic value that represents the "has_value" property of the /// optional at `OptionalLoc`. Returns null if `OptionalLoc` is null. BoolValue *getHasValue(Environment &Env, RecordStorageLocation *OptionalLoc) { @@ -413,9 +402,8 @@ void transferArrowOpCall(const Expr *UnwrapExpr, const Expr *ObjectExpr, void transferMakeOptionalCall(const CallExpr *E, const MatchFinder::MatchResult &, LatticeTransferState &State) { - State.Env.setValue( - *E, createOptionalValue(State.Env.getResultObjectLocation(*E), - State.Env.getBoolLiteralValue(true), State.Env)); + setHasValue(State.Env.getResultObjectLocation(*E), + State.Env.getBoolLiteralValue(true), State.Env); } void transferOptionalHasValueCall(const CXXMemberCallExpr *CallExpr, @@ -483,9 +471,6 @@ void transferValueOrNotEqX(const Expr *ComparisonExpr, void transferCallReturningOptional(const CallExpr *E, const MatchFinder::MatchResult &Result, LatticeTransferState &State) { - if (State.Env.getValue(*E) != nullptr) - return; - RecordStorageLocation *Loc = nullptr; if (E->isPRValue()) { Loc = &State.Env.getResultObjectLocation(*E); @@ -497,16 +482,16 @@ void transferCallReturningOptional(const CallExpr *E, } } - RecordValue &Val = - createOptionalValue(*Loc, State.Env.makeAtomicBoolValue(), State.Env); - if (E->isPRValue()) - State.Env.setValue(*E, Val); + if (State.Env.getValue(locForHasValue(*Loc)) != nullptr) + return; + + setHasValue(*Loc, State.Env.makeAtomicBoolValue(), State.Env); } void constructOptionalValue(const Expr &E, Environment &Env, BoolValue &HasValueVal) { RecordStorageLocation &Loc = Env.getResultObjectLocation(E); - Env.setValue(E, createOptionalValue(Loc, HasValueVal, Env)); + setHasValue(Loc, HasValueVal, Env); } /// Returns a symbolic value for the "has_value" property of an `optional<T>` @@ -555,7 +540,7 @@ void transferAssignment(const CXXOperatorCallExpr *E, BoolValue &HasValueVal, assert(E->getNumArgs() > 0); if (auto *Loc = State.Env.get<RecordStorageLocation>(*E->getArg(0))) { - createOptionalValue(*Loc, HasValueVal, State.Env); + setHasValue(*Loc, HasValueVal, State.Env); // Assi... [truncated] `````````` </details> https://github.com/llvm/llvm-project/pull/89052 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
