[clang] [clang][dataflow] Crash fix for `widenDistinctValues()`. (PR #89895)
via cfe-commits
cfe-commits at lists.llvm.org
Wed Apr 24 01:56:18 PDT 2024
https://github.com/martinboehme created https://github.com/llvm/llvm-project/pull/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.
>From 359abc507a2ddfba3b31674c3bbaaf7a99e29053 Mon Sep 17 00:00:00 2001
From: Martin Braenne <mboehme at google.com>
Date: Wed, 24 Apr 2024 08:55:40 +0000
Subject: [PATCH] [clang][dataflow] Crash fix for `widenDistinctValues()`.
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.
---
.../FlowSensitive/DataflowEnvironment.cpp | 13 ++--
.../Analysis/FlowSensitive/TransferTest.cpp | 65 +++++++++----------
2 files changed, 41 insertions(+), 37 deletions(-)
diff --git a/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp b/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
index 3cb656adcbdc0c..9b6e579d38f0f2 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 215e208615ac23..ec53afcb3eb359 100644
--- a/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
+++ b/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
@@ -3348,20 +3348,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);
});
}
@@ -3376,17 +3367,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));
});
@@ -3404,23 +3388,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