[lldb] Fix crash missing MSInheritanceAttr on CXXRecordDecl with DWARF on Windows #112928
Conversation
…r DWARF on Windows
|
This PR isn't complete yet, but early feedback is welcome. Next step is to get test coverage. There are similar cases for types like |
|
This might be related to #56458 even though my repro is different. |
|
The test caused a crash in the past: Now it survives: Next, we might want to query the complete type on demand to determine the actual byte sizes. |
|
@llvm/pr-subscribers-lldb Author: Stefan Gränitz (weliveindetail) ChangesIn the MS ABI, member pointers to Full diff: https://github.com/llvm/llvm-project/pull/112928.diff 2 Files Affected:
diff --git a/lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp b/lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
index fe0c53a7e9a3ea..a23dce97f3f299 100644
--- a/lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
+++ b/lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
@@ -2771,6 +2771,9 @@ static bool GetCompleteQualType(clang::ASTContext *ast,
ast, llvm::cast<clang::AttributedType>(qual_type)->getModifiedType(),
allow_completion);
+ case clang::Type::MemberPointer:
+ return !qual_type.getTypePtr()->isIncompleteType();
+
default:
break;
}
diff --git a/lldb/test/Shell/SymbolFile/DWARF/x86/ms-abi.cpp b/lldb/test/Shell/SymbolFile/DWARF/x86/ms-abi.cpp
new file mode 100644
index 00000000000000..0e51ec99934f4c
--- /dev/null
+++ b/lldb/test/Shell/SymbolFile/DWARF/x86/ms-abi.cpp
@@ -0,0 +1,45 @@
+// REQUIRES: lld
+
+// Check that we don't crash on variables without MSInheritanceAttr
+
+// RUN: %clang -c --target=x86_64-windows-msvc -gdwarf %s -o %t.obj
+// RUN: lld-link /out:%t.exe %t.obj /nodefaultlib /entry:main /debug
+// RUN: %lldb -f %t.exe -b -o "target variable mp1 mp2 mp3 mp4 mp5 mp6 mp7 mp8 mp9"
+
+class SI {
+ int si;
+};
+struct SI2 {
+ int si2;
+};
+class MI : SI, SI2 {
+ int mi;
+};
+class MI2 : MI {
+ int mi2;
+};
+class VI : virtual MI {
+ int vi;
+};
+class VI2 : virtual SI, virtual SI2 {
+ int vi;
+};
+class /* __unspecified_inheritance*/ UI;
+
+typedef void (SI::*SITYPE)();
+typedef void (MI::*MITYPE)();
+typedef void (MI2::*MI2TYPE)();
+typedef void (VI::*VITYPE)();
+typedef void (VI2::*VI2TYPE)();
+typedef void (UI::*UITYPE)();
+SITYPE mp1 = nullptr;
+MITYPE mp2 = nullptr;
+MI2TYPE mp3 = nullptr;
+VITYPE mp4 = nullptr;
+VI2TYPE mp5 = nullptr;
+UITYPE mp6 = nullptr;
+MITYPE *mp7 = nullptr;
+VI2TYPE *mp8 = nullptr;
+int SI::*mp9 = nullptr;
+
+int main() {}
|
| @@ -2771,6 +2771,9 @@ static bool GetCompleteQualType(clang::ASTContext *ast, | |||
| ast, llvm::cast<clang::AttributedType>(qual_type)->getModifiedType(), | |||
| allow_completion); | |||
|
|
|||
| case clang::Type::MemberPointer: | |||
| return !qual_type.getTypePtr()->isIncompleteType(); | |||
There was a problem hiding this comment.
I see, so for non-MS ABI isIncompleteType will pretty much work as before, and trivially return true. Otherwise it checks for the MSInheritanceAttr. Seems reasonable to me
There was a problem hiding this comment.
TypeSystemClang.cpp change LGTM. Though It'd be nice if we added the same test-coverage also for non-MSVC (only found shell tests for PDB, and 1 API test that didn't enumerate all the cases).
I guess a follow-up question is how to set the MSInheritanceAttr. Infer it from the class DIE? Or add a new DWARF attribute? Also, whether we still want to issue a warning when debugging using DWARF and the MS ABI (like suggested in https://reviews.llvm.org/D130942).
|
Thanks for the feedback! Yes, I think fixing the crash is a valuable addition on its own. It broke use-cases where we just want to set a breakpoint and encounter this type on the way, but don't actually care about its byte size.
I did some more digging and now I wonder: Couldn't |
It is ABI dependent, and we probably do want to rely on Clang to tell us this so we don't reimplement all that logic. E.g., for Itanium, the size would be |
|
Ok right, member function pointers are more than just offsets. So, hard-coding |
| // Itanium ABI: | ||
| // RUN: %clang --target=x86_64-pc-linux -gdwarf -c -o %t_linux.o %s | ||
| // RUN: ld.lld %t_linux.o -o %t_linux | ||
| // RUN: %lldb -f %t_linux -b -o "target variable mp1 mp2 mp3 mp4 mp5 mp6 mp7 mp8 mp9" | FileCheck %s |
There was a problem hiding this comment.
@Michael137 You think that's ok as test-coverage also for non-MSVC?
There was a problem hiding this comment.
Yup, thanks!
Don't think we even need to link (in either case).
Also lets add a CHECK-MSVC: as well. That'd notify us once that byte-size issue gets fixed on Windows
There was a problem hiding this comment.
Agree, added CHECK-MSVC: error: Unable to determine byte size. and dropped link-step for Itanium case. The latter doesn't seem to work for the MS case. It fails early with:
error: unable to resolve the module for file address 0x0 for variable 'mp1' in
S:\path\to\build\tools\lldb\test\Shell\SymbolFile\DWARF\x86\Output\member-pointers.cpp.tmp_win.obj
There was a problem hiding this comment.
Ah fair enough, latest test changes LGTM
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
Actually, rather this one: #56449 It adds |
| double SI::*mp1 = nullptr; | ||
| int MI::*mp2 = nullptr; | ||
| int MI2::*mp3 = nullptr; | ||
| int VI::*mp4 = nullptr; | ||
| int VI2::*mp5 = nullptr; | ||
| int UI::*mp6 = nullptr; | ||
| int MI::*mp7 = nullptr; | ||
| int VI2::*mp8 = nullptr; |
There was a problem hiding this comment.
What's up with all of these? I think we should either CHECK their results or delete them.
There was a problem hiding this comment.
Right. They exercise the various MSInheritance cases for the Microsoft ABI, but since there is no support in DWARF yet, we don't really need them. I reduced the test to the case that it checks.
|
I reverted this change with #113498 |
|
Hi @rastogishubham thanks for acting on this! The log of this particular run was deleted already, but https://green.lab.llvm.org/job/llvm.org/view/LLDB/job/as-lldb-cmake/13958 likely failed for the same reason: Apparently, |
…ows (#112928) Member pointers refer to data or function members of a `CXXRecordDecl`, which require a `MSInheritanceAttr` in order to be complete. Without that we cannot calculate the size of a member pointer in memory. The attempt has been causing a crash further down in the clang AST context. In order to implement the feature, DWARF will need a new attribtue to convey the information. For the moment, this patch teaches LLDB to handle to situation and avoid the crash.
|
I fixed the test and it now passes for me on macOS. Hope the bots are good now. |
|
@weliveindetail thanks for the fix! I was at the llvm dev meeting so I was slow to respond. The bots are still green so we are good! |
…m#112928) Member pointers refer to data or function members of a `CXXRecordDecl` and require a `MSInheritanceAttr` in order to be complete. Without that we cannot calculate their size in memory. The attempt has been causing a crash further down in the clang AST context. In order to implement the feature, DWARF will need a new attribtue to convey the information. For the moment, this patch teaches LLDB to handle to situation and avoid the crash.
…ith DWARF on Windows" (llvm#113498) Reverts llvm#112928 This is because it broke greendragon: SymbolFile/DWARF/x86/member-pointers.cpp
…ows (llvm#112928) Member pointers refer to data or function members of a `CXXRecordDecl`, which require a `MSInheritanceAttr` in order to be complete. Without that we cannot calculate the size of a member pointer in memory. The attempt has been causing a crash further down in the clang AST context. In order to implement the feature, DWARF will need a new attribtue to convey the information. For the moment, this patch teaches LLDB to handle to situation and avoid the crash.
…ows (llvm#112928) Member pointers refer to data or function members of a `CXXRecordDecl`, which require a `MSInheritanceAttr` in order to be complete. Without that we cannot calculate the size of a member pointer in memory. The attempt has been causing a crash further down in the clang AST context. In order to implement the feature, DWARF will need a new attribtue to convey the information. For the moment, this patch teaches LLDB to handle to situation and avoid the crash.
…ows (llvm#112928) Member pointers refer to data or function members of a `CXXRecordDecl`, which require a `MSInheritanceAttr` in order to be complete. Without that we cannot calculate the size of a member pointer in memory. The attempt has been causing a crash further down in the clang AST context. In order to implement the feature, DWARF will need a new attribtue to convey the information. For the moment, this patch teaches LLDB to handle to situation and avoid the crash.
…ows (llvm#112928) Member pointers refer to data or function members of a `CXXRecordDecl`, which require a `MSInheritanceAttr` in order to be complete. Without that we cannot calculate the size of a member pointer in memory. The attempt has been causing a crash further down in the clang AST context. In order to implement the feature, DWARF will need a new attribtue to convey the information. For the moment, this patch teaches LLDB to handle to situation and avoid the crash.
llvm#115177) Following up from llvm#112928, we can reuse the approach from Clang Sema to infer the MSInheritanceModel and add the necessary attribute manually. This allows the inspection of member function pointers with DWARF on Windows.
…ows (llvm#112928) Member pointers refer to data or function members of a `CXXRecordDecl`, which require a `MSInheritanceAttr` in order to be complete. Without that we cannot calculate the size of a member pointer in memory. The attempt has been causing a crash further down in the clang AST context. In order to implement the feature, DWARF will need a new attribtue to convey the information. For the moment, this patch teaches LLDB to handle to situation and avoid the crash.
llvm#115177) Following up from llvm#112928, we can reuse the approach from Clang Sema to infer the MSInheritanceModel and add the necessary attribute manually. This allows the inspection of member function pointers with DWARF on Windows.
llvm#115177) Following up from llvm#112928, we can reuse the approach from Clang Sema to infer the MSInheritanceModel and add the necessary attribute manually. This allows the inspection of member function pointers with DWARF on Windows.
…mber-pointers-crash 🍒 [lldb] Fix crash missing MSInheritanceAttr with DWARF on Windows (llvm#112928)
…ows (llvm#112928) Member pointers refer to data or function members of a `CXXRecordDecl`, which require a `MSInheritanceAttr` in order to be complete. Without that we cannot calculate the size of a member pointer in memory. The attempt has been causing a crash further down in the clang AST context. In order to implement the feature, DWARF will need a new attribtue to convey the information. For the moment, this patch teaches LLDB to handle to situation and avoid the crash. (cherry picked from commit c39692a)
llvm#115177) Following up from llvm#112928, we can reuse the approach from Clang Sema to infer the MSInheritanceModel and add the necessary attribute manually. This allows the inspection of member function pointers with DWARF on Windows. (cherry picked from commit e9dd419)
llvm#115177) Following up from llvm#112928, we can reuse the approach from Clang Sema to infer the MSInheritanceModel and add the necessary attribute manually. This allows the inspection of member function pointers with DWARF on Windows. (cherry picked from commit e9dd419)
llvm#115177) Following up from llvm#112928, we can reuse the approach from Clang Sema to infer the MSInheritanceModel and add the necessary attribute manually. This allows the inspection of member function pointers with DWARF on Windows. (cherry picked from commit e9dd419)
llvm#115177) Following up from llvm#112928, we can reuse the approach from Clang Sema to infer the MSInheritanceModel and add the necessary attribute manually. This allows the inspection of member function pointers with DWARF on Windows. (cherry picked from commit e9dd419)
…ows (llvm#112928) Member pointers refer to data or function members of a `CXXRecordDecl`, which require a `MSInheritanceAttr` in order to be complete. Without that we cannot calculate the size of a member pointer in memory. The attempt has been causing a crash further down in the clang AST context. In order to implement the feature, DWARF will need a new attribtue to convey the information. For the moment, this patch teaches LLDB to handle to situation and avoid the crash. (cherry picked from commit c39692a)
llvm#115177) Following up from llvm#112928, we can reuse the approach from Clang Sema to infer the MSInheritanceModel and add the necessary attribute manually. This allows the inspection of member function pointers with DWARF on Windows. (cherry picked from commit e9dd419)
llvm#115177) Following up from llvm#112928, we can reuse the approach from Clang Sema to infer the MSInheritanceModel and add the necessary attribute manually. This allows the inspection of member function pointers with DWARF on Windows. (cherry picked from commit e9dd419)
…ows (llvm#112928) Member pointers refer to data or function members of a `CXXRecordDecl`, which require a `MSInheritanceAttr` in order to be complete. Without that we cannot calculate the size of a member pointer in memory. The attempt has been causing a crash further down in the clang AST context. In order to implement the feature, DWARF will need a new attribtue to convey the information. For the moment, this patch teaches LLDB to handle to situation and avoid the crash. (cherry picked from commit c39692a)
llvm#115177) Following up from llvm#112928, we can reuse the approach from Clang Sema to infer the MSInheritanceModel and add the necessary attribute manually. This allows the inspection of member function pointers with DWARF on Windows. (cherry picked from commit e9dd419)
In the MS ABI, member pointers to
CXXRecordDecls must have aMSInheritanceAttrin order to be complete. Otherwise we cannot query their size in memory. This patch checksMemberPointertypes for completeness (eventually looking at the existence of the attribute) to avoid a crash further down in the clang AST context.