[clang] 080ee85 - [clang][dataflow] Add `Strict` versions of `Value` and `StorageLocation` accessors.
Martin Braenne via cfe-commits
cfe-commits at lists.llvm.org
Wed May 17 02:30:57 PDT 2023
Author: Martin Braenne
Date: 2023-05-17T09:30:47Z
New Revision: 080ee850c639be96b8bea8008088f2b036ff0f15
URL: https://github.com/llvm/llvm-project/commit/080ee850c639be96b8bea8008088f2b036ff0f15
DIFF: https://github.com/llvm/llvm-project/commit/080ee850c639be96b8bea8008088f2b036ff0f15.diff
LOG: [clang][dataflow] Add `Strict` versions of `Value` and `StorageLocation` accessors.
This is part of the gradual migration to strict handling of value categories, as described in the RFC at https://discourse.llvm.org/t/70086.
This patch migrates some representative calls of the newly deprecated accessors to the new `Strict` functions. Followup patches will migrate the remaining callers. (There are a large number of callers, with some subtlety involved in some of them, so it makes sense to split this up into multiple patches rather than migrating all callers in one go.)
The `Strict` accessors as implemented here have some differences in semantics compared to the semantics originally proposed in the RFC; specifically:
* `setStorageLocationStrict()`: The RFC proposes to create an intermediate
`ReferenceValue` that then refers to the `StorageLocation` for the glvalue.
It turns out though that, even today, most places in the code are not doing
this but are instead associating glvalues directly with their
`StorageLocation`. It therefore didn't seem to make sense to introduce new
`ReferenceValue`s where there were none previously, so I have chosen to
instead make `setStorageLocationStrict()` simply call through to
`setStorageLocation(const Expr &, StorageLocation &)` and merely add the
assertion that the expression must be a glvalue.
* `getStorageLocationStrict()`: The RFC proposes that this should assert that
the storage location for the glvalue expression is associated with an
intermediate `ReferenceValue`, but, as explained, this is often not true.
The current state is inconsistent: Sometimes the intermediate
`ReferenceValue` is there, sometimes it isn't. For this reason,
`getStorageLocationStrict()` skips past a `ReferenceValue` if it is there but
otherwise directly returns the storage location associated with the
expression. This behavior is equivalent to the existing behavior of
`SkipPast::Reference`.
* `setValueStrict()`: The RFC proposes that this should always create the same
`StorageLocation` for a given `Value`, but, in fact, the transfer functions
that exist today don't guarantee this; almost all transfer functions
unconditionally create a new `StorageLocation` when associating an expression
with a `Value`.
There appears to be one special case:
`TerminatorVisitor::extendFlowCondition()` checks whether the expression is
already associated with a `StorageLocation` and, if so, reuses the existing
`StorageLocation` instead of creating a new one.
For this reason, `setValueStrict()` implements this logic (preserve an
existing `StorageLocation`) but makes no attempt to always associate the same
`StorageLocation` with a given `Value`, as nothing in the framework appers to
require this.
As `TerminatorVisitor::extendFlowCondition()` is an interesting special case,
the `setValue()` call there is among the ones that this patch migrates to
`setValueStrict()`.
Reviewed By: sammccall, ymandel, xazax.hun
Differential Revision: https://reviews.llvm.org/D150653
Added:
Modified:
clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h
clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
clang/lib/Analysis/FlowSensitive/Transfer.cpp
clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp
Removed:
################################################################################
diff --git a/clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h b/clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h
index d734ae5c66fe8..6f2f7f4004f84 100644
--- a/clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h
+++ b/clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h
@@ -270,16 +270,54 @@ class Environment {
/// Assigns `Loc` as the storage location of `E` in the environment.
///
+ /// This function is deprecated; prefer `setStorageLocationStrict()`.
+ /// For details, see https://discourse.llvm.org/t/70086.
+ ///
/// Requirements:
///
/// `E` must not be assigned a storage location in the environment.
void setStorageLocation(const Expr &E, StorageLocation &Loc);
+ /// Assigns `Loc` as the storage location of the glvalue `E` in the
+ /// environment.
+ ///
+ /// This function is the preferred alternative to
+ /// `setStorageLocation(const Expr &, StorageLocation &)`. Once the migration
+ /// to strict handling of value categories is complete (see
+ /// https://discourse.llvm.org/t/70086), `setStorageLocation()` will be
+ /// removed and this function will be renamed to `setStorageLocation()`.
+ ///
+ /// Requirements:
+ ///
+ /// `E` must not be assigned a storage location in the environment.
+ /// `E` must be a glvalue or a `BuiltinType::BuiltinFn`
+ void setStorageLocationStrict(const Expr &E, StorageLocation &Loc);
+
/// Returns the storage location assigned to `E` in the environment, applying
/// the `SP` policy for skipping past indirections, or null if `E` isn't
/// assigned a storage location in the environment.
+ ///
+ /// This function is deprecated; prefer `getStorageLocationStrict()`.
+ /// For details, see https://discourse.llvm.org/t/70086.
StorageLocation *getStorageLocation(const Expr &E, SkipPast SP) const;
+ /// Returns the storage location assigned to the glvalue `E` in the
+ /// environment, or null if `E` isn't assigned a storage location in the
+ /// environment.
+ ///
+ /// If the storage location for `E` is associated with a
+ /// `ReferenceValue RefVal`, returns `RefVal.getReferentLoc()` instead.
+ ///
+ /// This function is the preferred alternative to
+ /// `getStorageLocation(const Expr &, SkipPast)`. Once the migration
+ /// to strict handling of value categories is complete (see
+ /// https://discourse.llvm.org/t/70086), `getStorageLocation()` will be
+ /// removed and this function will be renamed to `getStorageLocation()`.
+ ///
+ /// Requirements:
+ /// `E` must be a glvalue or a `BuiltinType::BuiltinFn`
+ StorageLocation *getStorageLocationStrict(const Expr &E) const;
+
/// Returns the storage location assigned to the `this` pointee in the
/// environment or null if the `this` pointee has no assigned storage location
/// in the environment.
@@ -305,6 +343,25 @@ class Environment {
/// Assigns `Val` as the value of `Loc` in the environment.
void setValue(const StorageLocation &Loc, Value &Val);
+ /// Assigns `Val` as the value of the prvalue `E` in the environment.
+ ///
+ /// If `E` is not yet associated with a storage location, associates it with
+ /// a newly created storage location. In any case, associates the storage
+ /// location of `E` with `Val`.
+ ///
+ /// Once the migration to strict handling of value categories is complete
+ /// (see https://discourse.llvm.org/t/70086), this function will be renamed to
+ /// `setValue()`. At this point, prvalue expressions will be associated
+ /// directly with `Value`s, and the legacy behavior of associating prvalue
+ /// expressions with storage locations (as described above) will be
+ /// eliminated.
+ ///
+ /// Requirements:
+ ///
+ /// `E` must be a prvalue
+ /// `Val` must not be a `ReferenceValue`
+ void setValueStrict(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.
Value *getValue(const StorageLocation &Loc) const;
@@ -315,8 +372,25 @@ class Environment {
/// Equivalent to `getValue(getStorageLocation(E, SP), SkipPast::None)` if `E`
/// is assigned a storage location in the environment, otherwise returns null.
+ ///
+ /// This function is deprecated; prefer `getValueStrict()`. For details, see
+ /// https://discourse.llvm.org/t/70086.
Value *getValue(const Expr &E, SkipPast SP) const;
+ /// Returns the `Value` assigned to the prvalue `E` in the environment, or
+ /// null if `E` isn't assigned a value in the environment.
+ ///
+ /// This function is the preferred alternative to
+ /// `getValue(const Expr &, SkipPast)`. Once the migration to strict handling
+ /// of value categories is complete (see https://discourse.llvm.org/t/70086),
+ /// `getValue()` will be removed and this function will be renamed to
+ /// `getValue()`.
+ ///
+ /// Requirements:
+ ///
+ /// `E` must be a prvalue
+ Value *getValueStrict(const Expr &E) const;
+
// FIXME: should we deprecate the following & call arena().create() directly?
/// Creates a `T` (some subclass of `Value`), forwarding `args` to the
diff --git a/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp b/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
index 7b1944f273cf0..aaa2f6f19eecb 100644
--- a/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
+++ b/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
@@ -623,6 +623,16 @@ void Environment::setStorageLocation(const Expr &E, StorageLocation &Loc) {
ExprToLoc[&CanonE] = &Loc;
}
+void Environment::setStorageLocationStrict(const Expr &E,
+ StorageLocation &Loc) {
+ // `DeclRefExpr`s to builtin function types aren't glvalues, for some reason,
+ // but we still want to be able to associate a `StorageLocation` with them,
+ // so allow these as an exception.
+ assert(E.isGLValue() ||
+ E.getType()->isSpecificBuiltinType(BuiltinType::BuiltinFn));
+ setStorageLocation(E, Loc);
+}
+
StorageLocation *Environment::getStorageLocation(const Expr &E,
SkipPast SP) const {
// FIXME: Add a test with parens.
@@ -630,6 +640,21 @@ StorageLocation *Environment::getStorageLocation(const Expr &E,
return It == ExprToLoc.end() ? nullptr : &skip(*It->second, SP);
}
+StorageLocation *Environment::getStorageLocationStrict(const Expr &E) const {
+ // See comment in `setStorageLocationStrict()`.
+ assert(E.isGLValue() ||
+ E.getType()->isSpecificBuiltinType(BuiltinType::BuiltinFn));
+ StorageLocation *Loc = getStorageLocation(E, SkipPast::None);
+
+ if (Loc == nullptr)
+ return nullptr;
+
+ if (auto *RefVal = dyn_cast_or_null<ReferenceValue>(getValue(*Loc)))
+ return &RefVal->getReferentLoc();
+
+ return Loc;
+}
+
StorageLocation *Environment::getThisPointeeStorageLocation() const {
return ThisPointeeLoc;
}
@@ -675,6 +700,18 @@ void Environment::setValue(const StorageLocation &Loc, Value &Val) {
}
}
+void Environment::setValueStrict(const Expr &E, Value &Val) {
+ assert(E.isPRValue());
+ assert(!isa<ReferenceValue>(Val));
+
+ StorageLocation *Loc = getStorageLocation(E, SkipPast::None);
+ if (Loc == nullptr) {
+ Loc = &createStorageLocation(E);
+ setStorageLocation(E, *Loc);
+ }
+ setValue(*Loc, Val);
+}
+
Value *Environment::getValue(const StorageLocation &Loc) const {
auto It = LocToVal.find(&Loc);
return It == LocToVal.end() ? nullptr : It->second;
@@ -694,6 +731,15 @@ Value *Environment::getValue(const Expr &E, SkipPast SP) const {
return getValue(*Loc);
}
+Value *Environment::getValueStrict(const Expr &E) const {
+ assert(E.isPRValue());
+ Value *Val = getValue(E, SkipPast::None);
+
+ assert(Val == nullptr || !isa<ReferenceValue>(Val));
+
+ return Val;
+}
+
Value *Environment::createValue(QualType Type) {
llvm::DenseSet<QualType> Visited;
int CreatedValuesCount = 0;
diff --git a/clang/lib/Analysis/FlowSensitive/Transfer.cpp b/clang/lib/Analysis/FlowSensitive/Transfer.cpp
index 95810a47fc632..45c601ed49146 100644
--- a/clang/lib/Analysis/FlowSensitive/Transfer.cpp
+++ b/clang/lib/Analysis/FlowSensitive/Transfer.cpp
@@ -223,7 +223,7 @@ class TransferVisitor : public ConstStmtVisitor<TransferVisitor> {
if (DeclLoc == nullptr)
return;
- Env.setStorageLocation(*S, *DeclLoc);
+ Env.setStorageLocationStrict(*S, *DeclLoc);
}
void VisitDeclStmt(const DeclStmt *S) {
@@ -343,15 +343,13 @@ class TransferVisitor : public ConstStmtVisitor<TransferVisitor> {
// This cast creates a new, boolean value from the integral value. We
// model that with a fresh value in the environment, unless it's already a
// boolean.
- auto &Loc = Env.createStorageLocation(*S);
- Env.setStorageLocation(*S, Loc);
- if (auto *SubExprVal = dyn_cast_or_null<BoolValue>(
- Env.getValue(*SubExpr, SkipPast::Reference)))
- Env.setValue(Loc, *SubExprVal);
+ if (auto *SubExprVal =
+ dyn_cast_or_null<BoolValue>(Env.getValueStrict(*SubExpr)))
+ Env.setValueStrict(*S, *SubExprVal);
else
// FIXME: If integer modeling is added, then update this code to create
// the boolean based on the integer model.
- Env.setValue(Loc, Env.makeAtomicBoolValue());
+ Env.setValueStrict(*S, Env.makeAtomicBoolValue());
break;
}
@@ -425,29 +423,22 @@ class TransferVisitor : public ConstStmtVisitor<TransferVisitor> {
switch (S->getOpcode()) {
case UO_Deref: {
const auto *SubExprVal =
- cast_or_null<PointerValue>(Env.getValue(*SubExpr, SkipPast::None));
+ cast_or_null<PointerValue>(Env.getValueStrict(*SubExpr));
if (SubExprVal == nullptr)
break;
- auto &Loc = Env.createStorageLocation(*S);
- Env.setStorageLocation(*S, Loc);
- Env.setValue(Loc,
- Env.create<ReferenceValue>(SubExprVal->getPointeeLoc()));
+ Env.setStorageLocationStrict(*S, SubExprVal->getPointeeLoc());
break;
}
case UO_AddrOf: {
// Do not form a pointer to a reference. If `SubExpr` is assigned a
// `ReferenceValue` then form a value that points to the location of its
// pointee.
- StorageLocation *PointeeLoc =
- Env.getStorageLocation(*SubExpr, SkipPast::Reference);
+ StorageLocation *PointeeLoc = Env.getStorageLocationStrict(*SubExpr);
if (PointeeLoc == nullptr)
break;
- auto &PointerLoc = Env.createStorageLocation(*S);
- auto &PointerVal = Env.create<PointerValue>(*PointeeLoc);
- Env.setStorageLocation(*S, PointerLoc);
- Env.setValue(PointerLoc, PointerVal);
+ Env.setValueStrict(*S, Env.create<PointerValue>(*PointeeLoc));
break;
}
case UO_LNot: {
diff --git a/clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp b/clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp
index 418a6cff21cc1..2e9893e78fa7d 100644
--- a/clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp
+++ b/clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp
@@ -135,14 +135,8 @@ class TerminatorVisitor
// entirety (even if they may share some clauses). So, we need *some* value
// for the condition expression, even if just an atom.
if (Val == nullptr) {
- // FIXME: Consider introducing a helper for this get-or-create pattern.
- auto *Loc = Env.getStorageLocation(Cond, SkipPast::None);
- if (Loc == nullptr) {
- Loc = &Env.createStorageLocation(Cond);
- Env.setStorageLocation(Cond, *Loc);
- }
Val = &Env.makeAtomicBoolValue();
- Env.setValue(*Loc, *Val);
+ Env.setValueStrict(Cond, *Val);
}
bool ConditionValue = true;
More information about the cfe-commits
mailing list