[clang] 23bfc27 - [clang][dataflow] Use `ignoreCFGOmittedNodes()` in `setValue()`. (#78245)

[via cfe-commits]

Author: martinboehme
Date: 2024-01-16T15:48:44+01:00
New Revision: 23bfc271a316345459809427d98e942455d0e2b6

URL: https://github.com/llvm/llvm-project/commit/23bfc271a316345459809427d98e942455d0e2b6
DIFF: https://github.com/llvm/llvm-project/commit/23bfc271a316345459809427d98e942455d0e2b6.diff

LOG: [clang][dataflow] Use `ignoreCFGOmittedNodes()` in `setValue()`. (#78245)

This is to be consistent with `getValue()`, which also uses
`ignoreCFGOmittedNodes()`.

Before this fix, it was not possible to retrieve a `Value` from a "CFG
omitted"
node that had previously been set using `setValue()`; see the
accompanying test,
which fails without the fix.

I discovered this issue while running internal integration tests on
https://github.com/llvm/llvm-project/pull/78127.

Added: 
    

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

Removed: 
    


################################################################################
diff  --git a/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp b/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
index a50ee57a3c11b4..57572e5b4e2f13 100644
--- a/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
+++ b/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
@@ -803,13 +803,15 @@ void Environment::setValue(const StorageLocation &Loc, Value &Val) {
 }
 
 void Environment::setValue(const Expr &E, Value &Val) {
+  const Expr &CanonE = ignoreCFGOmittedNodes(E);
+
   if (auto *RecordVal = dyn_cast<RecordValue>(&Val)) {
-    assert(isOriginalRecordConstructor(E) ||
-           &RecordVal->getLoc() == &getResultObjectLocation(E));
+    assert(isOriginalRecordConstructor(CanonE) ||
+           &RecordVal->getLoc() == &getResultObjectLocation(CanonE));
   }
 
-  assert(E.isPRValue());
-  ExprToVal[&E] = &Val;
+  assert(CanonE.isPRValue());
+  ExprToVal[&CanonE] = &Val;
 }
 
 Value *Environment::getValue(const StorageLocation &Loc) const {

diff  --git a/clang/unittests/Analysis/FlowSensitive/DataflowEnvironmentTest.cpp b/clang/unittests/Analysis/FlowSensitive/DataflowEnvironmentTest.cpp
index 003434a58b1075..8799d03dfd3c58 100644
--- a/clang/unittests/Analysis/FlowSensitive/DataflowEnvironmentTest.cpp
+++ b/clang/unittests/Analysis/FlowSensitive/DataflowEnvironmentTest.cpp
@@ -58,6 +58,52 @@ TEST_F(EnvironmentTest, FlowCondition) {
   EXPECT_FALSE(Env.allows(NotX));
 }
 
+TEST_F(EnvironmentTest, SetAndGetValueOnCfgOmittedNodes) {
+  // Check that we can set a value on an expression that is omitted from the CFG
+  // (see `ignoreCFGOmittedNodes()`), then retrieve that same value from the
+  // expression. This is a regression test; `setValue()` and `getValue()`
+  // previously did not use `ignoreCFGOmittedNodes()` consistently.
+
+  using namespace ast_matchers;
+
+  std::string Code = R"cc(
+    struct S {
+      int f();
+    };
+    void target() {
+      // Method call on a temporary produces an `ExprWithCleanups`.
+      S().f();
+      (1);
+    }
+  )cc";
+
+  auto Unit =
+      tooling::buildASTFromCodeWithArgs(Code, {"-fsyntax-only", "-std=c++17"});
+  auto &Context = Unit->getASTContext();
+
+  ASSERT_EQ(Context.getDiagnostics().getClient()->getNumErrors(), 0U);
+
+  const ExprWithCleanups *WithCleanups = selectFirst<ExprWithCleanups>(
+      "cleanups",
+      match(exprWithCleanups(hasType(isInteger())).bind("cleanups"), Context));
+  ASSERT_NE(WithCleanups, nullptr);
+
+  const ParenExpr *Paren = selectFirst<ParenExpr>(
+      "paren", match(parenExpr(hasType(isInteger())).bind("paren"), Context));
+  ASSERT_NE(Paren, nullptr);
+
+  Environment Env(DAContext);
+  IntegerValue *Val1 =
+      cast<IntegerValue>(Env.createValue(Unit->getASTContext().IntTy));
+  Env.setValue(*WithCleanups, *Val1);
+  EXPECT_EQ(Env.getValue(*WithCleanups), Val1);
+
+  IntegerValue *Val2 =
+      cast<IntegerValue>(Env.createValue(Unit->getASTContext().IntTy));
+  Env.setValue(*Paren, *Val2);
+  EXPECT_EQ(Env.getValue(*Paren), Val2);
+}
+
 TEST_F(EnvironmentTest, CreateValueRecursiveType) {
   using namespace ast_matchers;