[lldb] Fix crash when adding members to an "incomplete" type

[labath]

This fixes a regression caused by delayed type definition searching (#96755 and friends): If we end up adding a member (e.g. a typedef) to a type that we've already attempted to complete (and failed), the resulting AST would end up inconsistent (we would start to "forcibly" complete it, but never finish it), and importing it into an expression AST would crash.

This patch fixes this by detecting the situation and finishing the definition as well.

[llvmbot]

@llvm/pr-subscribers-lldb

Author: Pavel Labath (labath)

Changes

This fixes a regression caused by delayed type definition searching (#96755 and friends): If we end up adding a member (e.g. a typedef) to a type that we've already attempted to complete (and failed), the resulting AST would end up inconsistent (we would start to "forcibly" complete it, but never finish it), and importing it into an expression AST would crash.

This patch fixes this by detecting the situation and finishing the definition as well.

---

Full diff: https://github.com/llvm/llvm-project/pull/102116.diff

2 Files Affected:

• (modified) lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp (+9-2)
• (added) lldb/test/Shell/SymbolFile/DWARF/x86/typedef-in-incomplete-type.cpp (+23)

diff --git a/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp b/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
index a4dcde1629fc2..3968d5bf71e1c 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
+++ b/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
@@ -269,8 +269,15 @@ static void PrepareContextToReceiveMembers(TypeSystemClang &ast,
   }
 
   // We don't have a type definition and/or the import failed, but we need to
-  // add members to it. Start the definition to make that possible.
-  tag_decl_ctx->startDefinition();
+  // add members to it. Start the definition to make that possible. If the type
+  // has no external storage we also have to complete the definition. Otherwise,
+  // that will happen when we are asked to complete the type
+  // (CompleteTypeFromDWARF).
+  ast.StartTagDeclarationDefinition(type);
+  if (!tag_decl_ctx->hasExternalLexicalStorage()) {
+    ast.SetDeclIsForcefullyCompleted(tag_decl_ctx);
+    ast.CompleteTagDeclarationDefinition(type);
+  }
 }
 
 ParsedDWARFTypeAttributes::ParsedDWARFTypeAttributes(const DWARFDIE &die) {
diff --git a/lldb/test/Shell/SymbolFile/DWARF/x86/typedef-in-incomplete-type.cpp b/lldb/test/Shell/SymbolFile/DWARF/x86/typedef-in-incomplete-type.cpp
new file mode 100644
index 0000000000000..47ea1e2639c6e
--- /dev/null
+++ b/lldb/test/Shell/SymbolFile/DWARF/x86/typedef-in-incomplete-type.cpp
@@ -0,0 +1,23 @@
+// RUN: %clangxx --target=x86_64-pc-linux -o %t -c %s -g
+// RUN: %lldb %t -o "target var a" -o "expr -- var" -o exit | FileCheck %s
+
+// This forces lldb to attempt to complete the type A. Since it has no
+// definition it will fail.
+// CHECK: target var a
+// CHECK: (A) a = <incomplete type "A">
+
+// Now attempt to display the second variable, which will try to add a typedef
+// to the incomplete type. Make sure that succeeds. Use the expression command
+// to make sure the resulting AST can be imported correctly.
+// CHECK: expr -- var
+// CHECK: (A::X) $0 = 0
+
+struct A {
+  // Declare the constructor, but don't define it to avoid emitting the
+  // definition in the debug info.
+  A();
+  using X = int;
+};
+
+A a;
+A::X var;

[Michael137]

This would only crash with -flimit-debug-info right? Could we add that here?
Also should we move it to the non-x86 directory? Not sure what the intent behind the x86 directory is but it would be nice to get the coverage when running this on ARM machines too

[labath]

This would only crash with -flimit-debug-info right?

Correct. The type definition needs to be absent from the binary and the ~only way to achieve that is to compile at least a part of it with -flimit-debug-info. I will add that explicitly.

Not sure what the intent behind the x86 directory is but it would be nice to get the coverage when running this on ARM machines too

The x86 directory is conditionalized on the x86 target, so it will run whenever the local build supports the x86 architecture (ie X86 is in LLVM_TARGETS_TO_BUILD), regardless of the actual host architecture. (i.e., it works the same as as x86 directories in llvm)

The goal here isn't so much to hardcode the architecture (to x86), as much it is to hardcode the file format (to non-windows). We don't support working with unlinked object files on windows, and if I tried to link things then I'd need to depend on the linker and pull in other platform-specific stuff, which would make the test non-minimal.

The thing I like about a test like this is that it (again, like the llvm tests) runs the same way everywhere, so there's little risk of needing to scrable to find a freebsd-mips (I'm exaggerating, I know) machine to reproduce a problem somewhere. (And I would totally love if we had more tests hardcoding aarch64-macosx triples so I can get more macos coverage without switching to a mac machine)

[Michael137]

Ahh got it
I saw the tests in this directory weren't run on my M1 and misinterpreted that.

The thing I like about a test like this is that it (again, like the llvm tests) runs the same way everywhere, so there's little risk of needing to scrable to find a freebsd-mips (I'm exaggerating, I know) machine to reproduce a problem somewhere.

