[Lldb-commits] [lldb] Reapply "[lldb/aarch64] Fix unwinding when signal interrupts a leaf f… (PR #92503)
https://github.com/labath created https://github.com/llvm/llvm-project/pull/92503 …unction (#91321)" This reapplies fd1bd53ba5a06f344698a55578f6a5d79c457e30, which was reverted due to a test failure on aarch64/windows. The failure was caused by a combination of several factors: - clang targeting aarch64-windows (unlike msvc, and unlike clang targeting other aarch64 platforms) defaults to -fomit-frame-pointers - lldb's code for looking up register values for `` unwind rules is recursive - the test binary creates a very long chain of fp-less function frames (it manages to fit about 22k frames before it blows its stack) Together, these things have caused lldb to recreate the same deep recursion when unwinding through this, and blow its own stack as well. Since lldb frames are larger, about 4k frames like this was sufficient to trigger the stack overflow. This version of the patch works around this problem by increasing the frame size of the test binary, thereby causing it to blow its stack sooner. This doesn't fix the issue -- the same problem can occur with a real binary -- but it's not very likely, as it requires an infinite recursion in a simple (so it doesn't use the frame pointer) function with a very small frame (so you can fit a lot of them on the stack). A more principled fix would be to make lldb's lookup code non-recursive, but I believe that's out of scope for this patch. The original patch description follows: A leaf function may not store the link register to stack, but we it can still end up being a non-zero frame if it gets interrupted by a signal. Currently, we were unable to unwind past this function because we could not read the link register value. To make this work, this patch: - changes the function-entry unwind plan to include the `fp|lr = ` rules. This in turn necessitated an adjustment in the generic instruction emulation logic to ensure that `lr=[sp-X]` can override the `` rule. - allows the `` rule for pc and lr in all `m_all_registers_available` frames (and not just frame zero). The test verifies that we can unwind in a situation like this, and that the backtrace matches the one we computed before getting a signal. >From 47858edddfbccf989213d61f3761fcdb04814839 Mon Sep 17 00:00:00 2001 From: Pavel Labath Date: Fri, 17 May 2024 06:34:08 + Subject: [PATCH] Reapply "[lldb/aarch64] Fix unwinding when signal interrupts a leaf function (#91321)" This reapplies fd1bd53ba5a06f344698a55578f6a5d79c457e30, which was reverted due to a test failure on aarch64/windows. The failure was caused by a combination of several factors: - clang targeting aarch64-windows (unlike msvc, and unlike clang targeting other aarch64 platforms) defaults to -fomit-frame-pointers - lldb's code for looking up register values for `` unwind rules is recursive - the test binary creates a very long chain of fp-less function frames (it manages to fit about 22k frames before it blows its stack) Together, these things have caused lldb to recreate the same deep recursion when unwinding through this, and blow its own stack as well. Since lldb frames are larger, about 4k frames like this was sufficient to trigger the stack overflow. This version of the patch works around this problem by increasing the frame size of the test binary, thereby causing it to blow its stack sooner. This doesn't fix the issue -- the same problem can occur with a real binary -- but it's not very likely, as it requires an infinite recursion in a simple (so it doesn't use the frame pointer) function with a very small frame (so you can fit a lot of them on the stack). A more principled fix would be to make lldb's lookup code non-recursive, but I believe that's out of scope for this patch. The original patch description follows: A leaf function may not store the link register to stack, but we it can still end up being a non-zero frame if it gets interrupted by a signal. Currently, we were unable to unwind past this function because we could not read the link register value. To make this work, this patch: - changes the function-entry unwind plan to include the `fp|lr = ` rules. This in turn necessitated an adjustment in the generic instruction emulation logic to ensure that `lr=[sp-X]` can override the `` rule. - allows the `` rule for pc and lr in all `m_all_registers_available` frames (and not just frame zero). The test verifies that we can unwind in a situation like this, and that the backtrace matches the one we computed before getting a signal. --- .../ARM64/EmulateInstructionARM64.cpp | 2 ++ .../UnwindAssemblyInstEmulation.cpp | 4 +-- lldb/source/Target/RegisterContextUnwind.cpp | 6 ++--- .../API/functionalities/bt-interrupt/main.c | 1 + .../Inputs/signal-in-leaf-function-aarch64.c | 15 +++ .../signal-in-leaf-function-aarch64.test | 27 +++ .../ARM64/TestArm64InstEmulation.cpp | 24 ++--- 7 files changed, 69 insertions
[Lldb-commits] [lldb] Reapply "[lldb/aarch64] Fix unwinding when signal interrupts a leaf f… (PR #92503)
llvmbot wrote: @llvm/pr-subscribers-lldb Author: Pavel Labath (labath) Changes …unction (#91321)" This reapplies fd1bd53ba5a06f344698a55578f6a5d79c457e30, which was reverted due to a test failure on aarch64/windows. The failure was caused by a combination of several factors: - clang targeting aarch64-windows (unlike msvc, and unlike clang targeting other aarch64 platforms) defaults to -fomit-frame-pointers - lldb's code for looking up register values for `` unwind rules is recursive - the test binary creates a very long chain of fp-less function frames (it manages to fit about 22k frames before it blows its stack) Together, these things have caused lldb to recreate the same deep recursion when unwinding through this, and blow its own stack as well. Since lldb frames are larger, about 4k frames like this was sufficient to trigger the stack overflow. This version of the patch works around this problem by increasing the frame size of the test binary, thereby causing it to blow its stack sooner. This doesn't fix the issue -- the same problem can occur with a real binary -- but it's not very likely, as it requires an infinite recursion in a simple (so it doesn't use the frame pointer) function with a very small frame (so you can fit a lot of them on the stack). A more principled fix would be to make lldb's lookup code non-recursive, but I believe that's out of scope for this patch. The original patch description follows: A leaf function may not store the link register to stack, but we it can still end up being a non-zero frame if it gets interrupted by a signal. Currently, we were unable to unwind past this function because we could not read the link register value. To make this work, this patch: - changes the function-entry unwind plan to include the `fp|lr = ` rules. This in turn necessitated an adjustment in the generic instruction emulation logic to ensure that `lr=[sp-X]` can override the ` ` rule. - allows the ` ` rule for pc and lr in all `m_all_registers_available` frames (and not just frame zero). The test verifies that we can unwind in a situation like this, and that the backtrace matches the one we computed before getting a signal. --- Full diff: https://github.com/llvm/llvm-project/pull/92503.diff 7 Files Affected: - (modified) lldb/source/Plugins/Instruction/ARM64/EmulateInstructionARM64.cpp (+2) - (modified) lldb/source/Plugins/UnwindAssembly/InstEmulation/UnwindAssemblyInstEmulation.cpp (+1-3) - (modified) lldb/source/Target/RegisterContextUnwind.cpp (+3-3) - (modified) lldb/test/API/functionalities/bt-interrupt/main.c (+1) - (added) lldb/test/Shell/Unwind/Inputs/signal-in-leaf-function-aarch64.c (+15) - (added) lldb/test/Shell/Unwind/signal-in-leaf-function-aarch64.test (+27) - (modified) lldb/unittests/UnwindAssembly/ARM64/TestArm64InstEmulation.cpp (+20-4) ``diff diff --git a/lldb/source/Plugins/Instruction/ARM64/EmulateInstructionARM64.cpp b/lldb/source/Plugins/Instruction/ARM64/EmulateInstructionARM64.cpp index 6ca4fb052457e..62ecac3e0831d 100644 --- a/lldb/source/Plugins/Instruction/ARM64/EmulateInstructionARM64.cpp +++ b/lldb/source/Plugins/Instruction/ARM64/EmulateInstructionARM64.cpp @@ -444,6 +444,8 @@ bool EmulateInstructionARM64::CreateFunctionEntryUnwind( // Our previous Call Frame Address is the stack pointer row->GetCFAValue().SetIsRegisterPlusOffset(gpr_sp_arm64, 0); + row->SetRegisterLocationToSame(gpr_lr_arm64, /*must_replace=*/false); + row->SetRegisterLocationToSame(gpr_fp_arm64, /*must_replace=*/false); unwind_plan.AppendRow(row); unwind_plan.SetSourceName("EmulateInstructionARM64"); diff --git a/lldb/source/Plugins/UnwindAssembly/InstEmulation/UnwindAssemblyInstEmulation.cpp b/lldb/source/Plugins/UnwindAssembly/InstEmulation/UnwindAssemblyInstEmulation.cpp index c4a171ec7d01b..49edd40544e32 100644 --- a/lldb/source/Plugins/UnwindAssembly/InstEmulation/UnwindAssemblyInstEmulation.cpp +++ b/lldb/source/Plugins/UnwindAssembly/InstEmulation/UnwindAssemblyInstEmulation.cpp @@ -424,8 +424,6 @@ size_t UnwindAssemblyInstEmulation::WriteMemory( log->PutString(strm.GetString()); } - const bool cant_replace = false; - switch (context.type) { default: case EmulateInstruction::eContextInvalid: @@ -467,7 +465,7 @@ size_t UnwindAssemblyInstEmulation::WriteMemory( m_pushed_regs[reg_num] = addr; const int32_t offset = addr - m_initial_sp; m_curr_row->SetRegisterLocationToAtCFAPlusOffset(reg_num, offset, - cant_replace); + /*can_replace=*/true); m_curr_row_modified = true; } } diff --git a/lldb/source/Target/RegisterContextUnwind.cpp b/lldb/source/Target/RegisterContextUnwind.cpp index 13e101413a477..e2d712cb72eae 100644 --- a/lldb/source/Target/RegisterContextUnwind.cpp +++ b/lldb/source/Target/RegisterCont
[Lldb-commits] [lldb] Reapply "[lldb/aarch64] Fix unwinding when signal interrupts a leaf f… (PR #92503)
jasonmolenda wrote: Yes, originally lldb's unwinder was recursive for any register propagation and it was easy to hit the problem of lldb blowing out its own stack on a recursive inferior that had crashed. I changed most of the propagation to a loop to solve this (years and years ago) but it looks like we still have a case where it is recursing. We still need to skip the test case on macOS until I can come up with some idea to get proper unwind instruction for sigtramp on arm64. Most of the CI bots are x86_64 so it may pass on them, but that's The Past and I would prefer to skip this on Darwin until I can figure something out, I wrote a little TODO on myself in rdar://128031075 This looks good to me. https://github.com/llvm/llvm-project/pull/92503 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] Reapply "[lldb/aarch64] Fix unwinding when signal interrupts a leaf f… (PR #92503)
https://github.com/jasonmolenda approved this pull request. LGTM https://github.com/llvm/llvm-project/pull/92503 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] Reapply "[lldb/aarch64] Fix unwinding when signal interrupts a leaf f… (PR #92503)
labath wrote: > Yes, originally lldb's unwinder was recursive for any register propagation > and it was easy to hit the problem of lldb blowing out its own stack on a > recursive inferior that had crashed. I changed most of the propagation to a > loop to solve this (years and years ago) but it looks like we still have a > case where it is recursing. > > We still need to skip the test case on macOS until I can come up with some > idea to get proper unwind instruction for sigtramp on arm64. Most of the CI > bots are x86_64 so it may pass on them, but that's The Past and I would > prefer to skip this on Darwin until I can figure something out, I wrote a > little TODO on myself in rdar://128031075 Sounds good (I meant to xfail this on darwin, but it looks like I forgot). https://github.com/llvm/llvm-project/pull/92503 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] Reapply "[lldb/aarch64] Fix unwinding when signal interrupts a leaf f… (PR #92503)
https://github.com/labath updated
https://github.com/llvm/llvm-project/pull/92503
>From 47858edddfbccf989213d61f3761fcdb04814839 Mon Sep 17 00:00:00 2001
From: Pavel Labath
Date: Fri, 17 May 2024 06:34:08 +
Subject: [PATCH 1/2] Reapply "[lldb/aarch64] Fix unwinding when signal
interrupts a leaf function (#91321)"
This reapplies fd1bd53ba5a06f344698a55578f6a5d79c457e30, which was
reverted due to a test failure on aarch64/windows. The failure was
caused by a combination of several factors:
- clang targeting aarch64-windows (unlike msvc, and unlike clang
targeting other aarch64 platforms) defaults to -fomit-frame-pointers
- lldb's code for looking up register values for `` unwind rules
is recursive
- the test binary creates a very long chain of fp-less function frames
(it manages to fit about 22k frames before it blows its stack)
Together, these things have caused lldb to recreate the same deep
recursion when unwinding through this, and blow its own stack as well.
Since lldb frames are larger, about 4k frames like this was sufficient
to trigger the stack overflow.
This version of the patch works around this problem by increasing the
frame size of the test binary, thereby causing it to blow its stack
sooner. This doesn't fix the issue -- the same problem can occur with a
real binary -- but it's not very likely, as it requires an infinite
recursion in a simple (so it doesn't use the frame pointer) function
with a very small frame (so you can fit a lot of them on the stack).
A more principled fix would be to make lldb's lookup code non-recursive,
but I believe that's out of scope for this patch.
The original patch description follows:
A leaf function may not store the link register to stack, but we it can
still end up being a non-zero frame if it gets interrupted by a signal.
Currently, we were unable to unwind past this function because we could
not read the link register value.
To make this work, this patch:
- changes the function-entry unwind plan to include the `fp|lr = `
rules. This in turn necessitated an adjustment in the generic
instruction emulation logic to ensure that `lr=[sp-X]` can override the
`` rule.
- allows the `` rule for pc and lr in all
`m_all_registers_available` frames (and not just frame zero).
The test verifies that we can unwind in a situation like this, and that
the backtrace matches the one we computed before getting a signal.
---
.../ARM64/EmulateInstructionARM64.cpp | 2 ++
.../UnwindAssemblyInstEmulation.cpp | 4 +--
lldb/source/Target/RegisterContextUnwind.cpp | 6 ++---
.../API/functionalities/bt-interrupt/main.c | 1 +
.../Inputs/signal-in-leaf-function-aarch64.c | 15 +++
.../signal-in-leaf-function-aarch64.test | 27 +++
.../ARM64/TestArm64InstEmulation.cpp | 24 ++---
7 files changed, 69 insertions(+), 10 deletions(-)
create mode 100644
lldb/test/Shell/Unwind/Inputs/signal-in-leaf-function-aarch64.c
create mode 100644 lldb/test/Shell/Unwind/signal-in-leaf-function-aarch64.test
diff --git a/lldb/source/Plugins/Instruction/ARM64/EmulateInstructionARM64.cpp
b/lldb/source/Plugins/Instruction/ARM64/EmulateInstructionARM64.cpp
index 6ca4fb052457e..62ecac3e0831d 100644
--- a/lldb/source/Plugins/Instruction/ARM64/EmulateInstructionARM64.cpp
+++ b/lldb/source/Plugins/Instruction/ARM64/EmulateInstructionARM64.cpp
@@ -444,6 +444,8 @@ bool EmulateInstructionARM64::CreateFunctionEntryUnwind(
// Our previous Call Frame Address is the stack pointer
row->GetCFAValue().SetIsRegisterPlusOffset(gpr_sp_arm64, 0);
+ row->SetRegisterLocationToSame(gpr_lr_arm64, /*must_replace=*/false);
+ row->SetRegisterLocationToSame(gpr_fp_arm64, /*must_replace=*/false);
unwind_plan.AppendRow(row);
unwind_plan.SetSourceName("EmulateInstructionARM64");
diff --git
a/lldb/source/Plugins/UnwindAssembly/InstEmulation/UnwindAssemblyInstEmulation.cpp
b/lldb/source/Plugins/UnwindAssembly/InstEmulation/UnwindAssemblyInstEmulation.cpp
index c4a171ec7d01b..49edd40544e32 100644
---
a/lldb/source/Plugins/UnwindAssembly/InstEmulation/UnwindAssemblyInstEmulation.cpp
+++
b/lldb/source/Plugins/UnwindAssembly/InstEmulation/UnwindAssemblyInstEmulation.cpp
@@ -424,8 +424,6 @@ size_t UnwindAssemblyInstEmulation::WriteMemory(
log->PutString(strm.GetString());
}
- const bool cant_replace = false;
-
switch (context.type) {
default:
case EmulateInstruction::eContextInvalid:
@@ -467,7 +465,7 @@ size_t UnwindAssemblyInstEmulation::WriteMemory(
m_pushed_regs[reg_num] = addr;
const int32_t offset = addr - m_initial_sp;
m_curr_row->SetRegisterLocationToAtCFAPlusOffset(reg_num, offset,
- cant_replace);
+ /*can_replace=*/true);
m_curr_row_modified = true;
}
}
diff --git a/lldb/source/Target/RegisterContextUnwind.cpp
b/lldb/source/Target/Reg
[Lldb-commits] [lldb] Reapply "[lldb/aarch64] Fix unwinding when signal interrupts a leaf f… (PR #92503)
https://github.com/labath closed https://github.com/llvm/llvm-project/pull/92503 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] Reapply "[lldb/aarch64] Fix unwinding when signal interrupts a leaf f… (PR #92503)
jasonmolenda wrote: btw @labath I was thinking about "sigtramp routines" and fault / trap / interrupt handlers in general, and how lldb has this list of function names that it treats as fault handlers (`target.trap-handler-names`). And in the unwinder we have the same idea of "the frame above a designed sigtramp/fault/trap/interrupt function can retrieve all registers". But what if the unwinder had a method saying "Do you have an unwind rule to give me a value for register n, without iterating down the stack?" e.g. frame 5 is interrupted and frame 4 is sigtramp, with full eh_frame. From frame 5, I can say "does frame 4 have an unwind rule to provide x0, without iterating down to frame 3, etc." This also means if we have a sigtramp which is missing its eh_frame, we won't apply our "all registers available" rules to the frame above. An interesting case for a return-address target like aarch64 where normally when we ask for a caller frame's pc, we fetch the link register. But above a fault/trap/interrupt frame, we can retrieve both the pc and the link register and they are different values. Just something I started kicking around in my head, I don't have concrete plans to implement an overhaul like this but the more I think about it, the more I like it. https://github.com/llvm/llvm-project/pull/92503 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] Reapply "[lldb/aarch64] Fix unwinding when signal interrupts a leaf f… (PR #92503)
jasonmolenda wrote: We should be able to work correctly with a trap handler that has full eh_frame without knowing the function name. And we shouldn't treat a sigtramp missing eh_frame as having all registers. https://github.com/llvm/llvm-project/pull/92503 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] Reapply "[lldb/aarch64] Fix unwinding when signal interrupts a leaf f… (PR #92503)
jasonmolenda wrote: (with the caveat that a register location of IsSame for a volatile aka non-callee-spilled register would be treated as "did not have a location") https://github.com/llvm/llvm-project/pull/92503 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] Reapply "[lldb/aarch64] Fix unwinding when signal interrupts a leaf f… (PR #92503)
jasonmolenda wrote: as cool as this idea is, I do worry that it will make the code less readable, where instead of saying "BehavesLikeFrameZero / HasAllRegisters", we now need to ask "can the frame below supply register x", I don't know. it's just something I have running around my head today. https://github.com/llvm/llvm-project/pull/92503 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] Reapply "[lldb/aarch64] Fix unwinding when signal interrupts a leaf f… (PR #92503)
labath wrote: I'm not quite sure what you have in mind, but I can tell you what's been going through my mind in the context of the `m_all_registers_available` check in `lldb/source/Target/RegisterContextUnwind.cpp` . The way I see it, this check (at least the part about the RA register(*)) is heuristic that's impossible to get always right. Like, I could construct a test case using functions with non-standard ABIs where a non-leaf function legitimately has a `lr=` rule. Such code would execute correctly but lldb would refuse to unwind it due to the `lr=` restriction. The only thing needed to construct such a test case is one (possibly leaf) function, which "returns" to a register other than `lr` (it could even return to a memory address). Then, its caller could "call" that function by storing the return address to some other place, and leaving it's own return address register (i.e, `lr`) untouched. (I don't know why anyone would do such a thing, since it would likely mess up the CPUs branch predictor, but dwarf is perfectly capable of expressing code like this) Another interesting case is that of a function (an abi-respecting function this time), which chooses to save the `lr` to a different (non-volatile) register, instead of the usual stack location. This function could then call other functions as usual, but we wouldn't be able to unwind from it in a single step -- to get its value of `lr` (i.e., the `pc` of the frame *above* it), we would need to find where has the frame *below* stored the register that `lr` was saved to. (I also don't know of anyone writing code like this, but unlike the previous case, I can imagine some very specific situations where such an optimization might be profitable.) All of this is to say that I don't think there is a way to change this piece of code to be correct all the time -- we'd just be trading one set of edge cases for the other. I think that the most correct solution would be to remove this check altogether. I'm not sure why it exists, but I expect it has something to do with preventing looping stacks. However, if I remember correctly, we already have some checks to prevent stack loops (if not, then we should have, as there are other ways to create stack loops), so I think it should be possible to let the `lr=` (*) rule through here and catch erroneous cases further down the road. However, I also don't have any plans to pursue this direction. (*) I'm only talking about the `lr` rule everywhere. I *think* that a `pc=` rule would always be an error (even in signal handlers), so we should be able to keep that here. OTOH, if our loop detection code is robust enough, then there should be no harm in letting this through either... https://github.com/llvm/llvm-project/pull/92503 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] Reapply "[lldb/aarch64] Fix unwinding when signal interrupts a leaf f… (PR #92503)
jasonmolenda wrote: > The way I see it, this check (at least the part about the RA register(*)) is > heuristic that's impossible to get always right. Like, I could construct a > test case using functions with non-standard ABIs where a non-leaf function > legitimately has a `lr=` rule. Such code would execute correctly but > lldb would refuse to unwind it due to the `lr=` restriction. This would be an interesting idea. I don't think there's any unwind format which allows you to specify that a different register holds the return address, and lr is IsSame. You could say that lr=x9 to say that the return address is in x9, but you can't express that the return address is stored in a non-lr register. You could add an unwind rule for pc=x9 to say that the return address is in x9, and depend on the unwinder to not look for lr, but to try retrieving pc first. > > All of this is to say that I don't think there is a way to change this piece > of code to be correct all the time -- we'd just be trading one set of edge > cases for the other. I think that the most correct solution would be to > remove this check altogether. I'm not sure why it exists, but I expect it has > something to do with preventing looping stacks. The original goal was that if we're on frame 1, we don't want to surface a register value from frame 0 and use it in frame 1 unless it's a callee-spilled register. e.g. x0 on frame 1 may have been overwritten while frame 0 was executing, there is no unwind rule for x0 and the unwinder can't show frame 0's x0 value in frame 1. But if we're above a sigtramp etc frame which has the entire register context from when a function was interrupted, we can retrieve all the registers and want to show them. > (*) I'm only talking about the `lr` rule everywhere. I _think_ that a > `pc=` rule would always be an error (even in signal handlers), so we > should be able to keep that here. OTOH, if our loop detection code is robust > enough, then there should be no harm in letting this through either... Yeah I think there's improvements that can be made here for sure. https://github.com/llvm/llvm-project/pull/92503 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
