[clang][nullability] Remove RecordValue.
#89052
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
llvmbot
added
clang
|
@llvm/pr-subscribers-clang-analysis @llvm/pr-subscribers-clang Author: None (martinboehme) ChangesThis class no longer serves any purpose; see also the discussion here: A lot of existing tests in TransferTest.cpp check for the existence of 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:
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]
|
Xazax-hun
approved these changes
Apr 18, 2024
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.
martinboehme
force-pushed
the
piper_export_cl_608559453
branch
from
f5df084 to
a705853
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
RecordValues. Some of these checks are now simply redundant and have beenremoved. In other cases, tests were checking for the existence of a
RecordValueas a way of testing whether a record has been initialized. I havetypically changed these test to instead check whether a field of the record has
a value.