Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix unwind from signal handler on ARM #30

Merged
merged 1 commit into from
Jul 24, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 7 additions & 4 deletions src/DwarfInstructions.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -381,10 +381,13 @@ int DwarfInstructions<A, R>::stepWithDwarf(A &addressSpace, pint_t pc,
// Return address is address after call site instruction, so setting IP to
// that simulates a return.
//
// In case of this is frame of signal handler, the IP should be
// incremented, because the IP saved in the signal handler points to
// first non-executed instruction, while FDE/CIE expects IP to be after
// the first non-executed instruction.
// The +-1 situation is subtle.
// Return address points to the next instruction after the `call`
// instruction, but logically we're "inside" the call instruction, and
// FDEs are constructed accordingly.
// So our FDE parsing implicitly subtracts 1 from the address.
// But for signal return, there's no `call` instruction, and
// subtracting 1 would be incorrect. So we add 1 here to compensate.
newRegisters.setIP(returnAddress + cieInfo.isSignalFrame);

// Simulate the step by replacing the register set with the new ones.
Expand Down
9 changes: 8 additions & 1 deletion src/DwarfParser.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -452,7 +452,14 @@ bool CFI_Parser<A>::parseFDEInstructions(A &addressSpace,
")\n",
static_cast<uint64_t>(instructionsEnd));

// see DWARF Spec, section 6.4.2 for details on unwind opcodes
// see DWARF Spec, section 6.4.2 for details on unwind opcodes;
//
// Note that we're looking for the PrologInfo for address `codeOffset - 1`,
// hence '<' instead of '<=" in `codeOffset < pcoffset`
// (compare to DWARF Spec section 6.4.3 "Call Frame Instruction Usage").
// The -1 accounts for the fact that function return address points to the
// next instruction *after* the `call` instruction, while control is
// logically "inside" the `call` instruction.
while ((p < instructionsEnd) && (codeOffset < pcoffset)) {
uint64_t reg;
uint64_t reg2;
Expand Down
10 changes: 9 additions & 1 deletion src/UnwindCursor.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -2760,7 +2760,15 @@ int UnwindCursor<A, R>::stepThroughSigReturn(Registers_arm64 &) {
_registers.setRegister(UNW_AARCH64_X0 + i, value);
}
_registers.setSP(_addressSpace.get64(sigctx + kOffsetSp));
_registers.setIP(_addressSpace.get64(sigctx + kOffsetPc));

// The +1 story is the same as in DwarfInstructions::stepWithDwarf()
// (search for "returnAddress + cieInfo.isSignalFrame" or "Return address points to the next instruction").
// This is probably not the right place for this because this function is not necessarily used
// with DWARF. Need to research whether the other unwind methods have the same +-1 situation or
// are off by one.
pint_t returnAddress = _addressSpace.get64(sigctx + kOffsetPc);
_registers.setIP(returnAddress + 1);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AFAIU this patch it is not required with the last version of mine initial patch - https://github.com/llvm/llvm-project/pull/92291/files

Since now this +1 is in setInfoBasedOnIPRegister which is called for arm64 as well, so let's try to reapply proper version of the patch - #31


_isSignalFrame = true;
return UNW_STEP_SUCCESS;
}
Expand Down