[clang][dataflow] fix failing assert in copyRecord

[paulsemel]

Summary

When dealing with copy constructor, the compiler can emit an
UncheckedDerivedToBase implicit cast for the CXXConstructorExpr of the
base class. In such case, when trying to copy the src storage location
to its destination, we will fail on the assert checking that location
types are the same.

When copying from derived to base class, it is acceptable to break that
assumption to only copy common fields from the base class.

Note: the provided test crashes the process without the changes made to
copyRecord.

[mboehme]

clang/lib/Analysis/FlowSensitive/RecordOps.cpp

42–43

Can you assert(DstFieldLoc != nullptr); (similar to the way this was done for SrcFieldLoc before)?

clang/unittests/Analysis/FlowSensitive/TransferTest.cpp

2252–2254 ↗

(On Diff #542517)

Is this actually needed?

2260 ↗

(On Diff #542517)

Could we simply call B Target and omit its default constructor so that we can avoid all of the custom setup code?

2260 ↗

(On Diff #542517)

Alternatively, and even better, could we add checks that test that the copy constructor of A is indeed creating a "slice" of the B object?

2272–2275 ↗

(On Diff #542517)

I don't see a base-class initializer in the code, and this doesn't appear to be what the test is for. Is this copy-pasted from another test?

[paulsemel]

clang/unittests/Analysis/FlowSensitive/TransferTest.cpp

2252–2254 ↗

(On Diff #542517)

Yes, I think so, it's the only way I could find to have the compiler generate the UncheckedDerivedToBase implicit cast.

2260 ↗

(On Diff #542517)

Not sure what you meant here, but I can't write a body for the copy constructor for B, or otherwise it would create a DerivedToBase implicit cast and not a UncheckedDerivedToBase one anymore... so I essentially can't add an annotation there.

[sammccall]

clang/lib/Analysis/FlowSensitive/RecordOps.cpp

26

this variable is unused in a non-debug build, can you add (void)compatibleTypes; to suppress the warning?

(Thanks for this patch, have been testing it locally as it fixes crashes we're hitting in nullability)

[paulsemel]

apply Sam's suggested changes.

[mboehme]

When I originally reviewed this patch, I took only a very cursory look, as I was about to finish up for the day and wanted to get a few initial thoughts out (sorry...).

Having taken a closer look, I think the test for this would better live in RecordOpsTest.cpp. Also, it should just call copyRecord() directly, rather than testing it indirectly through the effects of a copy constructor (as examples, look at some of the other tests there).

So the test could look something like this:

struct A {
  int i;
};

struct B : public A {
};

void target(A a, B b) {
  (void)a.i;
  // [[p]]
}

And then in the callback, you would do something like this:

Environment Env = getEnvironmentAtAnnotation(Results, "p").fork();

const ValueDecl *IDecl = findValueDecl(ASTCtx, "i");
auto &A = getLocForDecl<AggregateStorageLocation>(ASTCtx, Env, "a");
auto &B = getLocForDecl<AggregateStorageLocation>(ASTCtx, Env, "b");

EXPECT_NE(Env.getValue(A.getChild(*IDecl)), Env.getValue(B.getChild(*IDecl)));

copyRecord(A, B, Env);

EXPECT_EQ(Env.getValue(A.getChild(*IDecl)), Env.getValue(B.getChild(*IDecl)));

(There's probably something missing here, but I hope you can fill in the blanks.)

This is closer in spirit to a unit test, and it requires less setup.

Does this make sense?

clang/unittests/Analysis/FlowSensitive/TransferTest.cpp

2252–2254 ↗

(On Diff #542517)

Can you clarify why it's important that the AST specifically contains an UncheckedDerivedToBase cast? (IOW, why do we otherwise not call copyRecord() in a way that tests the case that we're interested in?)

But see also my more general comments on testing.

2260 ↗

(On Diff #542517)

I see what you mean. So maybe it's not the wisest choice to call the derived class target.

Instead, couldn't we do something like this?

struct A {
  A(const A&) = default;
  int i;
};

struct B : public A {
};

void target() {
  B b = { 1 };
  A a = b;
  // [[p]]
}

And then write checks to verify that in the environment for p, the value for a.i is equal to b.i?

But I think we can do even better (see more general comment with regard to testing that I'm making as a patch-level comment.)

[mboehme]

Oh, and also wanted to give you a heads up that the changes in https://reviews.llvm.org/rG44f98d0101fe82352e7c5fa98f1b2e9dc1159200 may conflict with what you have here (should be easy to fix though).

[paulsemel]

I used Martin's test suggestion as-is, since I think it pretty much tests the initial failing reproduction case.

clang/unittests/Analysis/FlowSensitive/TransferTest.cpp

2252–2254 ↗

(On Diff #542517)

Note: this will be outdated since I changed the test to the proposed unittest.

If we're hitting a DerivedToBase, we'll just not go there because the latter is not handled in VisitImplicitCastExpr, which is why it was first hard for me to reproduce that failing assert.

[paulsemel]

rename test.

[mboehme]

clang/unittests/Analysis/FlowSensitive/TransferTest.cpp

2252–2254 ↗

(On Diff #542517)

Ah, got it, thanks for the explanation!

[mboehme]

Thanks, this is great!

clang/lib/Analysis/FlowSensitive/RecordOps.cpp

 
 #include "clang/Analysis/FlowSensitive/RecordOps.h"
 
 #define DEBUG_TYPE "dataflow"
 
 void clang::dataflow::copyRecord(AggregateStorageLocation &Src,
                                  AggregateStorageLocation &Dst,
                                  Environment &Env) {
+  auto SrcType = Src.getType().getCanonicalType().getUnqualifiedType();
+  auto DstType = Dst.getType().getCanonicalType().getUnqualifiedType();
+
+  auto SrcDecl = SrcType->getAsCXXRecordDecl();
+  auto DstDecl = DstType->getAsCXXRecordDecl();
+
+  bool compatibleTypes =
+      SrcType == DstType ||
+      (SrcDecl && DstDecl && SrcDecl->isDerivedFrom(DstDecl));
+  (void)compatibleTypes;
+
   LLVM_DEBUG({
-    if (Dst.getType().getCanonicalType().getUnqualifiedType() !=
-        Src.getType().getCanonicalType().getUnqualifiedType()) {
+    if (!compatibleTypes) {
       llvm::dbgs() << "Source type " << Src.getType() << "\n";
       llvm::dbgs() << "Destination type " << Dst.getType() << "\n";
     }
   });
-  assert(Dst.getType().getCanonicalType().getUnqualifiedType() ==
-         Src.getType().getCanonicalType().getUnqualifiedType());
+  assert(compatibleTypes);
 
-  for (auto [Field, SrcFieldLoc] : Src.children()) {
-    StorageLocation *DstFieldLoc = Dst.getChild(*Field);
+  for (auto [Field, DstFieldLoc] : Dst.children()) {
+    StorageLocation *SrcFieldLoc = Src.getChild(*Field);
 
     assert(Field->getType()->isReferenceType() ||
            (SrcFieldLoc != nullptr && DstFieldLoc != nullptr));
 
     if (Field->getType()->isRecordType()) {
       copyRecord(cast<AggregateStorageLocation>(*SrcFieldLoc),
                  cast<AggregateStorageLocation>(*DstFieldLoc), Env);
     } else if (Field->getType()->isReferenceType()) {
       Dst.setChild(*Field, SrcFieldLoc);
     } else {
       if (Value *Val = Env.getValue(*SrcFieldLoc))

clang/unittests/Analysis/FlowSensitive/RecordOpsTest.cpp

runDataflow(
         cast<StructValue>(Env.getValue(S1))
             ->setProperty("prop1", Env.getBoolLiteralValue(false));
         cast<StructValue>(Env.getValue(S2))
             ->setProperty("prop2", Env.getBoolLiteralValue(false));
         EXPECT_FALSE(recordsEqual(S1, S2, Env));
       });
 }
 
+TEST(TransferTest, CopyRecordFromDerivedToBase) {
+  std::string Code = R"(
+    struct A {
+      int i;
+    };
+
+    struct B : public A {
+    };
+
+    void target(A a, B b) {
+      (void)a.i;
+      // [[p]]
+    }
+  )";
+  runDataflow(
+      Code,
+      [](const llvm::StringMap<DataflowAnalysisState<NoopLattice>> &Results,
+         ASTContext &ASTCtx) {
+        Environment Env = getEnvironmentAtAnnotation(Results, "p").fork();
+
+        const ValueDecl *IDecl = findValueDecl(ASTCtx, "i");
+        auto &A = getLocForDecl<AggregateStorageLocation>(ASTCtx, Env, "a");
+        auto &B = getLocForDecl<AggregateStorageLocation>(ASTCtx, Env, "b");
+
+        EXPECT_NE(Env.getValue(*A.getChild(*IDecl)),
+                  Env.getValue(*B.getChild(*IDecl)));
+
+        copyRecord(B, A, Env);
+
+        EXPECT_EQ(Env.getValue(*A.getChild(*IDecl)),
+                  Env.getValue(*B.getChild(*IDecl)));
+      });
+}
+
 } // namespace
 } // namespace test
 } // namespace dataflow
 } // namespace clang