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/8.0] JIT: add missing xarch RMW case #92293

Merged
merged 2 commits into from
Sep 20, 2023

Conversation

github-actions[bot]
Copy link
Contributor

@github-actions github-actions bot commented Sep 19, 2023

Backport of #92252 to release/8.0
Fixes issue #92218

/cc @AndyAyersMS

Customer Impact

Unexpected null reference exception. Reported by customer via #92218 after testing .NET 8 RC1 on their app. This is a regression from .NET 7.

This issue might be hard for impacted customers to understand and work around.

Testing

Verified fix on customer-supplied test case. No SPMI diffs, passes jitstress.

Risk

Low. The underlying problem is a missing case in the "xarch" (x86/x64) code generator, in code where the user manages to indirectly modify one of the locals in a method via a "read modify write" update, eg

*p = &local;
(*p) += <non-constant>

Current JIT behavior can lose track of the address to update, effectively turning this into silent bad code:

*p = &local;
(*0) = <non-constant>

which when executed will cause a NRE.

The fix adds support for this missing case.

Handle the case where we're indirectly updating a local with a value
that is not a constant.

Fixes #92218.
@dotnet-issue-labeler dotnet-issue-labeler bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Sep 19, 2023
@ghost
Copy link

ghost commented Sep 19, 2023

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

Issue Details

Backport of #92252 to release/8.0

/cc @AndyAyersMS

Customer Impact

Testing

Risk

IMPORTANT: If this backport is for a servicing release, please verify that:

  • The PR target branch is release/X.0-staging, not release/X.0.

  • If the change touches code that ships in a NuGet package, you have added the necessary package authoring and gotten it explicitly reviewed.

Author: github-actions[bot]
Assignees: -
Labels:

area-CodeGen-coreclr

Milestone: -

@AndyAyersMS
Copy link
Member

@dotnet/jit-contrib PTAL

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 will take for consideration in 8.0.x

@jeffschwMSFT jeffschwMSFT added the Servicing-consider Issue for next servicing release review label Sep 19, 2023
@jeffschwMSFT jeffschwMSFT added this to the 8.0.0 milestone Sep 19, 2023
@carlossanlop
Copy link
Member

The snap has been done, so if this is meant to go to RC2, please retarget the PR to release/8.0-rc2 . In case you don't know, you can edit the PR title and you'll see the option to retarget the PR branch.

@AndyAyersMS AndyAyersMS changed the base branch from release/8.0 to release/8.0-rc2 September 19, 2023 21:23
@AndyAyersMS
Copy link
Member

The snap has been done, so if this is meant to go to RC2, please retarget the PR to release/8.0-rc2 . In case you don't know, you can edit the PR title and you'll see the option to retarget the PR branch.

Yes, this is for RC2. Updated per your info -- thanks!

@carlossanlop
Copy link
Member

Approved by Tactics via email.

The RC2 branch had a generalized failure for which we just merged a fix, so I updated this branch to ensure we have a green CI before merging.

@carlossanlop carlossanlop added Servicing-approved Approved for servicing release and removed Servicing-consider Issue for next servicing release review labels Sep 20, 2023
@carlossanlop carlossanlop merged commit f6489fe into release/8.0-rc2 Sep 20, 2023
121 of 122 checks passed
@carlossanlop carlossanlop deleted the backport/pr-92252-to-release/8.0 branch September 20, 2023 06:12
@radical radical mentioned this pull request Sep 26, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Oct 20, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI Servicing-approved Approved for servicing release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants