Skip to content

Conversation

@BruceForstall
Copy link
Contributor

Under ELT profiler stress, we encode the un-relocated address of the DummyProfilerELTStub function in the JIT codebase into the generated code. This can lead to spurious diffs if the address is different between base and diff JITs (which is likely).

To avoid this, under SuperPMI replay (which we only know in DEBUG builds currently), use a fixed constant for this address. The code isn't executed during SuperPMI replay, so it doesn't need to be a valid code address.

Under ELT profiler stress, we encode the un-relocated address
of the `DummyProfilerELTStub` function in the JIT codebase into
the generated code. This can lead to spurious diffs if the address
is different between base and diff JITs (which is likely).

To avoid this, under SuperPMI replay (which we only know in DEBUG
builds currently), use a fixed constant for this address. The code
isn't executed during SuperPMI replay, so it doesn't need to be a
valid code address.
@ghost ghost added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label May 12, 2023
@ghost ghost assigned BruceForstall May 12, 2023
@ghost
Copy link

ghost commented May 12, 2023

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

Issue Details

Under ELT profiler stress, we encode the un-relocated address of the DummyProfilerELTStub function in the JIT codebase into the generated code. This can lead to spurious diffs if the address is different between base and diff JITs (which is likely).

To avoid this, under SuperPMI replay (which we only know in DEBUG builds currently), use a fixed constant for this address. The code isn't executed during SuperPMI replay, so it doesn't need to be a valid code address.

Author: BruceForstall
Assignees: BruceForstall
Labels:

area-CodeGen-coreclr

Milestone: -

@BruceForstall BruceForstall requested a review from AndyAyersMS May 12, 2023 00:51
@BruceForstall
Copy link
Contributor Author

@AndyAyersMS PTAL
cc @dotnet/jit-contrib

Copy link
Member

@AndyAyersMS AndyAyersMS left a comment

Choose a reason for hiding this comment

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

Huh, never knew we did anything like this.

Change LGTM.

@BruceForstall BruceForstall merged commit 3bd3832 into dotnet:main May 12, 2023
@BruceForstall BruceForstall deleted the FixSpuriousSpmiDiffs branch May 12, 2023 04:02
@jakobbotsch
Copy link
Member

Fixes #82164

@BruceForstall
Copy link
Contributor Author

Fixes #82164

Guess I'd forgotten about that issue. Your suggestion there sounds better, but this fix was maybe easier -- it just doesn't work for Release.

@jakobbotsch
Copy link
Member

jakobbotsch commented May 12, 2023

Your suggestion there sounds better, but this fix was maybe easier -- it just doesn't work for Release.

Yeah agree this looks way easier. Seems fine since stress is debug-only anyway.

Though we probably should also fix the test that enables "general" JitStress. This was probably exposed by #83771 that has a one-time switch around of what modes JitStress will enable, so it's likely the test is not testing what it means to anymore. (It's also likely it wasn't before, since stress modes have been added/removed before.)

@jakobbotsch
Copy link
Member

jakobbotsch commented May 12, 2023

Looks like there are two tests one test that enables general stress:

  • JIT\opt\OSR\largefuncletframe
  • JIT\Regression\JitBlue\Runtime_64764\Runtime_64764 (nevermind, this one is just in a comment)

Might not be super easy to identify the exact stress modes that reproed the issue originally.

Edit: opened #86185 for this

@ghost ghost locked as resolved and limited conversation to collaborators Jun 12, 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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants