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

[jankratochvil]

Summary

When LLDB needs to use [[no_unique_address]] (which is in fact always but that is topic of the next patch D101237) 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?

[teemperor]

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)

[martong]

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.

[jankratochvil]

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.

[jankratochvil]

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: 0x00000000028d7195 ASTTests`clang::SourceManager::getFileIDLoaded(this=0x0000000004ecf640, 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.
   864    if (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=0x0000000004ecf640, SLocOffset=14405) const at SourceManager.cpp:865:5
    frame # 5: ASTTests`clang::SourceManager::getFileIDSlow(this=0x0000000004ecf640, 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=0x0000000004ecf640, Loc=(ID = 14405)) const at SourceManager.h:1247:18
    frame # 8: ASTTests`clang::SourceManager::getPresumedLoc(this=0x0000000004ecf640, Loc=(ID = 14405), UseLineDirectives=true) const at SourceManager.cpp:1521:41
    frame # 9: ASTTests`clang::SourceManager::isWrittenInBuiltinFile(this=0x0000000004ecf640, Loc=(ID = 14405)) const at SourceManager.h:1468:24
    frame #10: ASTTests`clang::ASTImporter::Import(this=0x00007ffff78f9010, FromLoc=(ID = 14405)) at ASTImporter.cpp:8834:27
    frame #11: ASTTests`llvm::Error clang::ASTImporter::importInto<clang::SourceLocation>(this=0x00007ffff78f9010, To=0x00007fffffffbe68, From=0x00007fffffffb8e8) at ASTImporter.h:336:22
    frame #12: ASTTests`llvm::Error clang::ASTNodeImporter::importInto<clang::SourceLocation>(this=0x00007fffffffbf60, To=0x00007fffffffbe68, From=0x00007fffffffb8e8) at ASTImporter.cpp:148:23
    frame #13: ASTTests`clang::ASTNodeImporter::ImportDeclParts(this=0x00007fffffffbf60, D=0x0000000004eb8190, DC=0x00007fffffffbe80, LexicalDC=0x00007fffffffbe78, Name=0x00007fffffffbe70, ToD=0x00007fffffffbe60, Loc=0x00007fffffffbe68) at ASTImporter.cpp:1654:19
    frame #14: ASTTests`clang::ASTNodeImporter::VisitRecordDecl(this=0x00007fffffffbf60, D=0x0000000004eb8190) at ASTImporter.cpp:2772:19
    frame #15: ASTTests`clang::declvisitor::Base<std::add_pointer, clang::ASTNodeImporter, llvm::Expected<clang::Decl*> >::VisitCXXRecordDecl(this=0x00007fffffffbf60, D=0x0000000004eb8190) at DeclNodes.inc:263:1
    frame #16: ASTTests`clang::declvisitor::Base<std::add_pointer, clang::ASTNodeImporter, llvm::Expected<clang::Decl*> >::Visit(this=0x00007fffffffbf60, D=0x0000000004eb8190) at DeclNodes.inc:263:1
    frame #17: ASTTests`clang::ASTImporter::ImportImpl(this=0x00007ffff78f9010, FromD=0x0000000004eb8190) at ASTImporter.cpp:8216:19
    frame #18: ASTTests`clang::ASTImporter::Import(this=0x00007ffff78f9010, FromD=0x0000000004eb8190) at ASTImporter.cpp:8387:27
    frame #19: ASTTests`llvm::Expected<clang::Decl*> clang::ASTNodeImporter::import<clang::Decl>(this=0x00007fffffffc6c8, From=0x0000000004eb8190) at ASTImporter.cpp:164:31
    frame #20: ASTTests`clang::ASTNodeImporter::ImportDeclContext(this=0x00007fffffffc6c8, FromDC=0x0000000004e886a8, ForceImport=true) at ASTImporter.cpp:1777:34
    frame #21: ASTTests`clang::ASTImporter::ImportDefinition(this=0x00007ffff78f9010, From=0x0000000004e88668) at ASTImporter.cpp:9072:19
    frame #22: ASTTests`clang::ast_matchers::LLDBMinimalImport_LLDBImportNoUniqueAddress_Test::TestBody() at ASTImporterTest.cpp:6408:50

[martong]

clang/unittests/AST/ASTImporterTest.cpp

6442

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).

[teemperor]

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

[jankratochvil]

IIUC it will get replaced by D101950.

[teemperor]

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.

[jankratochvil]

clang/lib/AST/ASTImporter.cpp

3684

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 such backtrace:

lldb-api :: commands/expression/codegen-crash-typedefdecl-not-in_declcontext/TestCodegenCrashTypedefDeclNotInDeclContext.py

clang/unittests/AST/ASTImporterTest.cpp

6441

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

[jankratochvil]

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

[jankratochvil]

cat >1.cpp <<EOH
template <typename T> struct DWrapper {};
typedef DWrapper<void> DW;
struct B {
  DW spd;
} b;
struct E {
  B &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<VarDecl>().match(FromTU, varDecl(hasName("e")));
...
  // ASTTests: .../llvm/include/llvm/Support/Casting.h:269: typename cast_retty<X, Y *>::ret_type llvm::cast(Y *) [X = clang::DeclContext, Y = clang::Decl]: Assertion `isa<X>(Val) && "cast<Ty>() argument of incompatible type!"' failed.
  llvm::Error Err = findFromTU(FromE)->Importer->ImportDefinition(FromE);

clang/lib/AST/ASTImporter.cpp

ExpectedDecl ASTNodeImporter::VisitFieldDecl(FieldDecl *D) {
 
   FieldDecl *ToField;
   if (GetImportedOrCreateDecl(ToField, D, Importer.getToContext(), DC,
                               ToInnerLocStart, Loc, Name.getAsIdentifierInfo(),
                               ToType, ToTInfo, ToBitWidth, D->isMutable(),
                               D->getInClassInitStyle()))
     return ToField;
 
+  // FieldDecl::isZeroSize may need to check these.
+  if (const Attr *FromAttr = D->getAttr<NoUniqueAddressAttr>()) {
+    if (auto ToAttrOrErr = Importer.Import(FromAttr))
+      ToField->addAttr(*ToAttrOrErr);
+    else
+      return ToAttrOrErr.takeError();
+#if 0
+    RecordType *FromRT =
+        const_cast<RecordType *>(D->getType()->getAs<RecordType>());
+    // Is this field a struct? FieldDecl::isZeroSize will need its definition.
+    if (FromRT) {
+      RecordDecl *FromRD = FromRT->getDecl();
+      RecordType *ToRT =
+          const_cast<RecordType *>(ToField->getType()->getAs<RecordType>());
+      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);
+      }
+    }
+#endif
+  }
+
   ToField->setAccess(D->getAccess());
   ToField->setLexicalDeclContext(LexicalDC);
   if (ToInitializer)
     ToField->setInClassInitializer(ToInitializer);
   ToField->setImplicit(D->isImplicit());
   if (ToCapturedVLAType)
     ToField->setCapturedVLAType(cast<VariableArrayType>(ToCapturedVLAType));
   LexicalDC->addDeclInternal(ToField);
Expected<Decl *> ASTImporter::Import(Decl *FromD) {
   if (auto Error = SharedState->getImportDeclErrorIfAny(ToD)) {
     setImportDeclError(FromD, *Error);
     return make_error<ImportError>(*Error);
   }
 
   // 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);
       if (ToAttrOrErr)
         ToD->addAttr(*ToAttrOrErr);
       else
         return ToAttrOrErr.takeError();
     }

clang/unittests/AST/ASTImporterTest.cpp

TEST_P(ASTImporterOptionSpecificTestBase, ImportEnumMemberSpecialization) {
 
   auto *ToD = Import(FromD, Lang_CXX03);
   EXPECT_TRUE(ToD);
   EXPECT_TRUE(ToD->getMemberSpecializationInfo());
   EXPECT_EQ(FromD->getTemplateSpecializationKind(),
             ToD->getTemplateSpecializationKind());
 }
 
