[clang] 597a315 - [clang][dataflow] Don't propagate result objects in unevaluated contexts (#90438)

[via cfe-commits]

Author: martinboehme
Date: 2024-05-02T08:35:13+02:00
New Revision: 597a3150e932a9423c65b5ea4b53dd431aff5865

URL: https://github.com/llvm/llvm-project/commit/597a3150e932a9423c65b5ea4b53dd431aff5865
DIFF: https://github.com/llvm/llvm-project/commit/597a3150e932a9423c65b5ea4b53dd431aff5865.diff

LOG: [clang][dataflow] Don't propagate result objects in unevaluated contexts (#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).

Added: 
    

Modified: 
    clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
    clang/unittests/Analysis/FlowSensitive/TransferTest.cpp

Removed: 
    


################################################################################
diff  --git a/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp b/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
index d79e734402892a..cb6c8b2ef1072b 100644
--- a/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
+++ b/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
@@ -350,6 +350,17 @@ class ResultObjectVisitor : public RecursiveASTVisitor<ResultObjectVisitor> {
     return RecursiveASTVisitor<ResultObjectVisitor>::TraverseDecl(D);
   }
 
+  // Don't traverse expressions in unevaluated contexts, as we don't model
+  // fields that are only used in these.
+  // Note: The operand of the `noexcept` operator is an unevaluated operand, but
+  // nevertheless it appears in the Clang CFG, so we don't exclude it here.
+  bool TraverseDecltypeTypeLoc(DecltypeTypeLoc) { return true; }
+  bool TraverseTypeOfExprTypeLoc(TypeOfExprTypeLoc) { return true; }
+  bool TraverseCXXTypeidExpr(CXXTypeidExpr *) { return true; }
+  bool TraverseUnaryExprOrTypeTraitExpr(UnaryExprOrTypeTraitExpr *) {
+    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..95d5f569c0c824 100644
--- a/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
+++ b/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
@@ -3331,6 +3331,58 @@ TEST(TransferTest, ResultObjectLocationDontVisitNestedRecordDecl) {
          ASTContext &ASTCtx) {});
 }
 
+TEST(TransferTest, ResultObjectLocationDontVisitUnevaluatedContexts) {
+  // This is a crash repro.
+  // We used to crash because when propagating result objects, we would visit
+  // unevaluated contexts, but we don't model fields used only in these.
+
+  auto testFunction = [](llvm::StringRef Code, llvm::StringRef TargetFun) {
+    runDataflow(
+        Code,
+        [](const llvm::StringMap<DataflowAnalysisState<NoopLattice>> &Results,
+           ASTContext &ASTCtx) {},
+        LangStandard::lang_gnucxx17,
+        /* ApplyBuiltinTransfer= */ true, TargetFun);
+  };
+
+  std::string Code = R"cc(
+    // Definitions needed for `typeid`.
+    namespace std {
+      class type_info {};
+      class bad_typeid {};
+    }  // namespace std
+
+    struct S1 {};
+    struct S2 { S1 s1; };
+
+    // We test each type of unevaluated context from a 
diff erent target
+    // function. Some types of unevaluated contexts may actually cause the
+    // field `s1` to be modeled, and we don't want this to "pollute" the tests
+    // for the other unevaluated contexts.
+    void decltypeTarget() {
+        decltype(S2{}) Dummy;
+    }
+    void typeofTarget() {
+        typeof(S2{}) Dummy;
+    }
+    void typeidTarget() {
+        typeid(S2{});
+    }
+    void sizeofTarget() {
+        sizeof(S2{});
+    }
+    void noexceptTarget() {
+        noexcept(S2{});
+    }
+  )cc";
+
+  testFunction(Code, "decltypeTarget");
+  testFunction(Code, "typeofTarget");
+  testFunction(Code, "typeidTarget");
+  testFunction(Code, "sizeofTarget");
+  testFunction(Code, "noexceptTarget");
+}
+
 TEST(TransferTest, StaticCast) {
   std::string Code = R"(
     void target(int Foo) {