subject:"\[PATCH\] D101236\: \[ASTImporter\] Import definitions required for layout of \[\[no_unique

[PATCH] D101236: [ASTImporter] Import definitions required for layout of [[no_unique_address]] from LLDB

2021-05-24 Thread Jan Kratochvil via Phabricator via cfe-commits

jankratochvil updated this revision to Diff 347478.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D101236/new/

https://reviews.llvm.org/D101236

Files:
  clang/lib/AST/ASTImporter.cpp
  clang/unittests/AST/ASTImporterTest.cpp
  lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
  lldb/test/Shell/Expr/no_unique_address.cpp
  lldb/test/Shell/SymbolFile/NativePDB/bitfields.cpp

Index: lldb/test/Shell/SymbolFile/NativePDB/bitfields.cpp
===
--- lldb/test/Shell/SymbolFile/NativePDB/bitfields.cpp
+++ lldb/test/Shell/SymbolFile/NativePDB/bitfields.cpp
@@ -44,18 +44,18 @@
 // CHECK: TranslationUnitDecl {{.*}}
 // CHECK: |-CXXRecordDecl {{.*}} struct Struct definition
 // CHECK: | |-FieldDecl {{.*}} A 'int'
-// CHECK: | | `-IntegerLiteral {{.*}} 'int' 5
+// CHECK: | | {{.}}-IntegerLiteral {{.*}} 'int' 5
 // CHECK: | |-FieldDecl {{.*}} B 'int'
-// CHECK: | | `-IntegerLiteral {{.*}} 'int' 7
+// CHECK: | | {{.}}-IntegerLiteral {{.*}} 'int' 7
 // CHECK: | |-FieldDecl {{.*}} C 'unsigned int'
-// CHECK: | | `-IntegerLiteral {{.*}} 'int' 3
+// CHECK: | | {{.}}-IntegerLiteral {{.*}} 'int' 3
 // CHECK: | |-FieldDecl {{.*}} D 'unsigned int'
-// CHECK: | | `-IntegerLiteral {{.*}} 'int' 15
+// CHECK: | | {{.}}-IntegerLiteral {{.*}} 'int' 15
 // CHECK: | |-FieldDecl {{.*}} E 'char'
-// CHECK: | | `-IntegerLiteral {{.*}} 'int' 1
+// CHECK: | | {{.}}-IntegerLiteral {{.*}} 'int' 1
 // CHECK: | |-FieldDecl {{.*}} F 'char'
-// CHECK: | | `-IntegerLiteral {{.*}} 'int' 2
+// CHECK: | | {{.}}-IntegerLiteral {{.*}} 'int' 2
 // CHECK: | |-FieldDecl {{.*}} G 'char'
-// CHECK: | | `-IntegerLiteral {{.*}} 'int' 3
+// CHECK: | | {{.}}-IntegerLiteral {{.*}} 'int' 3
 // CHECK: | `-FieldDecl {{.*}} H 'char'
-// CHECK: |   `-IntegerLiteral {{.*}} 'int' 3
+// CHECK: |   {{.}}-IntegerLiteral {{.*}} 'int' 3
Index: lldb/test/Shell/Expr/no_unique_address.cpp
===
--- /dev/null
+++ lldb/test/Shell/Expr/no_unique_address.cpp
@@ -0,0 +1,15 @@
+// Test LLDB does not assert on miscalculated field offsets.
+
+// RUN: %clang_host %s -g -c -o %t.o
+// RUN: %lldb %t.o -o "p c.c" -o exit | FileCheck %s
+
+// CHECK: (lldb) p c.c
+// CHECK-NEXT: (char) $0 = '\0'
+
+struct A {};
+struct B {
+  [[no_unique_address]] A a;
+};
+struct C : B {
+  char c;
+} c;
Index: lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
===
--- lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
+++ lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
@@ -7196,6 +7196,7 @@
 if (bit_width)
   field->setBitWidth(bit_width);
 SetMemberOwningModule(field, record_decl);
+field->addAttr(NoUniqueAddressAttr::CreateImplicit(ast->getASTContext()));
 
 if (name.empty()) {
   // Determine whether this field corresponds to an anonymous struct or
Index: clang/unittests/AST/ASTImporterTest.cpp
===
--- clang/unittests/AST/ASTImporterTest.cpp
+++ clang/unittests/AST/ASTImporterTest.cpp
@@ -6357,6 +6357,78 @@
 ToD->getTemplateSpecializationKind());
 }
 
+struct LLDBMinimalImport : ASTImporterOptionSpecificTestBase {
+  LLDBMinimalImport() {
+Creator = [](ASTContext , FileManager ,
+ ASTContext , FileManager ,
+ bool MinimalImport,
+ const std::shared_ptr ) {
+  auto retval = new ASTImporter(ToContext, ToFileManager, FromContext,
+FromFileManager, true /*MinimalImport*/,
+// We use the regular lookup.
+/*SharedState=*/nullptr);
+  retval->setODRHandling(clang::ASTImporter::ODRHandlingType::Liberal);
+  return retval;
+};
+  }
+};
+
+TEST_P(LLDBMinimalImport, LLDBImportNoUniqueAddress) {
+  TranslationUnitDecl *FromTU = getTuDecl(
+  R"(
+// lldb/test/API/commands/expression/codegen-crash-typedefdecl-not-in_declcontext/main.cpp
+template  struct DWrapper {};
+typedef DWrapper DW;
+struct B {
+  DW spd;
+} b;
+struct E {
+  B _ref = b;
+  static DW f() { return {}; }
+} e;
+  )",
+  Lang_CXX11);
+
+  auto *FromE = FirstDeclMatcher().match(FromTU, varDecl(hasName("e")));
+
+#if 0
+  // Set up a stub external storage.
+  FromTU->setHasExternalLexicalStorage(true);
+  // Set up DeclContextBits.HasLazyExternalLexicalLookups to true.
+  FromTU->setMustBuildLookupTable();
+  struct TestExternalASTSource : ExternalASTSource {};
+  FromTU->getASTContext().setExternalSource(new TestExternalASTSource());
+#endif
+
+  TranslationUnitDecl *ToTU = getToTuDecl(
+  R"(
+struct E;
+extern E e;
+  )",
+  Lang_CXX11);
+  auto *ToE = FirstDeclMatcher().match(ToTU, varDecl(hasName("e")));
+
+#if 1
+  // Set up a stub external storage.
+

[PATCH] D101236: [ASTImporter] Import definitions required for layout of [[no_unique_address]] from LLDB

2021-05-24 Thread Jan Kratochvil via Phabricator via cfe-commits

jankratochvil added a comment.

  cat >1.cpp < struct DWrapper {};
  typedef DWrapper DW;
  struct B {
DW spd;
  } b;
  struct E {
B _ref = b;
static DW f() { return {}; }
  } e;
  EOH
  $ (set -ex;./bin/clang -c -o 1.o 1.cpp -Wall -g;./bin/lldb -b ./1.o -o 'p E' 
-o 'p e')
  (lldb) p E
  (lldb) p e
  lldb: .../clang/lib/AST/Decl.cpp:4188: bool 
clang::FieldDecl::isZeroSize(const clang::ASTContext &) const: Assertion 
`isInvalidDecl() && "valid field has incomplete type"' failed.

So importing just the type `E` is insufficient but I somehow cannot import the 
variable `e`:

auto *FromE = FirstDeclMatcher().match(FromTU, 
varDecl(hasName("e")));
  ...
// ASTTests: .../llvm/include/llvm/Support/Casting.h:269: typename 
cast_retty::ret_type llvm::cast(Y *) [X = clang::DeclContext, Y = 
clang::Decl]: Assertion `isa(Val) && "cast() argument of incompatible 
type!"' failed.
llvm::Error Err = findFromTU(FromE)->Importer->ImportDefinition(FromE);


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D101236/new/

https://reviews.llvm.org/D101236

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D101236: [ASTImporter] Import definitions required for layout of [[no_unique_address]] from LLDB

2021-05-24 Thread Jan Kratochvil via Phabricator via cfe-commits

jankratochvil updated this revision to Diff 347475.
jankratochvil added a comment.

Update of the testcase but it still does not reproduce the LLDB problem.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D101236/new/

https://reviews.llvm.org/D101236

Files:
  clang/lib/AST/ASTImporter.cpp
  clang/unittests/AST/ASTImporterTest.cpp
  lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
  lldb/test/Shell/Expr/no_unique_address.cpp
  lldb/test/Shell/SymbolFile/NativePDB/bitfields.cpp

Index: lldb/test/Shell/SymbolFile/NativePDB/bitfields.cpp
===
--- lldb/test/Shell/SymbolFile/NativePDB/bitfields.cpp
+++ lldb/test/Shell/SymbolFile/NativePDB/bitfields.cpp
@@ -44,18 +44,18 @@
 // CHECK: TranslationUnitDecl {{.*}}
 // CHECK: |-CXXRecordDecl {{.*}} struct Struct definition
 // CHECK: | |-FieldDecl {{.*}} A 'int'
-// CHECK: | | `-IntegerLiteral {{.*}} 'int' 5
+// CHECK: | | {{.}}-IntegerLiteral {{.*}} 'int' 5
 // CHECK: | |-FieldDecl {{.*}} B 'int'
-// CHECK: | | `-IntegerLiteral {{.*}} 'int' 7
+// CHECK: | | {{.}}-IntegerLiteral {{.*}} 'int' 7
 // CHECK: | |-FieldDecl {{.*}} C 'unsigned int'
-// CHECK: | | `-IntegerLiteral {{.*}} 'int' 3
+// CHECK: | | {{.}}-IntegerLiteral {{.*}} 'int' 3
 // CHECK: | |-FieldDecl {{.*}} D 'unsigned int'
-// CHECK: | | `-IntegerLiteral {{.*}} 'int' 15
+// CHECK: | | {{.}}-IntegerLiteral {{.*}} 'int' 15
 // CHECK: | |-FieldDecl {{.*}} E 'char'
-// CHECK: | | `-IntegerLiteral {{.*}} 'int' 1
+// CHECK: | | {{.}}-IntegerLiteral {{.*}} 'int' 1
 // CHECK: | |-FieldDecl {{.*}} F 'char'
-// CHECK: | | `-IntegerLiteral {{.*}} 'int' 2
+// CHECK: | | {{.}}-IntegerLiteral {{.*}} 'int' 2
 // CHECK: | |-FieldDecl {{.*}} G 'char'
-// CHECK: | | `-IntegerLiteral {{.*}} 'int' 3
+// CHECK: | | {{.}}-IntegerLiteral {{.*}} 'int' 3
 // CHECK: | `-FieldDecl {{.*}} H 'char'
-// CHECK: |   `-IntegerLiteral {{.*}} 'int' 3
+// CHECK: |   {{.}}-IntegerLiteral {{.*}} 'int' 3
Index: lldb/test/Shell/Expr/no_unique_address.cpp
===
--- /dev/null
+++ lldb/test/Shell/Expr/no_unique_address.cpp
@@ -0,0 +1,15 @@
+// Test LLDB does not assert on miscalculated field offsets.
+
+// RUN: %clang_host %s -g -c -o %t.o
+// RUN: %lldb %t.o -o "p c.c" -o exit | FileCheck %s
+
+// CHECK: (lldb) p c.c
+// CHECK-NEXT: (char) $0 = '\0'
+
+struct A {};
+struct B {
+  [[no_unique_address]] A a;
+};
+struct C : B {
+  char c;
+} c;
Index: lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
===
--- lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
+++ lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
@@ -7196,6 +7196,7 @@
 if (bit_width)
   field->setBitWidth(bit_width);
 SetMemberOwningModule(field, record_decl);
+field->addAttr(NoUniqueAddressAttr::CreateImplicit(ast->getASTContext()));
 
 if (name.empty()) {
   // Determine whether this field corresponds to an anonymous struct or
Index: clang/unittests/AST/ASTImporterTest.cpp
===
--- clang/unittests/AST/ASTImporterTest.cpp
+++ clang/unittests/AST/ASTImporterTest.cpp
@@ -6357,6 +6357,78 @@
 ToD->getTemplateSpecializationKind());
 }
 
+struct LLDBMinimalImport : ASTImporterOptionSpecificTestBase {
+  LLDBMinimalImport() {
+Creator = [](ASTContext , FileManager ,
+ ASTContext , FileManager ,
+ bool MinimalImport,
+ const std::shared_ptr ) {
+  auto retval = new ASTImporter(ToContext, ToFileManager, FromContext,
+FromFileManager, true /*MinimalImport*/,
+// We use the regular lookup.
+/*SharedState=*/nullptr);
+  retval->setODRHandling(clang::ASTImporter::ODRHandlingType::Liberal);
+  return retval;
+};
+  }
+};
+
+TEST_P(LLDBMinimalImport, LLDBImportNoUniqueAddress) {
+  TranslationUnitDecl *FromTU = getTuDecl(
+  R"(
+// lldb/test/API/commands/expression/codegen-crash-typedefdecl-not-in_declcontext/main.cpp
+template  struct DWrapper {};
+typedef DWrapper DW;
+struct B {
+  DW spd;
+} b;
+struct E {
+  B _ref = b;
+  static DW f() { return {}; }
+} e;
+  )",
+  Lang_CXX11);
+
+  auto *FromE = FirstDeclMatcher().match(
+  FromTU, cxxRecordDecl(hasName("E")));
+
+#if 0
+  // Set up a stub external storage.
+  FromTU->setHasExternalLexicalStorage(true);
+  // Set up DeclContextBits.HasLazyExternalLexicalLookups to true.
+  FromTU->setMustBuildLookupTable();
+  struct TestExternalASTSource : ExternalASTSource {};
+  FromTU->getASTContext().setExternalSource(new TestExternalASTSource());
+#endif
+
+  TranslationUnitDecl *ToTU = getToTuDecl(
+  R"(
+struct E;
+  )",
+  Lang_CXX11);
+  auto *ToE = FirstDeclMatcher().match(
+

[PATCH] D101236: [ASTImporter] Import definitions required for layout of [[no_unique_address]] from LLDB

2021-05-24 Thread Jan Kratochvil via Phabricator via cfe-commits

jankratochvil added inline comments.



Comment at: clang/lib/AST/ASTImporter.cpp:3684
+  return ToAttrOrErr.takeError();
+#if 0
+RecordType *FromRT =

It is still not working. The testcase either PASSes despite this `#if 0` or it 
FAILs even if it is `#if 1`. Sure the goal is to reproduce the LLDB testcase 
behavior - PASS with `#if 1` and FAIL with `#if 0`.
With libstdc++11 and this form of patch it fails on:
```
  lldb-api :: 
commands/expression/codegen-crash-typedefdecl-not-in_declcontext/TestCodegenCrashTypedefDeclNotInDeclContext.py
  lldb-api :: 
commands/expression/completion-crash-incomplete-record/TestCompletionCrashIncompleteRecord.py
  lldb-api :: 
commands/expression/import-std-module/deque-dbg-info-content/TestDbgInfoContentDequeFromStdModule.py
  lldb-api :: 
functionalities/data-formatter/data-formatter-stl/libcxx/map/TestDataFormatterLibccMap.py
```
It also fails for this one but there are reasons not interesting for this patch:
```
  lldb-api :: 
functionalities/data-formatter/data-formatter-stl/libstdcpp/unique_ptr/TestDataFormatterStdUniquePtr.py
```
These two are too complex to debug IMO:
```
  lldb-api :: 
commands/expression/import-std-module/deque-dbg-info-content/TestDbgInfoContentDequeFromStdModule.py
  lldb-api :: 
functionalities/data-formatter/data-formatter-stl/libcxx/map/TestDataFormatterLibccMap.py
```
I tried to reproduce the first two, this one has [[ 
https://people.redhat.com/jkratoch/lldbbt1.txt | such backtrace ]]:
```
  lldb-api :: 
commands/expression/codegen-crash-typedefdecl-not-in_declcontext/TestCodegenCrashTypedefDeclNotInDeclContext.py
```



Comment at: clang/unittests/AST/ASTImporterTest.cpp:6441
+  findFromTU(FromB)->Importer->Imported(FromB, ToB);
+  llvm::Error Err = findFromTU(FromB)->Importer->ImportDefinition(FromB);
+  EXPECT_FALSE((bool)Err);

This code is trying to mimic the LLDB behavior from the backtrace of LLDB 
testcase: https://people.redhat.com/jkratoch/lldbbt1.txt



Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D101236/new/

https://reviews.llvm.org/D101236

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D101236: [ASTImporter] Import definitions required for layout of [[no_unique_address]] from LLDB

2021-05-24 Thread Jan Kratochvil via Phabricator via cfe-commits

jankratochvil updated this revision to Diff 347315.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D101236/new/

https://reviews.llvm.org/D101236

Files:
  clang/lib/AST/ASTImporter.cpp
  clang/unittests/AST/ASTImporterTest.cpp
  lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
  lldb/test/Shell/Expr/no_unique_address.cpp
  lldb/test/Shell/SymbolFile/NativePDB/bitfields.cpp

Index: lldb/test/Shell/SymbolFile/NativePDB/bitfields.cpp
===
--- lldb/test/Shell/SymbolFile/NativePDB/bitfields.cpp
+++ lldb/test/Shell/SymbolFile/NativePDB/bitfields.cpp
@@ -44,18 +44,18 @@
 // CHECK: TranslationUnitDecl {{.*}}
 // CHECK: |-CXXRecordDecl {{.*}} struct Struct definition
 // CHECK: | |-FieldDecl {{.*}} A 'int'
-// CHECK: | | `-IntegerLiteral {{.*}} 'int' 5
+// CHECK: | | {{.}}-IntegerLiteral {{.*}} 'int' 5
 // CHECK: | |-FieldDecl {{.*}} B 'int'
-// CHECK: | | `-IntegerLiteral {{.*}} 'int' 7
+// CHECK: | | {{.}}-IntegerLiteral {{.*}} 'int' 7
 // CHECK: | |-FieldDecl {{.*}} C 'unsigned int'
-// CHECK: | | `-IntegerLiteral {{.*}} 'int' 3
+// CHECK: | | {{.}}-IntegerLiteral {{.*}} 'int' 3
 // CHECK: | |-FieldDecl {{.*}} D 'unsigned int'
-// CHECK: | | `-IntegerLiteral {{.*}} 'int' 15
+// CHECK: | | {{.}}-IntegerLiteral {{.*}} 'int' 15
 // CHECK: | |-FieldDecl {{.*}} E 'char'
-// CHECK: | | `-IntegerLiteral {{.*}} 'int' 1
+// CHECK: | | {{.}}-IntegerLiteral {{.*}} 'int' 1
 // CHECK: | |-FieldDecl {{.*}} F 'char'
-// CHECK: | | `-IntegerLiteral {{.*}} 'int' 2
+// CHECK: | | {{.}}-IntegerLiteral {{.*}} 'int' 2
 // CHECK: | |-FieldDecl {{.*}} G 'char'
-// CHECK: | | `-IntegerLiteral {{.*}} 'int' 3
+// CHECK: | | {{.}}-IntegerLiteral {{.*}} 'int' 3
 // CHECK: | `-FieldDecl {{.*}} H 'char'
-// CHECK: |   `-IntegerLiteral {{.*}} 'int' 3
+// CHECK: |   {{.}}-IntegerLiteral {{.*}} 'int' 3
Index: lldb/test/Shell/Expr/no_unique_address.cpp
===
--- /dev/null
+++ lldb/test/Shell/Expr/no_unique_address.cpp
@@ -0,0 +1,15 @@
+// Test LLDB does not assert on miscalculated field offsets.
+
+// RUN: %clang_host %s -g -c -o %t.o
+// RUN: %lldb %t.o -o "p c.c" -o exit | FileCheck %s
+
+// CHECK: (lldb) p c.c
+// CHECK-NEXT: (char) $0 = '\0'
+
+struct A {};
+struct B {
+  [[no_unique_address]] A a;
+};
+struct C : B {
+  char c;
+} c;
Index: lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
===
--- lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
+++ lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
@@ -7196,6 +7196,7 @@
 if (bit_width)
   field->setBitWidth(bit_width);
 SetMemberOwningModule(field, record_decl);
+field->addAttr(NoUniqueAddressAttr::CreateImplicit(ast->getASTContext()));
 
 if (name.empty()) {
   // Determine whether this field corresponds to an anonymous struct or
Index: clang/unittests/AST/ASTImporterTest.cpp
===
--- clang/unittests/AST/ASTImporterTest.cpp
+++ clang/unittests/AST/ASTImporterTest.cpp
@@ -6357,6 +6357,92 @@
 ToD->getTemplateSpecializationKind());
 }
 
+struct LLDBMinimalImport : ASTImporterOptionSpecificTestBase {
+  LLDBMinimalImport() {
+Creator = [](ASTContext , FileManager ,
+ ASTContext , FileManager ,
+ bool MinimalImport,
+ const std::shared_ptr ) {
+  auto retval = new ASTImporter(ToContext, ToFileManager, FromContext,
+FromFileManager, true /*MinimalImport*/,
+// We use the regular lookup.
+/*SharedState=*/nullptr);
+  retval->setODRHandling(clang::ASTImporter::ODRHandlingType::Liberal);
+  return retval;
+};
+  }
+};
+
+TEST_P(LLDBMinimalImport, LLDBImportNoUniqueAddress) {
+  TranslationUnitDecl *FromTU = getTuDecl(
+  R"(
+// lldb/test/API/commands/expression/codegen-crash-typedefdecl-not-in_declcontext/main.cpp
+template  struct DWrapper {};
+
+struct D {};
+
+namespace NS {
+typedef DWrapper DW;
+}
+
+struct B {
+  NS::DW spd;
+  int a = 0;
+};
+
+struct E {
+  E(B ) : b_ref(b) {}
+  NS::DW f() { return {}; };
+  void g() {
+return; //%self.expect("p b_ref", substrs=['(B) $0 =', '(spd = NS::DW', 'a = 0)'])
+  }
+
+  B _ref;
+};
+B b;
+E e(b);
+  )",
+  Lang_CXX11);
+
+  auto *FromB = FirstDeclMatcher().match(
+  FromTU, cxxRecordDecl(hasName("B")));
+
+#if 0
+  // Set up a stub external storage.
+  FromTU->setHasExternalLexicalStorage(true);
+  // Set up DeclContextBits.HasLazyExternalLexicalLookups to true.
+  FromTU->setMustBuildLookupTable();
+  struct TestExternalASTSource : ExternalASTSource {};
+  FromTU->getASTContext().setExternalSource(new

[PATCH] D101236: [ASTImporter] Import definitions required for layout of [[no_unique_address]] from LLDB

2021-05-06 Thread Raphael Isemann via Phabricator via cfe-commits

teemperor added a comment.

Just to be clear, I am not sure when/if D101950 
 is ready. It's mostly blocked by me not 
having the time to work on it, so depending how my schedule is this might land 
soon or far in the future. Just linked it here as this (temporary) workaround 
would be on the list of things I would revert when D101950 
 lands. But in the meantime I'm ok with this 
workaround patch as I said above.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D101236/new/

https://reviews.llvm.org/D101236

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D101236: [ASTImporter] Import definitions required for layout of [[no_unique_address]] from LLDB

2021-05-06 Thread Jan Kratochvil via Phabricator via cfe-commits

jankratochvil abandoned this revision.
jankratochvil added a comment.

IIUC it will get replaced by D101950 .


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D101236/new/

https://reviews.llvm.org/D101236

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D101236: [ASTImporter] Import definitions required for layout of [[no_unique_address]] from LLDB

2021-05-05 Thread Raphael Isemann via Phabricator via cfe-commits

teemperor added a comment.

By the way, I set up a WIP patch for changing LLDB's import away from the 
current MinimalImport of records to something that is more in line with how 
Clang works: https://reviews.llvm.org/D101950


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D101236/new/

https://reviews.llvm.org/D101236

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D101236: [ASTImporter] Import definitions required for layout of [[no_unique_address]] from LLDB

2021-05-03 Thread Gabor Marton via Phabricator via cfe-commits

martong added inline comments.



Comment at: clang/unittests/AST/ASTImporterTest.cpp:6408
+  // Import the definition of the created class.
+  llvm::Error Err = findFromTU(FromC)->Importer->ImportDefinition(ToC);
+  EXPECT_FALSE((bool)Err);

I suppose that the problem is you try to import the definition of a class that 
is in the destination context (`ToC`). That class is not imported properly yet, 
and I guess that is why its source location is not yet imported also (basically 
that's the reason of the sloc assertion).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D101236/new/

https://reviews.llvm.org/D101236

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D101236: [ASTImporter] Import definitions required for layout of [[no_unique_address]] from LLDB

2021-05-02 Thread Jan Kratochvil via Phabricator via cfe-commits

jankratochvil added a comment.

In D101236#2716317 , @martong wrote:

> You can create a similar descendant class, but with the MinimalImport flag 
> set to true. Then you could call `ImportDefinition` subsequently after an 
> `Import` call. Perhaps that could trigger your assertion.

Do you have another hint or should I try harder? Thanks.

  clang/lib/Basic/SourceManager.cpp:865: clang::FileID 
clang::SourceManager::getFileIDLoaded(unsigned int) const: Assertion `0 && 
"Invalid SLocOffset or bad function choice"' failed.
  * thread #1, name = 'ASTTests', stop reason = hit program assert
  frame #4: 0x028d7195 
ASTTests`clang::SourceManager::getFileIDLoaded(this=0x04ecf640, 
SLocOffset=14405) const at SourceManager.cpp:865:5
 862  FileID SourceManager::getFileIDLoaded(unsigned SLocOffset) const {
 863// Sanity checking, otherwise a bug may lead to hanging in release 
build.
 864if (SLocOffset < CurrentLoadedOffset) {
  -> 865  assert(0 && "Invalid SLocOffset or bad function choice");
 866  return FileID();
 867}
 868
  (lldb) p SLocOffset
  (unsigned int) $0 = 14405
  (lldb) p CurrentLoadedOffset
  (unsigned int) $1 = 2147483648
  (lldb) bt
  frame # 3: libc.so.6`__assert_fail + 70
* frame # 4: 
ASTTests`clang::SourceManager::getFileIDLoaded(this=0x04ecf640, 
SLocOffset=14405) const at SourceManager.cpp:865:5
  frame # 5: 
ASTTests`clang::SourceManager::getFileIDSlow(this=0x04ecf640, 
SLocOffset=14405) const at SourceManager.cpp:773:10
  frame # 6: 
ASTTests`clang::SourceManager::getFileID(clang::SourceLocation) const at 
SourceManager.h:1107:12
  frame # 7: 
ASTTests`clang::SourceManager::getDecomposedExpansionLoc(this=0x04ecf640,
 Loc=(ID = 14405)) const at SourceManager.h:1247:18
  frame # 8: 
ASTTests`clang::SourceManager::getPresumedLoc(this=0x04ecf640, Loc=(ID 
= 14405), UseLineDirectives=true) const at SourceManager.cpp:1521:41
  frame # 9: 
ASTTests`clang::SourceManager::isWrittenInBuiltinFile(this=0x04ecf640, 
Loc=(ID = 14405)) const at SourceManager.h:1468:24
  frame #10: ASTTests`clang::ASTImporter::Import(this=0x778f9010, 
FromLoc=(ID = 14405)) at ASTImporter.cpp:8834:27
  frame #11: ASTTests`llvm::Error 
clang::ASTImporter::importInto(this=0x778f9010, 
To=0x7fffbe68, From=0x7fffb8e8) at ASTImporter.h:336:22
  frame #12: ASTTests`llvm::Error 
clang::ASTNodeImporter::importInto(this=0x7fffbf60,
 To=0x7fffbe68, From=0x7fffb8e8) at ASTImporter.cpp:148:23
  frame #13: 
ASTTests`clang::ASTNodeImporter::ImportDeclParts(this=0x7fffbf60, 
D=0x04eb8190, DC=0x7fffbe80, LexicalDC=0x7fffbe78, 
Name=0x7fffbe70, ToD=0x7fffbe60, Loc=0x7fffbe68) at 
ASTImporter.cpp:1654:19
  frame #14: 
ASTTests`clang::ASTNodeImporter::VisitRecordDecl(this=0x7fffbf60, 
D=0x04eb8190) at ASTImporter.cpp:2772:19
  frame #15: ASTTests`clang::declvisitor::Base 
>::VisitCXXRecordDecl(this=0x7fffbf60, D=0x04eb8190) at 
DeclNodes.inc:263:1
  frame #16: ASTTests`clang::declvisitor::Base 
>::Visit(this=0x7fffbf60, D=0x04eb8190) at DeclNodes.inc:263:1
  frame #17: 
ASTTests`clang::ASTImporter::ImportImpl(this=0x778f9010, 
FromD=0x04eb8190) at ASTImporter.cpp:8216:19
  frame #18: ASTTests`clang::ASTImporter::Import(this=0x778f9010, 
FromD=0x04eb8190) at ASTImporter.cpp:8387:27
  frame #19: ASTTests`llvm::Expected 
clang::ASTNodeImporter::import(this=0x7fffc6c8, 
From=0x04eb8190) at ASTImporter.cpp:164:31
  frame #20: 
ASTTests`clang::ASTNodeImporter::ImportDeclContext(this=0x7fffc6c8, 
FromDC=0x04e886a8, ForceImport=true) at ASTImporter.cpp:1777:34
  frame #21: 
ASTTests`clang::ASTImporter::ImportDefinition(this=0x778f9010, 
From=0x04e88668) at ASTImporter.cpp:9072:19
  frame #22: 
ASTTests`clang::ast_matchers::LLDBMinimalImport_LLDBImportNoUniqueAddress_Test::TestBody()
 at ASTImporterTest.cpp:6408:50


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D101236/new/

https://reviews.llvm.org/D101236

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D101236: [ASTImporter] Import definitions required for layout of [[no_unique_address]] from LLDB

2021-05-02 Thread Jan Kratochvil via Phabricator via cfe-commits

jankratochvil updated this revision to Diff 342249.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D101236/new/

https://reviews.llvm.org/D101236

Files:
  clang/lib/AST/ASTImporter.cpp
  clang/unittests/AST/ASTImporterTest.cpp

Index: clang/unittests/AST/ASTImporterTest.cpp
===
--- clang/unittests/AST/ASTImporterTest.cpp
+++ clang/unittests/AST/ASTImporterTest.cpp
@@ -6323,6 +6323,105 @@
 ToD->getTemplateSpecializationKind());
 }
 
+TEST_P(ASTImporterOptionSpecificTestBase, ImportNoUniqueAddress) {
+  Decl *FromTU = getTuDecl(
+  R"(
+  struct A {};
+  struct B {
+[[no_unique_address]] A a;
+  };
+  struct C : B {
+char c;
+  } c;
+  )", Lang_CXX20);
+
+  auto *BFromD = FirstDeclMatcher().match(
+  FromTU, cxxRecordDecl(hasName("B")));
+  ASSERT_TRUE(BFromD);
+  auto *BToD = Import(BFromD, Lang_CXX20);
+  EXPECT_TRUE(BToD);
+
+  auto *CFromD = FirstDeclMatcher().match(
+  FromTU, cxxRecordDecl(hasName("C")));
+  ASSERT_TRUE(CFromD);
+  auto *CToD = Import(CFromD, Lang_CXX20);
+  EXPECT_TRUE(CToD);
+
+  QualType CFType = FromTU->getASTContext().getRecordType(CFromD);
+  TypeInfo TFI = FromTU->getASTContext().getTypeInfo(CFType);
+  ASSERT_EQ(TFI.Width, 8U);
+  QualType CTType = FromTU->getASTContext().getRecordType(CToD);
+  TypeInfo TTI = FromTU->getASTContext().getTypeInfo(CTType);
+  // Here we test the NoUniqueAddressAttr import.
+  // But the Record definition part is not tested here.
+  ASSERT_EQ(TTI.Width, TFI.Width);
+}
+
+struct LLDBMinimalImport : ASTImporterOptionSpecificTestBase {
+  LLDBMinimalImport() {
+Creator = [](ASTContext , FileManager ,
+ ASTContext , FileManager ,
+ bool MinimalImport,
+ const std::shared_ptr ) {
+  return new ASTImporter(ToContext, ToFileManager, FromContext,
+ FromFileManager, true /*MinimalImport*/,
+ // We use the regular lookup.
+ /*SharedState=*/nullptr);
+};
+  }
+};
+
+TEST_P(LLDBMinimalImport, LLDBImportNoUniqueAddress) {
+  TranslationUnitDecl *ToTU = getToTuDecl(
+  R"(
+  struct A {};
+  struct B {
+[[no_unique_address]] A a;
+  };
+  struct C : B {
+char c;
+  } c;
+  )", Lang_CXX20);
+  auto *ToC = FirstDeclMatcher().match(
+  ToTU, cxxRecordDecl(hasName("C")));
+
+  // Set up a stub external storage.
+  ToTU->setHasExternalLexicalStorage(true);
+  // Set up DeclContextBits.HasLazyExternalLexicalLookups to true.
+  ToTU->setMustBuildLookupTable();
+  struct TestExternalASTSource : ExternalASTSource {};
+  ToTU->getASTContext().setExternalSource(new TestExternalASTSource());
+
+  Decl *FromTU = getTuDecl(
+  R"(
+struct C;
+  )",
+  Lang_CXX20);
+  auto *FromC = FirstDeclMatcher().match(
+  FromTU, cxxRecordDecl(hasName("C")));
+  auto *ImportedC = Import(FromC, Lang_CXX20);
+  // The lookup must find the existing class definition in the LinkageSpecDecl.
+  // Then the importer renders the existing and the new decl into one chain.
+  EXPECT_EQ(ImportedC->getCanonicalDecl(), ToC->getCanonicalDecl());
+
+  // Import the definition of the created class.
+  llvm::Error Err = findFromTU(FromC)->Importer->ImportDefinition(ToC);
+  EXPECT_FALSE((bool)Err);
+  consumeError(std::move(Err));
+
+  QualType CTType = ToTU->getASTContext().getRecordType(ToC);
+  TypeInfo TTI = ToTU->getASTContext().getTypeInfo(CTType);
+  fprintf(stderr,"LLDB To Width=%lu\n",TTI.Width);
+#if 0 // getASTRecordLayout(const clang::RecordDecl *) const: Assertion `D && "Cannot get layout of forward declarations!"' failed.
+  QualType CFType = FromTU->getASTContext().getRecordType(FromC);
+  TypeInfo TFI = FromTU->getASTContext().getTypeInfo(CFType);
+  fprintf(stderr,"LLDB From Width=%lu\n",TFI.Width);
+#endif
+  QualType CIType = FromTU->getASTContext().getRecordType(ImportedC);
+  TypeInfo TII = FromTU->getASTContext().getTypeInfo(CIType);
+  fprintf(stderr,"LLDB Imported Width=%lu\n",TII.Width);
+}
+
 INSTANTIATE_TEST_CASE_P(ParameterizedTests, ASTImporterLookupTableTest,
 DefaultTestValuesForRunOptions, );
 
@@ -6397,5 +6496,8 @@
 INSTANTIATE_TEST_CASE_P(ParameterizedTests, ImportWithExternalSource,
 DefaultTestValuesForRunOptions, );
 
+INSTANTIATE_TEST_CASE_P(ParameterizedTests, LLDBMinimalImport,
+DefaultTestValuesForRunOptions, );
+
 } // end namespace ast_matchers
 } // end namespace clang
Index: clang/lib/AST/ASTImporter.cpp
===
--- clang/lib/AST/ASTImporter.cpp
+++ clang/lib/AST/ASTImporter.cpp
@@ -3673,6 +3673,30 @@
   D->getInClassInitStyle()))
 return ToField;
 
+  // FieldDecl::isZeroSize may need to check these.
+  if (const

[PATCH] D101236: [ASTImporter] Import definitions required for layout of [[no_unique_address]] from LLDB

2021-04-26 Thread Jan Kratochvil via Phabricator via cfe-commits

jankratochvil planned changes to this revision.
jankratochvil added a comment.

In D101236#2716286 , @teemperor wrote:

> Not sure what the backtrace is for the `clang::FieldDecl::isZeroSize` crash 
> but what might relevant:

That happens only with half of the patch: 
https://people.redhat.com/jkratoch/lldb-iszerosize.patch
And only for these testcases:

  lldb-api :: 
commands/expression/codegen-crash-typedefdecl-not-in_declcontext/TestCodegenCrashTypedefDeclNotInDeclContext.py
  lldb-api :: 
commands/expression/completion-crash-incomplete-record/TestCompletionCrashIncompleteRecord.py
  lldb-api :: 
functionalities/data-formatter/data-formatter-stl/libcxx/map/TestDataFormatterLibccMap.py

The backtrace is: https://people.redhat.com/jkratoch/lldb-iszerosize.txt

> IMHO this is fine as a temporary solution until we finally fix the LLDB Decl 
> completion.

Great, thanks.

In D101236#2716317 , @martong wrote:

> You can create a similar descendant class, but with the MinimalImport flag 
> set to true. Then you could call `ImportDefinition` subsequently after an 
> `Import` call. Perhaps that could trigger your assertion.

I have been looking for such suggestion, thanks. I will try that.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D101236/new/

https://reviews.llvm.org/D101236

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D101236: [ASTImporter] Import definitions required for layout of [[no_unique_address]] from LLDB

2021-04-26 Thread Gabor Marton via Phabricator via cfe-commits

martong added a comment.

To provide unit tests that mimic the LLDB setup, a good starting point could be 
an existing test case:

  struct LLDBLookupTest : ASTImporterOptionSpecificTestBase {

You can create a similar descendant class, but with the MinimalImport flag set 
to true. Then you could call `ImportDefinition` subsequently after an `Import` 
call. Perhaps that could trigger your assertion.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D101236/new/

https://reviews.llvm.org/D101236

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D101236: [ASTImporter] Import definitions required for layout of [[no_unique_address]] from LLDB

2021-04-26 Thread Raphael Isemann via Phabricator via cfe-commits

teemperor added a comment.

Not sure what the backtrace is for the `clang::FieldDecl::isZeroSize` crash but 
what might relevant:

1. The ASTImporter test isn't using the 'Minimal' import mode that LLDB is 
using. In the tests we are importing all declarations directly. In LLDB we use 
the 'minimal' mode where we try to import declarations lazily (but the setup 
for that is wrong, hence why it's crashing for you).
2. The ASTImporter tests aren't generating code for imported nodes (at least to 
my knowledge?), so you can't reproduce CG crashes here.

If the assert here is actually for the FieldDecl not having a complete type 
then I think the actual problem is point 1.

Looking at the code we do actually get a request to complete the type before 
the assert:

  const RecordDecl *RD = RT->getDecl()->getDefinition();
  if (!RD) {
assert(isInvalidDecl() && "valid field has incomplete type");
return false;
  }

The call to `getDefinition` is expected to pull in the definition but this 
won't happen in the current LLDB implementation (as we don't properly implement 
that interface). In the past we worked around this issue in the same way as in 
this patch, so IMHO this is fine as a temporary solution until we finally fix 
the LLDB Decl completion. But I guess it's also a question of how much LLDB 
workarounds the ASTImporter maintainers want to have in their code until this 
happens.

(Side note: I think `isInvalidDecl` is also true when we run into semantic 
errors and just mark the Decl as being malformed, so I'm not sure the assert 
text can be fully trusted here. But if eagerly completing the type fixes the 
issue then I guess it's actually the incomplete type that causes the assert to 
trigger)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D101236/new/

https://reviews.llvm.org/D101236

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D101236: [ASTImporter] Import definitions required for layout of [[no_unique_address]] from LLDB

2021-04-25 Thread Jan Kratochvil via Phabricator via cfe-commits

jankratochvil updated this revision to Diff 340340.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D101236/new/

https://reviews.llvm.org/D101236

Files:
  clang/lib/AST/ASTImporter.cpp
  clang/unittests/AST/ASTImporterTest.cpp


Index: clang/unittests/AST/ASTImporterTest.cpp
===
--- clang/unittests/AST/ASTImporterTest.cpp
+++ clang/unittests/AST/ASTImporterTest.cpp
@@ -6323,6 +6323,33 @@
 ToD->getTemplateSpecializationKind());
 }
 
+TEST_P(ASTImporterOptionSpecificTestBase, ImportNoUniqueAddress) {
+  Decl *FromTU = getTuDecl(
+  R"(
+  struct A {};
+  struct B {
+[[no_unique_address]] A a;
+  };
+  struct C : B {
+char c;
+  } c;
+  )", Lang_CXX20);
+
+  // It does not fail even without the patch!
+  auto *BFromD = FirstDeclMatcher().match(
+  FromTU, cxxRecordDecl(hasName("B")));
+  ASSERT_TRUE(BFromD);
+  auto *BToD = Import(BFromD, Lang_CXX20);
+  EXPECT_TRUE(BToD);
+
+  // It does not fail even without the patch!
+  auto *CFromD = FirstDeclMatcher().match(
+  FromTU, cxxRecordDecl(hasName("C")));
+  ASSERT_TRUE(CFromD);
+  auto *CToD = Import(CFromD, Lang_CXX20);
+  EXPECT_TRUE(CToD);
+}
+
 INSTANTIATE_TEST_CASE_P(ParameterizedTests, ASTImporterLookupTableTest,
 DefaultTestValuesForRunOptions, );
 
Index: clang/lib/AST/ASTImporter.cpp
===
--- clang/lib/AST/ASTImporter.cpp
+++ clang/lib/AST/ASTImporter.cpp
@@ -3673,6 +3673,28 @@
   D->getInClassInitStyle()))
 return ToField;
 