+struct LLDBMinimalImport : ASTImporterOptionSpecificTestBase {
+  LLDBMinimalImport() {
+    Creator = [](ASTContext &ToContext, FileManager &ToFileManager,
+                 ASTContext &FromContext, FileManager &FromFileManager,
+                 bool MinimalImport,
+                 const std::shared_ptr<ASTImporterSharedState> &SharedState) {
+      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 <typename T> struct DWrapper {};
+
+struct D {};
+
+namespace NS {
+typedef DWrapper<D> DW;
+}
+
+struct B {
+  NS::DW spd;
+  int a = 0;
+};
+
+struct E {
+  E(B &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 &b_ref;
+};
+B b;
+E e(b);
+      )",
+      Lang_CXX11);
+
+  auto *FromB = FirstDeclMatcher<CXXRecordDecl>().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 TestExternalASTSource());
+#endif
+
+  TranslationUnitDecl *ToTU = getToTuDecl(
+      R"(
+struct B;
+      )",
+      Lang_CXX11);
+  auto *ToB = FirstDeclMatcher<CXXRecordDecl>().match(
+      ToTU, cxxRecordDecl(hasName("B")));
+
+#if 0
+  // 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());
+#endif
+
+  // With this line commented out findFromTU(FromB)->Importer == nullptr
+  // With this line uncommented: clang/lib/AST/ASTImporter.cpp:9383: clang::Decl *clang::ASTImporter::MapImported(clang::Decl *, clang::Decl *): Assertion `(Pos == ImportedDecls.end() || Pos->second == To) && "Try to import an already imported Decl"' failed.
+  auto *ImportedB = Import(FromB, Lang_CXX11);
+  findFromTU(FromB)->Importer->MapImported(FromB, ToB);
+  findFromTU(FromB)->Importer->Imported(FromB, ToB);
+  llvm::Error Err = findFromTU(FromB)->Importer->ImportDefinition(FromB);
+  EXPECT_FALSE((bool)Err);
+  consumeError(std::move(Err));
+}
+
 INSTANTIATE_TEST_SUITE_P(ParameterizedTests, ASTImporterLookupTableTest,
                          DefaultTestValuesForRunOptions);
 
 INSTANTIATE_TEST_SUITE_P(ParameterizedTests, ImportPath,
                          ::testing::Values(std::vector<std::string>()));
 
 INSTANTIATE_TEST_SUITE_P(ParameterizedTests, ImportExpr,
                          DefaultTestValuesForRunOptions);
INSTANTIATE_TEST_SUITE_P(ParameterizedTests, LLDBLookupTest,
                          DefaultTestValuesForRunOptions);
 
 INSTANTIATE_TEST_SUITE_P(ParameterizedTests, ImportSourceLocations,
                          DefaultTestValuesForRunOptions);
 
 INSTANTIATE_TEST_SUITE_P(ParameterizedTests, ImportWithExternalSource,
                          DefaultTestValuesForRunOptions);
 
+INSTANTIATE_TEST_SUITE_P(ParameterizedTests, LLDBMinimalImport,
+                         DefaultTestValuesForRunOptions);
+
 } // end namespace ast_matchers
 } // end namespace clang

lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp

clang::FieldDecl *TypeSystemClang::AddFieldToRecordType(
   if (record_decl) {
     field = clang::FieldDecl::CreateDeserialized(clang_ast, 0);
     field->setDeclContext(record_decl);
     field->setDeclName(ident);
     field->setType(ClangUtil::GetQualType(field_clang_type));
     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
       // union.
       if (const clang::TagType *TagT =
               field->getType()->getAs<clang::TagType>()) {
         if (clang::RecordDecl *Rec =
                 llvm::dyn_cast<clang::RecordDecl>(TagT->getDecl()))

lldb/test/Shell/Expr/no_unique_address.cpp

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

lldb/test/Shell/SymbolFile/NativePDB/bitfields.cpp

 // CHECK:   (char:3) H = '\x03'
 // CHECK: }
 //
 // CHECK: target modules dump ast
 // CHECK: Dumping clang ast for 1 modules.
 // 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