Page MenuHomePhabricator

Copy Argument Passing Restrictions setting when importing a CXXRecordDecl definition
ClosedPublic

Authored by shafik on Apr 25 2019, 11:02 AM.

Details

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

Diff Detail

Event Timeline

shafik created this revision.Apr 25 2019, 11:02 AM
aprantl accepted this revision.Apr 25 2019, 11:05 AM

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

This revision is now accepted and ready to land.Apr 25 2019, 11:05 AM

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;
+}

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;
+}
shafik edited the summary of this revision. (Show Details)Apr 25 2019, 1:42 PM

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 updated this revision to Diff 196757.Apr 25 2019, 4:41 PM

Added test

a_sidorin accepted this revision.Apr 25 2019, 11:53 PM

Looks good, thanks!

martong accepted this revision.Apr 26 2019, 3:06 AM
teemperor closed this revision.May 2 2019, 3:29 AM

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