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

jitstress failing widely with NRE #92990

Closed
jakobbotsch opened this issue Oct 4, 2023 · 3 comments · Fixed by #92998
Closed

jitstress failing widely with NRE #92990

jakobbotsch opened this issue Oct 4, 2023 · 3 comments · Fixed by #92998
Assignees
Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI blocking-clean-ci-optional Blocking optional rolling runs
Milestone

Comments

@jakobbotsch
Copy link
Member

There are many NRE failures in the latest jitstress/libraries-jitstress runs.

@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 4, 2023
@ghost ghost added the untriaged New issue has not been triaged by the area owner label Oct 4, 2023
@ghost
Copy link

ghost commented Oct 4, 2023

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

Issue Details

There are many NRE failures in the latest jitstress/libraries-jitstress runs.

Author: jakobbotsch
Assignees: -
Labels:

area-CodeGen-coreclr

Milestone: -

@jakobbotsch jakobbotsch added blocking-clean-ci-optional Blocking optional rolling runs and removed untriaged New issue has not been triaged by the area owner labels Oct 4, 2023
@jakobbotsch jakobbotsch added this to the 9.0.0 milestone Oct 4, 2023
@jakobbotsch
Copy link
Member Author

The first JIT commit that fails is 8ff1893. Oddly jitstress/libraries-jitstress both passed in #92710, but I'll take a closer look.

@jakobbotsch jakobbotsch self-assigned this Oct 4, 2023
@jakobbotsch
Copy link
Member Author

I made a last minute change in #92710 to remove an ordering side effect on a LCL_VAR where it wasn't supported, but that reveals that we're missing an ordering side effect in morph in some cases:

 fgMorphTree BB03, STMT00005 (before)
                [000018] DAC--------                         ▌  STORE_LCL_VAR ubyte  V02 tmp1         
                [000027] --C--------                         └──▌  EQ        int   
                [000036] ---XG------                            ├──▌  EQ        int   
                [000034] ---XG------                            │  ├──▌  AND       int   
                [000032] n--XG------                            │  │  ├──▌  IND       int   
                [000031] ---X-------                            │  │  │  └──▌  FIELD_ADDR byref  System.UriParser:_flags
                [000014] -----------                            │  │  │     └──▌  LCL_VAR   ref    V03 tmp2          (last use)
                [000033] -----------                            │  │  └──▌  CNS_INT   int    0x10000000
                [000035] -----------                            │  └──▌  CNS_INT   int    0
                [000026] -----------                            └──▌  CNS_INT   int    0
...

-     ( 15, 14) [000018] DA--GO-----                         ▌  STORE_LCL_VAR int    V02 tmp1         
-     ( 11, 11) [000036] ----GO-----                         └──▌  NE        int   
-     (  6,  9) [000034] ----GO-----                            ├──▌  AND       int   
-     (  4,  4) [000032] n---GO-----                            │  ├──▌  IND       int   
-     (  2,  2) [000049] -----O-N---                            │  │  └──▌  ADD       byref 
-     (  1,  1) [000047] -----O-----                            │  │     ├──▌  LCL_VAR   ref    V03 tmp2         
+     ( 15, 14) [000018] DA--G------                         ▌  STORE_LCL_VAR int    V02 tmp1         
+     ( 11, 11) [000036] ----G------                         └──▌  NE        int   
+     (  6,  9) [000034] ----G------                            ├──▌  AND       int   
+     (  4,  4) [000032] n---G------                            │  ├──▌  IND       int   
+     (  2,  2) [000049] -------N---                            │  │  └──▌  ADD       byref 
+     (  1,  1) [000047] -----------                            │  │     ├──▌  LCL_VAR   ref    V03 tmp2         
      (  1,  1) [000048] -----------                            │  │     └──▌  CNS_INT   long   16 Fseq[_flags]
      (  1,  4) [000033] -----------                            │  └──▌  CNS_INT   int    0x10000000
      (  1,  1) [000035] -----------                            └──▌  CNS_INT   int    0

We prove that the indirection [000032] is non-faulting, but that requires GTF_ORDER_SIDEEFF to be set on it. That was set on it previously only because it was propagated from [000047].

jakobbotsch added a commit to jakobbotsch/runtime that referenced this issue Oct 4, 2023
In dotnet#92710 I removed an ordering side effect set on a LCL_VAR node in
fgMorphExpandInstanceField, since the side effect is meaningless on a
local. However, I did not realize that setting the ordering effect was
important because propagating it to parent nodes was actually necessary.

This PR explicitly sets the ordering effect on the ADD<byref> nodes
created, since forming these and reporting them to GC is illegal if the
base is invalid.

Additionally, it sets the ordering effect on the consuming indirection;
if that indirection made use of the fact that the FIELD_ADDR is non-null
to become non-faulting, then it has an ordering dependency once we
expand the FIELD_ADDR to a control-flow based null-check. This is not
actually necessary since we always set end up with an ADD<byref> that
propagates it, but that seems more like a coincidence than anything
else.

Fix dotnet#92990
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Oct 4, 2023
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Oct 5, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Nov 4, 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 blocking-clean-ci-optional Blocking optional rolling runs
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant