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.

Sign up for GitHub

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

Jump to bottom

Always set addlDelta to zero on x86 #79467

Merged
merged 3 commits into from
Dec 14, 2022
Merged

Always set addlDelta to zero on x86 #79467

merged 3 commits into from
Dec 14, 2022

Conversation

SingleAccretion
Copy link
Contributor

@SingleAccretion SingleAccretion commented Dec 9, 2022 •
edited
Loading

As best as I am able to infer from reading the code, this value is only meant to be used for reporting relocations of RIP-relative addresses, which x86 doesn't have.

Fixes #79170.

No diffs are expected.

@SingleAccretion
Always set "addlDelta" to zero on x86
09260e6 
The value is used to compensate for the additional instruction bytes,
which should only be relevant for RIP-relative addressing, while x86
uses absolute addressing.
@SingleAccretion
Re-enable the test
32a1486
@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 Dec 9, 2022
@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Dec 9, 2022
@ghost
Copy link

ghost commented Dec 9, 2022

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

Issue Details

As best as I am able to infer from reading the code, this value is meant to be used for reporting relocations of RIP-relative addresses, which x86 doesn't have.

Fixes #79170.

No diffs are expected.

Author: SingleAccretion
Assignees: -
Labels:

area-CodeGen-coreclr
Milestone: -

@build-analysis build-analysis bot mentioned this pull request Dec 10, 2022
Closed
@SingleAccretion
Copy link
Contributor Author

SingleAccretion commented Dec 10, 2022 •
edited
Loading

Failure is #79439.

@dotnet/jit-contrib

@SingleAccretion SingleAccretion marked this pull request as ready for review December 10, 2022 13:04
@BruceForstall
Copy link
Member

This seems like a reasonable fix, but I wonder if we should consider this a crossgen2 bug.

I wrote a fix there that seems to work: #79627

This matches the behavior that the old ngen tool had: ignore addlDelta except for REL32 relocs, and the IMAGE_REL_BASED_DISP32 "pseudo-reloc" used here only maps to REL32 on x64; it maps to HIGHLOW on x86.

@BruceForstall BruceForstall mentioned this pull request Dec 14, 2022
Closed
@BruceForstall BruceForstall merged commit ef903fd into dotnet:main Dec 14, 2022
@SingleAccretion
Copy link
Contributor Author

I wonder if we should consider this a crossgen2 bug

Indeed, that was the main question here. What made me prefer a fix in the Jit was that it is easier to understand: there is no computation of data on one side where the other is expected to explicitly ignore it for things to work properly.

@SingleAccretion SingleAccretion deleted the Fixing-Reloc-Deltas-Upstream branch December 15, 2022 17:10
@ghost ghost locked as resolved and limited conversation to collaborators Jan 14, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@BruceForstall BruceForstall BruceForstall approved these changes

Assignees
No one assigned
Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

JIT.Regression\JitBlue\Runtime_31615\Runtime_31615\Runtime_31615 fails
2 participants
@SingleAccretion @BruceForstall