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

[release/9.0-preview1] Revert "Update HP libunwind to v1.8.0" #97679

Merged
merged 1 commit into from
Jan 30, 2024

Conversation

hoyosjs
Copy link
Member

@hoyosjs hoyosjs commented Jan 30, 2024

This reverts commit 5bb5840.

Most debugging and dump collection scenarios are broken on Unix platforms due to a null pointer read during remote unwinding introduced in libunwind/libunwind#524. The long term fix is in the works in libunwind/libunwind#714, revert this for preview 1 for now.

cc: @am11

@ghost
Copy link

ghost commented Jan 30, 2024

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

Issue Details

This reverts commit 5bb5840.

Most debugging and dump collection scenarios are broken on Unix platforms due to a null pointer read during remote unwinding introduced in libunwind/libunwind#524. The long term fix is in the works in libunwind/libunwind#714, revert this for preview 1 for now.

cc: @am11

Author: hoyosjs
Assignees: hoyosjs
Labels:

area-System.Reflection.Metadata

Milestone: -

@hoyosjs hoyosjs added Servicing-consider Issue for next servicing release review area-Meta and removed area-System.Reflection.Metadata labels Jan 30, 2024
@hoyosjs hoyosjs added this to the 9.0.0 milestone Jan 30, 2024
@ghost
Copy link

ghost commented Jan 30, 2024

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

Issue Details

This reverts commit 5bb5840.

Most debugging and dump collection scenarios are broken on Unix platforms due to a null pointer read during remote unwinding introduced in libunwind/libunwind#524. The long term fix is in the works in libunwind/libunwind#714, revert this for preview 1 for now.

cc: @am11

Author: hoyosjs
Assignees: hoyosjs
Labels:

Servicing-consider, area-Meta

Milestone: -

@hoyosjs
Copy link
Member Author

hoyosjs commented Jan 30, 2024

cc @mmitche @tommcdon

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.

🥲 for missing the preview 1 train, but we will get that fixed (or problematic commit reverted) in main soon'ish. Thanks to @cshung and @bregma for the help!

@janvorli
Copy link
Member

@am11 I have found that there is also some race condition in the new libunwind that manifests itself with my new EH enabled on arm64. The older libunwind works fine. The issue occurs when multiple threads are trying to unwind from the same function at the same time. The unw_step sometimes returns 0 in PC and nonsense in SP. When I added a call to it again right after that on a copy of the original context, it passes. I suspect the caching in libunwind is somehow involved in this. The baseservices/exceptions/regressions/V1/SEH/VJ/ExternalException test crashes within 50..100 iterations with the libunwind 1.8.0 and can pass thousands of iterations with the older one.

@janvorli
Copy link
Member

However, it seems that the new libunwind has also fixed some issues with unwinding that I have started observing in my testing of the new EH stuff after locally reverting the change, so it seems that in main, we should keep it and I'll need to figure out what's causing the race now. It seems that the caching is actually not the culprit there as even when I disable the caching in libunwind, the problem still happens.

@jeffschwMSFT jeffschwMSFT added Servicing-approved Approved for servicing release and removed Servicing-consider Issue for next servicing release review labels Jan 30, 2024
@mmitche
Copy link
Member

mmitche commented Jan 30, 2024

Merge when ready.

@hoyosjs
Copy link
Member Author

hoyosjs commented Jan 30, 2024

Hard to track the mono crash, it's a reflection crash.

@hoyosjs hoyosjs merged commit 4d80293 into release/9.0-preview1 Jan 30, 2024
179 of 188 checks passed
@hoyosjs hoyosjs deleted the juhoyosa/revert-unwind1.8 branch January 30, 2024 22:48
@EgorBo
Copy link
Member

EgorBo commented Feb 1, 2024

Should it be reverted from Main as well? It causes problems for VMR repo: dotnet/source-build#4007 (comment)

@am11
Copy link
Member

am11 commented Feb 1, 2024

@EgorBo, as @janvorli mentioned, I think we should fix the problem rather than reverting it. Some distos have already upgraded to libunwind v1.8 (and others will soon), so it won't help the source-build if we revert it, because:

  • distros use libunwind from the packages when building VMR
  • intenal source-build use distro-agnostic libunwind from the tree (which is not very relevant to the primary "distro" use-case)

In short, lets apply this upstream patch: https://github.com/libunwind/libunwind/pull/714.patch in source-build for the time being until that PR is merged, then we will apply it in runtime repo.

@github-actions github-actions bot locked and limited conversation to collaborators Mar 3, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-Meta Servicing-approved Approved for servicing release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants