Skip to content

Commit bbd259a

Browse files
ymandymand
authored
[clang][dataflow] Refactor widen API to be explicit about change effect. (llvm#87233)
The previous API relied on pointer equality of inputs and outputs to signal whether a change occured. This was too subtle and led to bugs in practice. It was also very limiting: the override could not return an equivalent (but not identical) value.
1 parent 22089ae commit bbd259a

File tree

4 files changed

+111
-68
lines changed

4 files changed

+111
-68
lines changed

‎clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h

Lines changed: 32 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -43,6 +43,15 @@ enum class ComparisonResult {
4343
Unknown,
4444
};
4545

46+
/// The result of a `widen` operation.
47+
struct WidenResult {
48+
/// Non-null pointer to a potentially widened version of the input value.
49+
Value *V;
50+
/// Whether `V` represents a "change" (that is, a different value) with
51+
/// respect to the previous value in the sequence.
52+
LatticeEffect Effect;
53+
};
54+
4655
/// Holds the state of the program (store and heap) at a given program point.
4756
///
4857
/// WARNING: Symbolic values that are created by the environment for static
@@ -104,14 +113,17 @@ class Environment {
104113
/// serve as a comparison operation, by indicating whether the widened value
105114
/// is equivalent to the previous value.
106115
///
107-
/// Returns either:
108-
///
109-
/// `nullptr`, if this value is not of interest to the model, or
110-
///
111-
/// `&Prev`, if the widened value is equivalent to `Prev`, or
112-
///
113-
/// A non-null value that approximates `Current`. `Prev` is available to
114-
/// inform the chosen approximation.
116+
/// Returns one of the folowing:
117+
/// * `std::nullopt`, if this value is not of interest to the
118+
/// model.
119+
/// * A `WidenResult` with:
120+
/// * A non-null `Value *` that points either to `Current` or a widened
121+
/// version of `Current`. This value must be consistent with
122+
/// the flow condition of `CurrentEnv`. We particularly caution
123+
/// against using `Prev`, which is rarely consistent.
124+
/// * A `LatticeEffect` indicating whether the value should be
125+
/// considered a new value (`Changed`) or one *equivalent* (if not
126+
/// necessarily equal) to `Prev` (`Unchanged`).
115127
///
116128
/// `PrevEnv` and `CurrentEnv` can be used to query child values and path
117129
/// condition implications of `Prev` and `Current`, respectively.
@@ -122,17 +134,19 @@ class Environment {
122134
///
123135
/// `Prev` and `Current` must be assigned to the same storage location in
124136
/// `PrevEnv` and `CurrentEnv`, respectively.
125-
virtual Value *widen(QualType Type, Value &Prev, const Environment &PrevEnv,
126-
Value &Current, Environment &CurrentEnv) {
137+
virtual std::optional<WidenResult> widen(QualType Type, Value &Prev,
138+
const Environment &PrevEnv,
139+
Value &Current,
140+
Environment &CurrentEnv) {
127141
// The default implementation reduces to just comparison, since comparison
128142
// is required by the API, even if no widening is performed.
129143
switch (compare(Type, Prev, PrevEnv, Current, CurrentEnv)) {
130-
case ComparisonResult::Same:
131-
return &Prev;
132-
case ComparisonResult::Different:
133-
return &Current;
134-
case ComparisonResult::Unknown:
135-
return nullptr;
144+
case ComparisonResult::Unknown:
145+
return std::nullopt;
146+
case ComparisonResult::Same:
147+
return WidenResult{&Current, LatticeEffect::Unchanged};
148+
case ComparisonResult::Different:
149+
return WidenResult{&Current, LatticeEffect::Changed};
136150
}
137151
llvm_unreachable("all cases in switch covered");
138152
}
@@ -236,8 +250,8 @@ class Environment {
236250
///
237251
/// `PrevEnv` must be the immediate previous version of the environment.
238252
/// `PrevEnv` and `this` must use the same `DataflowAnalysisContext`.
239-
LatticeJoinEffect widen(const Environment &PrevEnv,
240-
Environment::ValueModel &Model);
253+
LatticeEffect widen(const Environment &PrevEnv,
254+
Environment::ValueModel &Model);
241255

242256
// FIXME: Rename `createOrGetStorageLocation` to `getOrCreateStorageLocation`,
243257
// `getStableStorageLocation`, or something more appropriate.

‎clang/include/clang/Analysis/FlowSensitive/DataflowLattice.h

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -17,13 +17,13 @@
1717
namespace clang {
1818
namespace dataflow {
1919

20-
/// Effect indicating whether a lattice join operation resulted in a new value.
21-
// FIXME: Rename to `LatticeEffect` since `widen` uses it as well, and we are
22-
// likely removing it from `join`.
23-
enum class LatticeJoinEffect {
20+
/// Effect indicating whether a lattice operation resulted in a new value.
21+
enum class LatticeEffect {
2422
Unchanged,
2523
Changed,
2624
};
25+
// DEPRECATED. Use `LatticeEffect`.
26+
using LatticeJoinEffect = LatticeEffect;
2727

2828
} // namespace dataflow
2929
} // namespace clang

‎clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp

Lines changed: 25 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -166,17 +166,16 @@ static Value *joinDistinctValues(QualType Type, Value &Val1,
166166
return JoinedVal;
167167
}
168168

169-
// When widening does not change `Current`, return value will equal `&Prev`.
170-
static Value &widenDistinctValues(QualType Type, Value &Prev,
171-
const Environment &PrevEnv, Value &Current,
172-
Environment &CurrentEnv,
173-
Environment::ValueModel &Model) {
169+
static WidenResult widenDistinctValues(QualType Type, Value &Prev,
170+
const Environment &PrevEnv,
171+
Value &Current, Environment &CurrentEnv,
172+
Environment::ValueModel &Model) {
174173
// Boolean-model widening.
175174
if (auto *PrevBool = dyn_cast<BoolValue>(&Prev)) {
176-
// If previous value was already Top, re-use that to (implicitly) indicate
177-
// that no change occurred.
178175
if (isa<TopBoolValue>(Prev))
179-
return Prev;
176+
// Safe to return `Prev` here, because Top is never dependent on the
177+
// environment.
178+
return {&Prev, LatticeEffect::Unchanged};
180179

181180
// We may need to widen to Top, but before we do so, check whether both
182181
// values are implied to be either true or false in the current environment.
@@ -185,22 +184,24 @@ static Value &widenDistinctValues(QualType Type, Value &Prev,
185184
bool TruePrev = PrevEnv.proves(PrevBool->formula());
186185
bool TrueCur = CurrentEnv.proves(CurBool.formula());
187186
if (TruePrev && TrueCur)
188-
return CurrentEnv.getBoolLiteralValue(true);
187+
return {&CurrentEnv.getBoolLiteralValue(true), LatticeEffect::Unchanged};
189188
if (!TruePrev && !TrueCur &&
190189
PrevEnv.proves(PrevEnv.arena().makeNot(PrevBool->formula())) &&
191190
CurrentEnv.proves(CurrentEnv.arena().makeNot(CurBool.formula())))
192-
return CurrentEnv.getBoolLiteralValue(false);
191+
return {&CurrentEnv.getBoolLiteralValue(false), LatticeEffect::Unchanged};
193192

194-
return CurrentEnv.makeTopBoolValue();
193+
return {&CurrentEnv.makeTopBoolValue(), LatticeEffect::Changed};
195194
}
196195

197196
// FIXME: Add other built-in model widening.
198197

199198
// Custom-model widening.
200-
if (auto *W = Model.widen(Type, Prev, PrevEnv, Current, CurrentEnv))
201-
return *W;
199+
if (auto Result = Model.widen(Type, Prev, PrevEnv, Current, CurrentEnv))
200+
return *Result;
202201

203-
return equateUnknownValues(Prev.getKind()) ? Prev : Current;
202+
return {&Current, equateUnknownValues(Prev.getKind())
203+
? LatticeEffect::Unchanged
204+
: LatticeEffect::Changed};
204205
}
205206

206207
// Returns whether the values in `Map1` and `Map2` compare equal for those
@@ -271,7 +272,7 @@ llvm::MapVector<Key, Value *>
271272
widenKeyToValueMap(const llvm::MapVector<Key, Value *> &CurMap,
272273
const llvm::MapVector<Key, Value *> &PrevMap,
273274
Environment &CurEnv, const Environment &PrevEnv,
274-
Environment::ValueModel &Model, LatticeJoinEffect &Effect) {
275+
Environment::ValueModel &Model, LatticeEffect &Effect) {
275276
llvm::MapVector<Key, Value *> WidenedMap;
276277
for (auto &Entry : CurMap) {
277278
Key K = Entry.first;
@@ -290,11 +291,11 @@ widenKeyToValueMap(const llvm::MapVector<Key, Value *> &CurMap,
290291
continue;
291292
}
292293

293-
Value &WidenedVal = widenDistinctValues(K->getType(), *PrevIt->second,
294-
PrevEnv, *Val, CurEnv, Model);
295-
WidenedMap.insert({K, &WidenedVal});
296-
if (&WidenedVal != PrevIt->second)
297-
Effect = LatticeJoinEffect::Changed;
294+
auto [WidenedVal, ValEffect] = widenDistinctValues(
295+
K->getType(), *PrevIt->second, PrevEnv, *Val, CurEnv, Model);
296+
WidenedMap.insert({K, WidenedVal});
297+
if (ValEffect == LatticeEffect::Changed)
298+
Effect = LatticeEffect::Changed;
298299
}
299300

300301
return WidenedMap;
@@ -617,15 +618,15 @@ bool Environment::equivalentTo(const Environment &Other,
617618
return true;
618619
}
619620

620-
LatticeJoinEffect Environment::widen(const Environment &PrevEnv,
621-
Environment::ValueModel &Model) {
621+
LatticeEffect Environment::widen(const Environment &PrevEnv,
622+
Environment::ValueModel &Model) {
622623
assert(DACtx == PrevEnv.DACtx);
623624
assert(ReturnVal == PrevEnv.ReturnVal);
624625
assert(ReturnLoc == PrevEnv.ReturnLoc);
625626
assert(ThisPointeeLoc == PrevEnv.ThisPointeeLoc);
626627
assert(CallStack == PrevEnv.CallStack);
627628

628-
auto Effect = LatticeJoinEffect::Unchanged;
629+
auto Effect = LatticeEffect::Unchanged;
629630

630631
// By the API, `PrevEnv` is a previous version of the environment for the same
631632
// block, so we have some guarantees about its shape. In particular, it will
@@ -646,7 +647,7 @@ LatticeJoinEffect Environment::widen(const Environment &PrevEnv,
646647
ExprToLoc.size() != PrevEnv.ExprToLoc.size() ||
647648
ExprToVal.size() != PrevEnv.ExprToVal.size() ||
648649
LocToVal.size() != PrevEnv.LocToVal.size())
649-
Effect = LatticeJoinEffect::Changed;
650+
Effect = LatticeEffect::Changed;
650651

651652
return Effect;
652653
}

‎clang/unittests/Analysis/FlowSensitive/TypeErasedDataflowAnalysisTest.cpp

Lines changed: 50 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -805,6 +805,25 @@ class NullPointerAnalysis final
805805
else
806806
JoinedVal.setProperty("is_null", JoinedEnv.makeTopBoolValue());
807807
}
808+
809+
std::optional<WidenResult> widen(QualType Type, Value &Prev,
810+
const Environment &PrevEnv, Value &Current,
811+
Environment &CurrentEnv) override {
812+
switch (compare(Type, Prev, PrevEnv, Current, CurrentEnv)) {
813+
case ComparisonResult::Same:
814+
return WidenResult{&Current, LatticeJoinEffect::Unchanged};
815+
case ComparisonResult::Different: {
816+
auto &CurPtr = cast<PointerValue>(Current);
817+
auto &WidenedPtr =
818+
CurrentEnv.create<PointerValue>(CurPtr.getPointeeLoc());
819+
WidenedPtr.setProperty("is_null", CurrentEnv.makeTopBoolValue());
820+
return WidenResult{&WidenedPtr, LatticeJoinEffect::Changed};
821+
}
822+
case ComparisonResult::Unknown:
823+
return std::nullopt;
824+
}
825+
llvm_unreachable("all cases in switch covered");
826+
}
808827
};
809828

810829
class WideningTest : public Test {
@@ -846,7 +865,6 @@ TEST_F(WideningTest, JoinDistinctValuesWithDistinctProperties) {
846865
Code,
847866
[](const llvm::StringMap<DataflowAnalysisState<NoopLattice>> &Results,
848867
ASTContext &ASTCtx) {
849-
ASSERT_THAT(Results.keys(), UnorderedElementsAre("p1", "p2", "p3"));
850868
const Environment &Env1 = getEnvironmentAtAnnotation(Results, "p1");
851869
const Environment &Env2 = getEnvironmentAtAnnotation(Results, "p2");
852870
const Environment &Env3 = getEnvironmentAtAnnotation(Results, "p3");
@@ -889,8 +907,6 @@ TEST_F(WideningTest, JoinDistinctValuesWithSameProperties) {
889907
Code,
890908
[](const llvm::StringMap<DataflowAnalysisState<NoopLattice>> &Results,
891909
ASTContext &ASTCtx) {
892-
ASSERT_THAT(Results.keys(),
893-
UnorderedElementsAre("p1", "p2", "p3", "p4"));
894910
const Environment &Env1 = getEnvironmentAtAnnotation(Results, "p1");
895911
const Environment &Env2 = getEnvironmentAtAnnotation(Results, "p2");
896912
const Environment &Env3 = getEnvironmentAtAnnotation(Results, "p3");
@@ -929,19 +945,11 @@ TEST_F(WideningTest, DistinctPointersToTheSameLocationAreEquivalent) {
929945
Code,
930946
[](const llvm::StringMap<DataflowAnalysisState<NoopLattice>> &Results,
931947
ASTContext &ASTCtx) {
932-
ASSERT_THAT(Results.keys(), UnorderedElementsAre("p"));
933948
const Environment &Env = getEnvironmentAtAnnotation(Results, "p");
934-
935-
const ValueDecl *FooDecl = findValueDecl(ASTCtx, "Foo");
936-
ASSERT_THAT(FooDecl, NotNull());
937-
938-
const ValueDecl *BarDecl = findValueDecl(ASTCtx, "Bar");
939-
ASSERT_THAT(BarDecl, NotNull());
940-
941-
const auto *FooLoc =
942-
cast<ScalarStorageLocation>(Env.getStorageLocation(*FooDecl));
943-
const auto *BarVal = cast<PointerValue>(Env.getValue(*BarDecl));
944-
EXPECT_EQ(&BarVal->getPointeeLoc(), FooLoc);
949+
const auto &FooLoc =
950+
getLocForDecl<ScalarStorageLocation>(ASTCtx, Env, "Foo");
951+
const auto &BarVal = getValueForDecl<PointerValue>(ASTCtx, Env, "Bar");
952+
EXPECT_EQ(&BarVal.getPointeeLoc(), &FooLoc);
945953
});
946954
}
947955

@@ -963,18 +971,38 @@ TEST_F(WideningTest, DistinctValuesWithSamePropertiesAreEquivalent) {
963971
Code,
964972
[](const llvm::StringMap<DataflowAnalysisState<NoopLattice>> &Results,
965973
ASTContext &ASTCtx) {
966-
ASSERT_THAT(Results.keys(), UnorderedElementsAre("p"));
967974
const Environment &Env = getEnvironmentAtAnnotation(Results, "p");
968-
969-
const ValueDecl *FooDecl = findValueDecl(ASTCtx, "Foo");
970-
ASSERT_THAT(FooDecl, NotNull());
971-
972-
const auto *FooVal = Env.getValue(*FooDecl);
973-
EXPECT_EQ(FooVal->getProperty("is_null"),
975+
const auto &FooVal = getValueForDecl<Value>(ASTCtx, Env, "Foo");
976+
EXPECT_EQ(FooVal.getProperty("is_null"),
974977
&Env.getBoolLiteralValue(false));
975978
});
976979
}
977980

981+
TEST_F(WideningTest, DistinctValuesWithDifferentPropertiesWidenedToTop) {
982+
std::string Code = R"(
983+
void target(bool Cond) {
984+
int *Foo;
985+
int i = 0;
986+
Foo = nullptr;
987+
while (Cond) {
988+
Foo = &i;
989+
}
990+
(void)0;
991+
/*[[p]]*/
992+
}
993+
)";
994+
runDataflow(
995+
Code,
996+
[](const llvm::StringMap<DataflowAnalysisState<NoopLattice>> &Results,
997+
ASTContext &ASTCtx) {
998+
const Environment &Env = getEnvironmentAtAnnotation(Results, "p");
999+
const auto &FooVal = getValueForDecl<Value>(ASTCtx, Env, "Foo");
1000+
ASSERT_THAT(FooVal.getProperty("is_null"), NotNull());
1001+
EXPECT_TRUE(areEquivalentValues(*FooVal.getProperty("is_null"),
1002+
Env.makeTopBoolValue()));
1003+
});
1004+
}
1005+
9781006
class FlowConditionTest : public Test {
9791007
protected:
9801008
template <typename Matcher>

0 commit comments

Comments
 (0)