[WIP][lldb][DWARFASTParserClang] Eagerly search definitions for Objective-C classes #119860
Conversation
…tive-C classes This patch essentially reverts the definition DIE delay changes (in https://github.com/llvm/llvm-project/pull/98361/files) for Objective-C. The problem we've been seeing is as follows: 1. An Objetive-C class extension is represented in DWARF as: ``` DW_TAG_structure_type DW_AT_declaration (true) DW_AT_name ("ExtendedClass") DW_TAG_subprogram DW_AT_name ("extensionMethod") ... ``` I.e., it is a forward declaration of the extended class, but that forward declaration has children methods. 2. When we set a breakpoint in one of those methods we parse the subprogram DIE and try to create an `ObjCMethodDecl` for it (and attach it to the context). 3. When parsing the subprogram DIE, we first try to complete the DeclContext. Because `ExtendedClass` is only a forward declaration and we don't try to fetch the definition DIE eagerly anymore, LLDB has no idea that we're dealing with an Objective-C type. So it goes ahead and constructs it as a `CXXRecordDecl`. This confuses `DWARFASTParserClang::ParseObjCMethod` because it expects the context to be an `clang::ObjCObjectOrInterfaceType`. So it bails and we end up crashing because we try to attach a `FunctionDecl` to an incomplete `CXXRecordDecl` (which wasn't even forcefully completed). Since there's no good way to tell whether a forward declaration is an Objective-C type, the only solution I can really see here is to eagerly fetch the definition for Objective-C types.
Michael137
force-pushed
the
lldb/gmodules-dsym-objc-crash
branch
from
1096f65 to
4b1eb84
Compare
|
@llvm/pr-subscribers-lldb Author: Michael Buch (Michael137) ChangesThis patch essentially reverts the definition DIE delay changes (in https://github.com/llvm/llvm-project/pull/98361/files) for Objective-C. The problem we've been seeing is as follows:
I.e., it is a forward declaration of the extended class, but that forward declaration has children methods.
Since there's no good way to tell whether a forward declaration is an Objective-C type, the only solution I can really see here is to eagerly fetch the definition for Objective-C types. Full diff: https://github.com/llvm/llvm-project/pull/119860.diff 2 Files Affected:
diff --git a/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp b/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
index 37c1132c1c9f9a..131b2e6931f9f2 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
+++ b/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
@@ -1671,43 +1671,84 @@ DWARFASTParserClang::ParseStructureLikeDIE(const SymbolContext &sc,
attrs.is_forward_declaration = true;
}
+ DWARFDIE def_die;
+ if (attrs.is_forward_declaration && cu_language == eLanguageTypeObjC) {
+ def_die = dwarf->FindDefinitionDIE(die);
+
+ if (!def_die) {
+ SymbolFileDWARFDebugMap *debug_map_symfile = dwarf->GetDebugMapSymfile();
+ if (debug_map_symfile) {
+ // We weren't able to find a full declaration in this DWARF,
+ // see if we have a declaration anywhere else...
+ def_die = debug_map_symfile->FindDefinitionDIE(die);
+ }
+ }
+
+ if (log) {
+ dwarf->GetObjectFile()->GetModule()->LogMessage(
+ log,
+ "SymbolFileDWARF({0:p}) - {1:x16}}: {2} ({3}) type \"{4}\" is a "
+ "forward declaration, complete DIE is {5}",
+ static_cast<void *>(this), die.GetID(), DW_TAG_value_to_name(tag),
+ tag, attrs.name.GetCString(),
+ def_die ? llvm::utohexstr(def_die.GetID()) : "not found");
+ }
+
+ if (def_die) {
+ if (auto [it, inserted] = dwarf->GetDIEToType().try_emplace(
+ def_die.GetDIE(), DIE_IS_BEING_PARSED);
+ !inserted) {
+ if (it->getSecond() == nullptr ||
+ it->getSecond() == DIE_IS_BEING_PARSED)
+ return nullptr;
+ return it->getSecond()->shared_from_this();
+ }
+ attrs = ParsedDWARFTypeAttributes(def_die);
+ }
+ }
+
+ if (!def_die)
+ def_die = die;
+
if (attrs.name) {
- GetUniqueTypeNameAndDeclaration(die, cu_language, unique_typename,
+ GetUniqueTypeNameAndDeclaration(def_die, cu_language, unique_typename,
unique_decl);
if (log) {
dwarf->GetObjectFile()->GetModule()->LogMessage(
log, "SymbolFileDWARF({0:p}) - {1:x16}: {2} has unique name: {3} ",
- static_cast<void *>(this), die.GetID(), DW_TAG_value_to_name(tag),
+ static_cast<void *>(this), def_die.GetID(), DW_TAG_value_to_name(tag),
unique_typename.AsCString());
}
if (UniqueDWARFASTType *unique_ast_entry_type =
dwarf->GetUniqueDWARFASTTypeMap().Find(
- unique_typename, die, unique_decl, byte_size,
+ unique_typename, def_die, unique_decl, byte_size,
attrs.is_forward_declaration)) {
if (TypeSP type_sp = unique_ast_entry_type->m_type_sp) {
- dwarf->GetDIEToType()[die.GetDIE()] = type_sp.get();
+ dwarf->GetDIEToType()[def_die.GetDIE()] = type_sp.get();
LinkDeclContextToDIE(
- GetCachedClangDeclContextForDIE(unique_ast_entry_type->m_die), die);
+ GetCachedClangDeclContextForDIE(unique_ast_entry_type->m_die),
+ def_die);
// If the DIE being parsed in this function is a definition and the
// entry in the map is a declaration, then we need to update the entry
// to point to the definition DIE.
if (!attrs.is_forward_declaration &&
unique_ast_entry_type->m_is_forward_declaration) {
- unique_ast_entry_type->UpdateToDefDIE(die, unique_decl, byte_size);
+ unique_ast_entry_type->UpdateToDefDIE(def_die, unique_decl,
+ byte_size);
clang_type = type_sp->GetForwardCompilerType();
CompilerType compiler_type_no_qualifiers =
ClangUtil::RemoveFastQualifiers(clang_type);
dwarf->GetForwardDeclCompilerTypeToDIE().insert_or_assign(
compiler_type_no_qualifiers.GetOpaqueQualType(),
- *die.GetDIERef());
+ *def_die.GetDIERef());
}
return type_sp;
}
}
}
- DEBUG_PRINTF("0x%8.8" PRIx64 ": %s (\"%s\")\n", die.GetID(),
+ DEBUG_PRINTF("0x%8.8" PRIx64 ": %s (\"%s\")\n", def_die.GetID(),
DW_TAG_value_to_name(tag), type_name_cstr);
int tag_decl_kind = -1;
@@ -1726,14 +1767,14 @@ DWARFASTParserClang::ParseStructureLikeDIE(const SymbolContext &sc,
if ((attrs.class_language == eLanguageTypeObjC ||
attrs.class_language == eLanguageTypeObjC_plus_plus) &&
!attrs.is_complete_objc_class &&
- die.Supports_DW_AT_APPLE_objc_complete_type()) {
+ def_die.Supports_DW_AT_APPLE_objc_complete_type()) {
// We have a valid eSymbolTypeObjCClass class symbol whose name
// matches the current objective C class that we are trying to find
// and this DIE isn't the complete definition (we checked
// is_complete_objc_class above and know it is false), so the real
// definition is in here somewhere
TypeSP type_sp =
- dwarf->FindCompleteObjCDefinitionTypeForDIE(die, attrs.name, true);
+ dwarf->FindCompleteObjCDefinitionTypeForDIE(def_die, attrs.name, true);
if (!type_sp) {
SymbolFileDWARFDebugMap *debug_map_symfile = dwarf->GetDebugMapSymfile();
@@ -1741,7 +1782,7 @@ DWARFASTParserClang::ParseStructureLikeDIE(const SymbolContext &sc,
// We weren't able to find a full declaration in this DWARF,
// see if we have a declaration anywhere else...
type_sp = debug_map_symfile->FindCompleteObjCDefinitionTypeForDIE(
- die, attrs.name, true);
+ def_die, attrs.name, true);
}
}
@@ -1751,8 +1792,9 @@ DWARFASTParserClang::ParseStructureLikeDIE(const SymbolContext &sc,
log,
"SymbolFileDWARF({0:p}) - {1:x16}: {2} ({3}) type \"{4}\" is an "
"incomplete objc type, complete type is {5:x8}",
- static_cast<void *>(this), die.GetID(), DW_TAG_value_to_name(tag),
- tag, attrs.name.GetCString(), type_sp->GetID());
+ static_cast<void *>(this), def_die.GetID(),
+ DW_TAG_value_to_name(tag), tag, attrs.name.GetCString(),
+ type_sp->GetID());
}
return type_sp;
}
@@ -1761,7 +1803,7 @@ DWARFASTParserClang::ParseStructureLikeDIE(const SymbolContext &sc,
if (attrs.is_forward_declaration) {
// See if the type comes from a Clang module and if so, track down
// that type.
- TypeSP type_sp = ParseTypeFromClangModule(sc, die, log);
+ TypeSP type_sp = ParseTypeFromClangModule(sc, def_die, log);
if (type_sp)
return type_sp;
}
@@ -1769,10 +1811,10 @@ DWARFASTParserClang::ParseStructureLikeDIE(const SymbolContext &sc,
assert(tag_decl_kind != -1);
UNUSED_IF_ASSERT_DISABLED(tag_decl_kind);
clang::DeclContext *containing_decl_ctx =
- GetClangDeclContextContainingDIE(die, nullptr);
+ GetClangDeclContextContainingDIE(def_die, nullptr);
PrepareContextToReceiveMembers(m_ast, GetClangASTImporter(),
- containing_decl_ctx, die,
+ containing_decl_ctx, def_die,
attrs.name.GetCString());
if (attrs.accessibility == eAccessNone && containing_decl_ctx) {
@@ -1785,31 +1827,32 @@ DWARFASTParserClang::ParseStructureLikeDIE(const SymbolContext &sc,
}
ClangASTMetadata metadata;
- metadata.SetUserID(die.GetID());
- metadata.SetIsDynamicCXXType(dwarf->ClassOrStructIsVirtual(die));
+ metadata.SetUserID(def_die.GetID());
+ metadata.SetIsDynamicCXXType(dwarf->ClassOrStructIsVirtual(def_die));
TypeSystemClang::TemplateParameterInfos template_param_infos;
- if (ParseTemplateParameterInfos(die, template_param_infos)) {
+ if (ParseTemplateParameterInfos(def_die, template_param_infos)) {
clang::ClassTemplateDecl *class_template_decl =
m_ast.ParseClassTemplateDecl(
- containing_decl_ctx, GetOwningClangModule(die), attrs.accessibility,
- attrs.name.GetCString(), tag_decl_kind, template_param_infos);
+ containing_decl_ctx, GetOwningClangModule(def_die),
+ attrs.accessibility, attrs.name.GetCString(), tag_decl_kind,
+ template_param_infos);
if (!class_template_decl) {
if (log) {
dwarf->GetObjectFile()->GetModule()->LogMessage(
log,
"SymbolFileDWARF({0:p}) - {1:x16}: {2} ({3}) type \"{4}\" "
"clang::ClassTemplateDecl failed to return a decl.",
- static_cast<void *>(this), die.GetID(), DW_TAG_value_to_name(tag),
- tag, attrs.name.GetCString());
+ static_cast<void *>(this), def_die.GetID(),
+ DW_TAG_value_to_name(tag), tag, attrs.name.GetCString());
}
return TypeSP();
}
clang::ClassTemplateSpecializationDecl *class_specialization_decl =
m_ast.CreateClassTemplateSpecializationDecl(
- containing_decl_ctx, GetOwningClangModule(die), class_template_decl,
- tag_decl_kind, template_param_infos);
+ containing_decl_ctx, GetOwningClangModule(def_die),
+ class_template_decl, tag_decl_kind, template_param_infos);
clang_type =
m_ast.CreateClassTemplateSpecializationType(class_specialization_decl);
@@ -1819,13 +1862,13 @@ DWARFASTParserClang::ParseStructureLikeDIE(const SymbolContext &sc,
if (!clang_type) {
clang_type = m_ast.CreateRecordType(
- containing_decl_ctx, GetOwningClangModule(die), attrs.accessibility,
+ containing_decl_ctx, GetOwningClangModule(def_die), attrs.accessibility,
attrs.name.GetCString(), tag_decl_kind, attrs.class_language, metadata,
attrs.exports_symbols);
}
TypeSP type_sp = dwarf->MakeType(
- die.GetID(), attrs.name, attrs.byte_size, nullptr, LLDB_INVALID_UID,
+ def_die.GetID(), attrs.name, attrs.byte_size, nullptr, LLDB_INVALID_UID,
Type::eEncodingIsUID, &attrs.decl, clang_type,
Type::ResolveState::Forward,
TypePayloadClang(OptionalClangModuleID(), attrs.is_complete_objc_class));
@@ -1835,7 +1878,7 @@ DWARFASTParserClang::ParseStructureLikeDIE(const SymbolContext &sc,
// function prototypes.
clang::DeclContext *type_decl_ctx =
TypeSystemClang::GetDeclContextForType(clang_type);
- LinkDeclContextToDIE(type_decl_ctx, die);
+ LinkDeclContextToDIE(type_decl_ctx, def_die);
// UniqueDWARFASTType is large, so don't create a local variables on the
// stack, put it on the heap. This function is often called recursively and
@@ -1846,7 +1889,7 @@ DWARFASTParserClang::ParseStructureLikeDIE(const SymbolContext &sc,
// copies of the same type over and over in the ASTContext for our
// module
unique_ast_entry_up->m_type_sp = type_sp;
- unique_ast_entry_up->m_die = die;
+ unique_ast_entry_up->m_die = def_die;
unique_ast_entry_up->m_declaration = unique_decl;
unique_ast_entry_up->m_byte_size = byte_size;
unique_ast_entry_up->m_is_forward_declaration = attrs.is_forward_declaration;
@@ -1862,7 +1905,7 @@ DWARFASTParserClang::ParseStructureLikeDIE(const SymbolContext &sc,
dwarf->GetForwardDeclCompilerTypeToDIE()
.try_emplace(
ClangUtil::RemoveFastQualifiers(clang_type).GetOpaqueQualType(),
- *die.GetDIERef())
+ *def_die.GetDIERef())
.second;
assert(inserted && "Type already in the forward declaration map!");
(void)inserted;
diff --git a/lldb/test/Shell/SymbolFile/DWARF/objc-gmodules-class-extension.test b/lldb/test/Shell/SymbolFile/DWARF/objc-gmodules-class-extension.test
new file mode 100644
index 00000000000000..c78eb30c3889ec
--- /dev/null
+++ b/lldb/test/Shell/SymbolFile/DWARF/objc-gmodules-class-extension.test
@@ -0,0 +1,33 @@
+# REQUIRES: system-darwin
+
+# Test that we can set a breakpoint in a method of a class extension.
+# This requires us to parse the method into an AST type, and the context
+# too (which in DWARF is just a forward declaration).
+#
+# RUN: split-file %s %t
+# RUN: %clangxx_host %t/lib.m -c -g -gmodules -fmodules -o %t/lib.o
+# RUN: %clangxx_host %t/main.m -g -gmodules -fmodules %t/lib.o -o %t/a.out -framework Foundation
+#
+# RUN: %lldb %t/a.out -o "breakpoint set -f lib.m -l 6" -o exit | FileCheck %s
+
+# CHECK: (lldb) breakpoint set -f lib.m -l 6
+# CHECK: Breakpoint 1: where = a.out`-[NSObject(Foo) func]
+
+#--- main.m
+int main() {
+ return 0;
+}
+
+#--- lib.m
+#import <Foundation/Foundation.h>
+
+@implementation NSObject (Foo)
+- (NSError *)func {
+ NSLog(@"Hello, World!");
+ return 0;
+}
+@end
+
+NSObject * func() {
+ return 0;
+}
|
| @@ -1671,43 +1671,84 @@ DWARFASTParserClang::ParseStructureLikeDIE(const SymbolContext &sc, | |||
| attrs.is_forward_declaration = true; | |||
| } | |||
|
|
|||
| DWARFDIE def_die; | |||
| if (attrs.is_forward_declaration && cu_language == eLanguageTypeObjC) { | |||
There was a problem hiding this comment.
This is pretty much copied from ParseEnum
There was a problem hiding this comment.
You'd also need to check for eLanguageTypeObjCPlusPlus here.
There was a problem hiding this comment.
Would also be helpful to add a comment explaining why this block exists.
| if (attrs.name) { | ||
| GetUniqueTypeNameAndDeclaration(die, cu_language, unique_typename, | ||
| GetUniqueTypeNameAndDeclaration(def_die, cu_language, unique_typename, |
There was a problem hiding this comment.
I find the name def_die misleading here. IIUC, it may be a def die if the block above was run and successful?
There was a problem hiding this comment.
+1, for non-objc/objc++ cases, the parameter die could be a def die or decl die. Maybe move def_die into the above block and update the parameter die to def_die if def_die is valid.
There was a problem hiding this comment.
The only catch there is that then die needs to be a value parameter, but I think that's a fine tradeoff. I contemplated doing that in #96484 (which is where the def_die name comes from, I think), and I didn't do it only because knew (well, I thought I knew) that code was going to be temporary).
That makes sense. |
|
Unfortunate, but I guess there's nothing we can do to avoid it. |
…ions
In Objective-C, forward declarations are currently represented as:
```
DW_TAG_structure_type
DW_AT_name ("Foo")
DW_AT_declaration (true)
DW_AT_APPLE_runtime_class (DW_LANG_ObjC)
```
However, when compiling with `-gmodules`, when a class definition
is turned into a forward declaration within a `DW_TAG_module`, the
DIE for the forward declaration looks as follows:
```
DW_TAG_structure_type
DW_AT_name ("Foo")
DW_AT_declaration (true)
```
Note the absence of `DW_AT_APPLE_runtime_class`. With recent changes in
LLDB, not being able to differentiate between C++ and Objective-C
forward declarations has become problematic (see attached test-case
and explanation in llvm#119860).
…ions
In Objective-C, forward declarations are currently represented as:
```
DW_TAG_structure_type
DW_AT_name ("Foo")
DW_AT_declaration (true)
DW_AT_APPLE_runtime_class (DW_LANG_ObjC)
```
However, when compiling with `-gmodules`, when a class definition
is turned into a forward declaration within a `DW_TAG_module`, the
DIE for the forward declaration looks as follows:
```
DW_TAG_structure_type
DW_AT_name ("Foo")
DW_AT_declaration (true)
```
Note the absence of `DW_AT_APPLE_runtime_class`. With recent changes in
LLDB, not being able to differentiate between C++ and Objective-C
forward declarations has become problematic (see attached test-case
and explanation in llvm#119860).
|
On second thought, this is more a bug in the Though we might still need to do this eager definition fetching for users who can't recompile their objective-c codebases with newer compilers. Also, I'm not entirely sure this patch here is correct yet. I think the way it's currently written doesn't handle the case where we call |
…ions
In Objective-C, forward declarations are currently represented as:
```
DW_TAG_structure_type
DW_AT_name ("Foo")
DW_AT_declaration (true)
DW_AT_APPLE_runtime_class (DW_LANG_ObjC)
```
However, when compiling with `-gmodules`, when a class definition
is turned into a forward declaration within a `DW_TAG_module`, the
DIE for the forward declaration looks as follows:
```
DW_TAG_structure_type
DW_AT_name ("Foo")
DW_AT_declaration (true)
```
Note the absence of `DW_AT_APPLE_runtime_class`. With recent changes in
LLDB, not being able to differentiate between C++ and Objective-C
forward declarations has become problematic (see attached test-case
and explanation in llvm#119860).
(cherry picked from commit 7c7fd60)
…ions
In Objective-C, forward declarations are currently represented as:
```
DW_TAG_structure_type
DW_AT_name ("Foo")
DW_AT_declaration (true)
DW_AT_APPLE_runtime_class (DW_LANG_ObjC)
```
However, when compiling with `-gmodules`, when a class definition
is turned into a forward declaration within a `DW_TAG_module`, the
DIE for the forward declaration looks as follows:
```
DW_TAG_structure_type
DW_AT_name ("Foo")
DW_AT_declaration (true)
```
Note the absence of `DW_AT_APPLE_runtime_class`. With recent changes in
LLDB, not being able to differentiate between C++ and Objective-C
forward declarations has become problematic (see attached test-case
and explanation in llvm#119860).
…ions (#120154) In Objective-C, forward declarations are currently represented as: ``` DW_TAG_structure_type DW_AT_name ("Foo") DW_AT_declaration (true) DW_AT_APPLE_runtime_class (DW_LANG_ObjC) ``` However, when compiling with `-gmodules`, when a class definition is turned into a forward declaration within a `DW_TAG_module`, the DIE for the forward declaration looks as follows: ``` DW_TAG_structure_type DW_AT_name ("Foo") DW_AT_declaration (true) ``` Note the absence of `DW_AT_APPLE_runtime_class`. With recent changes in LLDB, not being able to differentiate between C++ and Objective-C forward declarations has become problematic (see attached test-case and explanation in #119860).
In Actually, there's already a block to eagerly find the complete type when llvm-project/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp Lines 1713 to 1715 in c9a5a6d is_complete_objc_class the same as !is_forward_declaration?
|
Yea that part of the ObjC support is particularly confusing. I added some context here: #120279 It previously had pretty much no coverage (and for some reason |
IIUC for (1) and (2), that block eagerly finds the definition die and resolve it in |
This patch essentially reverts the definition DIE delay changes (in https://github.com/llvm/llvm-project/pull/98361/files) for Objective-C.
The problem we've been seeing is as follows:
I.e., it is a forward declaration of the extended class, but that forward declaration has children methods.
When we set a breakpoint in one of those methods we parse the subprogram DIE and try to create an
ObjCMethodDeclfor it (and attach it to the context).When parsing the subprogram DIE, we first try to complete the DeclContext. Because
ExtendedClassis only a forward declaration and we don't try to fetch the definition DIE eagerly anymore, LLDB has no idea that we're dealing with an Objective-C type. So it goes ahead and constructs it as aCXXRecordDecl. This confusesDWARFASTParserClang::ParseObjCMethodbecause it expects the context to be anclang::ObjCObjectOrInterfaceType. So it bails and we end up crashing because we try to attach aFunctionDeclto an incompleteCXXRecordDecl(which wasn't even forcefully completed).Since there's no good way to tell whether a forward declaration is an Objective-C type, the only solution I can really see here is to eagerly fetch the definition for Objective-C types.