[lldb] Use forward decls with redeclared definitions instead of minimal import for records #8222

Michael137 · 2024-02-16T18:37:33Z

This patch is rewriting the way we move Clang types between different ASTS in Clang.

The motivation is that the current approach for 'completing types' in LLDB just doesn't work. We have all kinds of situations where we either have Clang crash due to for example having a Record without DefinitionData but with FieldDecls in it.

The reason for that is that at the moment we create record types ([CXX]RecordDecls and ObjCInterfaceDecls) and we always pretend that a type potentially has a definition that somehow could be lazily pulled in. However, Clang doesn't have any interface (at least none that is consistently called) that turns a RecordDecl without DefinitionData into a RecordDecl with DefinitionData. The only relevant API in the ExternalASTSource is CompleteType is suffering from the fact that it's essentially not used by generic Clang code.

The part of the ExternalASTSource API that is consistently called to pull in a potential definition is CompleteRedeclChain (which is for example automatically called when calling getDefinition on a RecordDecl). The problem with CompleteRedeclChain however is that it's not supposed to add definition data to the Decl its called on, but instead it should provide a redeclaration that is defining the record. That's a very different system than what we currently have and we can't just swap it out under the hood.

To make it short: We probably need to rewrite that part of LLDB.

So the solution here is essentially rewriting all our completion code to do this:

Instead of creating these weirdly defined records that have fields but maybe not definition and so on, we only create forward decls at the start.
We then bump the GenerationCounter of the ExternalASTSource of the current AST to force rebuilding of the redeclaration chain.
When Clang asks for the definition of the record, it will ask the ExternalASTSource to complete the redeclaration chain. At this point we build the definition and add it as a second decl.

The ASTImporter can now also stop using the minimal mode for records as there is no reason anymore. It just imports first the forward declaration and then the definition when needed.

adrian-prantl · 2024-02-16T21:48:19Z

