Fix unwind from signal handler on ARM #30
Merged
Conversation
This file contains 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
This was referenced Jul 23, 2024
alexey-milovidov
approved these changes
Jul 24, 2024
azat
reviewed
Jul 25, 2024
| // 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); |
There was a problem hiding this comment.
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Same fix as in #25 , but for the other code path that unwinds from signal handler.
#27 was incorrect because I didn't understand that the comparison was intentionally off-by-one, to implicitly subtract 1 from the address. Added a comment about it. I wish this -1 was (a) explicit, (b) skipped for signal return (instead of adding 1, then implicitly subtracting it).
Not upstreamable in current state because maybe-DWARF-specific code (with DWARF-specific comment at least) is in non-DWARF-specific part of the code. Need to figure out which of the unwind methods need this +1 and enable it for those. There's also a
--pcinsetInfoBasedOnIPRegister()which plays a similar role and should maybe be unified with all the other explicit and implicit +-1s. It's too much mess to clean up, I probably won't do it. This is probably good enough for clickhouse, we don't use all those other unwinding methods.