[clang] b9208ce - [clang][dataflow] Crash fix for `widenDistinctValues()`. (#89895)
via cfe-commits
cfe-commits at lists.llvm.org
Thu Apr 25 00:24:13 PDT 2024
Author: martinboehme
Date: 2024-04-25T09:24:08+02:00
New Revision: b9208ce318907b1a5ea4ad0d2aa4414dfba0616c
URL: https://github.com/llvm/llvm-project/commit/b9208ce318907b1a5ea4ad0d2aa4414dfba0616c
DIFF: https://github.com/llvm/llvm-project/commit/b9208ce318907b1a5ea4ad0d2aa4414dfba0616c.diff
LOG: [clang][dataflow] Crash fix for `widenDistinctValues()`. (#89895)
We used to crash if the previous iteration contained a `BoolValue` and
the
current iteration contained an `IntegerValue`. The accompanying test
sets up
this situation -- see comments there for details.
While I'm here, clean up the tests for integral casts to use the test
helpers we
have available now. I was looking at these tests to understand how we
handle
integral casts, and the test helpers make the tests easier to read.
Added:
Modified:
clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
Removed:
################################################################################
diff --git a/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp b/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
index 636d2302093983..d79e734402892a 100644
--- a/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
+++ b/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
@@ -157,7 +157,13 @@ static WidenResult widenDistinctValues(QualType Type, Value &Prev,
Value &Current, Environment &CurrentEnv,
Environment::ValueModel &Model) {
// Boolean-model widening.
- if (auto *PrevBool = dyn_cast<BoolValue>(&Prev)) {
+ if (isa<BoolValue>(Prev) && isa<BoolValue>(Current)) {
+ // FIXME: Checking both values should be unnecessary, but we can currently
+ // end up with `BoolValue`s in integer-typed variables. See comment in
+ // `joinDistinctValues()` for details.
+ auto &PrevBool = cast<BoolValue>(Prev);
+ auto &CurBool = cast<BoolValue>(Current);
+
if (isa<TopBoolValue>(Prev))
// Safe to return `Prev` here, because Top is never dependent on the
// environment.
@@ -166,13 +172,12 @@ static WidenResult widenDistinctValues(QualType Type, Value &Prev,
// We may need to widen to Top, but before we do so, check whether both
// values are implied to be either true or false in the current environment.
// In that case, we can simply return a literal instead.
- auto &CurBool = cast<BoolValue>(Current);
- bool TruePrev = PrevEnv.proves(PrevBool->formula());
+ bool TruePrev = PrevEnv.proves(PrevBool.formula());
bool TrueCur = CurrentEnv.proves(CurBool.formula());
if (TruePrev && TrueCur)
return {&CurrentEnv.getBoolLiteralValue(true), LatticeEffect::Unchanged};
if (!TruePrev && !TrueCur &&
- PrevEnv.proves(PrevEnv.arena().makeNot(PrevBool->formula())) &&
+ PrevEnv.proves(PrevEnv.arena().makeNot(PrevBool.formula())) &&
CurrentEnv.proves(CurrentEnv.arena().makeNot(CurBool.formula())))
return {&CurrentEnv.getBoolLiteralValue(false), LatticeEffect::Unchanged};
diff --git a/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp b/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
index 5c7c39e52612ec..d204700919d315 100644
--- a/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
+++ b/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
@@ -3370,20 +3370,11 @@ TEST(TransferTest, IntegralCast) {
Code,
[](const llvm::StringMap<DataflowAnalysisState<NoopLattice>> &Results,
ASTContext &ASTCtx) {
- ASSERT_THAT(Results.keys(), UnorderedElementsAre("p"));
const Environment &Env = getEnvironmentAtAnnotation(Results, "p");
- const ValueDecl *FooDecl = findValueDecl(ASTCtx, "Foo");
- ASSERT_THAT(FooDecl, NotNull());
-
- const ValueDecl *BarDecl = findValueDecl(ASTCtx, "Bar");
- ASSERT_THAT(BarDecl, NotNull());
-
- const auto *FooVal = Env.getValue(*FooDecl);
- const auto *BarVal = Env.getValue(*BarDecl);
- EXPECT_TRUE(isa<IntegerValue>(FooVal));
- EXPECT_TRUE(isa<IntegerValue>(BarVal));
- EXPECT_EQ(FooVal, BarVal);
+ const auto &FooVal = getValueForDecl<IntegerValue>(ASTCtx, Env, "Foo");
+ const auto &BarVal = getValueForDecl<IntegerValue>(ASTCtx, Env, "Bar");
+ EXPECT_EQ(&FooVal, &BarVal);
});
}
@@ -3398,17 +3389,10 @@ TEST(TransferTest, IntegraltoBooleanCast) {
Code,
[](const llvm::StringMap<DataflowAnalysisState<NoopLattice>> &Results,
ASTContext &ASTCtx) {
- ASSERT_THAT(Results.keys(), UnorderedElementsAre("p"));
const Environment &Env = getEnvironmentAtAnnotation(Results, "p");
- const ValueDecl *FooDecl = findValueDecl(ASTCtx, "Foo");
- ASSERT_THAT(FooDecl, NotNull());
-
- const ValueDecl *BarDecl = findValueDecl(ASTCtx, "Bar");
- ASSERT_THAT(BarDecl, NotNull());
-
- const auto *FooVal = Env.getValue(*FooDecl);
- const auto *BarVal = Env.getValue(*BarDecl);
+ const auto &FooVal = getValueForDecl(ASTCtx, Env, "Foo");
+ const auto &BarVal = getValueForDecl(ASTCtx, Env, "Bar");
EXPECT_TRUE(isa<IntegerValue>(FooVal));
EXPECT_TRUE(isa<BoolValue>(BarVal));
});
@@ -3426,23 +3410,38 @@ TEST(TransferTest, IntegralToBooleanCastFromBool) {
Code,
[](const llvm::StringMap<DataflowAnalysisState<NoopLattice>> &Results,
ASTContext &ASTCtx) {
- ASSERT_THAT(Results.keys(), UnorderedElementsAre("p"));
const Environment &Env = getEnvironmentAtAnnotation(Results, "p");
- const ValueDecl *FooDecl = findValueDecl(ASTCtx, "Foo");
- ASSERT_THAT(FooDecl, NotNull());
-
- const ValueDecl *BarDecl = findValueDecl(ASTCtx, "Bar");
- ASSERT_THAT(BarDecl, NotNull());
-
- const auto *FooVal = Env.getValue(*FooDecl);
- const auto *BarVal = Env.getValue(*BarDecl);
- EXPECT_TRUE(isa<BoolValue>(FooVal));
- EXPECT_TRUE(isa<BoolValue>(BarVal));
- EXPECT_EQ(FooVal, BarVal);
+ const auto &FooVal = getValueForDecl<BoolValue>(ASTCtx, Env, "Foo");
+ const auto &BarVal = getValueForDecl<BoolValue>(ASTCtx, Env, "Bar");
+ EXPECT_EQ(&FooVal, &BarVal);
});
}
+TEST(TransferTest, WidenBoolValueInIntegerVariable) {
+ // This is a crash repro.
+ // This test sets up a case where we perform widening on an integer variable
+ // that contains a `BoolValue` for the previous iteration and an
+ // `IntegerValue` for the current iteration. We used to crash on this because
+ // `widenDistinctValues()` assumed that if the previous iteration had a
+ // `BoolValue`, the current iteration would too.
+ // FIXME: The real fix here is to make sure we never store `BoolValue`s in
+ // integer variables; see also the comment in `widenDistinctValues()`.
+ std::string Code = R"cc(
+ struct S {
+ int i;
+ S *next;
+ };
+ void target(S *s) {
+ for (; s; s = s->next)
+ s->i = false;
+ }
+ )cc";
+ runDataflow(Code,
+ [](const llvm::StringMap<DataflowAnalysisState<NoopLattice>> &,
+ ASTContext &) {});
+}
+
TEST(TransferTest, NullToPointerCast) {
std::string Code = R"(
using my_nullptr_t = decltype(nullptr);
More information about the cfe-commits
mailing list