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-staging] JIT: Read back all replacements before statements with implicit EH control flow #109143

Open
wants to merge 3 commits into
base: release/9.0-staging
Choose a base branch
from

Conversation

github-actions[bot]
Copy link
Contributor

@github-actions github-actions bot commented Oct 23, 2024

Backport of #109107 to release/9.0

/cc @jakobbotsch

Customer Impact

  • Customer reported
  • Found internally

#108969

Silent bad code generation in some cases involving structs, type tests, and exception handling. Programs may unexpectedly crash or compute incorrectly.

As an example, given

    private struct S
    {
        public int A, B, C, D;
    }
   ...
        S v = default;
        try
        {
            v = Bar();
            Use((int?)o);
        }
        catch (Exception)
        {
        }

        Use(v.A);
        Use(v.A);

The JIT will promote the v.A field to a local, and so must ensure that all calls to Use have the correct value, even if an exception is thrown. If the exception site is within a type test (here induced by the (int?)o cast) or similar construct within a try, then value of the promoted local may not be initialized correctly.

There is a workaround (using the undocumented env var DOTNET_JitEnablePhysicalPromotion=0) but it will not be easy for customers to determine the underlying problem and realize there is a workaround available.

Regression

  • Yes
  • No

Regression from .NET 8.

Testing

Verified on the example in #108969 and new test case added to capture this particular pattern.

Risk

Low. The fix is fairly surgical. A small number of methods impacted in our SPMI suite.

…ntrol flow

Physical promotion sometimes needs to insert read backs of all stale
pending replacements when it encounters potential implicit EH control
flow (that is, intra-function control flow because of locally caught
exceptions). It would be possible for this to interact with QMARKs such
that we inserted readbacks inside one of the branches, yet believed we
had read back the replacement on all paths.

The fix here is to indiscriminately start reading back all replacements
before a statement that may cause potential intra-function control flow
to occur.

Fix #108969
@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 Oct 23, 2024
Copy link
Contributor

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

@jakobbotsch
Copy link
Member

This one should target 9.0-staging, but looks like that branch doesn't exist yet.

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.

lgtm. please get a code review. we will take for consideration in 9.0.x

@jeffschwMSFT jeffschwMSFT added this to the 9.0.x milestone Oct 23, 2024
@jeffschwMSFT jeffschwMSFT added the Servicing-consider Issue for next servicing release review label Oct 23, 2024
@rbhanda rbhanda modified the milestones: 9.0.x, 9.0.1 Oct 24, 2024
@rbhanda rbhanda added Servicing-approved Approved for servicing release and removed Servicing-consider Issue for next servicing release review labels Oct 24, 2024
Copy link
Member

@carlossanlop carlossanlop left a comment

Choose a reason for hiding this comment

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

@jakobbotsch - Please retarget this PR to the release/9.0-staging branch.

You can retarget to that branch by clicking on the Edit button on the top right (next to the PR title) and choosing the release/9.0-staging branch from the dropdown. Important: Please make sure you don't bring any unrelated changes from the release/9.0 branch when retargeting to release/9.0-staging.

@jakobbotsch jakobbotsch changed the base branch from release/9.0 to release/9.0-staging October 30, 2024 16:16
@carlossanlop carlossanlop modified the milestones: 9.0.1, 9.0.2 Nov 21, 2024
@carlossanlop carlossanlop changed the title [release/9.0] JIT: Read back all replacements before statements with implicit EH control flow [release/9.0-staging] JIT: Read back all replacements before statements with implicit EH control flow Nov 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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.

5 participants