Page MenuHomePhabricator

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

Authored by jankratochvil on Apr 24 2021, 9:27 AM.

Details

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?

Diff Detail

Unit TestsFailed

TimeTest
80 msx64 debian > Clang-Unit.AST/_/ASTTests::ParameterizedTests/LLDBMinimalImport.LLDBImportNoUniqueAddress/0
Script: -- /var/lib/buildkite-agent/builds/llvm-project/build/tools/clang/unittests/AST/./ASTTests --gtest_filter=ParameterizedTests/LLDBMinimalImport.LLDBImportNoUniqueAddress/0
80 msx64 debian > Clang-Unit.AST/_/ASTTests::ParameterizedTests/LLDBMinimalImport.LLDBImportNoUniqueAddress/1
Script: -- /var/lib/buildkite-agent/builds/llvm-project/build/tools/clang/unittests/AST/./ASTTests --gtest_filter=ParameterizedTests/LLDBMinimalImport.LLDBImportNoUniqueAddress/1
70 msx64 debian > Clang-Unit.AST/_/ASTTests::ParameterizedTests/LLDBMinimalImport.LLDBImportNoUniqueAddress/2
Script: -- /var/lib/buildkite-agent/builds/llvm-project/build/tools/clang/unittests/AST/./ASTTests --gtest_filter=ParameterizedTests/LLDBMinimalImport.LLDBImportNoUniqueAddress/2
70 msx64 debian > Clang-Unit.AST/_/ASTTests::ParameterizedTests/LLDBMinimalImport.LLDBImportNoUniqueAddress/3
Script: -- /var/lib/buildkite-agent/builds/llvm-project/build/tools/clang/unittests/AST/./ASTTests --gtest_filter=ParameterizedTests/LLDBMinimalImport.LLDBImportNoUniqueAddress/3

Event Timeline

jankratochvil created this revision.Apr 24 2021, 9:27 AM
jankratochvil requested review of this revision.Apr 24 2021, 9:27 AM

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)

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 planned changes to this revision.Apr 26 2021, 5:51 AM

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.

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.

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 added inline comments.May 3 2021, 1:15 AM
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).

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 abandoned this revision.May 6 2021, 8:27 AM

IIUC it will get replaced by D101950.

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 reclaimed this revision.May 6 2021, 8:31 AM
jankratochvil planned changes to this revision.
Herald added a project: Restricted Project. · View Herald TranscriptSun, May 23, 11:24 PM
jankratochvil added inline comments.Mon, May 24, 12:19 AM
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

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

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