This is an archive of the discontinued LLVM Phabricator instance.

[lldb] Introduce separate scratch ASTs for debug info types and types imported from C++ modules.
ClosedPublic

Authored by teemperor on Dec 7 2020, 6:01 AM.

Details

Summary

Right now we have one large AST for all types in LLDB. All ODR violations in types we reconstruct are resolved
by just letting the ASTImporter handle the conflicts (either by merging types or somehow trying to introduce a duplicated
declaration in the AST). This works ok for the normal types we build from debug information as most of them are just
simple CXXRecordDecls or empty template declarations.

However, with a loaded std C++ module we have alternative versions of pretty much all declarations in the std namespace
that are much more fleshed out than the debug information declarations. They have all the information that is lost when converting
to DWARF, such as default arguments, template default arguments, the actual uninstantiated template declarations and so on.

When we merge these C++ module types into the big scratch AST (that might already contain debug information types) we give the
ASTImporter the tricky task of somehow creating a consistent AST out of all these declarations. Usually this ends in a messy AST
that contains a mostly broken mix of both module and debug info declarations. The ASTImporter in LLDB is also importing types with the
MinimalImport setting, which usually means the only information we have when merging two types is often just the name of
the declaration and the information that it contains some child declarations. This makes it pretty much impossible to even
implement a better merging logic (as the names of C++ module declarations and debug info declarations are identical).

This patch works around this whole merging problem by separating C++ module types from debug information types. This is done
by splitting up the single scratch AST into two: One default AST for debug information and a dedicated AST for C++ module types.

The C++ module AST is implemented as a 'specialised AST' that lives within the default ScratchTypeSystemClang. When we select
the scratch AST we can explicitly request that we want such a isolated sub-AST of the scratch AST. I kept the infrastructure more general
as we probably can use the same mechanism for other features that introduce conflicting types (such as programs that
are compiled with a custom -wchar-size= option).

There are just two places where we explicitly have request the C++ module AST: When we export persistent declarations ($mytype) and
when we create our persistent result variable ($0, $1, ...). There are a few formatters that were previously assuming that there is
only one scratch AST which I cleaned up in a preparation revision here (D92757).

Diff Detail

Event Timeline

teemperor requested review of this revision.Dec 7 2020, 6:01 AM
teemperor created this revision.

Should the test also include types that don't come from modules, just to make sure there are no odd interactions with this change.

lldb/source/Plugins/ExpressionParser/Clang/ClangASTSource.cpp
77

Why DefaultAST and not m_ast_context->getLangOpts()?

teemperor updated this revision to Diff 310356.Dec 8 2020, 2:52 PM
  • Expanded test with mixed debug info/module types (thanks Shafik!)
teemperor updated this revision to Diff 310535.Dec 9 2020, 7:29 AM
  • Clarify ClangASTContext destructor code.
teemperor added inline comments.Dec 9 2020, 7:32 AM
lldb/source/Plugins/ExpressionParser/Clang/ClangASTSource.cpp
77

That would work too, but unregistering from all is the more defensive approach in case someone decided to export to a different AST than the one suggested by GetForTarget (unregistering an AST that was never registered doesn't do anything, but failing to unregister causes LLDB to access the deleted memory).

aprantl accepted this revision.Dec 10 2020, 8:40 AM

LGTM once all outstanding comments are addressed.

lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.h
383

GetScratchContext perhaps?

lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.h
1129

We usually add a -Kind suffix to these enums.

Sub to me implies that is part of the scratch AST. How about SideASTKind or SpecializedASTKind or IsolatedASTKind?

1197

\see

This revision is now accepted and ready to land.Dec 10 2020, 8:40 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 10 2020, 10:28 AM

This change doesn't build with gcc 5.3.1.
More details: https://bugs.llvm.org/show_bug.cgi?id=48869

It would be great if you have could have a look! thanks

Yeah, that's an unfortunate bug in GCC 5.x. I actually just fixed that before the weekend in https://reviews.llvm.org/rG37510f69b4cb8d76064f108d57bebe95984a23ae https://reviews.llvm.org/rG37510f69b4cb8d76064f108d57bebe95984a23ae

+CC Tom (who is the release manager AFAIK), as that commit probably deserves to be cherry-picked to the 11 release branch (which contains D92759 so it won't build with GCC 5.x )

Thanks!

  • Raphael