[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

38–39

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

clang/unittests/Analysis/FlowSensitive/TransferTest.cpp

2252–2254

Is this actually needed?

2260

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

2260

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

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

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

2260

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

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

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

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

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));
+
   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());
-
-  for (auto [Field, SrcFieldLoc] : Src.children()) {
-    assert(SrcFieldLoc != nullptr);
+  assert(compatibleTypes);
 
-    StorageLocation &DstFieldLoc = Dst.getChild(*Field);
+  for (auto [Field, DstFieldLoc] : Dst.children()) {
+    StorageLocation &SrcFieldLoc = Src.getChild(*Field);
 
     if (Field->getType()->isRecordType()) {
-      copyRecord(cast<AggregateStorageLocation>(*SrcFieldLoc),
-                 cast<AggregateStorageLocation>(DstFieldLoc), Env);
+      copyRecord(cast<AggregateStorageLocation>(SrcFieldLoc),
+                 cast<AggregateStorageLocation>(*DstFieldLoc), Env);
     } else {
-      if (Value *Val = Env.getValue(*SrcFieldLoc))
-        Env.setValue(DstFieldLoc, *Val);
+      if (Value *Val = Env.getValue(SrcFieldLoc))
+        Env.setValue(*DstFieldLoc, *Val);
       else
-        Env.clearValue(DstFieldLoc);
+        Env.clearValue(*DstFieldLoc);
     }
   }
 
   StructValue *SrcVal = cast_or_null<StructValue>(Env.getValue(Src));
   StructValue *DstVal = cast_or_null<StructValue>(Env.getValue(Dst));
 
   if (SrcVal == nullptr || DstVal == nullptr)
     return;

clang/lib/Analysis/FlowSensitive/Transfer.cpp

void VisitCXXConstructExpr(const CXXConstructExpr *S) {
     if (ConstructorDecl->isCopyOrMoveConstructor()) {
       // It is permissible for a copy/move constructor to have additional
       // parameters as long as they have default arguments defined for them.
       assert(S->getNumArgs() != 0);
 
       const Expr *Arg = S->getArg(0);
       assert(Arg != nullptr);
 
+      llvm::outs() << "TOTO\n";
+      S->dump();
+      llvm::outs() << Env.getStorageLocation(*Arg, SkipPast::Reference);
+
       auto *ArgLoc = cast_or_null<AggregateStorageLocation>(
           Env.getStorageLocation(*Arg, SkipPast::Reference));
       if (ArgLoc == nullptr)
         return;
 
       if (S->isElidable()) {
         Env.setStorageLocation(*S, *ArgLoc);
       } else {

clang/unittests/Analysis/FlowSensitive/TransferTest.cpp

runDataflow(
         const auto *FooBazVal =
             cast<IntegerValue>(getFieldValue(FooLoc, *BazDecl, Env));
         const auto *BarBazVal =
             cast<IntegerValue>(getFieldValue(BarLoc, *BazDecl, Env));
         EXPECT_EQ(FooBazVal, BarBazVal);
       });
 }
 
+TEST(TransferTest, UncheckedDerivedToBaseCopyConstructor) {
+  using ast_matchers::cxxConstructorDecl;
+  using ast_matchers::hasName;
+  using ast_matchers::ofClass;
+
+  std::string Code = R"(
+  class A {
+  public:
+      A() = default;
+      A(const A&) = default;
+      int a;
+  };
+
+  class B : public A {
+  public:
+      B() = default;
+      B(const B& b) =default;
+      B* copy() {
+          return new B(*this);
+      }
+  };
+  )";
+  ASSERT_THAT_ERROR(
+      checkDataflow<NoopAnalysis>(
+          AnalysisInputs<NoopAnalysis>(
+              Code, cxxConstructorDecl(ofClass(hasName("B"))),
+              [](ASTContext &C, Environment &) {
+                return NoopAnalysis(C, /*ApplyBuiltinTransfer=*/true);
+              })
+              .withASTBuildArgs(
+                  {"-fsyntax-only", "-fno-delayed-template-parsing",
+                   "-std=" + std::string(LangStandard::getLangStandardForKind(
+                                             LangStandard::lang_cxx17)
+                                             .getName())}),
+          /*VerifyResults=*/
+          [](const llvm::StringMap<DataflowAnalysisState<NoopLattice>> &Results,
+             const AnalysisOutputs &) {
+            // Regression test to verify that base-class initializers do not
+            // trigger an assertion. If we add support for such initializers in
+            // the future, we can expand this test to check more specific
+            // properties.
+          }),
+      llvm::Succeeded());
+}
+
 TEST(TransferTest, CopyConstructorWithParens) {
   std::string Code = R"(
     struct A {
       int Baz;
     };
 
     void target() {
       A Foo;