[clang] [clang][dataflow] Check for backedges directly (instead of loop statements). (PR #68923)
Yitzhak Mandelbaum via cfe-commits
cfe-commits at lists.llvm.org
Mon Oct 16 10:50:43 PDT 2023
https://github.com/ymand updated https://github.com/llvm/llvm-project/pull/68923
>From 83486a35e6d0e754dd99369fc546d04afedbe923 Mon Sep 17 00:00:00 2001
From: Yitzhak Mandelbaum <yitzhakm at google.com>
Date: Thu, 12 Oct 2023 19:35:54 +0000
Subject: [PATCH 1/3] [clang][dataflow] Check for backedges directly (instead
of loop statements).
Widen on backedge nodes, instead of nodes with a loop statement as terminator.
This fixes #67834 and a precision loss from assignment in a loop condition. The
commit contains tests for both of these issues.
---
.../TypeErasedDataflowAnalysis.cpp | 29 ++++++-------------
.../Analysis/FlowSensitive/TransferTest.cpp | 14 +++++++++
.../TypeErasedDataflowAnalysisTest.cpp | 28 ++++++++++++++++++
3 files changed, 51 insertions(+), 20 deletions(-)
diff --git a/clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp b/clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp
index 6b167891c1a3ac8..5da3a5b1ccf8a2c 100644
--- a/clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp
+++ b/clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp
@@ -12,7 +12,6 @@
//===----------------------------------------------------------------------===//
#include <algorithm>
-#include <memory>
#include <optional>
#include <system_error>
#include <utility>
@@ -33,8 +32,8 @@
#include "clang/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.h"
#include "clang/Analysis/FlowSensitive/Value.h"
#include "llvm/ADT/ArrayRef.h"
-#include "llvm/ADT/DenseSet.h"
#include "llvm/ADT/STLExtras.h"
+#include "llvm/ADT/SmallBitVector.h"
#include "llvm/Support/Debug.h"
#include "llvm/Support/Error.h"
@@ -53,19 +52,8 @@ static int blockIndexInPredecessor(const CFGBlock &Pred,
return BlockPos - Pred.succ_begin();
}
-static bool isLoopHead(const CFGBlock &B) {
- if (const auto *T = B.getTerminatorStmt())
- switch (T->getStmtClass()) {
- case Stmt::WhileStmtClass:
- case Stmt::DoStmtClass:
- case Stmt::ForStmtClass:
- case Stmt::CXXForRangeStmtClass:
- return true;
- default:
- return false;
- }
-
- return false;
+static bool isBackedgeNode(const CFGBlock &B) {
+ return B.getLoopTarget() != nullptr;
}
namespace {
@@ -502,14 +490,15 @@ runTypeErasedDataflowAnalysis(
PostVisitCFG) {
PrettyStackTraceAnalysis CrashInfo(CFCtx, "runTypeErasedDataflowAnalysis");
- PostOrderCFGView POV(&CFCtx.getCFG());
- ForwardDataflowWorklist Worklist(CFCtx.getCFG(), &POV);
+ const clang::CFG &CFG = CFCtx.getCFG();
+ PostOrderCFGView POV(&CFG);
+ ForwardDataflowWorklist Worklist(CFG, &POV);
std::vector<std::optional<TypeErasedDataflowAnalysisState>> BlockStates(
- CFCtx.getCFG().size());
+ CFG.size());
// The entry basic block doesn't contain statements so it can be skipped.
- const CFGBlock &Entry = CFCtx.getCFG().getEntry();
+ const CFGBlock &Entry = CFG.getEntry();
BlockStates[Entry.getBlockID()] = {Analysis.typeErasedInitialElement(),
InitEnv.fork()};
Worklist.enqueueSuccessors(&Entry);
@@ -553,7 +542,7 @@ runTypeErasedDataflowAnalysis(
llvm::errs() << "Old Env:\n";
OldBlockState->Env.dump();
});
- if (isLoopHead(*Block)) {
+ if (isBackedgeNode(*Block)) {
LatticeJoinEffect Effect1 = Analysis.widenTypeErased(
NewBlockState.Lattice, OldBlockState->Lattice);
LatticeJoinEffect Effect2 =
diff --git a/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp b/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
index 632632a1b30e78b..7c697fa5f87d941 100644
--- a/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
+++ b/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
@@ -4099,6 +4099,20 @@ TEST(TransferTest, LoopDereferencingChangingRecordPointerConverges) {
ASSERT_THAT_ERROR(checkDataflowWithNoopAnalysis(Code), llvm::Succeeded());
}
+TEST(TransferTest, LoopWithDisjunctiveConditionConverges) {
+ std::string Code = R"cc(
+ bool foo();
+
+ void target() {
+ bool c = false;
+ while (foo() || foo()) {
+ c = true;
+ }
+ }
+ )cc";
+ ASSERT_THAT_ERROR(checkDataflowWithNoopAnalysis(Code), llvm::Succeeded());
+}
+
TEST(TransferTest, DoesNotCrashOnUnionThisExpr) {
std::string Code = R"(
union Union {
diff --git a/clang/unittests/Analysis/FlowSensitive/TypeErasedDataflowAnalysisTest.cpp b/clang/unittests/Analysis/FlowSensitive/TypeErasedDataflowAnalysisTest.cpp
index 2425bb8711bdbaf..6800d736afd9b08 100644
--- a/clang/unittests/Analysis/FlowSensitive/TypeErasedDataflowAnalysisTest.cpp
+++ b/clang/unittests/Analysis/FlowSensitive/TypeErasedDataflowAnalysisTest.cpp
@@ -913,6 +913,34 @@ TEST_F(FlowConditionTest, WhileStmt) {
});
}
+TEST_F(FlowConditionTest, WhileStmtWithAssignmentInCondition) {
+ std::string Code = R"(
+ void target(bool Foo) {
+ // This test checks whether the analysis preserves the connection between
+ // the value of `Foo` and the assignment expression, despite widening.
+ // The equality operator generates a fresh boolean variable on each
+ // interpretation, which forces use of widening.
+ while ((Foo = (3 == 4))) {
+ (void)0;
+ /*[[p]]*/
+ }
+ }
+ )";
+ runDataflow(
+ Code,
+ [](const llvm::StringMap<DataflowAnalysisState<NoopLattice>> &Results,
+ ASTContext &ASTCtx) {
+ const ValueDecl *FooDecl = findValueDecl(ASTCtx, "Foo");
+ ASSERT_THAT(FooDecl, NotNull());
+
+ ASSERT_THAT(Results.keys(), UnorderedElementsAre("p"));
+ const Environment &Env = getEnvironmentAtAnnotation(Results, "p");
+
+ auto &FooVal = cast<BoolValue>(Env.getValue(*FooDecl))->formula();
+ EXPECT_TRUE(Env.flowConditionImplies(FooVal));
+ });
+}
+
TEST_F(FlowConditionTest, Conjunction) {
std::string Code = R"(
void target(bool Foo, bool Bar) {
>From bdda46db3376cffbceed2de392783456b4fcc62c Mon Sep 17 00:00:00 2001
From: Yitzhak Mandelbaum <yitzhakm at google.com>
Date: Thu, 12 Oct 2023 20:36:38 +0000
Subject: [PATCH 2/3] fixup! [clang][dataflow] Check for backedges directly
(instead of loop statements).
---
clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp b/clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp
index 5da3a5b1ccf8a2c..d96ad66dd6749c4 100644
--- a/clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp
+++ b/clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp
@@ -53,7 +53,7 @@ static int blockIndexInPredecessor(const CFGBlock &Pred,
}
static bool isBackedgeNode(const CFGBlock &B) {
- return B.getLoopTarget() != nullptr;
+ return B.getLoopTarget() != nullptr;
}
namespace {
>From f6edf027c22ee992983fc4af6257c5b5fb87f0a7 Mon Sep 17 00:00:00 2001
From: Yitzhak Mandelbaum <yitzhakm at google.com>
Date: Mon, 16 Oct 2023 17:50:13 +0000
Subject: [PATCH 3/3] fixup! fixup! [clang][dataflow] Check for backedges
directly (instead of loop statements).
---
.../Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp | 6 ++++++
clang/unittests/Analysis/FlowSensitive/TransferTest.cpp | 2 +-
.../FlowSensitive/TypeErasedDataflowAnalysisTest.cpp | 7 +------
3 files changed, 8 insertions(+), 7 deletions(-)
diff --git a/clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp b/clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp
index d96ad66dd6749c4..72d807fc36705da 100644
--- a/clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp
+++ b/clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp
@@ -52,6 +52,12 @@ static int blockIndexInPredecessor(const CFGBlock &Pred,
return BlockPos - Pred.succ_begin();
}
+// A "backedge" node is a block introduced in the CFG exclusively to indicate a
+// loop backedge. They are exactly identified by the presence of a non-null
+// pointer to the entry block of the loop condition. Note that this is not
+// necessarily the block with the loop statement as terminator, because
+// short-circuit operators will result in multiple blocks encoding the loop
+// condition, only one of which will contain the loop statement as terminator.
static bool isBackedgeNode(const CFGBlock &B) {
return B.getLoopTarget() != nullptr;
}
diff --git a/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp b/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
index 7c697fa5f87d941..ea36a3f705ee934 100644
--- a/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
+++ b/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
@@ -4099,7 +4099,7 @@ TEST(TransferTest, LoopDereferencingChangingRecordPointerConverges) {
ASSERT_THAT_ERROR(checkDataflowWithNoopAnalysis(Code), llvm::Succeeded());
}
-TEST(TransferTest, LoopWithDisjunctiveConditionConverges) {
+TEST(TransferTest, LoopWithShortCircuitedConditionConverges) {
std::string Code = R"cc(
bool foo();
diff --git a/clang/unittests/Analysis/FlowSensitive/TypeErasedDataflowAnalysisTest.cpp b/clang/unittests/Analysis/FlowSensitive/TypeErasedDataflowAnalysisTest.cpp
index 6800d736afd9b08..cf4a8fdca73fb33 100644
--- a/clang/unittests/Analysis/FlowSensitive/TypeErasedDataflowAnalysisTest.cpp
+++ b/clang/unittests/Analysis/FlowSensitive/TypeErasedDataflowAnalysisTest.cpp
@@ -930,13 +930,8 @@ TEST_F(FlowConditionTest, WhileStmtWithAssignmentInCondition) {
Code,
[](const llvm::StringMap<DataflowAnalysisState<NoopLattice>> &Results,
ASTContext &ASTCtx) {
- const ValueDecl *FooDecl = findValueDecl(ASTCtx, "Foo");
- ASSERT_THAT(FooDecl, NotNull());
-
- ASSERT_THAT(Results.keys(), UnorderedElementsAre("p"));
const Environment &Env = getEnvironmentAtAnnotation(Results, "p");
-
- auto &FooVal = cast<BoolValue>(Env.getValue(*FooDecl))->formula();
+ auto &FooVal = getValueForDecl<BoolValue>(ASTCtx, Env, "Foo").formula();
EXPECT_TRUE(Env.flowConditionImplies(FooVal));
});
}
More information about the cfe-commits
mailing list