[lldb][NFC] Split RegisterContextUnwind::SavedLocationForRegister #139817
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
RegisterContextUnwind::SavedLocationForRegister is around 450 lines that first find an abstract register location (e.g. "CFA-8") for a register by looking in the UnwindPlans. Then it evaluates the abstract register location to create a concrete register location (e.g. "stored at address 0x...", "live in register at frame 0"). There are some complicated cases in the first half of the method to handle return address register architectures correctly, in particular. Looking at the two halves, they're both exactly 226 lines long and there's little involvement between them except for passing an abstract register location along. (there were some parts in the "abstract register location" code that would set the concrete register location, unnecessarily) It's also a complex enough method that there are some bits of code that aren't actually doing anything at this point. This patch adds a RegisterContextUnwind::GetAbstractRegisterLocation method, which does the first half, and has a clearly defined return values. The code to convert an AbstractRegisterLocation into a ConcreteRegisterLocation remains in SavedLocationForRegister. It's a bit of a tricky patch to visually inspect, despite it not changing functionality, the reorganizations and rewrites make the diff unreadable. Nearly all the real changes are in the "find the abstract register location" first half of the method. I think reading the new code in its new form is the easiest way to inspect this PR. With a defined interface between the two of what is expected, it's pretty easy to look at the code and reason about whether it is written correctly. (whereas before, that was very difficult, for me at least.)
Member
|
@llvm/pr-subscribers-lldb Author: Jason Molenda (jasonmolenda) ChangesRegisterContextUnwind::SavedLocationForRegister is around 450 lines that first find an abstract register location (e.g. "CFA-8") for a register by looking in the UnwindPlans. Then it evaluates the abstract register location to create a concrete register location (e.g. "stored at address 0x...", "live in register at frame 0"). There are some complicated cases in the first half of the method to handle return address register architectures correctly, in particular. Looking at the two halves, they're both exactly 226 lines long and there's little involvement between them except for passing an abstract register location along. (there were some parts in the "abstract register location" code that would set the concrete register location, unnecessarily) It's also a complex enough method that there are some bits of code that aren't actually doing anything at this point. This patch adds a RegisterContextUnwind::GetAbstractRegisterLocation method, which does the first half, and has a clearly defined return values. The code to convert an AbstractRegisterLocation into a ConcreteRegisterLocation remains in SavedLocationForRegister. It's a bit of a tricky patch to visually inspect, despite it not changing functionality, the reorganizations and rewrites make the diff unreadable. Nearly all the real changes are in the "find the abstract register location" first half of the method. I think reading the new code in its new form is the easiest way to inspect this PR. With a defined interface between the two of what is expected, it's pretty easy to look at the code and reason about whether it is written correctly. (whereas before, that was very difficult, for me at least.) Patch is 34.22 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/139817.diff 3 Files Affected:
diff --git a/lldb/include/lldb/Target/RegisterContextUnwind.h b/lldb/include/lldb/Target/RegisterContextUnwind.h
index 044a387fe5aa2..b10a364823b83 100644
--- a/lldb/include/lldb/Target/RegisterContextUnwind.h
+++ b/lldb/include/lldb/Target/RegisterContextUnwind.h
@@ -151,6 +151,9 @@ class RegisterContextUnwind : public lldb_private::RegisterContext {
uint32_t lldb_regnum,
lldb_private::UnwindLLDB::ConcreteRegisterLocation ®loc);
+ std::optional<UnwindPlan::Row::AbstractRegisterLocation>
+ GetAbstractRegisterLocation(uint32_t lldb_regnum, lldb::RegisterKind &kind);
+
bool ReadRegisterValueFromRegisterLocation(
lldb_private::UnwindLLDB::ConcreteRegisterLocation regloc,
const lldb_private::RegisterInfo *reg_info,
diff --git a/lldb/source/Target/RegisterContextUnwind.cpp b/lldb/source/Target/RegisterContextUnwind.cpp
index cf4b96c6eda9f..a3931abefb054 100644
--- a/lldb/source/Target/RegisterContextUnwind.cpp
+++ b/lldb/source/Target/RegisterContextUnwind.cpp
@@ -1243,247 +1243,194 @@ bool RegisterContextUnwind::IsTrapHandlerSymbol(
return false;
}
-// Answer the question: Where did THIS frame save the CALLER frame ("previous"
-// frame)'s register value?
-
-enum UnwindLLDB::RegisterSearchResult
-RegisterContextUnwind::SavedLocationForRegister(
- uint32_t lldb_regnum,
- lldb_private::UnwindLLDB::ConcreteRegisterLocation ®loc) {
+// Search this stack frame's UnwindPlans for the AbstractRegisterLocation
+// for this register.
+//
+// When an AbstractRegisterLocation is found in an UnwindPlan, that is
+// returned, regardless of the ABI rules for volatile/non-volatile registers
+// in effect.
+//
+// If there is no unwind rule for a volatile (caller-preserved) register
+// the returned AbstractRegisterLocation will be IsUndefined,
+// indicating that we should stop searching.
+//
+// If there is no unwind rule for a non-volatile (callee-preserved)
+// register, the returned AbstractRegisterLocation will be IsSame.
+// In frame 0, IsSame means get the value from the live register context.
+// Else it means to continue descending down the stack to more-live frames
+// looking for a location/value.
+//
+// An empty optional indicates that there was an error in processing.
+std::optional<UnwindPlan::Row::AbstractRegisterLocation>
+RegisterContextUnwind::GetAbstractRegisterLocation(uint32_t lldb_regnum,
+ lldb::RegisterKind &kind) {
RegisterNumber regnum(m_thread, eRegisterKindLLDB, lldb_regnum);
Log *log = GetLog(LLDBLog::Unwind);
- // Have we already found this register location?
- if (!m_registers.empty()) {
- std::map<uint32_t,
- lldb_private::UnwindLLDB::ConcreteRegisterLocation>::const_iterator
- iterator;
- iterator = m_registers.find(regnum.GetAsKind(eRegisterKindLLDB));
- if (iterator != m_registers.end()) {
- regloc = iterator->second;
- UnwindLogMsg("supplying caller's saved %s (%d)'s location, cached",
- regnum.GetName(), regnum.GetAsKind(eRegisterKindLLDB));
- return UnwindLLDB::RegisterSearchResult::eRegisterFound;
- }
- }
-
- // Look through the available UnwindPlans for the register location.
-
UnwindPlan::Row::AbstractRegisterLocation unwindplan_regloc;
- bool have_unwindplan_regloc = false;
- RegisterKind unwindplan_registerkind = kNumRegisterKinds;
+ // First, try to find a register location via the FastUnwindPlan
if (m_fast_unwind_plan_sp) {
const UnwindPlan::Row *active_row =
m_fast_unwind_plan_sp->GetRowForFunctionOffset(m_current_offset);
- unwindplan_registerkind = m_fast_unwind_plan_sp->GetRegisterKind();
- if (regnum.GetAsKind(unwindplan_registerkind) == LLDB_INVALID_REGNUM) {
+ kind = m_fast_unwind_plan_sp->GetRegisterKind();
+ if (regnum.GetAsKind(kind) == LLDB_INVALID_REGNUM) {
UnwindLogMsg("could not convert lldb regnum %s (%d) into %d RegisterKind "
"reg numbering scheme",
regnum.GetName(), regnum.GetAsKind(eRegisterKindLLDB),
- (int)unwindplan_registerkind);
- return UnwindLLDB::RegisterSearchResult::eRegisterNotFound;
+ (int)kind);
+ return {};
}
// The architecture default unwind plan marks unknown registers as
// Undefined so that we don't forward them up the stack when a
// jitted stack frame may have overwritten them. But when the
// arch default unwind plan is used as the Fast Unwind Plan, we
// need to recognize this & switch over to the Full Unwind Plan
- // to see what unwind rule that (more knoweldgeable, probably)
- // UnwindPlan has. If the full UnwindPlan says the register
- // location is Undefined, then it really is.
- if (active_row->GetRegisterInfo(regnum.GetAsKind(unwindplan_registerkind),
+ // to see what unwind rule that (more knowledgeable, probably)
+ // UnwindPlan has.
+ if (active_row->GetRegisterInfo(regnum.GetAsKind(kind),
unwindplan_regloc) &&
!unwindplan_regloc.IsUndefined()) {
UnwindLogMsg(
"supplying caller's saved %s (%d)'s location using FastUnwindPlan",
regnum.GetName(), regnum.GetAsKind(eRegisterKindLLDB));
- have_unwindplan_regloc = true;
+ return unwindplan_regloc;
}
}
- if (!have_unwindplan_regloc) {
- // m_full_unwind_plan_sp being NULL means that we haven't tried to find a
- // full UnwindPlan yet
- bool got_new_full_unwindplan = false;
- if (!m_full_unwind_plan_sp) {
- m_full_unwind_plan_sp = GetFullUnwindPlanForFrame();
- got_new_full_unwindplan = true;
- }
+ // Second, try to find a register location via the FullUnwindPlan.
+ bool got_new_full_unwindplan = false;
+ if (!m_full_unwind_plan_sp) {
+ m_full_unwind_plan_sp = GetFullUnwindPlanForFrame();
+ got_new_full_unwindplan = true;
+ }
+ if (m_full_unwind_plan_sp) {
+ RegisterNumber pc_regnum(m_thread, eRegisterKindGeneric,
+ LLDB_REGNUM_GENERIC_PC);
- if (m_full_unwind_plan_sp) {
- RegisterNumber pc_regnum(m_thread, eRegisterKindGeneric,
- LLDB_REGNUM_GENERIC_PC);
+ const UnwindPlan::Row *active_row =
+ m_full_unwind_plan_sp->GetRowForFunctionOffset(
+ m_current_offset_backed_up_one);
+ kind = m_full_unwind_plan_sp->GetRegisterKind();
- const UnwindPlan::Row *active_row =
- m_full_unwind_plan_sp->GetRowForFunctionOffset(
- m_current_offset_backed_up_one);
- unwindplan_registerkind = m_full_unwind_plan_sp->GetRegisterKind();
+ if (got_new_full_unwindplan && active_row && log) {
+ StreamString active_row_strm;
+ ExecutionContext exe_ctx(m_thread.shared_from_this());
+ active_row->Dump(active_row_strm, m_full_unwind_plan_sp.get(), &m_thread,
+ m_start_pc.GetLoadAddress(exe_ctx.GetTargetPtr()));
+ UnwindLogMsg("Using full unwind plan '%s'",
+ m_full_unwind_plan_sp->GetSourceName().AsCString());
+ UnwindLogMsg("active row: %s", active_row_strm.GetData());
+ }
- if (got_new_full_unwindplan && active_row && log) {
- StreamString active_row_strm;
- ExecutionContext exe_ctx(m_thread.shared_from_this());
- active_row->Dump(active_row_strm, m_full_unwind_plan_sp.get(),
- &m_thread,
- m_start_pc.GetLoadAddress(exe_ctx.GetTargetPtr()));
- UnwindLogMsg("Using full unwind plan '%s'",
- m_full_unwind_plan_sp->GetSourceName().AsCString());
- UnwindLogMsg("active row: %s", active_row_strm.GetData());
+ if (regnum.GetAsKind(kind) == LLDB_INVALID_REGNUM) {
+ if (kind == eRegisterKindGeneric) {
+ UnwindLogMsg("could not convert lldb regnum %s (%d) into "
+ "eRegisterKindGeneric reg numbering scheme",
+ regnum.GetName(), regnum.GetAsKind(eRegisterKindLLDB));
+ } else {
+ UnwindLogMsg("could not convert lldb regnum %s (%d) into %d "
+ "RegisterKind reg numbering scheme",
+ regnum.GetName(), regnum.GetAsKind(eRegisterKindLLDB),
+ (int)kind);
}
+ return {};
+ }
+
+ if (regnum.IsValid() && active_row &&
+ active_row->GetRegisterInfo(regnum.GetAsKind(kind),
+ unwindplan_regloc)) {
+ UnwindLogMsg(
+ "supplying caller's saved %s (%d)'s location using %s UnwindPlan",
+ regnum.GetName(), regnum.GetAsKind(eRegisterKindLLDB),
+ m_full_unwind_plan_sp->GetSourceName().GetCString());
+ return unwindplan_regloc;
+ }
+
+ // When asking for the caller's pc, check if we have a
+ // Return Address register on this target.
+ //
+ // On a Return Address Register architecture like arm/mips/riscv,
+ // the caller's pc is in the RA register, and will be spilled to
+ // stack before any other function can be called. We may have a
+ // register location saying
+ // pc=RAReg {caller's retrun addr is in RA register}
+ // ra=IsSame {caller's return addr is live in RA register}
+ // ra=StackAddr {caller's return addr spilled to stack}
+ // or none of the above, which means the caller's pc is live in the
+ // return address reg if this is frame 0, or the frame below is
+ // a trap/sigtramp/interrupt handler function. Any other mid-stack
+ // function must have an unwind rule for PC/RA giving a location/value.
+ //
+ // In the case of an interrupted function -- the function above sigtramp,
+ // or a function interrupted asynchronously, or that has faulted to
+ // a trap handler -- it is valid to ask both the "pc" value -- the
+ // instruction that was executing when the interrupt/fault happend --
+ // and the RA Register value. If a frameless function (which doesn't
+ // create a stack frame, doesn't save the RA reg to stack) is interrupted,
+ // the trap handler will have a rule to provide the pc (the instruction
+ // that was executing) AND a rule to provide the RA Register, which we
+ // need to use to find the caller function:
+ // pc=StackAddr1
+ // ra=StackAddr2
+ // and we don't want to rewrite a request of "pc" to "ra" here, because
+ // they mean different things.
+
+ if (pc_regnum.IsValid() && pc_regnum == regnum) {
RegisterNumber return_address_reg;
+ uint32_t return_address_regnum =
+ LLDB_INVALID_REGNUM; // in full UnwindPlan's numbering
+
+ // Get the return address register number from the UnwindPlan
+ // or the arch register set.
+ if (m_full_unwind_plan_sp->GetReturnAddressRegister() !=
+ LLDB_INVALID_REGNUM) {
+ return_address_regnum =
+ m_full_unwind_plan_sp->GetReturnAddressRegister();
+ } else {
+ RegisterNumber arch_default_ra_regnum(m_thread, eRegisterKindGeneric,
+ LLDB_REGNUM_GENERIC_RA);
+ return_address_regnum = arch_default_ra_regnum.GetAsKind(kind);
+ }
- // If we're fetching the saved pc and this UnwindPlan defines a
- // ReturnAddress register (e.g. lr on arm), look for the return address
- // register number in the UnwindPlan's row.
- if (pc_regnum.IsValid() && pc_regnum == regnum &&
- m_full_unwind_plan_sp->GetReturnAddressRegister() !=
- LLDB_INVALID_REGNUM) {
- // If this is a trap handler frame, we should have access to
- // the complete register context when the interrupt/async
- // signal was received, we should fetch the actual saved $pc
- // value instead of the Return Address register.
- // If $pc is not available, fall back to the RA reg.
+ if (return_address_regnum != LLDB_INVALID_REGNUM) {
UnwindPlan::Row::AbstractRegisterLocation scratch;
+ // This is a sigtramp/interrupt handler - treat a
+ // request for "pc" and "ra" as distinct.
if (m_frame_type == eTrapHandlerFrame && active_row &&
- active_row->GetRegisterInfo(
- pc_regnum.GetAsKind(unwindplan_registerkind), scratch)) {
+ active_row->GetRegisterInfo(pc_regnum.GetAsKind(kind), scratch)) {
UnwindLogMsg("Providing pc register instead of rewriting to "
"RA reg because this is a trap handler and there is "
"a location for the saved pc register value.");
} else {
- return_address_reg.init(
- m_thread, m_full_unwind_plan_sp->GetRegisterKind(),
- m_full_unwind_plan_sp->GetReturnAddressRegister());
+ // This is a normal function, there's no rule for
+ // finding the caller's pc value, look for the caller's
+ // return address register value.
+ return_address_reg.init(m_thread,
+ m_full_unwind_plan_sp->GetRegisterKind(),
+ return_address_regnum);
regnum = return_address_reg;
UnwindLogMsg("requested caller's saved PC but this UnwindPlan uses a "
"RA reg; getting %s (%d) instead",
return_address_reg.GetName(),
return_address_reg.GetAsKind(eRegisterKindLLDB));
- }
- } else {
- if (regnum.GetAsKind(unwindplan_registerkind) == LLDB_INVALID_REGNUM) {
- if (unwindplan_registerkind == eRegisterKindGeneric) {
- UnwindLogMsg("could not convert lldb regnum %s (%d) into "
- "eRegisterKindGeneric reg numbering scheme",
- regnum.GetName(), regnum.GetAsKind(eRegisterKindLLDB));
- } else {
- UnwindLogMsg("could not convert lldb regnum %s (%d) into %d "
- "RegisterKind reg numbering scheme",
+ if (active_row && active_row->GetRegisterInfo(regnum.GetAsKind(kind),
+ unwindplan_regloc)) {
+ UnwindLogMsg("supplying caller's saved %s (%d)'s location using "
+ "%s UnwindPlan",
regnum.GetName(), regnum.GetAsKind(eRegisterKindLLDB),
- (int)unwindplan_registerkind);
- }
- return UnwindLLDB::RegisterSearchResult::eRegisterNotFound;
- }
- }
-
- // Check if the active_row has a register location listed.
- if (regnum.IsValid() && active_row &&
- active_row->GetRegisterInfo(regnum.GetAsKind(unwindplan_registerkind),
- unwindplan_regloc)) {
- have_unwindplan_regloc = true;
- UnwindLogMsg(
- "supplying caller's saved %s (%d)'s location using %s UnwindPlan",
- regnum.GetName(), regnum.GetAsKind(eRegisterKindLLDB),
- m_full_unwind_plan_sp->GetSourceName().GetCString());
- }
-
- // This is frame 0 and we're retrieving the PC and it's saved in a Return
- // Address register and it hasn't been saved anywhere yet -- that is,
- // it's still live in the actual register. Handle this specially.
- if (!have_unwindplan_regloc && return_address_reg.IsValid() &&
- return_address_reg.GetAsKind(eRegisterKindLLDB) !=
- LLDB_INVALID_REGNUM) {
- if (IsFrameZero()) {
- lldb_private::UnwindLLDB::ConcreteRegisterLocation new_regloc;
- new_regloc.type = UnwindLLDB::ConcreteRegisterLocation::
- eRegisterInLiveRegisterContext;
- new_regloc.location.register_number =
- return_address_reg.GetAsKind(eRegisterKindLLDB);
- m_registers[regnum.GetAsKind(eRegisterKindLLDB)] = new_regloc;
- regloc = new_regloc;
- UnwindLogMsg("supplying caller's register %s (%d) from the live "
- "RegisterContext at frame 0, saved in %d",
- return_address_reg.GetName(),
- return_address_reg.GetAsKind(eRegisterKindLLDB),
- return_address_reg.GetAsKind(eRegisterKindLLDB));
- return UnwindLLDB::RegisterSearchResult::eRegisterFound;
- } else if (BehavesLikeZerothFrame()) {
- // This function was interrupted asynchronously -- it faulted,
- // an async interrupt, a timer fired, a debugger expression etc.
- // The caller's pc is in the Return Address register, but the
- // UnwindPlan for this function may have no location rule for
- // the RA reg.
- // This means that the caller's return address is in the RA reg
- // when the function was interrupted--descend down one stack frame
- // to retrieve it from the trap handler's saved context.
- unwindplan_regloc.SetSame();
- have_unwindplan_regloc = true;
- }
- }
-
- // If this architecture stores the return address in a register (it
- // defines a Return Address register) and we're on a non-zero stack frame
- // and the Full UnwindPlan says that the pc is stored in the
- // RA registers (e.g. lr on arm), then we know that the full unwindplan is
- // not trustworthy -- this
- // is an impossible situation and the instruction emulation code has
- // likely been misled. If this stack frame meets those criteria, we need
- // to throw away the Full UnwindPlan that the instruction emulation came
- // up with and fall back to the architecture's Default UnwindPlan so the
- // stack walk can get past this point.
-
- // Special note: If the Full UnwindPlan was generated from the compiler,
- // don't second-guess it when we're at a call site location.
-
- // arch_default_ra_regnum is the return address register # in the Full
- // UnwindPlan register numbering
- RegisterNumber arch_default_ra_regnum(m_thread, eRegisterKindGeneric,
- LLDB_REGNUM_GENERIC_RA);
-
- if (arch_default_ra_regnum.GetAsKind(unwindplan_registerkind) !=
- LLDB_INVALID_REGNUM &&
- pc_regnum == regnum && unwindplan_regloc.IsInOtherRegister() &&
- unwindplan_regloc.GetRegisterNumber() ==
- arch_default_ra_regnum.GetAsKind(unwindplan_registerkind) &&
- m_full_unwind_plan_sp->GetSourcedFromCompiler() != eLazyBoolYes &&
- !m_all_registers_available) {
- UnwindLogMsg("%s UnwindPlan tried to restore the pc from the link "
- "register but this is a non-zero frame",
- m_full_unwind_plan_sp->GetSourceName().GetCString());
-
- // Throw away the full unwindplan; install the arch default unwindplan
- if (ForceSwitchToFallbackUnwindPlan()) {
- // Update for the possibly new unwind plan
- unwindplan_registerkind = m_full_unwind_plan_sp->GetRegisterKind();
- const UnwindPlan::Row *active_row =
- m_full_unwind_plan_sp->GetRowForFunctionOffset(m_current_offset);
-
- // Sanity check: Verify that we can fetch a pc value and CFA value
- // with this unwind plan
-
- RegisterNumber arch_default_pc_reg(m_thread, eRegisterKindGeneric,
- LLDB_REGNUM_GENERIC_PC);
- bool can_fetch_pc_value = false;
- bool can_fetch_cfa = false;
- addr_t cfa_value;
- if (active_row) {
- if (arch_default_pc_reg.GetAsKind(unwindplan_registerkind) !=
- LLDB_INVALID_REGNUM &&
- active_row->GetRegisterInfo(
- arch_default_pc_reg.GetAsKind(unwindplan_registerkind),
- unwindplan_regloc)) {
- can_fetch_pc_value = true;
- }
- if (ReadFrameAddress(unwindplan_registerkind,
- active_row->GetCFAValue(), cfa_value)) {
- can_fetch_cfa = true;
+ m_full_unwind_plan_sp->GetSourceName().GetCString());
+ if (unwindplan_regloc.IsSame())
+ unwindplan_regloc.SetInRegister(regnum.GetAsKind(kind));
+ return unwindplan_regloc;
+ } else {
+ // No unwind rule for the return address reg on frame
+ // 0 means that the caller's...
[truncated]
|
that wasn't doing anything.
-- has an Undefined abstract register location.
Collaborator
Author
|
to be clear: The diff for the first half of the old SavedLocationForRegister is entirely pointless to look at. Reading RegisterContextUnwind::GetAbstractRegisterLocation in the "new" version of the file is the way to go, and I'm pretty comfortable with everything that I've kept in here, I think the behaviors are easy to reason as correct when I read it. The diff for the second half -- taking an AbstractRegisterLocation and converting it into a ConcreteRegisterLocation -- involved very few changes and is readable. I've run it through our testsuite on macOS and it shows no issues, but I look forward to all the CI bots getting their hands on it & seeing that result confirmed. |
rewrite/clarify a bunch of them. Tiny tweaks to the code to make it more readable.
labath
approved these changes
May 14, 2025
| RegisterContextUnwind::SavedLocationForRegister( | ||
| uint32_t lldb_regnum, | ||
| lldb_private::UnwindLLDB::ConcreteRegisterLocation ®loc) { | ||
| /// Search this stack frame's UnwindPlans for the AbstractRegisterLocation |
Collaborator
There was a problem hiding this comment.
The docstring should go in the header. If there are details you think are only relevant for the implementation (not the interface), they can stay here.
Co-authored-by: Pavel Labath <[email protected]>
I do want it next to the method because I think it makes it clear (to people editing the method) what the expected behaviors are, more than people who might be *calling* the method and looking at the header file. But it is unusual so we'll see what other people think.
Collaborator
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/18/builds/17248 Here is the relevant piece of the build log for the reference |
labath
added a commit
to labath/llvm-project
that referenced
this pull request
Jun 10, 2025
This fixes a regression caused by llvm#139817, where we would fail to unwind (using eh_frame) when using debug stubs (like qemu) which do not provide eh_frame register numbers. Unwinding fails because the unwinder tries to convert pc register number to the "eh_frame" scheme (and fails). Previously that worked because the code used a slightly different pattern which ended up comparing the number by converting it to the "generic" scheme (which we do provide). I don't think that's a bug in the unwinder -- we just need to make sure we provide the register number all the time. The associated test is not particularly useful, but I'm hoping it could be used to test other changes like this as well. Other register augmentation changes are tested in an end-to-end fashion (by injecting gdb-remote packets and then trying to read registers). That works well for testing the addition of subregisters, but it isn't particularly suitable for testing register numbers, as the only place (that I know of) where this manifests is during unwinding, and recreating an unwindable process in a test like this is fairly challenging.
labath
added a commit
that referenced
this pull request
Jun 10, 2025
This fixes a regression caused by #139817, where we would fail to unwind (using eh_frame) when using debug stubs (like qemu) which do not provide eh_frame register numbers. Unwinding fails because the unwinder tries to convert pc register number to the "eh_frame" scheme (and fails). Previously that worked because the code used a slightly different pattern which ended up comparing the number by converting it to the "generic" scheme (which we do provide). I don't think that's a bug in the unwinder -- we just need to make sure we provide the register number all the time. The associated test is not particularly useful, but I'm hoping it could be used to test other changes like this as well. Other register augmentation changes are tested in an end-to-end fashion (by injecting gdb-remote packets and then trying to read registers). That works well for testing the addition of subregisters, but it isn't particularly suitable for testing register numbers, as the only place (that I know of) where this manifests is during unwinding, and recreating an unwindable process in a test like this is fairly challenging. Probably the best way to ensure coverage for this scenario would be to have lldb-server stop sending eh_frame register numbers (align it with other gdb-like stubs).
tomtor
pushed a commit
to tomtor/llvm-project
that referenced
this pull request
Jun 14, 2025
…vm#139817) RegisterContextUnwind::SavedLocationForRegister is around 450 lines that first find an abstract register location (e.g. "CFA-8") for a register by looking in the UnwindPlans. Then it evaluates the abstract register location to create a concrete register location (e.g. "stored at address 0x...", "live in register at frame 0"). There are some complicated cases in the first half of the method to handle return address register architectures correctly, in particular. Looking at the two halves, they're both exactly 226 lines long and there's little involvement between them except for passing an abstract register location along. (there were some parts in the "abstract register location" code that would set the concrete register location, unnecessarily) It's also a complex enough method that there are some bits of code that aren't actually doing anything at this point. This patch adds a RegisterContextUnwind::GetAbstractRegisterLocation method, which does the first half, and has a clearly defined return values. The code to convert an AbstractRegisterLocation into a ConcreteRegisterLocation remains in SavedLocationForRegister. It's a bit of a tricky patch to visually inspect, despite it not changing functionality, the reorganizations and rewrites make the diff unreadable. Nearly all the real changes are in the "find the abstract register location" first half of the method. I think reading the new code in its new form is the easiest way to inspect this PR. With a defined interface between the two of what is expected, it's pretty easy to look at the code and reason about whether it is written correctly. (whereas before, that was very difficult, for me at least.) --------- Co-authored-by: Pavel Labath <[email protected]>
tomtor
pushed a commit
to tomtor/llvm-project
that referenced
this pull request
Jun 14, 2025
This fixes a regression caused by llvm#139817, where we would fail to unwind (using eh_frame) when using debug stubs (like qemu) which do not provide eh_frame register numbers. Unwinding fails because the unwinder tries to convert pc register number to the "eh_frame" scheme (and fails). Previously that worked because the code used a slightly different pattern which ended up comparing the number by converting it to the "generic" scheme (which we do provide). I don't think that's a bug in the unwinder -- we just need to make sure we provide the register number all the time. The associated test is not particularly useful, but I'm hoping it could be used to test other changes like this as well. Other register augmentation changes are tested in an end-to-end fashion (by injecting gdb-remote packets and then trying to read registers). That works well for testing the addition of subregisters, but it isn't particularly suitable for testing register numbers, as the only place (that I know of) where this manifests is during unwinding, and recreating an unwindable process in a test like this is fairly challenging. Probably the best way to ensure coverage for this scenario would be to have lldb-server stop sending eh_frame register numbers (align it with other gdb-like stubs).
akuhlens
pushed a commit
to akuhlens/llvm-project
that referenced
this pull request
Jun 24, 2025
This fixes a regression caused by llvm#139817, where we would fail to unwind (using eh_frame) when using debug stubs (like qemu) which do not provide eh_frame register numbers. Unwinding fails because the unwinder tries to convert pc register number to the "eh_frame" scheme (and fails). Previously that worked because the code used a slightly different pattern which ended up comparing the number by converting it to the "generic" scheme (which we do provide). I don't think that's a bug in the unwinder -- we just need to make sure we provide the register number all the time. The associated test is not particularly useful, but I'm hoping it could be used to test other changes like this as well. Other register augmentation changes are tested in an end-to-end fashion (by injecting gdb-remote packets and then trying to read registers). That works well for testing the addition of subregisters, but it isn't particularly suitable for testing register numbers, as the only place (that I know of) where this manifests is during unwinding, and recreating an unwindable process in a test like this is fairly challenging. Probably the best way to ensure coverage for this scenario would be to have lldb-server stop sending eh_frame register numbers (align it with other gdb-like stubs).
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
RegisterContextUnwind::SavedLocationForRegister is around 450 lines that first find an abstract register location (e.g. "CFA-8") for a register by looking in the UnwindPlans. Then it evaluates the abstract register location to create a concrete register location (e.g. "stored at address 0x...", "live in register at frame 0"). There are some complicated cases in the first half of the method to handle return address register architectures correctly, in particular.
Looking at the two halves, they're both exactly 226 lines long and there's little involvement between them except for passing an abstract register location along.
(there were some parts in the "abstract register location" code that would set the concrete register location, unnecessarily)
It's also a complex enough method that there are some bits of code that aren't actually doing anything at this point.
This patch adds a RegisterContextUnwind::GetAbstractRegisterLocation method, which does the first half, and has a clearly defined return values.
The code to convert an AbstractRegisterLocation into a ConcreteRegisterLocation remains in SavedLocationForRegister.
It's a bit of a tricky patch to visually inspect, despite it not changing functionality, the reorganizations and rewrites make the diff unreadable. Nearly all the real changes are in the "find the abstract register location" first half of the method. I think reading the new code in its new form is the easiest way to inspect this PR. With a defined interface between the two of what is expected, it's pretty easy to look at the code and reason about whether it is written correctly.
(whereas before, that was very difficult, for me at least.)