Thanks for the nice summary Raphael,
this isn't exactly my playground (though I may end up having to poke in
this soon), but it overall, it sounds to me like this "modern" thingy is
a better overall design. However, your penultimate bullet point seems
pretty important to me. I don't think we can avoid "merging" namespace
defined in two shared libraries. If the ExternalASTMerger does not
support this, then it may not work for us.
Given that it is only used from lldb, it may be possible to refactor it
to support this use case, but then again, we should be able to do the
same with the existing mechanism too.
So at the end of the day, I think I'd go for removing this, but I think
that you're probably the most qualified to make decision here..
pl
On 11/12/2019 11:13, Raphael “Teemperor” Isemann via lldb-dev wrote:
> Hi,
>
> some of you may have seen that I’ve been working on the ‘modern-type-lookup’
> mode in LLDB over the last months. I thought it’s a good time now to start a
> discussion about what’s the current state, goals and the future of this
> feature.
>
> Some background about the topic to get everyone on the same page:
> * LLDB uses Clang’s AST to model the types/functions/variables/etc. in the
> target program.
> * Clang’s AST is usually represented as a ASTContext (= AST + associated
> data).
> * LLDB has several ASTContexts for different purposes:
> - Usually we have several ASTContexts that just contain the AST nodes we
> create from debug information on the disk.
> - There are some minor ones floating around like the ASTContext we use to
> store AST nodes that we load from Clang modules (*.pcm files).
> - There are temporary ASTContexts we create to run expressions (every
> expression is its own temporary ASTContext that gets deleted after we ran the
> expression).
> - There is one ASTContext that is the one we put in all the persistent
> $-variables and their associated types.
> * The last two ASTContexts don’t have any associated data source that they
> represent but are supposed to be ‘filled' by the other two kinds of
> ASTContexts.
> * To fill an ASTContext we move AST nodes from one ASTContext to another.
> * Moving ASTNodes is done with clang’s ASTImporter implementation which
> ‘imports’ AST nodes.
> * Nearly all importing in LLDB is done lazily which the ASTImporter calls
> ‘MinimalImport’. This means the ASTImporter only imports what it needs for
> certain declarations and then imports the rest as it is needed.
> * The lazy importing is a big source of problems in LLDB. If we don’t
> correctly import enough information to make Clang happy then we usually end
> up hitting an assert. However it is also avoiding the loading unnecessary
> debug information from disk which makes LLDB faster. There are no accurate
> numbers on how much faster as we don’t have an easy way to run LLDB without
> MinimalImport.
>
> Now let’s move on to the modern-type-lookup part.
>
> What is modern-type-lookup?
> * modern-type-lookup is a flag that makes LLDB use `clang::ExternalASTMerger`
> instead of directly using the `clang::ASTImporter` via our ClangASTImporter
> wrapper.
> * `clang::ExternalASTMerger` is some kind of manager for clang’s ASTImporter.
> It keeps track of several ASTImporter instances (that each have an associate
> ASTContext source) and one target ASTContext that all ASTImporters import
> nodes into. When the ASTImporters import certain nodes into the single target
> ASTContext it does the bookkeeping to associate the imported information with
> the ASTImporter/source ASTContext.
> * The ExternalASTMerger also does some other smaller book keeping such as
> having a ‘reverse ASTImporter’ for every ‘ASTImporter’.
> * The ExternalASTMerger is only used by LLDB (and clang-import-test which is
> its testing binary).
>
> What is good about modern-type-lookup:
> * The ExternalASTMerger is better code than our current system of directly
> (ab-)using the ASTImporter. Most notably it doesn’t use globals like our
> current system (yay).
> * The ExternalASTMerger is easer to test because it comes with a testing
> binary (clang-import-test) and it doesn’t depend on anything beside the
> ASTImporter. In comparison our current system depends on literally all of
> LLDB.
> * It brings better organisation to our ASTImporter network which allows us to
> implement some tricky features (e.g. https://reviews.llvm.org/D67803).
> * It actually is a quite small (but sadly very invasive) change into LLDB and
> Clang.
>
> What is bad and ugly about modern-type-lookup:
> * modern-type-lookup in LLDB was completely untested until recently. The idea
> was the testing was done by running the whole test suite with the setting
> enabled by default [1] and then fix the found issues [2], but none of this
> happened for various reasons. The only dedicated tests for it are the ones I
> added while I was trying to see what it even does (see the
>
