Skip to content

Commit

Permalink
[clang][nullability] Remove RecordValue. (#89052)
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@martinboehme
martinboehme committed Apr 19, 2024
1 parent b2323f4 commit e8fce95
Show file tree
Hide file tree
Showing 13 changed files with 130 additions and 396 deletions.
38 changes: 18 additions & 20 deletions clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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.
Expand All @@ -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.
Expand All @@ -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
Expand Down Expand Up @@ -775,12 +779,6 @@ RecordStorageLocation *getImplicitObjectLocation(const CXXMemberCallExpr &MCE,
RecordStorageLocation *getBaseObjectLocation(const MemberExpr &ME,
const Environment &Env);

/// 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

Expand Down
41 changes: 0 additions & 41 deletions clang/include/clang/Analysis/FlowSensitive/Value.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,6 @@ class Value {
enum class Kind {
Integer,
Pointer,
Record,

// TODO: Top values should not be need to be type-specific.
TopBool,
Expand Down Expand Up @@ -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);
}

Expand Down Expand Up @@ -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
Expand Down
98 changes: 26 additions & 72 deletions clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,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>
Expand Down Expand Up @@ -80,7 +81,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;
Expand Down Expand Up @@ -145,25 +145,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);

Expand Down Expand Up @@ -565,7 +547,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).
Expand Down Expand Up @@ -709,10 +690,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,
Expand Down Expand Up @@ -936,24 +913,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);
}

Expand All @@ -965,6 +941,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;
Expand Down Expand Up @@ -993,6 +972,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) ||
Expand Down Expand Up @@ -1021,15 +1001,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;
}

Expand All @@ -1039,20 +1010,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;
}

Expand All @@ -1064,10 +1038,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(
Expand Down Expand Up @@ -1125,7 +1100,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)
Expand Down Expand Up @@ -1264,25 +1238,5 @@ RecordStorageLocation *getBaseObjectLocation(const MemberExpr &ME,
return Env.get<RecordStorageLocation>(*Base);
}

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
2 changes: 0 additions & 2 deletions clang/lib/Analysis/FlowSensitive/DebugSupport.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down

0 comments on commit e8fce95

Please sign in to comment.