[clang][dataflow] Don't propagate result objects in unevaluated contexts

[martinboehme]

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).

[llvmbot]

@llvm/pr-subscribers-clang-analysis

Author: None (martinboehme)

Changes

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). --- Full diff: https://github.com/llvm/llvm-project/pull/90438.diff 2 Files Affected: - (modified) clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp (+7) - (modified) clang/unittests/Analysis/FlowSensitive/TransferTest.cpp (+17) ``````````diff 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 { return RecursiveASTVisitor::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> &Results, + ASTContext &ASTCtx) {}); +} + TEST(TransferTest, StaticCast) { std::string Code = R"( void target(int Foo) { ``````````

[llvmbot]

@llvm/pr-subscribers-clang

Author: None (martinboehme)

Changes

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). --- Full diff: https://github.com/llvm/llvm-project/pull/90438.diff 2 Files Affected: - (modified) clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp (+7) - (modified) clang/unittests/Analysis/FlowSensitive/TransferTest.cpp (+17) ``````````diff 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 { return RecursiveASTVisitor::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> &Results, + ASTContext &ASTCtx) {}); +} + TEST(TransferTest, StaticCast) { std::string Code = R"( void target(int Foo) { ``````````

[martinboehme]

Windows CI failure is in Driver/amdgpu-toolchain.c. This looks unrelated.

[cHackz18]

Hello and good morning from the UK!

It looks as though this change caused a test failure on the following buildbot: https://lab.llvm.org/buildbot/#/builders/216/builds/38446

Are you able to take a look see?

many thanks for yoru time, Tom W

[TomWeaver18]

Reverted in 2252c5c42b95dd12dda0c7fb1b2811f943b21949

[martinboehme]

Hello and good morning from the UK!

It looks as though this change caused a test failure on the following buildbot: https://lab.llvm.org/buildbot/#/builders/216/builds/38446

Are you able to take a look see?

many thanks for yoru time, Tom W

Sorry, didn't see this in time.

Reverted in 2252c5c

Thanks for the revert!

Will investigate and reland with a fix.