[clang] [clang][nullability] Remove `RecordValue`. (PR #89052)
via cfe-commits
cfe-commits at lists.llvm.org
Wed Apr 17 04:32:18 PDT 2024
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
More information about the cfe-commits
mailing list