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/6.0] Restore FP pairs when unwinding in ARM64 #69359

Merged
merged 3 commits into from May 16, 2022

Conversation

hoyosjs
Copy link
Member

@hoyosjs hoyosjs commented May 14, 2022

Description

This change ensures that unwinding codes of the type save_fregp that are emitted for sequences like stp dX, dY, [sp, #Z] are correctly unwound. Without this, non-volatile registers are not correctly restored and frames above the handler frames can experience data corruption in the case of an exception in Unix based systems.

Customer Impact

Corruptions caused by this bug are hard to diagnose and lead to silent data loss.

Regression

No.

Testing

Added regression test that fails without the change.

Risk

Very low - equivalent fix exists in the OS tree.

Package authoring signed off?

N/A

@hoyosjs hoyosjs added the Servicing-consider Issue for next servicing release review label May 14, 2022
@hoyosjs hoyosjs self-assigned this May 14, 2022
@dotnet-issue-labeler
Copy link

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

Copy link
Member

@jeffschwMSFT jeffschwMSFT left a comment

Choose a reason for hiding this comment

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

Approved. We are considering for 6.0.x

@jeffschwMSFT
Copy link
Member

@hoyosjs please ensure that this has a code review.

Copy link
Member

@davidwrighton davidwrighton left a comment

Choose a reason for hiding this comment

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

This is a good targetted fix for .NET 6.0

@hoyosjs
Copy link
Member Author

hoyosjs commented May 16, 2022

This test is failing even against 6.0.5 on lab machines for Windows (basically without this change. This code path doesn't get used on Windows, which means the OS unwinder in the lab machines is likely not patched? At least for DDARM64-173, although that one reports having (Windows 10.0.19044.1645). I've seen it also fail in 10.0.19041.1415. This passes on windows 11 though. I could turn the test off for Windows given that it's only testing the OS at that point, but feels odd to be in that spot.

@jeffschwMSFT jeffschwMSFT added Servicing-approved Approved for servicing release and removed Servicing-consider Issue for next servicing release review labels May 16, 2022
@jeffschwMSFT jeffschwMSFT added this to the 6.0.6 milestone May 16, 2022
@mmitche
Copy link
Member

mmitche commented May 16, 2022

Reminder that code complete is today.

@davidwrighton
Copy link
Member

@hoyosjs I've confirmed that the OS unwinder does not have this fix on that version of Windows. Please just turn the test off for Windows, and we'll figure out what to do to best handle the situation later.

@jeffschwMSFT
Copy link
Member

@hoyosjs are we still investigating PR failures, or are we ready to merge?

cc @carlossanlop

@hoyosjs hoyosjs merged commit c213c5b into dotnet:release/6.0 May 16, 2022
@hoyosjs hoyosjs deleted the juhoyosa/unwinder-arm64-fix branch May 16, 2022 22:09
@dotnet dotnet locked as resolved and limited conversation to collaborators Jun 16, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Servicing-approved Approved for servicing release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants