[clang] 3ce03c4 - [clang][dataflow] Fix 2 bugs in `MemberExpr` interpretation.

[Yitzhak Mandelbaum via cfe-commits]

Author: Yitzhak Mandelbaum
Date: 2023-01-10T15:48:00Z
New Revision: 3ce03c42dbb46531968c527d80c0243c2db7bc0e

URL: https://github.com/llvm/llvm-project/commit/3ce03c42dbb46531968c527d80c0243c2db7bc0e
DIFF: https://github.com/llvm/llvm-project/commit/3ce03c42dbb46531968c527d80c0243c2db7bc0e.diff

LOG: [clang][dataflow] Fix 2 bugs in `MemberExpr` interpretation.

There were two (small) bugs causing crashes in the analysis.  This patch fixes both of them.

1. An enum value was accessed as a class member. Now, the engine gracefully
ignores such member expressions.

2. Field access in `MemberExpr` of struct/class-typed global variables. Analysis
didn't interpret fields of global vars, because the vars were initialized before
the fields were added to the "allowlist". Now, the allowlist is set _before_
init of globals.

Differential Revision: https://reviews.llvm.org/D141384

Added: 
    

Modified: 
    clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
    clang/lib/Analysis/FlowSensitive/Transfer.cpp
    clang/unittests/Analysis/FlowSensitive/DataflowEnvironmentTest.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 7d10a75b36e3e..37d200509e8d2 100644
--- a/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
+++ b/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
@@ -250,11 +250,12 @@ Environment::Environment(DataflowAnalysisContext &DACtx,
     }
     getFieldsAndGlobalVars(*FuncDecl->getBody(), Fields, Vars);
 
-    initVars(Vars);
-    // These have to be set before the lines that follow to ensure that create*
-    // work correctly for structs.
+    // These have to be added before the lines that follow to ensure that
+    // `create*` work correctly for structs.
     DACtx.addModeledFields(Fields);
 
+    initVars(Vars);
+
     for (const auto *ParamDecl : FuncDecl->parameters()) {
       assert(ParamDecl != nullptr);
       auto &ParamLoc = createStorageLocation(*ParamDecl);
@@ -341,9 +342,13 @@ void Environment::pushCallInternal(const FunctionDecl *FuncDecl,
     }
   }
   getFieldsAndGlobalVars(*FuncDecl->getBody(), Fields, Vars);
-  initVars(Vars);
+
+  // These have to be added before the lines that follow to ensure that
+  // `create*` work correctly for structs.
   DACtx->addModeledFields(Fields);
 
+  initVars(Vars);
+
   const auto *ParamIt = FuncDecl->param_begin();
 
   // FIXME: Parameters don't always map to arguments 1:1; examples include

diff  --git a/clang/lib/Analysis/FlowSensitive/Transfer.cpp b/clang/lib/Analysis/FlowSensitive/Transfer.cpp
index c2bf18754be51..259b82d647981 100644
--- a/clang/lib/Analysis/FlowSensitive/Transfer.cpp
+++ b/clang/lib/Analysis/FlowSensitive/Transfer.cpp
@@ -461,6 +461,10 @@ class TransferVisitor : public ConstStmtVisitor<TransferVisitor> {
     if (Member->isFunctionOrFunctionTemplate())
       return;
 
+    // FIXME: if/when we add support for modeling enums, use that support here.
+    if (isa<EnumConstantDecl>(Member))
+      return;
+
     if (auto *D = dyn_cast<VarDecl>(Member)) {
       if (D->hasGlobalStorage()) {
         auto *VarDeclLoc = Env.getStorageLocation(*D, SkipPast::None);

diff  --git a/clang/unittests/Analysis/FlowSensitive/DataflowEnvironmentTest.cpp b/clang/unittests/Analysis/FlowSensitive/DataflowEnvironmentTest.cpp
index e66ed70be4feb..dfbd4ff274154 100644
--- a/clang/unittests/Analysis/FlowSensitive/DataflowEnvironmentTest.cpp
+++ b/clang/unittests/Analysis/FlowSensitive/DataflowEnvironmentTest.cpp
@@ -118,7 +118,7 @@ TEST_F(EnvironmentTest, InitGlobalVarsFun) {
             Context);
   const auto *Fun = selectFirst<FunctionDecl>("target", Results);
   const auto *Var = selectFirst<VarDecl>("global", Results);
-  ASSERT_TRUE(Fun != nullptr);
+  ASSERT_THAT(Fun, NotNull());
   ASSERT_THAT(Var, NotNull());
 
   // Verify the global variable is populated when we analyze `Target`.
@@ -126,6 +126,53 @@ TEST_F(EnvironmentTest, InitGlobalVarsFun) {
   EXPECT_THAT(Env.getValue(*Var, SkipPast::None), NotNull());
 }
 
+TEST_F(EnvironmentTest, InitGlobalVarsFieldFun) {
+  using namespace ast_matchers;
+
+  std::string Code = R"cc(
+     struct S { int Bar; };
+     S Global = {0};
+     int Target () { return Global.Bar; }
+  )cc";
+
+  auto Unit =
+      tooling::buildASTFromCodeWithArgs(Code, {"-fsyntax-only", "-std=c++11"});
+  auto &Context = Unit->getASTContext();
+
+  ASSERT_EQ(Context.getDiagnostics().getClient()->getNumErrors(), 0U);
+
+  auto Results =
+      match(decl(anyOf(varDecl(hasName("Global")).bind("global"),
+                       functionDecl(hasName("Target")).bind("target"))),
+            Context);
+  const auto *Fun = selectFirst<FunctionDecl>("target", Results);
+  const auto *GlobalDecl = selectFirst<VarDecl>("global", Results);
+  ASSERT_THAT(Fun, NotNull());
+  ASSERT_THAT(GlobalDecl, NotNull());
+
+  ASSERT_TRUE(GlobalDecl->getType()->isStructureType());
+  auto GlobalFields = GlobalDecl->getType()->getAsRecordDecl()->fields();
+
+  FieldDecl *BarDecl = nullptr;
+  for (FieldDecl *Field : GlobalFields) {
+    if (Field->getNameAsString() == "Bar") {
+      BarDecl = Field;
+      break;
+    }
+    FAIL() << "Unexpected field: " << Field->getNameAsString();
+  }
+  ASSERT_THAT(BarDecl, NotNull());
+
+  // Verify the global variable is populated when we analyze `Target`.
+  Environment Env(DAContext, *Fun);
+  const auto *GlobalLoc = cast<AggregateStorageLocation>(
+      Env.getStorageLocation(*GlobalDecl, SkipPast::None));
+  const auto *GlobalVal = cast<StructValue>(Env.getValue(*GlobalLoc));
+  const auto *BarVal = GlobalVal->getChild(*BarDecl);
+  ASSERT_THAT(BarVal, NotNull());
+  EXPECT_TRUE(isa<IntegerValue>(BarVal));
+}
+
 TEST_F(EnvironmentTest, InitGlobalVarsConstructor) {
   using namespace ast_matchers;
 

diff  --git a/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp b/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
index 6c6584cdc8921..b4bf699b22918 100644
--- a/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
+++ b/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
@@ -1066,6 +1066,28 @@ TEST(TransferTest, StructMember) {
       });
 }
 
+TEST(TransferTest, StructMemberEnum) {
+  std::string Code = R"(
+    struct A {
+      int Bar;
+      enum E { ONE, TWO };
+    };
+
+    void target(A Foo) {
+      A::E Baz = Foo.ONE;
+      // [[p]]
+    }
+  )";
+  // Minimal expectations -- we're just testing that it doesn't crash, since
+  // enums aren't interpreted.
+  runDataflow(
+      Code,
+      [](const llvm::StringMap<DataflowAnalysisState<NoopLattice>> &Results,
+         ASTContext &ASTCtx) {
+        EXPECT_THAT(Results.keys(), UnorderedElementsAre("p"));
+      });
+}
+
 TEST(TransferTest, DerivedBaseMemberClass) {
   std::string Code = R"(
     class A {