[clang] [clang][dataflow] Process terminator condition within `transferCFGBlock()`. (PR #77750)
martinboehme wrote: Reverting: Causes build bots to fail because `TerminatorVisitor::StmtToEnv` is now unused. https://github.com/llvm/llvm-project/pull/77750 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][dataflow] Process terminator condition within `transferCFGBlock()`. (PR #77750)
https://github.com/martinboehme closed https://github.com/llvm/llvm-project/pull/77750 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][dataflow] Process terminator condition within `transferCFGBlock()`. (PR #77750)
@@ -489,6 +482,31 @@ transferCFGBlock(const CFGBlock , AnalysisContext
,
}
AC.Log.recordState(State);
}
+
+ // If we have a terminator, evaluate its condition.
+ // This `Expr` may not appear as a `CFGElement` anywhere else, and it's
+ // important that we evaluate it here (rather than while processing the
+ // terminator) so that we put the corresponding value in the right
+ // environment.
+ if (const Expr *TerminatorCond =
+ dyn_cast_or_null(Block.getTerminatorCondition())) {
+if (State.Env.getValue(*TerminatorCond) == nullptr)
+ // FIXME: This only runs the builtin transfer, not the analysis-specific
+ // transfer. Fixing this isn't trivial, as the analysis-specific transfer
+ // takes a `CFGElement` as input, but some expressions only show up as a
+ // terminator condition, but not as a `CFGElement`. The condition of an
if
martinboehme wrote:
Hm, that's an idea!
I'd still like to leave this to a future patch, as it's a further change in
behavior. Note that the code we had so far also only ran the builtin transfer;
this FIXME just documents this previously undocumented deficiency, so we're not
regressing in any way.
I'd like to make this change separately so that we can separate out any
unwanted side-effects of the two changes.
https://github.com/llvm/llvm-project/pull/77750
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][dataflow] Process terminator condition within `transferCFGBlock()`. (PR #77750)
https://github.com/Xazax-hun approved this pull request. Once @ymand's comment is resolved, it looks good to me. Thanks! https://github.com/llvm/llvm-project/pull/77750 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][dataflow] Process terminator condition within `transferCFGBlock()`. (PR #77750)
https://github.com/ymand approved this pull request. https://github.com/llvm/llvm-project/pull/77750 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][dataflow] Process terminator condition within `transferCFGBlock()`. (PR #77750)
https://github.com/ymand edited https://github.com/llvm/llvm-project/pull/77750 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][dataflow] Process terminator condition within `transferCFGBlock()`. (PR #77750)
@@ -489,6 +482,31 @@ transferCFGBlock(const CFGBlock , AnalysisContext
,
}
AC.Log.recordState(State);
}
+
+ // If we have a terminator, evaluate its condition.
+ // This `Expr` may not appear as a `CFGElement` anywhere else, and it's
+ // important that we evaluate it here (rather than while processing the
+ // terminator) so that we put the corresponding value in the right
+ // environment.
+ if (const Expr *TerminatorCond =
+ dyn_cast_or_null(Block.getTerminatorCondition())) {
+if (State.Env.getValue(*TerminatorCond) == nullptr)
+ // FIXME: This only runs the builtin transfer, not the analysis-specific
+ // transfer. Fixing this isn't trivial, as the analysis-specific transfer
+ // takes a `CFGElement` as input, but some expressions only show up as a
+ // terminator condition, but not as a `CFGElement`. The condition of an
if
ymand wrote:
CFGElement is cheap to construct. Can you just wrap the expression and pass it
on?
https://github.com/llvm/llvm-project/pull/77750
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][dataflow] Process terminator condition within `transferCFGBlock()`. (PR #77750)
https://github.com/martinboehme updated
https://github.com/llvm/llvm-project/pull/77750
>From 74ad27d843947e17d1cce38bee5a1bff2d9045f7 Mon Sep 17 00:00:00 2001
From: Martin Braenne
Date: Thu, 11 Jan 2024 10:47:41 +
Subject: [PATCH] [clang][dataflow] Process terminator condition within
`transferCFGBlock()`.
In particular, it's important that we create the "fallback" atomic at this point
(which we produce if the transfer function didn't produce a value for the
expression) so that it is placed in the correct environment.
Previously, we processed the terminator condition in the `TerminatorVisitor`,
which put the fallback atomic in a copy of the environment that is produced as
input for the _successor_ block, rather than the environment for the block
containing the expression for which we produce the fallback atomic.
As a result, we produce different fallback atomics every time we process the
successor block, and hence we don't have a consistent representation of the
terminator condition in the flow condition.
This patch includes a test (authored by ymand@) that fails without the fix.
---
.../TypeErasedDataflowAnalysis.cpp| 42 +--
.../Analysis/FlowSensitive/LoggerTest.cpp | 1 +
.../Analysis/FlowSensitive/TransferTest.cpp | 31 ++
3 files changed, 62 insertions(+), 12 deletions(-)
diff --git a/clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp
b/clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp
index faf83a8920d4ea..fdb2aeacf063ab 100644
--- a/clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp
+++ b/clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp
@@ -126,19 +126,12 @@ class TerminatorVisitor
private:
TerminatorVisitorRetTy extendFlowCondition(const Expr ) {
-// The terminator sub-expression might not be evaluated.
-if (Env.getValue(Cond) == nullptr)
- transfer(StmtToEnv, Cond, Env);
-
auto *Val = Env.get(Cond);
-// Value merging depends on flow conditions from different environments
-// being mutually exclusive -- that is, they cannot both be true in their
-// entirety (even if they may share some clauses). So, we need *some* value
-// for the condition expression, even if just an atom.
-if (Val == nullptr) {
- Val = ();
- Env.setValue(Cond, *Val);
-}
+// In transferCFGBlock(), we ensure that we always have a `Value` for the
+// terminator condition, so assert this.
+// We consciously assert ourselves instead of asserting via `cast()` so
+// that we get a more meaningful line number if the assertion fails.
+assert(Val != nullptr);
bool ConditionValue = true;
// The condition must be inverted for the successor that encompasses the
@@ -489,6 +482,31 @@ transferCFGBlock(const CFGBlock , AnalysisContext
,
}
AC.Log.recordState(State);
}
+
+ // If we have a terminator, evaluate its condition.
+ // This `Expr` may not appear as a `CFGElement` anywhere else, and it's
+ // important that we evaluate it here (rather than while processing the
+ // terminator) so that we put the corresponding value in the right
+ // environment.
+ if (const Expr *TerminatorCond =
+ dyn_cast_or_null(Block.getTerminatorCondition())) {
+if (State.Env.getValue(*TerminatorCond) == nullptr)
+ // FIXME: This only runs the builtin transfer, not the analysis-specific
+ // transfer. Fixing this isn't trivial, as the analysis-specific transfer
+ // takes a `CFGElement` as input, but some expressions only show up as a
+ // terminator condition, but not as a `CFGElement`. The condition of an
if
+ // statement is one such example.
+ transfer(StmtToEnvMap(AC.CFCtx, AC.BlockStates), *TerminatorCond,
+ State.Env);
+
+// If the transfer function didn't produce a value, create an atom so that
+// we have *some* value for the condition expression. This ensures that
+// when we extend the flow condition, it actually changes.
+if (State.Env.getValue(*TerminatorCond) == nullptr)
+ State.Env.setValue(*TerminatorCond, State.Env.makeAtomicBoolValue());
+AC.Log.recordState(State);
+ }
+
return State;
}
diff --git a/clang/unittests/Analysis/FlowSensitive/LoggerTest.cpp
b/clang/unittests/Analysis/FlowSensitive/LoggerTest.cpp
index a60dbe1f61f6d6..c5594aa3fe9d1f 100644
--- a/clang/unittests/Analysis/FlowSensitive/LoggerTest.cpp
+++ b/clang/unittests/Analysis/FlowSensitive/LoggerTest.cpp
@@ -123,6 +123,7 @@ recordState(Elements=1, Branches=0, Joins=0)
enterElement(b (ImplicitCastExpr, LValueToRValue, _Bool))
transfer()
recordState(Elements=2, Branches=0, Joins=0)
+recordState(Elements=2, Branches=0, Joins=0)
enterBlock(3, false)
transferBranch(0)
diff --git a/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
b/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
index 056c4f3383d832..6d88e25f77c807 100644
---
[clang] [clang][dataflow] Process terminator condition within `transferCFGBlock()`. (PR #77750)
github-actions[bot] wrote:
:warning: C/C++ code formatter, clang-format found issues in your code.
:warning:
You can test this locally with the following command:
``bash
git-clang-format --diff cc21aa1922b3d0c4fde52046d8d16d1048f8064e
732a0b343b24eee4bb5f17e4c2edbe7268aa8955 --
clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp
clang/unittests/Analysis/FlowSensitive/LoggerTest.cpp
clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
``
View the diff from clang-format here.
``diff
diff --git a/clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp
b/clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp
index b2b1acd288..fdb2aeacf0 100644
--- a/clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp
+++ b/clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp
@@ -488,8 +488,8 @@ transferCFGBlock(const CFGBlock , AnalysisContext ,
// important that we evaluate it here (rather than while processing the
// terminator) so that we put the corresponding value in the right
// environment.
- if (const Expr *TerminatorCond = dyn_cast_or_null(
- Block.getTerminatorCondition())) {
+ if (const Expr *TerminatorCond =
+ dyn_cast_or_null(Block.getTerminatorCondition())) {
if (State.Env.getValue(*TerminatorCond) == nullptr)
// FIXME: This only runs the builtin transfer, not the analysis-specific
// transfer. Fixing this isn't trivial, as the analysis-specific transfer
``
https://github.com/llvm/llvm-project/pull/77750
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][dataflow] Process terminator condition within `transferCFGBlock()`. (PR #77750)
llvmbot wrote:
@llvm/pr-subscribers-clang
Author: None (martinboehme)
Changes
In particular, it's important that we create the "fallback" atomic at this point
(which we produce if the transfer function didn't produce a value for the
expression) so that it is placed in the correct environment.
Previously, we processed the terminator condition in the `TerminatorVisitor`,
which put the fallback atomic in a copy of the environment that is produced as
input for the _successor_ block, rather than the environment for the block
containing the expression for which we produce the fallback atomic.
As a result, we produce different fallback atomics every time we process the
successor block, and hence we don't have a consistent representation of the
terminator condition in the flow condition.
This patch includes a test (authored by ymand@) that fails without the fix.
---
Full diff: https://github.com/llvm/llvm-project/pull/77750.diff
3 Files Affected:
- (modified) clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp
(+30-12)
- (modified) clang/unittests/Analysis/FlowSensitive/LoggerTest.cpp (+1)
- (modified) clang/unittests/Analysis/FlowSensitive/TransferTest.cpp (+31)
``diff
diff --git a/clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp
b/clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp
index faf83a8920d4ea..b2b1acd288bd9f 100644
--- a/clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp
+++ b/clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp
@@ -126,19 +126,12 @@ class TerminatorVisitor
private:
TerminatorVisitorRetTy extendFlowCondition(const Expr ) {
-// The terminator sub-expression might not be evaluated.
-if (Env.getValue(Cond) == nullptr)
- transfer(StmtToEnv, Cond, Env);
-
auto *Val = Env.get(Cond);
-// Value merging depends on flow conditions from different environments
-// being mutually exclusive -- that is, they cannot both be true in their
-// entirety (even if they may share some clauses). So, we need *some* value
-// for the condition expression, even if just an atom.
-if (Val == nullptr) {
- Val = ();
- Env.setValue(Cond, *Val);
-}
+// In transferCFGBlock(), we ensure that we always have a `Value` for the
+// terminator condition, so assert this.
+// We consciously assert ourselves instead of asserting via `cast()` so
+// that we get a more meaningful line number if the assertion fails.
+assert(Val != nullptr);
bool ConditionValue = true;
// The condition must be inverted for the successor that encompasses the
@@ -489,6 +482,31 @@ transferCFGBlock(const CFGBlock , AnalysisContext
,
}
AC.Log.recordState(State);
}
+
+ // If we have a terminator, evaluate its condition.
+ // This `Expr` may not appear as a `CFGElement` anywhere else, and it's
+ // important that we evaluate it here (rather than while processing the
+ // terminator) so that we put the corresponding value in the right
+ // environment.
+ if (const Expr *TerminatorCond = dyn_cast_or_null(
+ Block.getTerminatorCondition())) {
+if (State.Env.getValue(*TerminatorCond) == nullptr)
+ // FIXME: This only runs the builtin transfer, not the analysis-specific
+ // transfer. Fixing this isn't trivial, as the analysis-specific transfer
+ // takes a `CFGElement` as input, but some expressions only show up as a
+ // terminator condition, but not as a `CFGElement`. The condition of an
if
+ // statement is one such example.
+ transfer(StmtToEnvMap(AC.CFCtx, AC.BlockStates), *TerminatorCond,
+ State.Env);
+
+// If the transfer function didn't produce a value, create an atom so that
+// we have *some* value for the condition expression. This ensures that
+// when we extend the flow condition, it actually changes.
+if (State.Env.getValue(*TerminatorCond) == nullptr)
+ State.Env.setValue(*TerminatorCond, State.Env.makeAtomicBoolValue());
+AC.Log.recordState(State);
+ }
+
return State;
}
diff --git a/clang/unittests/Analysis/FlowSensitive/LoggerTest.cpp
b/clang/unittests/Analysis/FlowSensitive/LoggerTest.cpp
index a60dbe1f61f6d6..c5594aa3fe9d1f 100644
--- a/clang/unittests/Analysis/FlowSensitive/LoggerTest.cpp
+++ b/clang/unittests/Analysis/FlowSensitive/LoggerTest.cpp
@@ -123,6 +123,7 @@ recordState(Elements=1, Branches=0, Joins=0)
enterElement(b (ImplicitCastExpr, LValueToRValue, _Bool))
transfer()
recordState(Elements=2, Branches=0, Joins=0)
+recordState(Elements=2, Branches=0, Joins=0)
enterBlock(3, false)
transferBranch(0)
diff --git a/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
b/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
index 056c4f3383d832..6d88e25f77c807 100644
--- a/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
+++ b/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
@@ -6408,4 +6408,35 @@ TEST(TransferTest,
[clang] [clang][dataflow] Process terminator condition within `transferCFGBlock()`. (PR #77750)
https://github.com/martinboehme created
https://github.com/llvm/llvm-project/pull/77750
In particular, it's important that we create the "fallback" atomic at this point
(which we produce if the transfer function didn't produce a value for the
expression) so that it is placed in the correct environment.
Previously, we processed the terminator condition in the `TerminatorVisitor`,
which put the fallback atomic in a copy of the environment that is produced as
input for the _successor_ block, rather than the environment for the block
containing the expression for which we produce the fallback atomic.
As a result, we produce different fallback atomics every time we process the
successor block, and hence we don't have a consistent representation of the
terminator condition in the flow condition.
This patch includes a test (authored by ymand@) that fails without the fix.
>From 732a0b343b24eee4bb5f17e4c2edbe7268aa8955 Mon Sep 17 00:00:00 2001
From: Martin Braenne
Date: Thu, 11 Jan 2024 10:42:23 +
Subject: [PATCH] [clang][dataflow] Process terminator condition within
`transferCFGBlock()`.
In particular, it's important that we create the "fallback" atomic at this point
(which we produce if the transfer function didn't produce a value for the
expression) so that it is placed in the correct environment.
Previously, we processed the terminator condition in the `TerminatorVisitor`,
which put the fallback atomic in a copy of the environment that is produced as
input for the _successor_ block, rather than the environment for the block
containing the expression for which we produce the fallback atomic.
As a result, we produce different fallback atomics every time we process the
successor block, and hence we don't have a consistent representation of the
terminator condition in the flow condition.
This patch includes a test (authored by ymand@) that fails without the fix.
---
.../TypeErasedDataflowAnalysis.cpp| 42 +--
.../Analysis/FlowSensitive/LoggerTest.cpp | 1 +
.../Analysis/FlowSensitive/TransferTest.cpp | 31 ++
3 files changed, 62 insertions(+), 12 deletions(-)
diff --git a/clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp
b/clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp
index faf83a8920d4ea..b2b1acd288bd9f 100644
--- a/clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp
+++ b/clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp
@@ -126,19 +126,12 @@ class TerminatorVisitor
private:
TerminatorVisitorRetTy extendFlowCondition(const Expr ) {
-// The terminator sub-expression might not be evaluated.
-if (Env.getValue(Cond) == nullptr)
- transfer(StmtToEnv, Cond, Env);
-
auto *Val = Env.get(Cond);
-// Value merging depends on flow conditions from different environments
-// being mutually exclusive -- that is, they cannot both be true in their
-// entirety (even if they may share some clauses). So, we need *some* value
-// for the condition expression, even if just an atom.
-if (Val == nullptr) {
- Val = ();
- Env.setValue(Cond, *Val);
-}
+// In transferCFGBlock(), we ensure that we always have a `Value` for the
+// terminator condition, so assert this.
+// We consciously assert ourselves instead of asserting via `cast()` so
+// that we get a more meaningful line number if the assertion fails.
+assert(Val != nullptr);
bool ConditionValue = true;
// The condition must be inverted for the successor that encompasses the
@@ -489,6 +482,31 @@ transferCFGBlock(const CFGBlock , AnalysisContext
,
}
AC.Log.recordState(State);
}
+
+ // If we have a terminator, evaluate its condition.
+ // This `Expr` may not appear as a `CFGElement` anywhere else, and it's
+ // important that we evaluate it here (rather than while processing the
+ // terminator) so that we put the corresponding value in the right
+ // environment.
+ if (const Expr *TerminatorCond = dyn_cast_or_null(
+ Block.getTerminatorCondition())) {
+if (State.Env.getValue(*TerminatorCond) == nullptr)
+ // FIXME: This only runs the builtin transfer, not the analysis-specific
+ // transfer. Fixing this isn't trivial, as the analysis-specific transfer
+ // takes a `CFGElement` as input, but some expressions only show up as a
+ // terminator condition, but not as a `CFGElement`. The condition of an
if
+ // statement is one such example.
+ transfer(StmtToEnvMap(AC.CFCtx, AC.BlockStates), *TerminatorCond,
+ State.Env);
+
+// If the transfer function didn't produce a value, create an atom so that
+// we have *some* value for the condition expression. This ensures that
+// when we extend the flow condition, it actually changes.
+if (State.Env.getValue(*TerminatorCond) == nullptr)
+ State.Env.setValue(*TerminatorCond, State.Env.makeAtomicBoolValue());
+
