From 8f7795c558f91e14507d1c2ffc34595b1af201f9 Mon Sep 17 00:00:00 2001 From: Azat Khuzhin Date: Thu, 25 Jul 2024 11:38:33 +0200 Subject: [PATCH 1/3] Revert "Merge pull request #30 from ClickHouse/sigret" This reverts commit a89d904befea07814628c6ce0b44083c4e149c62, reversing changes made to fe854449e24bedfa26e38465b84374312dbd587f. --- src/DwarfInstructions.hpp | 11 ++++------- src/DwarfParser.hpp | 9 +-------- src/UnwindCursor.hpp | 10 +--------- 3 files changed, 6 insertions(+), 24 deletions(-) diff --git a/src/DwarfInstructions.hpp b/src/DwarfInstructions.hpp index 96744e5..042c860 100644 --- a/src/DwarfInstructions.hpp +++ b/src/DwarfInstructions.hpp @@ -381,13 +381,10 @@ int DwarfInstructions::stepWithDwarf(A &addressSpace, pint_t pc, // Return address is address after call site instruction, so setting IP to // that simulates a return. // - // 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. + // 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. newRegisters.setIP(returnAddress + cieInfo.isSignalFrame); // Simulate the step by replacing the register set with the new ones. diff --git a/src/DwarfParser.hpp b/src/DwarfParser.hpp index 488043d..7adf96a 100644 --- a/src/DwarfParser.hpp +++ b/src/DwarfParser.hpp @@ -452,14 +452,7 @@ bool CFI_Parser::parseFDEInstructions(A &addressSpace, ")\n", static_cast(instructionsEnd)); - // 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. + // see DWARF Spec, section 6.4.2 for details on unwind opcodes while ((p < instructionsEnd) && (codeOffset < pcoffset)) { uint64_t reg; uint64_t reg2; diff --git a/src/UnwindCursor.hpp b/src/UnwindCursor.hpp index 11fc03f..093e88a 100644 --- a/src/UnwindCursor.hpp +++ b/src/UnwindCursor.hpp @@ -2760,15 +2760,7 @@ int UnwindCursor::stepThroughSigReturn(Registers_arm64 &) { _registers.setRegister(UNW_AARCH64_X0 + i, value); } _registers.setSP(_addressSpace.get64(sigctx + kOffsetSp)); - - // 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); - + _registers.setIP(_addressSpace.get64(sigctx + kOffsetPc)); _isSignalFrame = true; return UNW_STEP_SUCCESS; } From 15f1a572aec0d51c3794fc9583702d7e26757b41 Mon Sep 17 00:00:00 2001 From: Azat Khuzhin Date: Thu, 25 Jul 2024 11:39:28 +0200 Subject: [PATCH 2/3] Revert "Fix unwind from signal handler" This reverts commit 02f17ec85cd6b28540d0a4b42f32c2f8a7c8d79d. --- src/DwarfInstructions.hpp | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/src/DwarfInstructions.hpp b/src/DwarfInstructions.hpp index 042c860..8955248 100644 --- a/src/DwarfInstructions.hpp +++ b/src/DwarfInstructions.hpp @@ -379,13 +379,8 @@ int DwarfInstructions::stepWithDwarf(A &addressSpace, pint_t pc, #endif // 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. - newRegisters.setIP(returnAddress + cieInfo.isSignalFrame); + // that does simulates a return. + newRegisters.setIP(returnAddress); // Simulate the step by replacing the register set with the new ones. registers = newRegisters; From 7946894ce7d2a5e6715c4867555799b3da583b29 Mon Sep 17 00:00:00 2001 From: Azat Khuzhin Date: Thu, 25 Jul 2024 11:41:12 +0200 Subject: [PATCH 3/3] [libunwind] fix unwinding from signal handler (#92291) 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. Refs: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=26208 Upstream commit: llvm/llvm-project@7b604cdf75fd1c741a15138684ea0e98dca5e46f --- src/UnwindCursor.hpp | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/src/UnwindCursor.hpp b/src/UnwindCursor.hpp index 093e88a..21c9b3b 100644 --- a/src/UnwindCursor.hpp +++ b/src/UnwindCursor.hpp @@ -2605,6 +2605,14 @@ void UnwindCursor::setInfoBasedOnIPRegister(bool isReturnAddress) { --pc; #endif +#if !(defined(_LIBUNWIND_SUPPORT_SEH_UNWIND) && defined(_WIN32)) + // In case of this is frame of signal handler, 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. + if (_isSignalFrame) + ++pc; +#endif + // Ask address space object to find unwind sections for this pc. UnwindInfoSections sects; if (_addressSpace.findUnwindSections(pc, sects)) {