[PATCH] D125821: [clang][dataflow] Fix double visitation of nested logical operators

[Eric Li via Phabricator via cfe-commits]

li.zhe.hua updated this revision to Diff 430152.
li.zhe.hua added a comment.

Address comments


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D125821/new/

https://reviews.llvm.org/D125821

Files:
  clang/lib/Analysis/FlowSensitive/Transfer.cpp
  clang/unittests/Analysis/FlowSensitive/TransferTest.cpp


Index: clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
===================================================================
--- clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
+++ clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
@@ -2467,6 +2467,67 @@
           EXPECT_EQ(&BazVal->getRightSubValue(), BarVal);
         });
   }
+
+  {
+    std::string Code = R"(
+      void target(bool A, bool B, bool C, bool D) {
+        bool Foo = ((A && B) && C) && D;
+        // [[p]]
+      }
+    )";
+    runDataflow(
+        Code, [](llvm::ArrayRef<
+                     std::pair<std::string, DataflowAnalysisState<NoopLattice>>>
+                     Results,
+                 ASTContext &ASTCtx) {
+          ASSERT_THAT(Results, ElementsAre(Pair("p", _)));
+          const Environment &Env = Results[0].second.Env;
+
+          const ValueDecl *ADecl = findValueDecl(ASTCtx, "A");
+          ASSERT_THAT(ADecl, NotNull());
+
+          const auto *AVal =
+              dyn_cast_or_null<BoolValue>(Env.getValue(*ADecl, SkipPast::None));
+          ASSERT_THAT(AVal, NotNull());
+
+          const ValueDecl *BDecl = findValueDecl(ASTCtx, "B");
+          ASSERT_THAT(BDecl, NotNull());
+
+          const auto *BVal =
+              dyn_cast_or_null<BoolValue>(Env.getValue(*BDecl, SkipPast::None));
+          ASSERT_THAT(BVal, NotNull());
+
+          const ValueDecl *CDecl = findValueDecl(ASTCtx, "C");
+          ASSERT_THAT(CDecl, NotNull());
+
+          const auto *CVal =
+              dyn_cast_or_null<BoolValue>(Env.getValue(*CDecl, SkipPast::None));
+          ASSERT_THAT(CVal, NotNull());
+
+          const ValueDecl *DDecl = findValueDecl(ASTCtx, "D");
+          ASSERT_THAT(DDecl, NotNull());
+
+          const auto *DVal =
+              dyn_cast_or_null<BoolValue>(Env.getValue(*DDecl, SkipPast::None));
+          ASSERT_THAT(DVal, NotNull());
+
+          const ValueDecl *FooDecl = findValueDecl(ASTCtx, "Foo");
+          ASSERT_THAT(FooDecl, NotNull());
+
+          const auto *FooVal = dyn_cast_or_null<ConjunctionValue>(
+              Env.getValue(*FooDecl, SkipPast::None));
+          ASSERT_THAT(FooVal, NotNull());
+
+          const auto &FooLeftSubVal =
+              cast<ConjunctionValue>(FooVal->getLeftSubValue());
+          const auto &FooLeftLeftSubVal =
+              cast<ConjunctionValue>(FooLeftSubVal.getLeftSubValue());
+          EXPECT_EQ(&FooLeftLeftSubVal.getLeftSubValue(), AVal);
+          EXPECT_EQ(&FooLeftLeftSubVal.getRightSubValue(), BVal);
+          EXPECT_EQ(&FooLeftSubVal.getRightSubValue(), CVal);
+          EXPECT_EQ(&FooVal->getRightSubValue(), DVal);
+        });
+  }
 }
 
 TEST_F(TransferTest, AssignFromBoolNegation) {
Index: clang/lib/Analysis/FlowSensitive/Transfer.cpp
===================================================================
--- clang/lib/Analysis/FlowSensitive/Transfer.cpp
+++ clang/lib/Analysis/FlowSensitive/Transfer.cpp
@@ -571,12 +571,15 @@
         return *Val;
     }
 
-    // Sub-expressions that are logic operators are not added in basic blocks
-    // (e.g. see CFG for `bool d = a && (b || c);`). If `SubExpr` is a logic
-    // operator, it isn't evaluated and assigned a value yet. In that case, we
-    // need to first visit `SubExpr` and then try to get the value that gets
-    // assigned to it.
-    Visit(&SubExpr);
+    if (Env.getStorageLocation(SubExpr, SkipPast::None) == nullptr) {
+      // Sub-expressions that are logic operators are not added in basic blocks
+      // (e.g. see CFG for `bool d = a && (b || c);`). If `SubExpr` is a logic
+      // operator, it may not have been evaluated and assigned a value yet. In
+      // that case, we need to first visit `SubExpr` and then try to get the
+      // value that gets assigned to it.
+      Visit(&SubExpr);
+    }
+
     if (auto *Val = dyn_cast_or_null<BoolValue>(
             Env.getValue(SubExpr, SkipPast::Reference)))
       return *Val;


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D125821.430152.patch
Type: text/x-patch
Size: 3950 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20220517/999e5a99/attachment.bin>