ARM32 out of proc unwind support #26082
Conversation
Removed the old dwarf unwind info decoding and use libunwind for both dwarf and arm exidx.
|
@janvorli/@jkotas do you think these changes are too risky to get into 3.0 (Preview 9)? |
How confident we are in the testing we have for this? If we have good test coverage for this and this change is required to make something important work that meets the bar; I think it would be ok to take for 3.0. |
|
I've done extensive manually testing using "createdump" which enumerates all the thread's stacks. I added some temp code to compare/assert the new code with the old code results on amd64/arm64. On all three architectures, I manually verified that createdump was correctly enumerating all the thread's stack frames by comparing the debug logging with a native debugger backtrace under lldb/gdb on the core dump generated. I also verified that the DAC's usage of this PAL unwind function (via SOS's clrstack cmd) works to unwind the special uncached method helper frame (the main reason for this change). There are no automated tests. |
| #include "_OOP_internal.h" | ||
|
|
||
| int | ||
| _OOP_find_proc_info ( |
There was a problem hiding this comment.
This seems like your own function. Does it come from libunwind? Is it a specific version other than libunwind 1.3? Otherwise, if it's yours, is the header necessary?
There was a problem hiding this comment.
@jkotas should I replace the license at the beginning of these two new files (OOP*.c) that I wrote with the MIT one?
There was a problem hiding this comment.
Yes.
I have not realized that you wrote this code. I assumed that you have just copied this file over from libunwind..when I saw the copyright.
|
Hey folks, is there still a plan to merge this into libunwind itself? Unfortunately, this change means that support for using the system-wide version of libunwind is broken since the system-wide version of libunwind wont have your custom function. |
It is still desirable long term. @mikem8361 added a work around for x64 in .NET 5 which made this lower priority. It currently is on my backlog. The libunwind/libunwind#187 PR was temporarily closed because I didn't have time to address the comments (add it to the build system and add tests...). I will probably get to it in the next 6 months, but not much sooner. |
|
Thanks for the update! Please let me know if there's anything I can do to help move this along. Using system libraries is generally a use case that I (and Red Hat) care about quite a bit. |
|
I didn't realize this affected 3.1 too. Perhaps we can consider backporting @mikem8361 change dotnet/runtime#39417 to 3.1. As for this task, my issue was that I didn't understand the code well enough to draft reasonable tests. I don't think it would take terribly long, so if this is important I can increase priority. Part of the reason the priority dropped is this wouldn't be in existing libunwind release versions, so we didn't think it would fix source build issues for a while. Until distros picked up libunwind next (1.7?) |
|
I am still not sure it affects 3.1. For 3.1, I only saw issues unwinding on aarch64, which I think I can live with. For x86_64, using the system libunwind works for me on Fedora and RHEL 7. For 5.0, it seems like using the system libunwind broke everything, at least according to https://github.com/dotnet/source-build/blob/52e0279afa047ec925c2228c9d728eef1f6d0373/repos/runtime.common.props#L24 |
|
You should be able to revert that comment and reenable |
Recent discussion on dotnet/coreclr#26082 suggests this should be working.
|
Thanks @sdmaclea, that seems to work! |
ARM32 out of proc unwind support Removed the old dwarf unwind info decoding and use libunwind for both dwarf and arm exidx. Commit migrated from dotnet/coreclr@e8f08ff
5 participants
Removed the old dwarf unwind info decoding and use libunwind for both dwarf and arm exidx.
Simplifies the out of proc unwind code by using the libunwind coreclr has directly to find and decode the unwind info.