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

Reapply patch to fix SIGSEGV in case of unwind from signal handler (should be aarch64-safe) #31

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

azat
Copy link

@azat azat commented Jul 25, 2024

Changes

azat added 3 commits July 25, 2024 11:38
This reverts commit a89d904, reversing
changes made to fe85444.
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@7b604cd
@al13n321
Copy link
Member

al13n321 commented Jul 25, 2024

IIUC, llvm/llvm-project@7b604cd affects which FDE is selected, but doesn't affect which range within the FDE is selected. Like, there are two separate mechanisms for decrementing pc in non-top/non-signal frames: (1) the --pc in setInfoBasedOnIPRegister(), which is used for finding an FDE, (2) the "off-by-one" codeOffset < pcoffset in parseFDEInstructions(), which is used for finding a range withing an FDE.

EDIT: Not quite: parseFDEInstructions() is called both from setInfoBasedOnIPRegister() (with adjusted pc) and from DwarfInstructions<A, R>::stepWithDwarf() (with unadjusted pc). I'm not sure what determines which one happens; when debugging on ARM, I saw the call from stepWithDwarf(). The call from setInfoBasedOnIPRegister() seems off-by-one independent of any of these fixes: it uses decremented pc, but parseFDEInstructions() seems to expect non-decremented pc, as comment in https://github.com/ClickHouse/libunwind/pull/30/files explains.

EDIT2: If I were to fix it, I'd do:

  • Make parseFDEInstructions() take decremented pc (codeOffset <= pcoffset), matching DWARF Spec section 6.4.3 "Call Frame Instruction Usage".
  • Do the pc adjustment in only one place, probably in setInfoBasedOnIPRegister(). Probably save the adjusted value in the cursor, next to _isSignalFrame (and maybe _isSignalFrame is not used for anything else and can be removed). Use the adjusted pc in stepWithDwarf().
  • The difficult part: research aaalllll the other unwinding methods and platforms (compact, SEH, TBTAB, EHABI, whatever any of those words mean, PPC, MIPS, SPARK, RISKV, etc) to see which of them need this adjustment. Or gate all changes to only affect DWARF. Or something in the middle.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants