[lldb-dev] The future of modern-type-lookup in LLDB

Thu Dec 12 04:14:23 PST 2019

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 functionalities/modern-type-lookup folder).
> * It doesn’t work as of now. I fixed most of the failing tests but the remaining ones are not trivial to fix (and fixing some of them seems to break others again). Here is a full test run of the test suit with modern-type-lookup enabled by default [3]. Note that is was run on Linux and we have more tests and failures that are only running on macOS (Mostly Objective-C tests).
> * We need to maintain both systems in theory. In practice we just ignore modern-type-lookup as it had no tests. Some of the test failures are due to features we only added to the old system (e.g. the import-std-module failures are relying on features of the old system. Non-trivial -gmodules importing is also broken).
> * It wasn’t developed in a way that allows an incremental migration. If we ever turn it on by default we can only do it by doing one big switch over. We also can’t just turn it on for certain parts of LLDB or anything like that.
> * It’s not clear if the idea of the ExternalASTMerger is compatible with what LLDB does. There are a lot of assumptions in the ExternalASTMerger on how our sources behave and it’s not clear to me if that fits to what we do in LLDB (e.g. every DeclContext having exactly one source which can provide its containing declarations is an assumption. This which might not work with identical namespaces in multiple sources).
> * The ExternalASTMerger isn’t very well tested. It has some tests but most of them are just testing the underlying ASTImporter implementation.
> 
> This brings us to the big question what should happen to this feature. From what I can see we have three options:
> 1. We keep modern-type-lookup around and really focus on fixing it soon (which will be *a lot* of work and might not even be possible). Then we do the big switch over to the new system.
> 2. We keep modern-type-lookup around and try to get it working in the long-term. In the meantime we should maintain both the old system and modern-type-lookup.
> 3. We get rid of modern-type-lookup and incrementally refactor the old system until it has the same benefits as the new system.
> 
> (I should note that deleting modern-type-lookup does not mean we also instantly delete the ExternalASTMerger. As clang-import-test also provides test coverage for the ASTImporter we either need to rewrite the few tests there as ASTImporter unit tests or we migrate clang-import-test to directly using the ASTImporter).
> 
> Anyway, this is the current state of this whole thing. Let me know what you think.
> 
>  - Raphael
> 
> [1] https://bugs.llvm.org/show_bug.cgi?id=34771
> [2] https://bugs.llvm.org/show_bug.cgi?id=34772
> [3] https://teemperor.de/pub/TestModernTypeLookup.log
> _______________________________________________
> lldb-dev mailing list
> lldb-dev at lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-dev
> 