[lldb][Windows] Fix x86_64 default unwind plan#210076
Conversation
|
@llvm/pr-subscribers-lldb Author: Charles Zablit (charles-zablit) Changes
This patch uses the This patch also ports Full diff: https://github.com/llvm/llvm-project/pull/210076.diff 4 Files Affected:
diff --git a/lldb/source/Plugins/ABI/X86/ABIWindows_x86_64.cpp b/lldb/source/Plugins/ABI/X86/ABIWindows_x86_64.cpp
index 079b22a307602..be3c83880bd60 100644
--- a/lldb/source/Plugins/ABI/X86/ABIWindows_x86_64.cpp
+++ b/lldb/source/Plugins/ABI/X86/ABIWindows_x86_64.cpp
@@ -751,21 +751,15 @@ UnwindPlanSP ABIWindows_x86_64::CreateFunctionEntryUnwindPlan() {
// Windows-x86_64 doesn't use %rbp
// No available Unwind information for Windows-x86_64 (section .pdata)
-// Let's use SysV-x86_64 one for now
UnwindPlanSP ABIWindows_x86_64::CreateDefaultUnwindPlan() {
- uint32_t fp_reg_num = dwarf_rbp;
uint32_t sp_reg_num = dwarf_rsp;
uint32_t pc_reg_num = dwarf_rip;
UnwindPlan::Row row;
-
- const int32_t ptr_size = 8;
- row.GetCFAValue().SetIsRegisterPlusOffset(dwarf_rbp, 2 * ptr_size);
row.SetOffset(0);
row.SetUnspecifiedRegistersAreUndefined(true);
-
- row.SetRegisterLocationToAtCFAPlusOffset(fp_reg_num, ptr_size * -2, true);
- row.SetRegisterLocationToAtCFAPlusOffset(pc_reg_num, ptr_size * -1, true);
+ row.GetCFAValue().SetIsRegisterPlusOffset(sp_reg_num, 8);
+ row.SetRegisterLocationToAtCFAPlusOffset(pc_reg_num, -8, false);
row.SetRegisterLocationToIsCFAPlusOffset(sp_reg_num, 0, true);
auto plan_sp = std::make_shared<UnwindPlan>(eRegisterKindDWARF);
diff --git a/lldb/test/API/lang/c/trampoline_stepping/Makefile b/lldb/test/API/lang/c/trampoline_stepping/Makefile
new file mode 100644
index 0000000000000..10495940055b6
--- /dev/null
+++ b/lldb/test/API/lang/c/trampoline_stepping/Makefile
@@ -0,0 +1,3 @@
+C_SOURCES := main.c
+
+include Makefile.rules
diff --git a/lldb/test/API/lang/c/trampoline_stepping/TestTrampolineStepping.py b/lldb/test/API/lang/c/trampoline_stepping/TestTrampolineStepping.py
new file mode 100644
index 0000000000000..bbf1dc8cb7fea
--- /dev/null
+++ b/lldb/test/API/lang/c/trampoline_stepping/TestTrampolineStepping.py
@@ -0,0 +1,100 @@
+"""Test that stepping in/out of trampolines works as expected."""
+
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+
+class TestTrampoline(TestBase):
+ def setup(self, bkpt_str):
+ self.build()
+
+ _, _, thread, _ = lldbutil.run_to_source_breakpoint(
+ self, bkpt_str, lldb.SBFileSpec("main.c")
+ )
+ return thread
+
+ def test_direct_call(self):
+ thread = self.setup("Break here for direct")
+
+ # Sanity check that we start out in the correct function.
+ name = thread.frames[0].GetFunctionName()
+ self.assertIn("direct_trampoline_call", name)
+
+ # Check that stepping in will take us directly to the trampoline target.
+ thread.StepInto()
+ name = thread.frames[0].GetFunctionName()
+ self.assertIn("foo", name)
+
+ # Check that stepping out takes us back to the trampoline caller.
+ thread.StepOut()
+ name = thread.frames[0].GetFunctionName()
+ self.assertIn("direct_trampoline_call", name)
+
+ # Check that stepping over the end of the trampoline target
+ # takes us back to the trampoline caller.
+ thread.StepInto()
+ thread.StepOver()
+ name = thread.frames[0].GetFunctionName()
+ self.assertIn("direct_trampoline_call", name)
+
+ def test_chained_call(self):
+ thread = self.setup("Break here for chained")
+
+ # Sanity check that we start out in the correct function.
+ name = thread.frames[0].GetFunctionName()
+ self.assertIn("chained_trampoline_call", name)
+
+ # Check that stepping in will take us directly to the trampoline target.
+ thread.StepInto()
+ name = thread.frames[0].GetFunctionName()
+ self.assertIn("foo", name)
+
+ # Check that stepping out takes us back to the trampoline caller.
+ thread.StepOut()
+ name = thread.frames[0].GetFunctionName()
+ self.assertIn("chained_trampoline_call", name)
+
+ # Check that stepping over the end of the trampoline target
+ # takes us back to the trampoline caller.
+ thread.StepInto()
+ thread.StepOver()
+ name = thread.frames[0].GetFunctionName()
+ self.assertIn("chained_trampoline_call", name)
+
+ def test_trampoline_after_nodebug(self):
+ thread = self.setup("Break here for nodebug then trampoline")
+
+ # Sanity check that we start out in the correct function.
+ name = thread.frames[0].GetFunctionName()
+ self.assertIn("trampoline_after_nodebug", name)
+
+ # Check that stepping in will take us directly to the trampoline target.
+ thread.StepInto()
+ name = thread.frames[0].GetFunctionName()
+ self.assertIn("foo", name)
+
+ # Check that stepping out takes us back to the trampoline caller.
+ thread.StepOut()
+ name = thread.frames[0].GetFunctionName()
+ self.assertIn("trampoline_after_nodebug", name)
+
+ # Check that stepping over the end of the trampoline target
+ # takes us back to the trampoline caller.
+ thread.StepInto()
+ thread.StepOver()
+ name = thread.frames[0].GetFunctionName()
+ self.assertIn("trampoline_after_nodebug", name)
+
+ def test_unused_target(self):
+ thread = self.setup("Break here for unused")
+
+ # Sanity check that we start out in the correct function.
+ name = thread.frames[0].GetFunctionName()
+ self.assertIn("unused_target", name)
+
+ # Check that stepping into a trampoline that doesn't call its target
+ # jumps back to its caller.
+ thread.StepInto()
+ name = thread.frames[0].GetFunctionName()
+ self.assertIn("unused_target", name)
diff --git a/lldb/test/API/lang/c/trampoline_stepping/main.c b/lldb/test/API/lang/c/trampoline_stepping/main.c
new file mode 100644
index 0000000000000..cb98be00ca1f6
--- /dev/null
+++ b/lldb/test/API/lang/c/trampoline_stepping/main.c
@@ -0,0 +1,52 @@
+void foo(void) {}
+
+__attribute__((transparent_stepping))
+void bar(void) {
+ foo();
+}
+
+__attribute__((transparent_stepping))
+void baz(void) {
+ bar();
+}
+
+__attribute__((nodebug))
+void nodebug(void) {}
+
+__attribute__((transparent_stepping))
+void nodebug_then_trampoline(void) {
+ nodebug();
+ baz();
+}
+
+__attribute__((transparent_stepping))
+void doesnt_call_trampoline(void) {}
+
+void direct_trampoline_call(void) {
+ bar(); // Break here for direct
+ bar();
+}
+
+void chained_trampoline_call(void) {
+ baz(); // Break here for chained
+ baz();
+}
+
+void trampoline_after_nodebug(void) {
+ nodebug_then_trampoline(); // Break here for nodebug then trampoline
+ nodebug_then_trampoline();
+}
+
+void unused_target(void) {
+ doesnt_call_trampoline(); // Break here for unused
+}
+
+
+int main(void) {
+ direct_trampoline_call();
+ chained_trampoline_call();
+ trampoline_after_nodebug();
+ unused_target();
+ return 0;
+}
+
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
|
To make sure I understand:
According to this, you are saying that CFA is one row above where the dashed line "call B" is on this picture? 
|
|
Would it also have worked to say that |
|
|
||
| UnwindPlan::Row row; | ||
|
|
||
| const int32_t ptr_size = 8; |
There was a problem hiding this comment.
any particular reason to remove the ptr_size variable?
There was a problem hiding this comment.
Not really, I copied the implementation from
llvm-project/lldb/source/Plugins/ABI/X86/ABISysV_x86_64.cpp
Lines 859 to 862 in a9e5bb5
I'm happy to add it back.
| row.SetRegisterLocationToAtCFAPlusOffset(pc_reg_num, ptr_size * -1, true); | ||
| row.GetCFAValue().SetIsRegisterPlusOffset(sp_reg_num, 8); | ||
| row.SetRegisterLocationToAtCFAPlusOffset(pc_reg_num, -8, false); | ||
| row.SetRegisterLocationToIsCFAPlusOffset(sp_reg_num, 0, true); |
There was a problem hiding this comment.
this line is also interesting in that I can't find rsp on the picture I linked
There was a problem hiding this comment.
Is it even right to say that rsp itself is spilled on the stack at row 0?
There was a problem hiding this comment.
@jasonmolenda maybe you can help us interpret that picture and answer this question
There was a problem hiding this comment.
I copied the implementation from
llvm-project/lldb/source/Plugins/ABI/X86/ABISysV_x86_64.cpp
Lines 859 to 862 in a9e5bb5
As I understand, on Windows CFA = rsp+8 and pc = [CFA-8] given the following:
On most processors, the CFA is simply "the stack pointer value when this function was called." Local variables are at negative offsets from the CFA (below it on the stack), and the return address is at a fixed position relative to the CFA.
2.https://learn.microsoft.com/en-us/cpp/build/prolog-and-epilog?view=msvc-170#prolog-code
The code for a typical prolog might be:
mov [RSP + 8], RCX
Is it even right to say that rsp itself is spilled on the stack at row 0?
This is using the Is variant of the method, not the At variant. As I understand it, the register is not spilled on the stack (rightfully so). And that's why we don't see it in the diagram.
|
I don't see how this can be correct, but I have zero experience with Windows. rsp is especially inflexible on x86 because the CALL/RET instructions adjust it and push/pop the return address, so I'm not sure how this could be possible. If I have local variables in a function that require stack storage, RSP is decremented to make space for them, isn't it? So when I do a CALL, the return address is saved to stack and RSP decremented again. If RSP was not decremented past the local variables, when I do a CALL, the locals would be overwritten. e.g. The correct unwind state of this program at every instruction point is On function entry, the CFA is But as soon as we spill the caller's rbp, now rsp has decremented and our CFA rule changes. Then we copy rsp into rbp (to establish the frame base). Then we subtract 16 from rsp again to make room for the local variables. It's possible for clang to do "omit frame pointer" codegen when the stack frame size is fixed, of course, e.g. this is what clang does here clang pushed a scratch register to stack (rcx) as a cheaper way to decrement $sp, then it puts the two variables on the stack where it had rcx. And in the epilogue it pointlessly restores rcx from stack. But you can see in this function, once we do the |
|
On targets that use "omit-frame-pointer" codegen, there is no valid default unwind plan, really. We need function-specific knowledge to know how much rsp was decremented during the function, to do any unwinding at all. |
|
(even when omit-frame-pointer codegen is being done, you can force a frame pointer register to be used if you use a non-fixed stack frame size. On unix alloca() will do this, or with modern C, a variable length array VLA. e.g. |
|
FTR I wouldn't be shocked if Windows codegen is omit-frame-pointer by default. On 32-bit i386 it was a real performance boost to get one more general purpose register (rbp). On 64-bit x86_64, it's a much smaller performance boost but they were probably in the habit of that codegen style already, and stuck with it. On targets like AArch64 or RISC-V, the register file is quite large and there's no benefit to omit-frame-pointer. |
Yes because we are reading from the same stack slot:
|
Yes, I matched what we do here: llvm-project/lldb/source/Plugins/ABI/X86/ABISysV_x86_64.cpp Lines 859 to 862 in a9e5bb5 This effectively makes Windows equal to the AtFunctionEntry unwindplan. I could not find any source that Windows is
My understanding is that |
3 participants
ABIWindows_x86_64::CreateDefaultUnwindPlan()uses a "CFA = rbp + 16" as a placeholder, however,Windows-x86_64does not use rbp as a frame pointer. With this bogus rbp based CFA, the unwinder could not produce the caller frame, so stepping out of such a function fails with "Could not create return address breakpoint".This patch uses the
Windows-x86_64correct rule for the fallback: the return address is at the top of the stack, soCFA = rsp + 8andpc = [CFA - 8].This patch also ports
lang/c/trampoline_steppingfrom Swiftlang.