[clang] 330d5bc - [clang][dataflow] Don't associate prvalue expressions with storage locations.
Martin Braenne via cfe-commits
cfe-commits at lists.llvm.org
Tue Aug 29 00:28:55 PDT 2023
Author: Martin Braenne
Date: 2023-08-29T07:28:46Z
New Revision: 330d5bcbf61043b5ca2bf5a65bd4488718c85e6e
URL: https://github.com/llvm/llvm-project/commit/330d5bcbf61043b5ca2bf5a65bd4488718c85e6e
DIFF: https://github.com/llvm/llvm-project/commit/330d5bcbf61043b5ca2bf5a65bd4488718c85e6e.diff
LOG: [clang][dataflow] Don't associate prvalue expressions with storage locations.
Instead, map prvalue expressions directly to values in a newly introduced map `Environment::ExprToVal`.
This change introduces an additional member variable in `Environment` but is an overall win:
- It is more conceptually correctly, since prvalues don't have storage
locations.
- It eliminates complexity from
`Environment::setValue(const Expr &E, Value &Val)`.
- It reduces the amount of data stored in `Environment`: A prvalue now has a
single entry in `ExprToVal` instead of one in `ExprToLoc` and one in
`LocToVal`.
- Not allocating `StorageLocation`s for prvalues additionally reduces memory
usage.
This patch is the last step in the migration to strict handling of value categories (see https://discourse.llvm.org/t/70086 for details). The changes here are almost entirely internal to `Environment`.
The only externally observable change is that when associating a `RecordValue` with the location returned by `Environment::getResultObjectLocation()` for a given expression, callers additionally need to associate the `RecordValue` with the expression themselves.
Reviewed By: xazax.hun
Differential Revision: https://reviews.llvm.org/D158977
Added:
Modified:
clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h
clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
clang/lib/Analysis/FlowSensitive/Transfer.cpp
Removed:
################################################################################
diff --git a/clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h b/clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h
index f812c01d42040b..95514d940c3f6a 100644
--- a/clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h
+++ b/clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h
@@ -651,14 +651,17 @@ class Environment {
// function being analyzed is only a function and not a method.
RecordStorageLocation *ThisPointeeLoc = nullptr;
- // Maps from program declarations and statements to storage locations that are
+ // Maps from declarations and glvalue expression to storage locations that are
// assigned to them. Unlike the maps in `DataflowAnalysisContext`, these
// include only storage locations that are in scope for a particular basic
// block.
llvm::DenseMap<const ValueDecl *, StorageLocation *> DeclToLoc;
llvm::DenseMap<const Expr *, StorageLocation *> ExprToLoc;
+ // Maps from prvalue expressions and storage locations to the values that
+ // are assigned to them.
// We preserve insertion order so that join/widen process values in
// deterministic sequence. This in turn produces deterministic SAT formulas.
+ llvm::MapVector<const Expr *, Value *> ExprToVal;
llvm::MapVector<const StorageLocation *, Value *> LocToVal;
Atom FlowConditionToken;
diff --git a/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp b/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
index f6c8f7d2d395ec..f15ea52d6b4699 100644
--- a/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
+++ b/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
@@ -171,6 +171,105 @@ static Value &widenDistinctValues(QualType Type, Value &Prev,
return Current;
}
+// Returns whether the values in `Map1` and `Map2` compare equal for those
+// keys that `Map1` and `Map2` have in common.
+template <typename Key>
+bool compareKeyToValueMaps(const llvm::MapVector<Key, Value *> &Map1,
+ const llvm::MapVector<Key, Value *> &Map2,
+ const Environment &Env1, const Environment &Env2,
+ Environment::ValueModel &Model) {
+ for (auto &Entry : Map1) {
+ Key K = Entry.first;
+ assert(K != nullptr);
+
+ Value *Val = Entry.second;
+ assert(Val != nullptr);
+
+ auto It = Map2.find(K);
+ if (It == Map2.end())
+ continue;
+ assert(It->second != nullptr);
+
+ if (!areEquivalentValues(*Val, *It->second) &&
+ !compareDistinctValues(K->getType(), *Val, Env1, *It->second, Env2,
+ Model))
+ return false;
+ }
+
+ return true;
+}
+
+// Perform a join on either `LocToVal` or `ExprToVal`. `Key` must be either
+// `const StorageLocation *` or `const Expr *`.
+template <typename Key>
+llvm::MapVector<Key, Value *>
+joinKeyToValueMap(const llvm::MapVector<Key, Value *> &Map1,
+ const llvm::MapVector<Key, Value *> &Map2,
+ const Environment &Env1, const Environment &Env2,
+ Environment &JoinedEnv, Environment::ValueModel &Model) {
+ llvm::MapVector<Key, Value *> MergedMap;
+ for (auto &Entry : Map1) {
+ Key K = Entry.first;
+ assert(K != nullptr);
+
+ Value *Val = Entry.second;
+ assert(Val != nullptr);
+
+ auto It = Map2.find(K);
+ if (It == Map2.end())
+ continue;
+ assert(It->second != nullptr);
+
+ if (areEquivalentValues(*Val, *It->second)) {
+ MergedMap.insert({K, Val});
+ continue;
+ }
+
+ if (Value *MergedVal = mergeDistinctValues(
+ K->getType(), *Val, Env1, *It->second, Env2, JoinedEnv, Model)) {
+ MergedMap.insert({K, MergedVal});
+ }
+ }
+
+ return MergedMap;
+}
+
+// Perform widening on either `LocToVal` or `ExprToVal`. `Key` must be either
+// `const StorageLocation *` or `const Expr *`.
+template <typename Key>
+llvm::MapVector<Key, Value *>
+widenKeyToValueMap(const llvm::MapVector<Key, Value *> &CurMap,
+ const llvm::MapVector<Key, Value *> &PrevMap,
+ Environment &CurEnv, const Environment &PrevEnv,
+ Environment::ValueModel &Model, LatticeJoinEffect &Effect) {
+ llvm::MapVector<Key, Value *> WidenedMap;
+ for (auto &Entry : CurMap) {
+ Key K = Entry.first;
+ assert(K != nullptr);
+
+ Value *Val = Entry.second;
+ assert(Val != nullptr);
+
+ auto PrevIt = PrevMap.find(K);
+ if (PrevIt == PrevMap.end())
+ continue;
+ assert(PrevIt->second != nullptr);
+
+ if (areEquivalentValues(*Val, *PrevIt->second)) {
+ WidenedMap.insert({K, Val});
+ continue;
+ }
+
+ Value &WidenedVal = widenDistinctValues(K->getType(), *PrevIt->second,
+ PrevEnv, *Val, CurEnv, Model);
+ WidenedMap.insert({K, &WidenedVal});
+ if (&WidenedVal != PrevIt->second)
+ Effect = LatticeJoinEffect::Changed;
+ }
+
+ return WidenedMap;
+}
+
/// Initializes a global storage value.
static void insertIfGlobal(const Decl &D,
llvm::DenseSet<const VarDecl *> &Vars) {
@@ -384,13 +483,13 @@ void Environment::pushCallInternal(const FunctionDecl *FuncDecl,
}
void Environment::popCall(const CallExpr *Call, const Environment &CalleeEnv) {
- // We ignore `DACtx` because it's already the same in both. We don't want the
- // callee's `DeclCtx`, `ReturnVal`, `ReturnLoc` or `ThisPointeeLoc`. We don't
- // bring back `DeclToLoc` and `ExprToLoc` because we want to be able to later
- // analyze the same callee in a
diff erent context, and `setStorageLocation`
- // requires there to not already be a storage location assigned. Conceptually,
- // these maps capture information from the local scope, so when popping that
- // scope, we do not propagate the maps.
+ // We ignore some entries of `CalleeEnv`:
+ // - `DACtx` because is already the same in both
+ // - We don't want the callee's `DeclCtx`, `ReturnVal`, `ReturnLoc` or
+ // `ThisPointeeLoc` because they don't apply to us.
+ // - `DeclToLoc`, `ExprToLoc`, and `ExprToVal` capture information from the
+ // callee's local scope, so when popping that scope, we do not propagate
+ // the maps.
this->LocToVal = std::move(CalleeEnv.LocToVal);
this->FlowConditionToken = std::move(CalleeEnv.FlowConditionToken);
@@ -433,24 +532,11 @@ bool Environment::equivalentTo(const Environment &Other,
if (ExprToLoc != Other.ExprToLoc)
return false;
- // Compare the contents for the intersection of their domains.
- for (auto &Entry : LocToVal) {
- const StorageLocation *Loc = Entry.first;
- assert(Loc != nullptr);
-
- Value *Val = Entry.second;
- assert(Val != nullptr);
-
- auto It = Other.LocToVal.find(Loc);
- if (It == Other.LocToVal.end())
- continue;
- assert(It->second != nullptr);
+ if (!compareKeyToValueMaps(ExprToVal, Other.ExprToVal, *this, Other, Model))
+ return false;
- if (!areEquivalentValues(*Val, *It->second) &&
- !compareDistinctValues(Loc->getType(), *Val, *this, *It->second, Other,
- Model))
- return false;
- }
+ if (!compareKeyToValueMaps(LocToVal, Other.LocToVal, *this, Other, Model))
+ return false;
return true;
}
@@ -468,39 +554,21 @@ LatticeJoinEffect Environment::widen(const Environment &PrevEnv,
// By the API, `PrevEnv` is a previous version of the environment for the same
// block, so we have some guarantees about its shape. In particular, it will
// be the result of a join or widen operation on previous values for this
- // block. For `DeclToLoc` and `ExprToLoc`, join guarantees that these maps are
- // subsets of the maps in `PrevEnv`. So, as long as we maintain this property
- // here, we don't need change their current values to widen.
+ // block. For `DeclToLoc`, `ExprToVal`, and `ExprToLoc`, join guarantees that
+ // these maps are subsets of the maps in `PrevEnv`. So, as long as we maintain
+ // this property here, we don't need change their current values to widen.
assert(DeclToLoc.size() <= PrevEnv.DeclToLoc.size());
+ assert(ExprToVal.size() <= PrevEnv.ExprToVal.size());
assert(ExprToLoc.size() <= PrevEnv.ExprToLoc.size());
- llvm::MapVector<const StorageLocation *, Value *> WidenedLocToVal;
- for (auto &Entry : LocToVal) {
- const StorageLocation *Loc = Entry.first;
- assert(Loc != nullptr);
-
- Value *Val = Entry.second;
- assert(Val != nullptr);
-
- auto PrevIt = PrevEnv.LocToVal.find(Loc);
- if (PrevIt == PrevEnv.LocToVal.end())
- continue;
- assert(PrevIt->second != nullptr);
-
- if (areEquivalentValues(*Val, *PrevIt->second)) {
- WidenedLocToVal.insert({Loc, Val});
- continue;
- }
+ ExprToVal = widenKeyToValueMap(ExprToVal, PrevEnv.ExprToVal, *this, PrevEnv,
+ Model, Effect);
- Value &WidenedVal = widenDistinctValues(Loc->getType(), *PrevIt->second,
- PrevEnv, *Val, *this, Model);
- WidenedLocToVal.insert({Loc, &WidenedVal});
- if (&WidenedVal != PrevIt->second)
- Effect = LatticeJoinEffect::Changed;
- }
- LocToVal = std::move(WidenedLocToVal);
+ LocToVal = widenKeyToValueMap(LocToVal, PrevEnv.LocToVal, *this, PrevEnv,
+ Model, Effect);
if (DeclToLoc.size() != PrevEnv.DeclToLoc.size() ||
ExprToLoc.size() != PrevEnv.ExprToLoc.size() ||
+ ExprToVal.size() != PrevEnv.ExprToVal.size() ||
LocToVal.size() != PrevEnv.LocToVal.size())
Effect = LatticeJoinEffect::Changed;
@@ -559,28 +627,11 @@ Environment Environment::join(const Environment &EnvA, const Environment &EnvB,
JoinedEnv.FlowConditionToken = EnvA.DACtx->joinFlowConditions(
EnvA.FlowConditionToken, EnvB.FlowConditionToken);
- for (auto &Entry : EnvA.LocToVal) {
- const StorageLocation *Loc = Entry.first;
- assert(Loc != nullptr);
-
- Value *Val = Entry.second;
- assert(Val != nullptr);
-
- auto It = EnvB.LocToVal.find(Loc);
- if (It == EnvB.LocToVal.end())
- continue;
- assert(It->second != nullptr);
-
- if (areEquivalentValues(*Val, *It->second)) {
- JoinedEnv.LocToVal.insert({Loc, Val});
- continue;
- }
+ JoinedEnv.ExprToVal = joinKeyToValueMap(EnvA.ExprToVal, EnvB.ExprToVal, EnvA,
+ EnvB, JoinedEnv, Model);
- if (Value *MergedVal = mergeDistinctValues(
- Loc->getType(), *Val, EnvA, *It->second, EnvB, JoinedEnv, Model)) {
- JoinedEnv.LocToVal.insert({Loc, MergedVal});
- }
- }
+ JoinedEnv.LocToVal = joinKeyToValueMap(EnvA.LocToVal, EnvB.LocToVal, EnvA,
+ EnvB, JoinedEnv, Model);
return JoinedEnv;
}
@@ -663,26 +714,7 @@ void Environment::setValue(const StorageLocation &Loc, Value &Val) {
void Environment::setValue(const Expr &E, Value &Val) {
assert(E.isPRValue());
-
- if (auto *RecordVal = dyn_cast<RecordValue>(&Val)) {
- if ([[maybe_unused]] auto *ExistingVal =
- cast_or_null<RecordValue>(getValue(E)))
- assert(&ExistingVal->getLoc() == &RecordVal->getLoc());
- if ([[maybe_unused]] StorageLocation *ExistingLoc =
- getStorageLocationInternal(E))
- assert(ExistingLoc == &RecordVal->getLoc());
- else
- setStorageLocationInternal(E, RecordVal->getLoc());
- setValue(RecordVal->getLoc(), Val);
- return;
- }
-
- StorageLocation *Loc = getStorageLocationInternal(E);
- if (Loc == nullptr) {
- Loc = &createStorageLocation(E);
- setStorageLocationInternal(E, *Loc);
- }
- setValue(*Loc, Val);
+ ExprToVal[&E] = &Val;
}
Value *Environment::getValue(const StorageLocation &Loc) const {
@@ -697,6 +729,11 @@ Value *Environment::getValue(const ValueDecl &D) const {
}
Value *Environment::getValue(const Expr &E) const {
+ if (E.isPRValue()) {
+ auto It = ExprToVal.find(&ignoreCFGOmittedNodes(E));
+ return It == ExprToVal.end() ? nullptr : It->second;
+ }
+
auto It = ExprToLoc.find(&ignoreCFGOmittedNodes(E));
if (It == ExprToLoc.end())
return nullptr;
@@ -879,6 +916,10 @@ void Environment::dump(raw_ostream &OS) const {
for (auto [E, L] : ExprToLoc)
OS << " [" << E << ", " << L << "]\n";
+ OS << "ExprToVal:\n";
+ for (auto [E, V] : ExprToVal)
+ OS << " [" << E << ", " << V << "]\n";
+
OS << "LocToVal:\n";
for (auto [L, V] : LocToVal) {
OS << " [" << L << ", " << V << ": " << *V << "]\n";
diff --git a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
index 961a06d0aa0afe..e40bd3d0199bdd 100644
--- a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
+++ b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
@@ -458,8 +458,9 @@ void transferArrowOpCall(const Expr *UnwrapExpr, const Expr *ObjectExpr,
void transferMakeOptionalCall(const CallExpr *E,
const MatchFinder::MatchResult &,
LatticeTransferState &State) {
- createOptionalValue(State.Env.getResultObjectLocation(*E),
- State.Env.getBoolLiteralValue(true), State.Env);
+ State.Env.setValue(
+ *E, createOptionalValue(State.Env.getResultObjectLocation(*E),
+ State.Env.getBoolLiteralValue(true), State.Env));
}
void transferOptionalHasValueCall(const CXXMemberCallExpr *CallExpr,
@@ -543,7 +544,10 @@ void transferCallReturningOptional(const CallExpr *E,
}
}
- createOptionalValue(*Loc, State.Env.makeAtomicBoolValue(), State.Env);
+ RecordValue &Val =
+ createOptionalValue(*Loc, State.Env.makeAtomicBoolValue(), State.Env);
+ if (E->isPRValue())
+ State.Env.setValue(*E, Val);
}
void constructOptionalValue(const Expr &E, Environment &Env,
diff --git a/clang/lib/Analysis/FlowSensitive/Transfer.cpp b/clang/lib/Analysis/FlowSensitive/Transfer.cpp
index 200a981584bb76..c0092694ef385d 100644
--- a/clang/lib/Analysis/FlowSensitive/Transfer.cpp
+++ b/clang/lib/Analysis/FlowSensitive/Transfer.cpp
@@ -488,6 +488,7 @@ class TransferVisitor : public ConstStmtVisitor<TransferVisitor> {
// we've got a record type.
if (S->getType()->isRecordType()) {
auto &InitialVal = *cast<RecordValue>(Env.createValue(S->getType()));
+ Env.setValue(*S, InitialVal);
copyRecord(InitialVal.getLoc(), Env.getResultObjectLocation(*S), Env);
}
More information about the cfe-commits
mailing list