[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

2536

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

[mboehme]

Pre-merge failure looks unrelated

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 CK_UserDefinedConversion:
       // CK_ConstructorConversion, and CK_UserDefinedConversion.
     case CK_NoOp: {
       // FIXME: Consider making `Environment::getStorageLocation` skip noop
       // expressions (this and other similar expressions in the file) instead
       // of assigning them storage locations.
       propagateValueOrStorageLocation(*SubExpr, *S, Env);
       break;
     }
-    case CK_NullToPointer:
-    case CK_NullToMemberPointer: {
+    case CK_NullToPointer: {
       auto &Loc = Env.createStorageLocation(S->getType());
       Env.setStorageLocation(*S, Loc);
 
       auto &NullPointerVal =
           Env.getOrCreateNullPointerValue(S->getType()->getPointeeType());
       Env.setValue(Loc, NullPointerVal);
       break;
     }
+    case CK_NullToMemberPointer:
+      // FIXME: Implement pointers to members. For now, don't associate a value
+      // with this expression.
+      break;
     case CK_FunctionToPointerDecay:
     case CK_BuiltinFnToFnPtr: {
       StorageLocation *PointeeLoc =
           Env.getStorageLocation(*SubExpr, SkipPast::Reference);
       if (PointeeLoc == nullptr)
         break;
 
       auto &PointerLoc = Env.createStorageLocation(*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)
+      // FIXME: Model pointers to members.
+      if (S->getType()->isMemberPointerType())
         break;
 
+      if (StorageLocation *PointeeLoc = Env.getStorageLocationStrict(*SubExpr))
         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::*MemberPointer = &S::i;
+      // [[p]]
+    }
+  )";
+  runDataflow(
+      Code,
+      [](const llvm::StringMap<DataflowAnalysisState<NoopLattice>> &Results,
+         ASTContext &ASTCtx) {
+        const Environment &Env = getEnvironmentAtAnnotation(Results, "p");
+
+        const ValueDecl *MemberPointerDecl =
+            findValueDecl(ASTCtx, "MemberPointer");
+        ASSERT_THAT(MemberPointerDecl, NotNull());
+        ASSERT_THAT(Env.getValue(*MemberPointerDecl), IsNull());
+      });
+}
+
+TEST(TransferTest, PointerToMemberFunction) {
+  std::string Code = R"(
+    struct S {
+      void Method();
+    };
+    void target() {
+      void (S::*MemberPointer)() = &S::Method;
+      // [[p]]
+    }
+  )";
+  runDataflow(
+      Code,
+      [](const llvm::StringMap<DataflowAnalysisState<NoopLattice>> &Results,
+         ASTContext &ASTCtx) {
+        const Environment &Env = getEnvironmentAtAnnotation(Results, "p");
+
+        const ValueDecl *MemberPointerDecl =
+            findValueDecl(ASTCtx, "MemberPointer");
+        ASSERT_THAT(MemberPointerDecl, NotNull());
+        ASSERT_THAT(Env.getValue(*MemberPointerDecl), IsNull());
+      });
+}
+
 TEST(TransferTest, NullToMemberPointerCast) {
   std::string Code = R"(
     struct Foo {};
-    void target(Foo *Foo) {
+    void target() {
       int Foo::*MemberPointer = nullptr;
       // [[p]]
     }
   )";
   runDataflow(
       Code,
       [](const llvm::StringMap<DataflowAnalysisState<NoopLattice>> &Results,
          ASTContext &ASTCtx) {
         ASSERT_THAT(Results.keys(), UnorderedElementsAre("p"));
         const Environment &Env = getEnvironmentAtAnnotation(Results, "p");
 
         const ValueDecl *MemberPointerDecl =
             findValueDecl(ASTCtx, "MemberPointer");
         ASSERT_THAT(MemberPointerDecl, NotNull());
-
-        const auto *MemberPointerVal =
-            cast<PointerValue>(Env.getValue(*MemberPointerDecl));
-
-        const StorageLocation &MemberLoc = MemberPointerVal->getPointeeLoc();
-        EXPECT_THAT(Env.getValue(MemberLoc), IsNull());
+        ASSERT_THAT(Env.getValue(*MemberPointerDecl), IsNull());
       });
 }
 
 TEST(TransferTest, AddrOfValue) {
   std::string Code = R"(
     void target() {
       int Foo;
       int *Bar = &Foo;