[RFC][LLDB] More reliable completion of record types

Subprojects LLDB
1

This RFC proposes a change to LLDB’s mechanism of completing C/C++/Objective-C record types with the goal of getting rid of an entire class of bugs where Clang requires a definition, but LLDB wasn’t able to create one in time. Let us know if you have any concerns or questions about the outlined approach.

We revisit the refactor that was originally proposed with the draft patch in D101950.

Background

LLDB uses an embedded Clang compiler instance to keep track of and fulfil queries about types in the debugee. We constructs types, declarations and definitions by parsing debug information and creating corresponding Clang AST nodes. By default, we will create “forward declarations” for record types, and then create definitions (i.e., complete the types) on-demand only.

Frequently, queries about a type require LLDB to have completed it. E.g., “how many children does this type have?”, “is this type polymorphic?”, or “get me the second base-class of this type”. These queries could originate form anywhere in LLDB, including from a Python script through the SBAPI, a frame variable command (via data formatters), formatting of the current frame status, or various parts of the expression evaluator (via the expression command).

Current Type Completion Mechanism

The life of a record type in LLDB starts as a CompilerType in the ResolveState::Forward state. It’s essentially a clang::RecordType , which depending on whether it is a DW_AT_external (i.e., a forward declaration in DWARF) may or may not have a DefinitionData allocated for its associated clang::CXXRecordDecl . This happens in DWARFASTParserClang::ParseStructureLikeDIE.

LLDB relies on someone calling the ExternalASTSource::CompleteType API in time before we dereference getDefinition, e.g., [1][2][3]. But there is no single mechanism by which this is guaranteed to happen. Usually Type::GetFullCompilerType is eventually called, at which point we run DWARFASTParserClang::CompleteTypeFromDWARF , parsing the record’s children (as “forward declarations”), and completing the record’s definition.

LLDB allocates the clang types/decls it creates from DWARF into a clang::ASTContext (one context dedicated per LLDB module). Then it moves these AST nodes into a new AST that lives for the duration of an expression (aka the Expression AST). Moving the decls between ASTContext’s is done via the clang::ASTImporter . To avoid redundantly importing the definition (and all children) of a DeclContext, LLDB uses the ASTImporter in a dedicated MinimalImport mode. This mode only imports the “shell” of a DeclContext without a definition [4], relying on LLDB to call ImportDefinition to pull it in when it needs to [5].

A clang::DeclContext maintains information about whether it contains any undeserialized declarations (see DeclContextBitfields). These are checked in following places:

Shortcomings of Current Mechanism

While the current technique of completing types makes sure we do as little work as possible, it suffers from a few correctness issues, which has led to crashes that are hard to debug and workaround over the years [6][7][8].

  1. If the type we’re parsing from DWARF is not a forward declaration, we allocate a default DefinitionData structure for it but don’t parse/complete any of its children, meaning we leave the declaration in a sort of invalid transient state, relying on something to complete it later when necessary.
  2. If it is a forward declaration according to DWARF, we leave the DefinitionData unallocated, relying on something pulling in the definition later (we try to call FindDefinitionTypeForDWARFDeclContext but that isn’t guaranteed to succeed).

In both cases, if we don’t actually complete the type by the time the type’s definition is queried, we will most likely violate Clang invariants.

(see also previous discussion of type completion mechanism benefits/drawbacks)

Proposed Solution

The ExternalASTSource API that Clang will consistently call on every access to a definition is ExternalASTSource::CompleteRedeclChain (see DeclLink callback initiated by a call to dataPtr ). The core idea of this RFC is to rely on this API to do type completion in LLDB, instead of the previously less well-defined ExternalASTSource::CompleteType API.

We propose implementing the ClangASTSource::CompleteRedeclChain in terms of TypeSystemClang::CompleteTagDecl (which calls into DWARFASTParserClang::CompleteTypeFromDWARF). Then whenever anyone asks for getDefinition on a RecordDecl , we would ensure that LLDB had the chance to actually fetch the definition. Since CompleteRedeclChain only gets called when a declaration chain is deemed to be outdated (see generation counter check in LazyGenerationalUpdatePtr) we create redeclaration chains when parsing DWARF and increment the owning AST’s generation counter.

Changes

We merged a prototype of this RFC behind an experimental setting into a apple/llvm-project branch (see 0261a39) to gather feedback. Here’s a high-level summary of the core changes:

  • DWARFASTParserClang :
    • Parsing a structure will now neither start (i.e., allocate) a definition nor set the hasExternalLexicalStorage/hasExternalVisibleStorage flags.
    • CompleteRecordType will now start and complete the definition
      • This addresses the “we allocated by didn’t complete a definition” problem.
  • TypeSystemClang:
    • We can now implement completion in terms of getDefinition.
    • CompleteTagDeclarationDefinition no longer unsets the hasExternalLexicalStorage/hasExternalVisibleStorage flags.
  • ASTImporter
    • Will import entire DeclContext for structure instead of minimally importing.
      • Now doesn’t require manual ImportDefinition calls in LLDB.
    • This will allow us to potentially get rid of the MinimalImport mode entirely (or at least remove some of the workarounds for minimal import of record types).
  • Since we’re now dealing with redeclaration chains, we need to make sure that wherever LLDB caches a decl, it does so using the canonical decl (i.e., the first decl of the chain). In our implementation, the first decl is also the one that holds the definition (i.e., the one we ultimately complete).

Caveats

Performance

The prototype as-is currently significantly regresses performance of any part of LLDB that performs type completion. This stems from the fact that we are now completing types more aggressively, resulting in more work for the ASTImporter and also more DWARF parsing (particularly we would be searching through more object files than we currently do on top-of-tree). We benchmarked this extensively so let me know if you want a breakdown of the regression.

The main remaining problem we identified is that we’re currently fully resolving C++ class methods, completing parameters and return types in the process whenever we complete a type. This vastly expands the set of types we need to complete. The following two remaining tasks will get us back to top-of-tree performance:

  1. Stop resolving member function DIEs in DWARFASTParserClang::CompleteRecordType (we really only want to stop resovling non-virtual methods, since virtual methods contribute to class layouts)
  2. Stop importing method parameters in the ASTImporter in VisitFunctionDecl on a class method

If we stop fully resolving member function DIEs, we will need a way to lazily complete them when needed. We could introduce a mechanism similar to LoadFieldsFromExternalStorage to load the methods when asked for. We would have to add back logic to set the “has external storage” flags, but that should be much less problematic for methods because their absence is less critical than fields are, e.g., it wouldn’t break the layout builder.

Summary

To eliminate the crashes that stem from overly lazy type completion, we plan to refactor the LLDB’s type completion mechanism such that we guarantee to always get a type’s definition when Clang or LLDB ask for it. This will also help simplify a lot of the MinimalImport infrastructure in the ASTImporter and LLDB.

We’ll be monitoring how effectively our prototype helps eliminate LLDB crashes in the field over the next couple of weeks. Meanwhile we will also be addressing the above two performance issues and will then explore how to integrate the patch series into the main branch.

Looking forward to hearing your feedback!

CCing some people who might be interested in this (@jingham @clayborg @dblaikie @adrian.prantl @Teemperor @pogo59 @Felipe)

3 Likes
2

Awesome - glad to hear this work is being picked up again!

1 Like