[clang] [clang][dataflow] Don't propagate result objects inside `decltype`. (PR #90438)
via cfe-commits
cfe-commits at lists.llvm.org
Mon Apr 29 01:05:11 PDT 2024
https://github.com/martinboehme created https://github.com/llvm/llvm-project/pull/90438
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).
>From 75a1198172f8e9fac6f83ec7e786fa108dbbfa3d Mon Sep 17 00:00:00 2001
From: Martin Braenne <mboehme at google.com>
Date: Mon, 29 Apr 2024 08:02:26 +0000
Subject: [PATCH] [clang][dataflow] Don't propagate result objects inside
`decltype`.
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).
---
.../FlowSensitive/DataflowEnvironment.cpp | 7 +++++++
.../Analysis/FlowSensitive/TransferTest.cpp | 17 +++++++++++++++++
2 files changed, 24 insertions(+)
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) {
More information about the cfe-commits
mailing list