lldb/source/Plugins/ExpressionParser/Clang/ASTUtils.h

  void CompleteRedeclChain(const clang::Decl *D) override {
-    for (size_t i = 0; i < Sources.size(); ++i)
+    for (size_t i = 0; i < Sources.size(); ++i) {


Suggested change

for (size_t i = 0; i < Sources.size(); ++i) {

for (auto &Source : Sources) {

adrian-prantl · 2024-02-16T21:52:19Z

lldb/source/Plugins/ExpressionParser/Clang/ClangASTImporter.cpp

  ImporterDelegateSP delegate_sp(
      GetDelegate(&interface_decl->getASTContext(), decl_origin.ctx));

-  if (delegate_sp)
-    delegate_sp->ImportDefinitionTo(interface_decl, decl_origin.decl);
+  if (!TypeSystemClang::UseRedeclCompletion()) {


I think all other conditions were inverted so far. It would be nice to consistently check for the positive case so it's less confusing to read.

adrian-prantl · 2024-02-16T21:57:06Z

lldb/source/Plugins/ExpressionParser/Clang/ClangASTImporter.cpp


  return true;
 }

 bool ClangASTImporter::CompleteAndFetchChildren(clang::QualType type) {
-  if (!RequireCompleteType(type))
-    return false;
+  const auto ret = RequireCompleteType(type);


What's the return value of RequireCompleteType?

adrian-prantl · 2024-02-16T21:57:19Z

lldb/source/Plugins/ExpressionParser/Clang/ClangASTImporter.cpp

-  if (!RequireCompleteType(type))
-    return false;
+  const auto ret = RequireCompleteType(type);
+  if (TypeSystemClang::UseRedeclCompletion() || !ret)


I would write this as two separate conditions.

adrian-prantl · 2024-02-16T22:00:00Z

lldb/source/Plugins/ExpressionParser/Clang/ClangASTImporter.cpp

@@ -882,6 +963,14 @@ ClangASTImporter::ASTImporterDelegate::ImportImpl(Decl *From) {
    for (clang::Decl *candidate : lr) {
      if (candidate->getKind() == From->getKind()) {
        RegisterImportedDecl(From, candidate);
+
+        // If we're dealing with redecl chains. We want to find the definition,


Suggested change

// If we're dealing with redecl chains. We want to find the definition,

// If we're dealing with redecl chains, we want to find the definition,

adrian-prantl · 2024-02-16T22:02:34Z

lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.h

  lldb_private::TypeSystemClang &m_ast;
  DIEToDeclMap m_die_to_decl;
  DIEToDeclContextMap m_die_to_decl_ctx;
  DeclContextToDIEMap m_decl_ctx_to_die;
  DIEToModuleMap m_die_to_module;
+  DIEToRecordMap m_die_to_record_map;


Doxygen comment what this map is used for?

adrian-prantl · 2024-02-16T22:03:09Z

lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.h

  std::unique_ptr<lldb_private::ClangASTImporter> m_clang_ast_importer_up;
+
+  struct TypeToComplete {


Why foes this have both a CompilerType and a lldb::Type?

adrian-prantl · 2024-02-16T22:03:21Z

lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.h

@@ -227,6 +245,9 @@ class DWARFASTParserClang : public lldb_private::plugin::dwarf::DWARFASTParser {
  void LinkDeclToDIE(clang::Decl *decl,
                     const lldb_private::plugin::dwarf::DWARFDIE &die);

+  void RegisterDIE(lldb_private::plugin::dwarf::DWARFDebugInfoEntry *die,


Doxygen comment?

adrian-prantl · 2024-02-16T22:03:57Z

lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp

@@ -2615,6 +2706,120 @@ TypeSystemClang::GetDeclContextForType(clang::QualType type) {
  return nullptr;
 }

+static clang::Type const *GetCompleteRecordType(clang::ASTContext *ast,


adrian-prantl · 2024-02-16T22:04:38Z

lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp

@@ -8035,9 +8187,13 @@ clang::ObjCMethodDecl *TypeSystemClang::AddMethodToObjCObjectType(
    return nullptr;

  clang::ObjCInterfaceDecl *class_interface_decl = GetAsObjCInterfaceDecl(type);
+  if (class_interface_decl)
+    if (auto * def = class_interface_decl->getDefinition())


clan-format

Michael137 · 2024-02-19T12:34:37Z

@swift-ci test

Michael137 · 2024-02-19T13:26:12Z

@swift-ci test

Michael137 · 2024-02-19T13:56:24Z

Please test with following pull request:
swiftlang/swift#71724

@swift-ci Please test macOS platform

Michael137 · 2024-02-19T14:47:35Z

Please test with following pull request:
swiftlang/swift#71724

@swift-ci test

Michael137 · 2024-02-19T17:45:43Z

Please test with following pull request:
swiftlang/swift#71724

@swift-ci test

adrian-prantl · 2024-02-19T22:53:58Z

I don't see the test reports? Is that because the PR is in DRAFT mode?

…hodToObjCObjectType (llvm#73832) (cherry picked from commit bcb621f)

This patch backports a one-liner `std::to_underlying` that came with C++23. This is useful for refactoring unscoped enums into scoped enums, because the latter are not implicitly convertible to integer types. I followed libc++ implementation, but I consider their testing too heavy for us, so I wrote a simpler set of tests. (cherry picked from commit d0caa4e)

Michael137 · 2024-02-19T23:52:35Z

I don't see the test reports? Is that because the PR is in DRAFT mode?

The build wasn't passing, so tests weren't running yet. Should be fixed now

Michael137 · 2024-02-19T23:52:43Z

Please test with following pull request:
swiftlang/swift#71724

@swift-ci test

Michael137 · 2024-02-20T12:21:20Z

Please test with following pull request:
swiftlang/swift#71724

@swift-ci test

Michael137 · 2024-02-20T12:21:32Z

Please test with following pull request:
swiftlang/swift#71724

@swift-ci test Windows

Michael137 · 2024-02-20T14:28:07Z

Please test with following pull request:
swiftlang/swift#71724

@swift-ci test

adrian-prantl · 2024-02-20T16:25:51Z

swiftlang/swift#71724
@swift-ci test windows

Note, this is a no-op when the LLDB setting `plugin.typesystem.clang.experimental-redecl-completion` is not set (which is currently the default). So this patch has no affect unless the user explicitly opts into it. The type-completion rework (aka redecl-completion) implemented in swiftlang#8222 comes with a large performance penalty, since we now eagerly complete `RecordType`s. Completing a `RecordType` previously unconditionally resolved all member function of said type. With redecl-completion completion, however, this meant we were now pulling in many more definitions than needed. Without redecl-completion, this isn't a problem, since importing method parameters is cheap (they are imported minimally), so we wouldn't notice that we always resolved all member functions. This patch tries to load methods lazily when in redecl-completion mode. We do this by introducing a new `ExternalASTSource::FindExternalVisibleMethods` API which Clang parses a member access expression. The callback into LLDB will do a `FindFunctions` call for all methods with the method name we're looking for, filters out any mismatches, and lets Clang continue with it's parsing. We still load following methods eagerly: 1. virtual functions: currently overrides are resolved in `CompleteRecordType` 2. operators: currently I couldn't find a point at which Clang can call into LLDB here to facilitate lazy loading 3. ctors/dtors: same reason as (2) In our benchmark harness, we saw this patch bring down redecl-completion expression evaluation on-par with top-of-tree expression evaluation.

…lazy-method-loading Note, this is a no-op when the LLDB setting `plugin.typesystem.clang.experimental-redecl-completion` is not set (which is currently the default). So this patch has no effect unless the user explicitly opts into it. The type-completion rework (aka redecl-completion) implemented in #8222 comes with a large performance penalty, since we now eagerly complete `RecordType`s. Completing a `RecordType` previously unconditionally resolved all member functions of said type. With redecl-completion completion, however, this meant we were now pulling in many more definitions than needed. Without redecl-completion, this isn't a problem, since importing method parameters is cheap (they are imported minimally), so we wouldn't notice that we always resolved all member functions. This patch tries to load methods lazily when in redecl-completion mode. We do this by introducing a new `ExternalASTSource::FindExternalVisibleMethods` API which Clang calls when parsing a member access expression. The callback into LLDB will do a `FindFunctions` call for all methods with the method name we're looking for, filters out any mismatches, and lets Clang continue with its parsing. We still load following methods eagerly: 1. virtual functions: currently overrides are resolved in `CompleteRecordType` 2. operators: currently I couldn't find a point at which Clang can call into LLDB here to facilitate lazy loading 3. ctors/dtors: same reason as (2) In our benchmark harness, we saw this patch bring down redecl-completion expression evaluation performance on-par with top-of-tree expression evaluation.

…crash-fixes [lldb][Type Completion] Fix various redecl-completion issues that stem from the `UseRedeclCompletion` setting split Debugging larger programs with the redecl-completion setting enabled was causing various crashes. This patch series resolves these. All of them were caused by either missed cherry-picks or incorrect UseRedeclCompletion guards. Follow-ups from #8222

…crash-fixes-to-next [lldb][Type Completion] Fix various redecl-completion issues that stem from the UseRedeclCompletion setting split Debugging larger programs with the redecl-completion setting enabled was causing various crashes. This patch series resolves these. All of them were caused by either missed cherry-picks or incorrect UseRedeclCompletion guards. Follow-ups from #8222

Note, this is a no-op when the LLDB setting `plugin.typesystem.clang.experimental-redecl-completion` is not set (which is currently the default). So this patch has no affect unless the user explicitly opts into it. The type-completion rework (aka redecl-completion) implemented in swiftlang#8222 comes with a large performance penalty, since we now eagerly complete `RecordType`s. Completing a `RecordType` previously unconditionally resolved all member function of said type. With redecl-completion completion, however, this meant we were now pulling in many more definitions than needed. Without redecl-completion, this isn't a problem, since importing method parameters is cheap (they are imported minimally), so we wouldn't notice that we always resolved all member functions. This patch tries to load methods lazily when in redecl-completion mode. We do this by introducing a new `ExternalASTSource::FindExternalVisibleMethods` API which Clang parses a member access expression. The callback into LLDB will do a `FindFunctions` call for all methods with the method name we're looking for, filters out any mismatches, and lets Clang continue with it's parsing. We still load following methods eagerly: 1. virtual functions: currently overrides are resolved in `CompleteRecordType` 2. operators: currently I couldn't find a point at which Clang can call into LLDB here to facilitate lazy loading 3. ctors/dtors: same reason as (2) In our benchmark harness, we saw this patch bring down redecl-completion expression evaluation on-par with top-of-tree expression evaluation. (cherry picked from commit ff8fdb9)

Note, this is a no-op when the LLDB setting `plugin.typesystem.clang.experimental-redecl-completion` is not set (which is currently the default). So this patch has no affect unless the user explicitly opts into it. The type-completion rework (aka redecl-completion) implemented in swiftlang#8222 comes with a large performance penalty, since we now eagerly complete `RecordType`s. Completing a `RecordType` previously unconditionally resolved all member function of said type. With redecl-completion completion, however, this meant we were now pulling in many more definitions than needed. Without redecl-completion, this isn't a problem, since importing method parameters is cheap (they are imported minimally), so we wouldn't notice that we always resolved all member functions. This patch tries to load methods lazily when in redecl-completion mode. We do this by introducing a new `ExternalASTSource::FindExternalVisibleMethods` API which Clang parses a member access expression. The callback into LLDB will do a `FindFunctions` call for all methods with the method name we're looking for, filters out any mismatches, and lets Clang continue with it's parsing. We still load following methods eagerly: 1. virtual functions: currently overrides are resolved in `CompleteRecordType` 2. operators: currently I couldn't find a point at which Clang can call into LLDB here to facilitate lazy loading 3. ctors/dtors: same reason as (2) In our benchmark harness, we saw this patch bring down redecl-completion expression evaluation on-par with top-of-tree expression evaluation.

Michael137 requested a review from adrian-prantl February 16, 2024 18:37

adrian-prantl reviewed Feb 16, 2024

View reviewed changes

Michael137 force-pushed the lldb/type-completion-rework/WIP/to-20230725 branch 4 times, most recently from ac62cbe to d092bd7 Compare February 19, 2024 12:34

Michael137 force-pushed the lldb/type-completion-rework/WIP/to-20230725 branch from d092bd7 to f4e9874 Compare February 19, 2024 13:25

Michael137 mentioned this pull request Feb 19, 2024

[build-script] Run LLDB tests with new redecl-completion setting swiftlang/swift#71724

Merged

Michael137 force-pushed the lldb/type-completion-rework/WIP/to-20230725 branch from f4e9874 to 3686ac4 Compare February 19, 2024 14:47

Michael137 force-pushed the lldb/type-completion-rework/WIP/to-20230725 branch from 3686ac4 to d487b84 Compare February 19, 2024 17:45

Michael137 force-pushed the lldb/type-completion-rework/WIP/to-20230725 branch 2 times, most recently from 63abffc to 3ebe89e Compare February 19, 2024 22:53

Michael137 force-pushed the lldb/type-completion-rework/WIP/to-20230725 branch from 3ebe89e to 8ace0bc Compare February 19, 2024 23:02

Michael137 and others added 2 commits February 19, 2024 23:04

[lldb][DWARFASTParserClang][NFC] Remove redundant parameter to AddMet…

4cefb29

…hodToObjCObjectType (llvm#73832) (cherry picked from commit bcb621f)

Michael137 force-pushed the lldb/type-completion-rework/WIP/to-20230725 branch from 8ace0bc to 3fa5017 Compare February 19, 2024 23:04

Michael137 force-pushed the lldb/type-completion-rework/WIP/to-20230725 branch from 3fa5017 to 70c9abd Compare February 20, 2024 12:16

Michael137 force-pushed the lldb/type-completion-rework/WIP/to-20230725 branch from 70c9abd to 2e68ca9 Compare February 20, 2024 14:27

Michael137 mentioned this pull request Apr 16, 2024

[lldb][Type Completion] Load C++ methods lazily from DWARF #8578

Merged

Michael137 mentioned this pull request Apr 16, 2024

[lldb][Type Completion] Load C++ methods lazily from DWARF #8579

Merged

This was referenced Apr 30, 2024

[lldb][Type Completion] Fix various redecl-completion issues that stem from the UseRedeclCompletion setting split #8660

Merged

[lldb][Type Completion] Fix various redecl-completion issues that stem from the UseRedeclCompletion setting split #8659

Merged

Michael137 mentioned this pull request May 17, 2024

[WIP] [lldb][Progress] Report progress when completing types from DWARF llvm/llvm-project#91452

Merged

Michael137 mentioned this pull request Jun 12, 2024

[WIP][lldb] Use forward decls with redeclared definitions instead of minimal import for records llvm/llvm-project#95100

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[lldb] Use forward decls with redeclared definitions instead of minimal import for records #8222

[lldb] Use forward decls with redeclared definitions instead of minimal import for records #8222

Michael137 commented Feb 16, 2024

adrian-prantl Feb 16, 2024

adrian-prantl Feb 16, 2024

adrian-prantl Feb 16, 2024

adrian-prantl Feb 16, 2024

adrian-prantl Feb 16, 2024

adrian-prantl Feb 16, 2024

adrian-prantl Feb 16, 2024

adrian-prantl Feb 16, 2024

adrian-prantl Feb 16, 2024

adrian-prantl Feb 16, 2024

Michael137 commented Feb 19, 2024

Michael137 commented Feb 19, 2024

Michael137 commented Feb 19, 2024

Michael137 commented Feb 19, 2024

Michael137 commented Feb 19, 2024

adrian-prantl commented Feb 19, 2024

Michael137 commented Feb 19, 2024

Michael137 commented Feb 19, 2024

Michael137 commented Feb 20, 2024

Michael137 commented Feb 20, 2024

Michael137 commented Feb 20, 2024

adrian-prantl commented Feb 20, 2024

	for (size_t i = 0; i < Sources.size(); ++i) {
	for (auto &Source : Sources) {

	// If we're dealing with redecl chains. We want to find the definition,
	// If we're dealing with redecl chains, we want to find the definition,

		std::unique_ptr<lldb_private::ClangASTImporter> m_clang_ast_importer_up;

		struct TypeToComplete {

[lldb] Use forward decls with redeclared definitions instead of minimal import for records #8222

[lldb] Use forward decls with redeclared definitions instead of minimal import for records #8222

Conversation

Michael137 commented Feb 16, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Michael137 commented Feb 19, 2024

Michael137 commented Feb 19, 2024

Michael137 commented Feb 19, 2024

Michael137 commented Feb 19, 2024

Michael137 commented Feb 19, 2024

adrian-prantl commented Feb 19, 2024

Michael137 commented Feb 19, 2024

Michael137 commented Feb 19, 2024

Michael137 commented Feb 20, 2024

Michael137 commented Feb 20, 2024

Michael137 commented Feb 20, 2024

adrian-prantl commented Feb 20, 2024