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

Patch libunwind manually to fix the access violation to unblock source build #97813

Merged
merged 2 commits into from
Feb 1, 2024

Conversation

cshung
Copy link
Member

@cshung cshung commented Feb 1, 2024

This is meant to port libunwind/libunwind#717 directly from libunwind to unblock source build.

It is meant to be temporarily, once we have the libunwind updated we will revert this an take a proper update from there.

See the conversation here for the context.

@am11, @EgorBo

@dotnet-issue-labeler dotnet-issue-labeler bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Feb 1, 2024
@ghost ghost assigned cshung Feb 1, 2024
@cshung cshung added area-Diagnostics-coreclr and removed needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels Feb 1, 2024
@ghost
Copy link

ghost commented Feb 1, 2024

Tagging subscribers to this area: @tommcdon
See info in area-owners.md if you want to be subscribed.

Issue Details

This is meant to port libunwind/libunwind#717 directly from libunwind to unblock source build.

It is meant to be temporarily, once we have the libunwind updated we will revert this an take a proper update from there.

@am11, @EgorBo

Author: cshung
Assignees: cshung
Labels:

area-Diagnostics-coreclr, needs-area-label

Milestone: -

@janvorli
Copy link
Member

janvorli commented Feb 1, 2024

@cshung is the issue that this fixes really blocking the source build? I believe this blocks debuggers only as it is remote unwind specific. I have thought that the issue that was blocking the source build was the race that I've found yesterday.

@cshung
Copy link
Member Author

cshung commented Feb 1, 2024

@cshung is the issue that this fixes really blocking the source build? I believe this blocks debuggers only as it is remote unwind specific. I have thought that the issue that was blocking the source build was the race that I've found yesterday.

I don't actually know exactly what happened there, I attached the link to the conversation leading to this PR above. Note that it also unblocks collecting dump on CI.

@am11
Copy link
Member

am11 commented Feb 1, 2024

Please add a line Apply https://github.com/libunwind/libunwind/pull/714 in https://github.com/dotnet/runtime/blob/main/src/native/external/libunwind-version.txt so we can keep track of delta.

@cshung cshung force-pushed the public/unblock-source-build branch from 818ddae to 459eae6 Compare February 1, 2024 14:34
Copy link
Member

@am11 am11 left a comment

Choose a reason for hiding this comment

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

Looks great, thank you! :)

For the future, maybe we can add a few tests to catch these issues earlier, if possible? (as a separate PR)

@janvorli
Copy link
Member

janvorli commented Feb 1, 2024

My patch PR for the other issue is here:
#97817

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!

@cshung
Copy link
Member Author

cshung commented Feb 1, 2024

Looks great, thank you! :)

For the future, maybe we can add a few tests to catch these issues earlier, if possible? (as a separate PR)

We chatted about it, since this is a regression in libunwind, it makes sense to have the test there so that we catch it upstream.

We were invited to write a unit test for it here. It would be great if you can help.

@am11
Copy link
Member

am11 commented Feb 1, 2024

We were invited to write a unit test for it here. It would be great if you can help.

I meant if we could create a test in runtime repo, that would be useful. We are updating libunwind once a year, failing one build leg like the CG2 error message in source-build CI would be enough to catch it earlier on.

I’m actually interested in writing an integration test in libunwind, which will run periodically and test build runtime (docker container in GitHub Actions pipeline), so we don’t have to troubleshoot one year worth of issues after the release. Will draft a proposal to libunwind maintainers over this weekend or so.

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

Successfully merging this pull request may close these issues.

3 participants