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

[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