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

Move macOS remote unwinding to libunwind directory #69720

Merged
merged 1 commit into from
Jun 29, 2022

Conversation

am11
Copy link
Member

@am11 am11 commented May 24, 2022

We were duplicating headers between Windows and macOS remote unwind code. This PR applies upstream patch from libunwind/libunwind#365 and adjusts our local cmake configs to deduplicate the code.

@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label May 24, 2022
@ghost
Copy link

ghost commented May 24, 2022

Tagging subscribers to this area: @dotnet/area-infrastructure-libraries
See info in area-owners.md if you want to be subscribed.

Issue Details

We were duplicating headers between Windows and macOS remote unwind code. This PR applies upstream patch from libunwind/libunwind#365 and adjusts our local cmake configs to deduplicate the code.

Author: am11
Assignees: -
Labels:

area-Infrastructure-libraries, community-contribution

Milestone: -

@ViktorHofer ViktorHofer requested a review from janvorli May 24, 2022 16:13
@am11 am11 marked this pull request as ready for review May 24, 2022 16:39
@am11
Copy link
Member Author

am11 commented May 24, 2022

cc @janvorli, @AaronRobinsonMSFT, @jkotas
The missing functions on Windows for remote unwind were added to libunwind in libunwind/libunwind@693f30a, this PR moves macOS missing functions on the same plan and aligns file naming.

@AaronRobinsonMSFT AaronRobinsonMSFT added this to the 7.0.0 milestone May 24, 2022
@am11 am11 closed this May 26, 2022
@am11 am11 reopened this May 26, 2022
@am11 am11 force-pushed the feature/native/cleanups branch from 49fff60 to e0b95db Compare June 9, 2022 04:32
@am11
Copy link
Member Author

am11 commented Jun 10, 2022

wasm failure is unrelated. (#69806?)
@janvorli, PTAL.

@AaronRobinsonMSFT
Copy link
Member

@am11 I think we are just waiting to see if the upstream changes get taken. I like the clean-up for sure. Any concerns about upstream acceptance?

@am11
Copy link
Member Author

am11 commented Jun 10, 2022

@AaronRobinsonMSFT, in the past, we have upstreamed in parallel like this. It is totally reasonable to hold this PR until upstream one is reviewed/merged. There is no rush.

I am planning on upstreaming two remaining items after this:

  1. JanV's 1b5719c
  2. Revive Add _OOP_find_proc_info libunwind/libunwind#187 by molding it into pattern used by Expose function-pointer-to-name API libunwind/libunwind#299 (manpage, basic test and build options). That will cleanup src/native/external/libunwind_extras/oop from our side (among few adjustments for "use system libunwind" case; run-time / version-based check to consume this new API).

@AaronRobinsonMSFT
Copy link
Member

It is totally reasonable to hold this PR until upstream one is reviewed/merged

Glad we are on the same page. That is really the only reason I haven't merged this in.

Copy link
Member

@janvorli janvorli left a comment

Choose a reason for hiding this comment

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

LGTM, thank you! I am sorry for the big delay, my github account somehow decided that my email is no longer valid and stopped sending me notifications on comments / PR requests and I've missed this one.

@AaronRobinsonMSFT
Copy link
Member

@am11 Seems like the libunwind changes were merged into main. Let's make sure this is still green and then get it in.

@am11
Copy link
Member Author

am11 commented Jun 24, 2022

The changes made in upstream directory src/native/external/libunwind in main has no upstream PR/commit at all, has no mention in libunwind-version.txt entry and they are unnecessary changes (do not contribute in build in runtime repo at all). We should revert them to keep things synchronized..

@am11
Copy link
Member Author

am11 commented Jun 29, 2022

@AaronRobinsonMSFT, upstream PR is merged now.

@AaronRobinsonMSFT AaronRobinsonMSFT merged commit 8b71701 into dotnet:main Jun 29, 2022
@am11 am11 deleted the feature/native/cleanups branch June 29, 2022 04:10
@ghost ghost locked as resolved and limited conversation to collaborators Jul 29, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-PAL-coreclr community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants