[Lldb-commits] [clang] [lldb] [ASTImporter][lldb] Avoid implicit imports in VisitFieldDecl (PR #107828)

[Andrew Savonichev via lldb-commits]

asavonic wrote:

Thanks Michael, I'm glad I'm not the only one seeing this problem.

> > If an implicit import happens between (2) and (3), it may indirectly bring the same field into the context. When (3) happens we add it again, duplicating the field and breaking the record. This is not detected until we crash later during layout computation:
> 
> And the LLDB test that was XFAILed, is a special case of exactly this problem (summarized well in https://reviews.llvm.org/D102993).

Right, I saw this patch too. It tries to workaround the problem in LLDB, but seem to cause other issues. For example:
```
clang/lib/AST/ASTImporter.cpp:1933:
     Error clang::ASTNodeImporter::ImportDeclContext(DeclContext *, bool):
     Assertion `ToDC == ToD->getLexicalDeclContext() && ToDC->containsDecl(ToD)' failed.
```

> The more complete solution here is to make LLDB stop relying on this sort of import-pattern. We've been told in the past that the ASTImporter just wasn't designed for this use-case. Ideally, we'd remove the need for minimally importing record types (instead we should just always use `IDK_Everything` for importing definitions). In my opinion, adding complexity like this into the ASTImporter just to support LLDB is going to make it a maintenance burden on both. So I'd vote for trying to solve this properly (which is actually something I've been working on, though I can't promise a timeline of when this is supposed to land).

Oh, I think a redesign with this problem in mind would be great. 

As I understand it, minimal import is used in LLDB for performance reasons, so we don't waste time parsing and loading AST elements that we don't need. It sounds like a fine idea in general. Implicit imports of external sources in Clang, however, turn import process into a minefield, where you have no idea if a "pure" operation can cause a major change the target AST. "Complete" types are really incomplete, and there is no single point at which they are all guaranteed to be finalized.

Perhaps this approach was taken to minimize impact of external sources on the rest of Clang, to avoid introducing a concept of "not-fully-loaded-AST" everywhere. Understandable, but we have these issues now.

I don't know if I see the whole picture here. I might be wrong on some of these points or reasons behind them.

I'm curious what your plan is to address this problem.

> I do wonder whether there's something about the build configuration that makes you hit this crash more likely in UE. Are you building with precompiled headers by any chance? Or with `-flimit-debug-info`?

Unfortunately, I don't know. I'm using pre-built binaries downloaded from Epic.
PCHs are likely used, not sure about `-flimit-debug-info`.

> I'm actually looking at a similar crash at the moment for which I have a reproducer. I'm trying to extract it into a smaller test-case.

Please let me know if you can get it. 

I reduced mine down to ~200KB of DWARF. It crashes on the same assertion in `RecordLayoutBuilder.cpp`, but this patch no longer helps. Perhaps there is another case of the issue, or the reduced DWARF is somehow invalid.

https://github.com/llvm/llvm-project/pull/107828