+  // FieldDecl::isZeroSize may need to check these.
+  if (const Attr *FromAttr = D->getAttr()) {
+if (auto ToAttrOrErr = Importer.Import(FromAttr))
+  ToField->addAttr(*ToAttrOrErr);
+else
+  return ToAttrOrErr.takeError();
+RecordType *FromRT =
+const_cast(D->getType()->getAs());
+// Is this field a struct? FieldDecl::isZeroSize will need its definition.
+if (FromRT) {
+  RecordDecl *FromRD = FromRT->getDecl();
+  RecordType *ToRT =
+  const_cast(ToField->getType()->getAs());
+  assert(ToRT && "RecordType import has different type");
+  RecordDecl *ToRD = ToRT->getDecl();
+  if (FromRD->isCompleteDefinition() && !ToRD->isCompleteDefinition()) {
+if (Error Err = ImportDefinition(FromRD, ToRD, IDK_Basic))
+  return std::move(Err);
+  }
+}
+  }
+
   ToField->setAccess(D->getAccess());
   ToField->setLexicalDeclContext(LexicalDC);
   if (ToInitializer)
@@ -8445,6 +8467,8 @@
   // Make sure that ImportImpl registered the imported decl.
   assert(ImportedDecls.count(FromD) != 0 && "Missing call to MapImported?");
 
+  // There may have been NoUniqueAddressAttr for FieldDecl::isZeroSize.
+  ToD->dropAttrs();
   if (FromD->hasAttrs())
 for (const Attr *FromAttr : FromD->getAttrs()) {
   auto ToAttrOrErr = Import(FromAttr);


Index: clang/unittests/AST/ASTImporterTest.cpp
===
--- clang/unittests/AST/ASTImporterTest.cpp
+++ clang/unittests/AST/ASTImporterTest.cpp
@@ -6323,6 +6323,33 @@
 ToD->getTemplateSpecializationKind());
 }
 
+TEST_P(ASTImporterOptionSpecificTestBase, ImportNoUniqueAddress) {
+  Decl *FromTU = getTuDecl(
+  R"(
+  struct A {};
+  struct B {
+[[no_unique_address]] A a;
+  };
+  struct C : B {
+char c;
+  } c;
+  )", Lang_CXX20);
+
+  // It does not fail even without the patch!
+  auto *BFromD = FirstDeclMatcher().match(
+  FromTU, cxxRecordDecl(hasName("B")));
+  ASSERT_TRUE(BFromD);
+  auto *BToD = Import(BFromD, Lang_CXX20);
+  EXPECT_TRUE(BToD);
+
+  // It does not fail even without the patch!
+  auto *CFromD = FirstDeclMatcher().match(
+  FromTU, cxxRecordDecl(hasName("C")));
+  ASSERT_TRUE(CFromD);
+  auto *CToD = Import(CFromD, Lang_CXX20);
+  EXPECT_TRUE(CToD);
+}
+
 INSTANTIATE_TEST_CASE_P(ParameterizedTests, ASTImporterLookupTableTest,
 DefaultTestValuesForRunOptions, );
 
Index: clang/lib/AST/ASTImporter.cpp
===
--- clang/lib/AST/ASTImporter.cpp
+++ clang/lib/AST/ASTImporter.cpp
@@ -3673,6 +3673,28 @@
   D->getInClassInitStyle()))
 return ToField;
 
+  // FieldDecl::isZeroSize may need to check these.
+  if (const Attr *FromAttr = D->getAttr()) {
+if (auto ToAttrOrErr = Importer.Import(FromAttr))
+  ToField->addAttr(*ToAttrOrErr);
+else
+  return ToAttrOrErr.takeError();
+RecordType *FromRT =
+const_cast(D->getType()->getAs());
+// Is this field a struct? FieldDecl::isZeroSize will need its definition.
+if (FromRT) {
+  RecordDecl *FromRD = FromRT->getDecl();
+  RecordType *ToRT =
+  const_cast(ToField->getType()->getAs());
+

[PATCH] D101236: [ASTImporter] Import definitions required for layout of [[no_unique_address]] from LLDB

2021-04-24 Thread Jan Kratochvil via Phabricator via cfe-commits

jankratochvil created this revision.
jankratochvil added reviewers: teemperor, shafik, dblaikie.
jankratochvil added a project: clang.
Herald added a subscriber: martong.
Herald added a reviewer: a.sidorin.
jankratochvil requested review of this revision.

When LLDB needs to use `[[no_unique_address]]` (which is in fact always but 
that is topic of the next patch) `FieldDecl::isZeroSize` was missing the 
`NoUniqueAddressAttr` attribute calculating wrong field offsets and thus 
failing on an assertion:
lldb: clang/lib/CodeGen/CGRecordLayoutBuilder.cpp:802: void (anonymous 
namespace)::CGRecordLowering::insertPadding(): Assertion `Offset >= Size' 
failed.
After importing just the `NoUniqueAddressAttr` attribute various testcases were 
failing on:
clang/lib/AST/Decl.cpp:4163: bool clang::FieldDecl::isZeroSize(const 
clang::ASTContext &) const: Assertion `isInvalidDecl() && "valid field has 
incomplete type"' failed.
The LLDB testsuite exercises the patch fine. But the clang testcase I have 
provided here succeeds even with no patch applied. I expect I would need to 
import it X->Y->Z and not just X->Y but despite I tried some such code it was 
still working even without my patch.
Is it OK without a testcase or is there some suggestion how to write the clang 
testcase?


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D101236

Files:
  clang/lib/AST/ASTImporter.cpp
  clang/unittests/AST/ASTImporterTest.cpp


Index: clang/unittests/AST/ASTImporterTest.cpp
===
--- clang/unittests/AST/ASTImporterTest.cpp
+++ clang/unittests/AST/ASTImporterTest.cpp
@@ -6323,6 +6323,38 @@
 ToD->getTemplateSpecializationKind());
 }
 
+TEST_P(ASTImporterOptionSpecificTestBase, ImportNoUniqueAddress) {
+  auto SrcCode =
+  R"(
+  struct A {};
+  struct B {
+[[no_unique_address]] A a;
+  };
+  struct C : B {
+char c;
+  } c;
+  )";
+  Decl *FromTU = getTuDecl(SrcCode, Lang_CXX20);
+
+  // It does not fail even without the patch!
+  auto *BFromD = FirstDeclMatcher().match(
+  FromTU, cxxRecordDecl(hasName("B")));
+  ASSERT_TRUE(BFromD);
+  auto *BToD = Import(BFromD, Lang_CXX20);
+  EXPECT_TRUE(BToD);
+  auto *BTo2D = Import(BToD, Lang_CXX20);
+  EXPECT_TRUE(BTo2D);
+
+  // It does not fail even without the patch!
+  auto *CFromD = FirstDeclMatcher().match(
+  FromTU, cxxRecordDecl(hasName("C")));
+  ASSERT_TRUE(CFromD);
+  auto *CToD = Import(CFromD, Lang_CXX20);
+  EXPECT_TRUE(CToD);
+  auto *CTo2D = Import(CToD, Lang_CXX20);
+  EXPECT_TRUE(CTo2D);
+}
+
 INSTANTIATE_TEST_CASE_P(ParameterizedTests, ASTImporterLookupTableTest,
 DefaultTestValuesForRunOptions, );
 
Index: clang/lib/AST/ASTImporter.cpp
===
--- clang/lib/AST/ASTImporter.cpp
+++ clang/lib/AST/ASTImporter.cpp
@@ -3673,6 +3673,28 @@
   D->getInClassInitStyle()))
 return ToField;
 
+  // FieldDecl::isZeroSize may need to check these.
+  if (auto FromAttr = D->getAttr()) {
+if (auto ToAttrOrErr = Importer.Import(FromAttr))
+  ToField->addAttr(*ToAttrOrErr);
+else
+  return ToAttrOrErr.takeError();
+RecordType *FromRT =
+const_cast(D->getType()->getAs());
+// Is this field a struct? FieldDecl::isZeroSize will need its definition.
+if (FromRT) {
+  RecordDecl *FromRD = FromRT->getDecl();
+  RecordType *ToRT =
+  const_cast(ToField->getType()->getAs());
+  assert(ToRT && "RecordType import has different type");
+  RecordDecl *ToRD = ToRT->getDecl();
+  if (FromRD->isCompleteDefinition() && !ToRD->isCompleteDefinition()) {
+if (Error Err = ImportDefinition(FromRD, ToRD, IDK_Basic))
+  return std::move(Err);
+  }
+}
+  }
+
   ToField->setAccess(D->getAccess());
   ToField->setLexicalDeclContext(LexicalDC);
   if (ToInitializer)
@@ -8445,6 +8467,8 @@
   // Make sure that ImportImpl registered the imported decl.
   assert(ImportedDecls.count(FromD) != 0 && "Missing call to MapImported?");
 
+  // There may have been NoUniqueAddressAttr for FieldDecl::isZeroSize.
+  ToD->dropAttrs();
   if (FromD->hasAttrs())
 for (const Attr *FromAttr : FromD->getAttrs()) {
   auto ToAttrOrErr = Import(FromAttr);


Index: clang/unittests/AST/ASTImporterTest.cpp
===
--- clang/unittests/AST/ASTImporterTest.cpp
+++ clang/unittests/AST/ASTImporterTest.cpp
@@ -6323,6 +6323,38 @@
 ToD->getTemplateSpecializationKind());
 }
 
+TEST_P(ASTImporterOptionSpecificTestBase, ImportNoUniqueAddress) {
+  auto SrcCode =
+  R"(
+  struct A {};
+  struct B {
+[[no_unique_address]] A a;
+  };
+  struct C : B {
+char c;
+  } c;
+  )";
+  Decl *FromTU = getTuDecl(SrcCode, Lang_CXX20);
+
+  // It does not fail even without the patch!
+  auto *BFromD =

[PATCH] D101236: [ASTImporter] Import definitions required for layout of [[no_unique_address]] from LLDB

[PATCH] D101236: [ASTImporter] Import definitions required for layout of [[no_unique_address]] from LLDB

[PATCH] D101236: [ASTImporter] Import definitions required for layout of [[no_unique_address]] from LLDB

[PATCH] D101236: [ASTImporter] Import definitions required for layout of [[no_unique_address]] from LLDB

[PATCH] D101236: [ASTImporter] Import definitions required for layout of [[no_unique_address]] from LLDB

[PATCH] D101236: [ASTImporter] Import definitions required for layout of [[no_unique_address]] from LLDB

[PATCH] D101236: [ASTImporter] Import definitions required for layout of [[no_unique_address]] from LLDB

[PATCH] D101236: [ASTImporter] Import definitions required for layout of [[no_unique_address]] from LLDB

[PATCH] D101236: [ASTImporter] Import definitions required for layout of [[no_unique_address]] from LLDB

[PATCH] D101236: [ASTImporter] Import definitions required for layout of [[no_unique_address]] from LLDB

[PATCH] D101236: [ASTImporter] Import definitions required for layout of [[no_unique_address]] from LLDB

[PATCH] D101236: [ASTImporter] Import definitions required for layout of [[no_unique_address]] from LLDB

[PATCH] D101236: [ASTImporter] Import definitions required for layout of [[no_unique_address]] from LLDB

[PATCH] D101236: [ASTImporter] Import definitions required for layout of [[no_unique_address]] from LLDB

[PATCH] D101236: [ASTImporter] Import definitions required for layout of [[no_unique_address]] from LLDB

[PATCH] D101236: [ASTImporter] Import definitions required for layout of [[no_unique_address]] from LLDB

16 matches

Site Navigation

Mail list logo

Footer information