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?
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Unit Tests
Event Timeline
Not sure what the backtrace is for the clang::FieldDecl::isZeroSize crash but what might relevant:
- 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).
- 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.
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.
I have been looking for such suggestion, thanks. I will try that.
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
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
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.
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. 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 |
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);
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:
It also fails for this one but there are reasons not interesting for this patch:
These two are too complex to debug IMO:
I tried to reproduce the first two, this one has such backtrace: