[lldb][DWARFASTParserClang] Adjust language type for conflicting Objective-C++ forward declarations #130768
Conversation
…ctive-C++ forward declarations WebKit does things like the following: ``` @Class NSString; class NSString; ``` This would produce DWARF where `NSString` is a C++ forward declaration in a C++ CU, and an Objective-C forward declaration in an Objective-C++ CU. But the type really is only ever used as an Objective-C type, so it's not a "problematic" ODR violation. This confuses LLDB, however, because when we parse a this in the context of a C++ CU, we cannot tell whether to create a `clang::RecordType` or `clang::ObjCInterfaceType`. If we create a `RecordType` from this, we run into all sort of issues later down the line. This is an unfortunate use-case, which no longer works after llvm#90663 (previously we would've just found the Objective-C definition for this forward declaration). This is similar to what motivated llvm#119860, though in that case there was a way to disambiguate the forward declarations in DWARF by fixing it on the compiler side. In this case, we really can't do anything because we're dealing with CUs of different languages. This patch attempts to fix this by doing a `FindTypes` lookup for the forward declaration in question, and checking whether all the types we found were in fact Objective-C types, in which case, it's an Objective-C++ forward declaration.
|
@llvm/pr-subscribers-lldb Author: Michael Buch (Michael137) ChangesWebKit does things like the following: This would produce DWARF where This is an unfortunate use-case, which no longer works after #90663 (previously we would've just found the Objective-C definition for this forward declaration). This is similar to what motivated #119860, though in that case there was a way to disambiguate the forward declarations in DWARF by fixing it on the compiler side. In this case, we really can't do anything because we're dealing with CUs of different languages. This patch attempts to fix this by doing a Full diff: https://github.com/llvm/llvm-project/pull/130768.diff 2 Files Affected:
diff --git a/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp b/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
index 0b632751574ad..8a32d8f4aa35b 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
+++ b/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
@@ -351,6 +351,41 @@ static void PrepareContextToReceiveMembers(TypeSystemClang &ast,
}
}
+/// Returns \c true if a forward declaration in C or C++ is actually an
+/// Objective-C++ forward declaration. Otherwise, or on error, returns
+/// \c false.
+static bool IsCppForwardDeclObjC(SymbolFileDWARF &dwarf,
+ const ParsedDWARFTypeAttributes &attrs) {
+ assert(attrs.is_forward_declaration);
+
+ if (Language::LanguageIsObjC(attrs.class_language))
+ return false;
+
+ TypeQuery query(attrs.name);
+ TypeResults results;
+
+ if (SymbolFileDWARFDebugMap *debug_map_symfile = dwarf.GetDebugMapSymfile())
+ debug_map_symfile->FindTypes(query, results);
+ else
+ dwarf.FindTypes(query, results);
+
+ // Check that all types we found are Objective-C++ types.
+ // Otherwise we're dealing with an actual ODR violation and
+ // we can't say for sure what language this forward declaration
+ // referred to.
+ bool all_objc_types = true;
+ results.GetTypeMap().ForEach([&](const lldb::TypeSP &t) -> bool {
+ assert(t);
+
+ all_objc_types &= Language::LanguageIsObjC(
+ t->GetForwardCompilerType().GetMinimumLanguage());
+
+ return true;
+ });
+
+ return all_objc_types;
+}
+
ParsedDWARFTypeAttributes::ParsedDWARFTypeAttributes(const DWARFDIE &die) {
DWARFAttributes attributes = die.GetAttributes();
for (size_t i = 0; i < attributes.Size(); ++i) {
@@ -1810,6 +1845,9 @@ DWARFASTParserClang::ParseStructureLikeDIE(const SymbolContext &sc,
}
if (attrs.is_forward_declaration) {
+ if (IsCppForwardDeclObjC(*dwarf, attrs))
+ attrs.class_language = eLanguageTypeObjC_plus_plus;
+
// See if the type comes from a Clang module and if so, track down
// that type.
TypeSP type_sp = ParseTypeFromClangModule(sc, die, log);
diff --git a/lldb/test/Shell/SymbolFile/DWARF/objcxx-forward-decls.test b/lldb/test/Shell/SymbolFile/DWARF/objcxx-forward-decls.test
new file mode 100644
index 0000000000000..30109c2943c9b
--- /dev/null
+++ b/lldb/test/Shell/SymbolFile/DWARF/objcxx-forward-decls.test
@@ -0,0 +1,70 @@
+# REQUIRES: system-darwin
+
+# In this test we have two CUs with conflicting forward declaration
+# depending on the CU language (one is C++ and the other is Objective-C++).
+# We are then stopped in the C++ CU and try to print the type, at which
+# point LLDB will try to make it into an Clang AST node. If LLDB were to
+# interpret the type as C++ instead of Objective-C, we'd violate Clang
+# invariants and crash.
+#
+# RUN: split-file %s %t
+# RUN: %clangxx_host -c -g -x objective-c++ %t/request.m -o %t/request_objc.o
+# RUN: %clangxx_host -c -g %t/main.cpp -o %t/main.o
+# RUN: %clangxx_host %t/main.o %t/request_objc.o -framework Foundation -o %t/a.out
+#
+# RUN: %lldb %t/a.out \
+# RUN: -o "breakpoint set -p return -X main" \
+# RUN: -o run \
+# RUN: -o "frame variable r" \
+# RUN: -o exit | FileCheck %s
+#
+# RUN: dsymutil %t/a.out
+#
+# RUN: %lldb %t/a.out \
+# RUN: -o "breakpoint set -p return -X main" \
+# RUN: -o run \
+# RUN: -o "frame variable r" \
+# RUN: -o exit | FileCheck %s --check-prefix=CHECK-DSYM
+
+# CHECK: (lldb) frame variable r
+# CHECK-NEXT: (Request) ::r = (m_request = "Hello, World!")
+
+# CHECK-DSYM: (lldb) frame variable r
+# CHECK-DSYM-NEXT: (Request) ::r = (m_request = "Hello, World!")
+
+#--- lib.h
+#ifndef LIB_H_IN
+#define LIB_H_IN
+
+#ifdef __OBJC__
+@class NSString;
+#else
+class NSString;
+#endif
+
+struct Request {
+ NSString * m_request = nullptr;
+};
+
+#endif // _H_IN
+
+#--- main.cpp
+#include "lib.h"
+
+void process(Request *);
+
+Request r;
+
+int main() {
+ process(&r);
+ return 0;
+}
+
+#--- request.m
+#import <Foundation/Foundation.h>
+
+#include "lib.h"
+
+void process(Request * r) {
+ r->m_request = @"Hello, World!";
+}
|
| if (Language::LanguageIsObjC(attrs.class_language)) | ||
| return false; | ||
|
|
||
| TypeQuery query(attrs.name); |
There was a problem hiding this comment.
I'm hoping we can be smarter about this lookup to not incur a big penalty for doing these type lookups
There was a problem hiding this comment.
There's also some filtering we got to do on the context, we only want to consider types in the global namespace
|
oh boy.. this is going to be messy. So what would happen if we treated |
| @@ -1810,6 +1845,9 @@ DWARFASTParserClang::ParseStructureLikeDIE(const SymbolContext &sc, | |||
| } | |||
|
|
|||
| if (attrs.is_forward_declaration) { | |||
| if (IsCppForwardDeclObjC(*dwarf, attrs)) | |||
| attrs.class_language = eLanguageTypeObjC_plus_plus; | |||
There was a problem hiding this comment.
Does that mean we do full global search for every C++ fwddecl we encounter?
|
Since this makes lldb to do type searching more aggressively, I want to get a sense of performance impact on simplified template names from this PR. When I running the a test program with LLDB (with this PR), LLDB crashes. I don't have a reduced test case yet, will probably working on getting one when I have time: |
I'll try giving this a shot. If we can pull this off that'd be great. We'll probably need to modify the
If we were to go down the route I proposed, I would only do a |
I'm not sure how creating a new type would help, since at that point the c++ type already exists and could be referred to from a bunch of places. I was imagining we would just recognise that we did not find a C++ definition for this type and then do whatever we do when we don't find a definition (leave the type incomplete? forcefully complete it?...). Later, if we reach the type through an ObjC declaration, we find the ObjC defintion and complete it using that. I think the tricky part is what happens with types like your
For our use cases, ~all of the code is in a single module anyway, so not searching in other modules doesn't help much. Limiting it to the global namespace definitely helps, but due to how the accelerator tables work, it still means that you have to iterate through all DIEs with the given name anywhere in the program, just to find out whether they are at the top level or not. I don't know what how much of a performance hit would that be in practice. What would help is if we could short-circuit this code in the (common, on linux) situation where you don't have any ObjC code in your module. I'm not sure if we have a way to do that. (I don't know if we're currently parsing all CU dies, but if we are, we could check if any of them is an ObjC unit) I also think the FindTypes call is unnecessarily heavy (and dangerously recursive) for this. Since, in this world, we want to treat the two types as equivalent, I don't think we need to find all possible definitions of that type. I think it should be sufficient to settle on a canonical definition for the type (per the ODR rule and all), so we can just pick the first one we find. And we can do the search at the DWARF DIE level -- and then also record the definition die in the unique type map, so that we pick the same one when its time to complete the type. |
Good point, yea ideally we do want to support showing the value. Unfortunately that's something users have been relying upon. Modelling this similar to how we do forceful completion logic hasn't crossed my mind, but now I'm more inclined to go with the DWARF search route you suggested
Essentially you're suggesting going back to pre-#90663, but only if we have ObjC/ObjC++ CUs in the current module? Seems sensible to me. Will also help prevent similar issues from creeping up in the future |
Yes, I think that is the best approach do to do definition search (devil's in the details of course), though I'm still not very happy about having to do this search in the first place. Overall, I think I'd like the "forceful completion" approach more, though I understand it's fraught with more perils. I'm reasonably confident it can be made to work in the expression evaluator, but things like "frame variable" (which work directly on the module AST) are trickier, and I never got that to work with -flimit-debug-info (ran out of time). This case should be slightly simpler than that for two reasons:
So, it might be possible to implement something like this inside the |
Coming back to this now. IIUC, you're saying when we |
|
I was imagining storing this in the ClangASTMetadata class (that's where the forceful completion bit is)? |
…types This patch is a temporary alternative to llvm#130768 If we detect a situation where a forward declaration is C++ and the definition DIE is Objective-C, then just don't try to complete the type (it would crash otherwise). In the long term we might want to add support for completing such types rdar://145959981
|
Interestingly this alternative stop-gap (where we just bail from |
…types This patch is a temporary alternative to llvm#130768 If we detect a situation where a forward declaration is C++ and the definition DIE is Objective-C, then just don't try to complete the type (it would crash otherwise). In the long term we might want to add support for completing such types rdar://145959981
WebKit does things like the following:
This would produce DWARF where
NSStringis a C++ forward declaration in a C++ CU, and an Objective-C forward declaration in an Objective-C++ CU. But the type really is only ever used as an Objective-C type, so it's not a "problematic" ODR violation. This confuses LLDB, however, because when we parse this in the context of a C++ CU, we cannot tell whether to create aclang::RecordTypeorclang::ObjCInterfaceType. If we create aRecordTypefrom this, we run into all sort of issues later down the line.This is an unfortunate use-case, which no longer works after #90663 (previously we would've just found the Objective-C definition for this forward declaration). This is similar to what motivated #119860, though in that case there was a way to disambiguate the forward declarations in DWARF by fixing it on the compiler side. In this case, we really can't do anything because we're dealing with CUs of different languages.
This patch attempts to fix this by doing a
FindTypeslookup for the forward declaration in question, and checking whether all the types we found were in fact Objective-C types, in which case, it's an Objective-C++ forward declaration.