[clang][dataflow] Don't crash when creating pointers to members.

[mboehme]

Summary

The newly added tests crash without the other changes in this patch.

[gribozavr2]

Using PointerValue to model pointers to data members does not look right to me, because a pointer to data member is an offset within an object that we apply this pointer to, it is not a specific storage location.

I think we either shouldn't be modeling them at all (current implementation) and fix any crashes in the downstream code to allow non-modeled constructs in more places, or we should have a distinct kind of Value for pointers to members.

[sammccall]

Per offline discussion, I think modelling pointer-to-member as PointerValue does make sense, though that's not completely obvious.
Can we document this on PointerValue? (And that it points to a StorageLocation for the decl, which has no value)

clang/lib/Analysis/FlowSensitive/Transfer.cpp

236

I'm a bit confused by this. Why bail out rather than create the storage location?

C::x is going to be part of &C::x, and we're going to create the storagelocation by calling getStorageLocation() on line 456. So why not do it already?

[mboehme]

In D153960#4455661, @gribozavr2 wrote:

Using PointerValue to model pointers to data members does not look right to me, because a pointer to data member is an offset within an object that we apply this pointer to, it is not a specific storage location.

I think we either shouldn't be modeling them at all (current implementation) and fix any crashes in the downstream code to allow non-modeled constructs in more places, or we should have a distinct kind of Value for pointers to members.

After some offline discussion, I'll proceed by not modeling pointers to members at all for the time being (i.e. not associating them with a Value) but fixing the crashes.

Modeling pointers-to-members as a PointerValue works to the extent that it allows us to compare different pointers-to-members for equality, but where it breaks down is if we want to dereference them (assuming that we know unambiguously what value the pointer-to-member will have). At that point, what we would probably want is some MemberPointerValue that refers to the Decl for the field or member function being referenced.

I'll ping this patch once it's ready to review again.

[mboehme]

clang/lib/Analysis/FlowSensitive/Transfer.cpp

236

We can't associate a storage location with *S because it's not a glvalue, and only glvalues are allowed to be associated with storage locations. So we have to defer this work until we get to the & operator.

[mboehme]

Don't produce values for pointers to members. For consistency, also make
the NullToMemberPointer cast not produce any values.

[mboehme]

PTAL

clang/lib/Analysis/FlowSensitive/Transfer.cpp

447

While I'm here, streamline the code for the not-pointer-to-member case.

clang/unittests/Analysis/FlowSensitive/TransferTest.cpp

2529

Noticed that the parameter here was unused, so I removed it.

[mboehme]

Pre-merge failure looks unrelated

clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp

 }
 
 static void insertIfFunction(const Decl &D,
                              llvm::DenseSet<const FunctionDecl *> &Funcs) {
   if (auto *FD = dyn_cast<FunctionDecl>(&D))
     Funcs.insert(FD);
 }
 
+static void insertIfField(const Decl &D,
+                          llvm::DenseSet<const FieldDecl *> &Fields) {
+  if (const auto *FD = dyn_cast<FieldDecl>(&D))
+    Fields.insert(FD);
+}
+
 static void
 getFieldsGlobalsAndFuncs(const Decl &D,
                          llvm::DenseSet<const FieldDecl *> &Fields,
                          llvm::DenseSet<const VarDecl *> &Vars,
                          llvm::DenseSet<const FunctionDecl *> &Funcs) {
   insertIfGlobal(D, Vars);
   insertIfFunction(D, Funcs);
   if (const auto *Decomp = dyn_cast<DecompositionDecl>(&D))
if (auto *DS = dyn_cast<DeclStmt>(&S)) {
     if (DS->isSingleDecl())
       getFieldsGlobalsAndFuncs(*DS->getSingleDecl(), Fields, Vars, Funcs);
     else
       for (auto *D : DS->getDeclGroup())
         getFieldsGlobalsAndFuncs(*D, Fields, Vars, Funcs);
   } else if (auto *E = dyn_cast<DeclRefExpr>(&S)) {
     insertIfGlobal(*E->getDecl(), Vars);
     insertIfFunction(*E->getDecl(), Funcs);
+    insertIfField(*E->getDecl(), Fields);
   } else if (auto *E = dyn_cast<MemberExpr>(&S)) {
     // FIXME: should we be using `E->getFoundDecl()`?
     const ValueDecl *VD = E->getMemberDecl();
     insertIfGlobal(*VD, Vars);
     insertIfFunction(*VD, Funcs);
-    if (const auto *FD = dyn_cast<FieldDecl>(VD))
-      Fields.insert(FD);
+    insertIfField(*VD, Fields);
   }
 }
 
 // FIXME: Add support for resetting globals after function calls to enable
 // the implementation of sound analyses.
 void Environment::initFieldsGlobalsAndFuncs(const FunctionDecl *FuncDecl) {
   assert(FuncDecl->getBody() != nullptr);
 
void Environment::initFieldsGlobalsAndFuncs(const FunctionDecl *FuncDecl) {
   }
 
   for (const FunctionDecl *FD : Funcs) {
     if (getStorageLocation(*FD) != nullptr)
       continue;
     auto &Loc = createStorageLocation(FD->getType());
     setStorageLocation(*FD, Loc);
   }
+
+  // Storage locations for fields are used in pointers to member variables.
+  for (const FieldDecl *Field : Fields) {
+    if (getStorageLocation(*Field) != nullptr)
+      continue;
+    auto &Loc = createStorageLocation(Field->getType());
+    setStorageLocation(*Field, Loc);
+  }
 }
 
 Environment::Environment(DataflowAnalysisContext &DACtx)
     : DACtx(&DACtx),
       FlowConditionToken(&DACtx.arena().makeFlowConditionToken()) {}
 
 Environment Environment::fork() const {
   Environment Copy(*this);

clang/lib/Analysis/FlowSensitive/Transfer.cpp

void VisitBinaryOperator(const BinaryOperator *S) {
     default:
       break;
     }
   }
 
   void VisitDeclRefExpr(const DeclRefExpr *S) {
     const ValueDecl *VD = S->getDecl();
     assert(VD != nullptr);
+
+    // `DeclRefExpr`s to fields and non-static methods aren't glvalues, and
+    // there's also no sensible `Value` we can assign to them, so skip them.
+    if (isa<FieldDecl>(VD))
+      return;
+    if (auto *Method = dyn_cast<CXXMethodDecl>(VD);
+        Method && !Method->isStatic())
+      return;
+
     auto *DeclLoc = Env.getStorageLocation(*VD);
     if (DeclLoc == nullptr)
       return;
 
     Env.setStorageLocationStrict(*S, *DeclLoc);
   }
 
   void VisitDeclStmt(const DeclStmt *S) {
case UO_Deref: {
           cast_or_null<PointerValue>(Env.getValueStrict(*SubExpr));
       if (SubExprVal == nullptr)
         break;
 
       Env.setStorageLocationStrict(*S, SubExprVal->getPointeeLoc());
       break;
     }
     case UO_AddrOf: {
-      // Do not form a pointer to a reference. If `SubExpr` is assigned a
-      // `ReferenceValue` then form a value that points to the location of its
-      // pointee.
-      StorageLocation *PointeeLoc = Env.getStorageLocationStrict(*SubExpr);
-      if (PointeeLoc == nullptr)
-        break;
+      StorageLocation *PointeeLoc = nullptr;
+      if (S->getType()->isMemberPointerType()) {
+        if (auto *DeclRef = dyn_cast<DeclRefExpr>(SubExpr))
+          if (auto *D = DeclRef->getDecl())
+            PointeeLoc = Env.getStorageLocation(*D);
+      } else {
+        PointeeLoc = Env.getStorageLocationStrict(*SubExpr);
+      }
 
+      if (PointeeLoc)
         Env.setValueStrict(*S, Env.create<PointerValue>(*PointeeLoc));
+
       break;
     }
     case UO_LNot: {
       auto *SubExprVal =
           dyn_cast_or_null<BoolValue>(Env.getValueStrict(*SubExpr));
       if (SubExprVal == nullptr)
         break;
 

clang/unittests/Analysis/FlowSensitive/TransferTest.cpp

runDataflow(
         EXPECT_THAT(Env.getValue(BazPointeeLoc), IsNull());
 
         const StorageLocation &NullPointeeLoc = NullVal->getPointeeLoc();
         EXPECT_TRUE(isa<ScalarStorageLocation>(NullPointeeLoc));
         EXPECT_THAT(Env.getValue(NullPointeeLoc), IsNull());
       });
 }
 
+TEST(TransferTest, PointerToMemberVariable) {
+  std::string Code = R"(
+    struct S {
+      int i;
+    };
+    void target() {
+      int S::*P1 = &S::i;
+      int S::*P2 = &S::i;
+      // [[p]]
+    }
+  )";
+  runDataflow(
+      Code,
+      [](const llvm::StringMap<DataflowAnalysisState<NoopLattice>> &Results,
+         ASTContext &ASTCtx) {
+        const Environment &Env = getEnvironmentAtAnnotation(Results, "p");
+
+        auto &P1Val = getValueForDecl<PointerValue>(ASTCtx, Env, "P1");
+        auto &P2Val = getValueForDecl<PointerValue>(ASTCtx, Env, "P2");
+
+        ASSERT_EQ(&P1Val.getPointeeLoc(), &P2Val.getPointeeLoc());
+      });
+}
+
+TEST(TransferTest, PointerToMemberFunction) {
+  std::string Code = R"(
+    struct S {
+      void Method();
+    };
+    void target() {
+      void (S::*P1)() = &S::Method;
+      void (S::*P2)() = &S::Method;
+      // [[p]]
+    }
+  )";
+  runDataflow(
+      Code,
+      [](const llvm::StringMap<DataflowAnalysisState<NoopLattice>> &Results,
+         ASTContext &ASTCtx) {
+        const Environment &Env = getEnvironmentAtAnnotation(Results, "p");
+
+        auto &P1Val = getValueForDecl<PointerValue>(ASTCtx, Env, "P1");
+        auto &P2Val = getValueForDecl<PointerValue>(ASTCtx, Env, "P2");
+
+        ASSERT_EQ(&P1Val.getPointeeLoc(), &P2Val.getPointeeLoc());
+      });
+}
+
 TEST(TransferTest, NullToMemberPointerCast) {
   std::string Code = R"(
     struct Foo {};
     void target(Foo *Foo) {
       int Foo::*MemberPointer = nullptr;
       // [[p]]
     }
   )";
   runDataflow(
       Code,
       [](const llvm::StringMap<DataflowAnalysisState<NoopLattice>> &Results,
          ASTContext &ASTCtx) {