[clang][dataflow] Don't propagate result objects in unevaluated contexts #90438
Conversation
llvmbot
added
clang
|
@llvm/pr-subscribers-clang-analysis Author: None (martinboehme) ChangesTrying to do so can cause crashes -- see newly added test and the comments in We're starting to see a repeating pattern here: We're getting crashes because I think we should ensure consistency between these two parts of the code by I'd like to focus this PR, however, on a targeted fix for the current crash and Full diff: https://github.com/llvm/llvm-project/pull/90438.diff 2 Files Affected:
diff --git a/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp b/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
index d79e734402892a..f32ee4a2528a8b 100644
--- a/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
+++ b/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
@@ -350,6 +350,13 @@ class ResultObjectVisitor : public RecursiveASTVisitor<ResultObjectVisitor> {
return RecursiveASTVisitor<ResultObjectVisitor>::TraverseDecl(D);
}
+ bool TraverseDecltypeTypeLoc(DecltypeTypeLoc Node) {
+ // Don't traverse `decltype` because we don't analyze its argument (as it
+ // isn't executed) and hence don't model fields that are only used in the
+ // argument of a `decltype`.
+ return true;
+ }
+
bool TraverseBindingDecl(BindingDecl *BD) {
// `RecursiveASTVisitor` doesn't traverse holding variables for
// `BindingDecl`s by itself, so we need to tell it to.
diff --git a/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp b/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
index 301bec32c0cf1d..3b8fde0eb244c1 100644
--- a/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
+++ b/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
@@ -3331,6 +3331,23 @@ TEST(TransferTest, ResultObjectLocationDontVisitNestedRecordDecl) {
ASTContext &ASTCtx) {});
}
+TEST(TransferTest, ResultObjectLocationDontVisitDeclTypeArg) {
+ // This is a crash repro.
+ // We used to crash because when propagating result objects, we would visit
+ // the argument of `decltype()`, but we don't model fields used only in these.
+ std::string Code = R"cc(
+ struct S1 {};
+ struct S2 { S1 s1; };
+ void target() {
+ decltype(S2{ S1() }) Dummy;
+ }
+ )cc";
+ runDataflow(
+ Code,
+ [](const llvm::StringMap<DataflowAnalysisState<NoopLattice>> &Results,
+ ASTContext &ASTCtx) {});
+}
+
TEST(TransferTest, StaticCast) {
std::string Code = R"(
void target(int Foo) {
Footnotes
|
|
@llvm/pr-subscribers-clang Author: None (martinboehme) ChangesTrying to do so can cause crashes -- see newly added test and the comments in We're starting to see a repeating pattern here: We're getting crashes because I think we should ensure consistency between these two parts of the code by I'd like to focus this PR, however, on a targeted fix for the current crash and Full diff: https://github.com/llvm/llvm-project/pull/90438.diff 2 Files Affected:
diff --git a/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp b/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
index d79e734402892a..f32ee4a2528a8b 100644
--- a/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
+++ b/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
@@ -350,6 +350,13 @@ class ResultObjectVisitor : public RecursiveASTVisitor<ResultObjectVisitor> {
return RecursiveASTVisitor<ResultObjectVisitor>::TraverseDecl(D);
}
+ bool TraverseDecltypeTypeLoc(DecltypeTypeLoc Node) {
+ // Don't traverse `decltype` because we don't analyze its argument (as it
+ // isn't executed) and hence don't model fields that are only used in the
+ // argument of a `decltype`.
+ return true;
+ }
+
bool TraverseBindingDecl(BindingDecl *BD) {
// `RecursiveASTVisitor` doesn't traverse holding variables for
// `BindingDecl`s by itself, so we need to tell it to.
diff --git a/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp b/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
index 301bec32c0cf1d..3b8fde0eb244c1 100644
--- a/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
+++ b/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
@@ -3331,6 +3331,23 @@ TEST(TransferTest, ResultObjectLocationDontVisitNestedRecordDecl) {
ASTContext &ASTCtx) {});
}
+TEST(TransferTest, ResultObjectLocationDontVisitDeclTypeArg) {
+ // This is a crash repro.
+ // We used to crash because when propagating result objects, we would visit
+ // the argument of `decltype()`, but we don't model fields used only in these.
+ std::string Code = R"cc(
+ struct S1 {};
+ struct S2 { S1 s1; };
+ void target() {
+ decltype(S2{ S1() }) Dummy;
+ }
+ )cc";
+ runDataflow(
+ Code,
+ [](const llvm::StringMap<DataflowAnalysisState<NoopLattice>> &Results,
+ ASTContext &ASTCtx) {});
+}
+
TEST(TransferTest, StaticCast) {
std::string Code = R"(
void target(int Foo) {
Footnotes
|
| @@ -350,6 +350,13 @@ class ResultObjectVisitor : public RecursiveASTVisitor<ResultObjectVisitor> { | |||
| return RecursiveASTVisitor<ResultObjectVisitor>::TraverseDecl(D); | |||
| } | |||
|
|
|||
| bool TraverseDecltypeTypeLoc(DecltypeTypeLoc Node) { | |||
There was a problem hiding this comment.
There are other instances in the language that are unevaluated including: typeid, noexcept, sizeof. Do you anticipate similar problems with those?
There was a problem hiding this comment.
There are other instances in the language that are unevaluated including:
typeid,noexcept,sizeof. Do you anticipate similar problems with those?
Excellent point, thanks. I'll expand this PR to more generally exclude all unevaluated contexts. Will ping this PR when I've changed this.
There was a problem hiding this comment.
Done -- ready for review again.
It turns out that for some of these (typeid, sizeof), the tests already passed even before I excluded them in ResultObjectVisitor. This is because getReferencedDecls() does actually collect declarations referenced in these unevaluated contexts (even though maybe it shouldn't). Nevertheless, I have decided to exclude these unevaluated contexts from ResultObjectVisitor because the transfer function will never visit them, so they don't need result objects to be propagated into them.
It also turns out that, even though the operand of the noexcept operator in an unevaluated operand, it still shows up in the CFG for some reason, and hence we also need to propagate result objects for it (see also comments added to the code). Maybe there's a reason for this, or maybe it's something that should be fixed in the CFG, but for the time being, I've chosen to leave this as it is here.
There was a problem hiding this comment.
It also turns out that, even though the operand of the noexcept operator in an unevaluated operand, it still shows up in the CFG for some reason
This surprises me, I can't think of a reason why we would want to have it in the CFG. This keyword appears a lot in generic code, so maybe excluding it from the CFG might have measurable perf/memory benefits. I'd think it might be worth to give this a try and see if something breaks (in a separate PR).
There was a problem hiding this comment.
Makes sense, I'll take a look at that!
|
Windows CI failure is in Driver/amdgpu-toolchain.c. This looks unrelated. |
…xts. Trying to do so can cause crashes -- see newly added test and the comments in the fix. We're starting to see a repeating pattern here: We're getting crashes because `ResultObjectVisitor` and `getReferencedDecls()` don't agree on which parts of the AST to visit and, hence, which fields should be modeled. I think we should ensure consistency between these two parts of the code by using a `RecursiveASTVisitor` in `getReferencedDecls()`[^1]; the `Traverse...()` functions that control which parts of the AST we visit would go in a common base class that would be used for both `ResultObjectVisitor` and `getReferencedDecls()`. I'd like to focus this PR, however, on a targeted fix for the current crash and postpone the refactoring to a later PR (which will be easier to revert if there are unintended side-effects). [^1]: As an added bonus, this would make the code better structured and more efficient than the current sequence of `if (dyn_cast<T>(...))` statements).
martinboehme
force-pushed
the
piper_export_cl_628985572
branch
from
75a1198 to
2cf2947
Compare
martinboehme
changed the title
decltype.
|
Hello and good morning from the UK! It looks as though this change caused a test failure on the following buildbot: Are you able to take a look see? many thanks for yoru time, |
…ed contexts (#90438)" This reverts commit 597a315. Caused test failure on the following buildbot: https://lab.llvm.org/buildbot/#/builders/216/builds/38446
|
Reverted in 2252c5c |
Sorry, didn't see this in time.
Thanks for the revert! Will investigate and reland with a fix. |
…ted contexts (llvm#90438)" This reverts commit 2252c5c.
…nevaluated contexts (llvm#90438)"
…xts (reland #90438) (#91172) This relands #90348 with a fix for a [buildbot failure](https://lab.llvm.org/buildbot/#/builders/216/builds/38446) caused by the test being run with `-fno-rtti`.
Trying to do so can cause crashes -- see newly added test and the comments in
the fix.
We're starting to see a repeating pattern here: We're getting crashes because
ResultObjectVisitorandgetReferencedDecls()don't agree on which parts ofthe AST to visit and, hence, which fields should be modeled.
I think we should ensure consistency between these two parts of the code by
using a
RecursiveASTVisitoringetReferencedDecls()1; theTraverse...()functions that control which parts of the AST we visit would goin a common base class that would be used for both
ResultObjectVisitorandgetReferencedDecls().I'd like to focus this PR, however, on a targeted fix for the current crash and
postpone the refactoring to a later PR (which will be easier to revert if there
are unintended side-effects).
Footnotes
As an added bonus, this would make the code better structured and more
efficient than the current sequence of
if (dyn_cast<T>(...))statements). ↩