Copy Argument Passing Restrictions setting when importing a CXXRecordDecl definition

[shafik]

Summary

For a CXXRecordDecl the RecordDeclBits are stored in the DeclContext. Currently when we import the definition of a CXXRecordDecl via the ASTImporter we do not copy over this data.

We had a LLDB expression parsing bug where we would set it to not pass in registers:

setArgPassingRestrictions(clang::RecordDecl::APK_CannotPassInRegs);

but when imported this setting would be lost. So dumping the CXXRecordDecl before importing we would see:

CXXRecordDecl 0x7faaba292e50 <<invalid sloc>> <invalid sloc> struct Bounds definition
|-DefinitionData standard_layout has_user_declared_ctor can_const_default_init
...

but after importing it would show the following:

CXXRecordDecl 0x7ff286823c50 <<invalid sloc>> <invalid sloc> struct Bounds definition
|-DefinitionData pass_in_registers standard_layout has_user_declared_ctor can_const_default_init
                   ^^^^^^^^^^^^^

There will be a separate LLDB PR that will have a test that covers this and introduces a related fix for LLDB.

Note, we did not copy over any other of the RecordDeclBits since we don't have tests for those. We know that copying over LoadedFieldsFromExternalStorage would be a error and that may be the case for others as well.

The companion LLDB review: https://reviews.llvm.org/D61146

[aprantl]

Could we test this by doing -dump-ast of From and To and FileCheck-ing the output?

[teemperor]

In D61140#1479111, @aprantl wrote:

Could we test this by doing -dump-ast of From and To and FileCheck-ing the output?

+1 for this. Importing a simple record should test this? E.g. something like this:

diff --git a/clang/test/Import/cxx-record-flags/Inputs/F.cpp b/clang/test/Import/cxx-record-flags/Inputs/F.cpp
new file mode 100644
index 00000000000..8d1d88c632d
--- /dev/null
+++ b/clang/test/Import/cxx-record-flags/Inputs/F.cpp
@@ -0,0 +1,11 @@
+class FTrivial {
+  int i;
+};
+
+void foo();
+
+struct FNonTrivial {
+  FNonTrivial(const FNonTrivial &F) { foo(); }
+  int i;
+};
+
diff --git a/clang/test/Import/cxx-record-flags/test.cpp b/clang/test/Import/cxx-record-flags/test.cpp
new file mode 100644
index 00000000000..c51558b4793
--- /dev/null
+++ b/clang/test/Import/cxx-record-flags/test.cpp
@@ -0,0 +1,15 @@
+// RUN: clang-import-test -dump-ast -import %S/Inputs/F.cpp -expression %s | FileCheck %s
+
+// CHECK: FTrivial
+// CHECK-NEXT: DefinitionData
+// CHECK-SAME: pass_in_registers
+
+// CHECK: FNonTrivial
+// CHECK-NEXT: DefinitionData
+// CHECK-NOT: pass_in_registers
+// CHECK: DefaultConstructor
+
+void expr() {
+  FTrivial f1;
+  FNonTrivial f2;
+}

[teemperor]

Sorry, that test case actually was a bit too complicated. This seems to work:

diff --git a/clang/test/Import/cxx-record-flags/Inputs/F.cpp b/clang/test/Import/cxx-record-flags/Inputs/F.cpp
new file mode 100644
index 00000000000..1294c67f68d
--- /dev/null
+++ b/clang/test/Import/cxx-record-flags/Inputs/F.cpp
@@ -0,0 +1,9 @@
+class FTrivial {
+  int i;
+};
+
+struct FNonTrivial {
+  virtual ~FNonTrivial() = default;
+  int i;
+};
+
diff --git a/clang/test/Import/cxx-record-flags/test.cpp b/clang/test/Import/cxx-record-flags/test.cpp
new file mode 100644
index 00000000000..bff76274fba
--- /dev/null
+++ b/clang/test/Import/cxx-record-flags/test.cpp
@@ -0,0 +1,14 @@
+// RUN: clang-import-test -dump-ast -import %S/Inputs/F.cpp -expression %s | FileCheck %s
+
+// CHECK: FTrivial
+// CHECK: DefinitionData
+// CHECK-SAME: pass_in_registers
+
+// CHECK: FNonTrivial
+// CHECK-NOT: pass_in_registers
+// CHECK: DefaultConstructor
+
+void expr() {
+  FTrivial f1;
+  FNonTrivial f2;
+}

[a_sidorin]

Hello Shafik!
The patch itself is fine, but, as other reviewers pointed, tests are appreciated. I suggest to add a test into ASTImporterTests.cpp - you will find several ways to write tests of different complexity here. I think this change can be tested even with the simplest testImport() facility.

[shafik]

Added test

[a_sidorin]

Looks good, thanks!

[teemperor]

This has been landed as rC359338 but somehow Phabricator didn't close this. Closing this manually.

lib/AST/ASTImporter.cpp

ToData.ImplicitCopyConstructorCanHaveConstParamForNonVBase
       = FromData.ImplicitCopyConstructorCanHaveConstParamForNonVBase;
     ToData.ImplicitCopyAssignmentHasConstParam
       = FromData.ImplicitCopyAssignmentHasConstParam;
     ToData.HasDeclaredCopyConstructorWithConstParam
       = FromData.HasDeclaredCopyConstructorWithConstParam;
     ToData.HasDeclaredCopyAssignmentWithConstParam
       = FromData.HasDeclaredCopyAssignmentWithConstParam;
 
+    // Copy over the data stored in RecordDeclBits
+    ToCXX->setArgPassingRestrictions(FromCXX->getArgPassingRestrictions());
+
     SmallVector<CXXBaseSpecifier *, 4> Bases;
     for (const auto &Base1 : FromCXX->bases()) {
       ExpectedType TyOrErr = import(Base1.getType());
       if (!TyOrErr)
         return TyOrErr.takeError();
 
       SourceLocation EllipsisLoc;
       if (Base1.isPackExpansion()) {

test/Import/cxx-record-flags/Inputs/F.cpp

+class FTrivial {
+  int i;
+};
+
+struct FNonTrivial {
+  virtual ~FNonTrivial() = default;
+  int i;
+};
+

test/Import/cxx-record-flags/test.cpp

+// RUN: clang-import-test -dump-ast -import %S/Inputs/F.cpp -expression %s | FileCheck %s
+
+// CHECK: FTrivial
+// CHECK: DefinitionData
+// CHECK-SAME: pass_in_registers
+
+// CHECK: FNonTrivial
+// CHECK-NOT: pass_in_registers
+// CHECK: DefaultConstructor
+
+void expr() {
+  FTrivial f1;
+  FNonTrivial f2;
+}