Agreed

[Michael137]

LGTM, thanks!

So the actual problem is that the ASTImporter will only copy over the definition to the destination decl if the source decl isCompleteDefinition. So then when we tried to add the typedef decl to the CXXRecordDecl that the ASTImporter created for A, it would try to query its definition which didn't get copied over (cause we only ever started the definition for A, but nothing completed it, especially since it had no external storage), and hence we assert.

[labath]

LGTM, thanks!

So the actual problem is that the ASTImporter will only copy over the definition to the destination decl if the source decl isCompleteDefinition. So then when we tried to add the typedef decl to the CXXRecordDecl that the ASTImporter created for A, it would try to query its definition which didn't get copied over (cause we only ever started the definition for A, but nothing completed it, especially since it had no external storage), and hence we assert.

No, the problem was that A was stuck in this half-complete state, where we've begun -- but not finished -- it's definition, and it also didn't have an external ast source which could complete it (because we've already tried completing it, and CompleteTypeFromDWARF has cleared the "external" flag). AFAICT, the ast importer is just not prepared to handle this situation. The crash happened when it tried to query some property (probably to copy it over) of the source Decl, and this crashed because the decl didn't have the DefinitionData field set up.

[Michael137]

LGTM, thanks!
So the actual problem is that the ASTImporter will only copy over the definition to the destination decl if the source decl isCompleteDefinition. So then when we tried to add the typedef decl to the CXXRecordDecl that the ASTImporter created for A, it would try to query its definition which didn't get copied over (cause we only ever started the definition for A, but nothing completed it, especially since it had no external storage), and hence we assert.

No, the problem was that A was stuck in this half-complete state, where we've begun -- but not finished -- it's definition, and it also didn't have an external ast source which could complete it (because we've already tried completing it, and CompleteTypeFromDWARF has cleared the "external" flag). AFAICT, the ast importer is just not prepared to handle this situation. The crash happened when it tried to query some property (probably to copy it over) of the source Decl, and this crashed because the decl didn't have the DefinitionData field set up.

Yea agreed. I was just curious where exactly the difference in the ASTImporter codepath came from with/without the patch. And it looks like without completing the definition, we lose the DefinitionData during the import process. What I meant was: we did allocate DefinitionData for A in PrepareContextToRecieveMembers, but when we imported/copied the decl for A::X into the expression AST, the ASTImporter didn't import the definition for A because the isCompleteDefinition bit wasn't set (which happens here). With this patch, we make sure that the call to ImportDefinition happens before we try adding the TypedefDecl to the A context. Let me know if I'm still misunderstanding. But either way, I agree with your description of "A is in a half-complete state and the ASTImporter isn't equipped to deal with it".

[labath]

You're right -- I misremembered. I thought the DefinitionData is set when completing the definition, but it happens when the definition is started. In that case, yes, the crash happened because the ast importer didn't copy over the definition data, but then tried to access it when adding the typedef.

[labath]

/cherry-pick 57cd100

[llvmbot]

Failed to cherry-pick: 57cd100

https://github.com/llvm/llvm-project/actions/runs/10352006063

Please manually backport the fix and push it to your github fork. Once this is done, please create a pull request

[labath]

/cherry-pick 57cd100

[llvmbot]

/pull-request #102895