Only apply addlDelta for IMAGE_REL_BASED_REL32 relocs #79627
Conversation
This matches what ngen did, and makes sense since only IP-relative relocations need to addlDelta to determine the address of the next instruction. Fixes dotnet#79170
ghost
added
the
area-CodeGen-coreclr
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch Issue DetailsThis matches what ngen did, and makes sense since only IP-relative relocations need to addlDelta to determine the address of the next instruction. Fixes #79170
|
|
/azp run runtime-coreclr outerloop |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run runtime-coreclr outerloop |
|
Azure Pipelines successfully started running 1 pipeline(s). |
| TargetArchitecture targetArchitecture = _compilation.TypeSystemContext.Target.Architecture; | ||
| RelocType relocType = GetRelocType(targetArchitecture, fRelocType); | ||
|
|
||
| if (relocType == RelocType.IMAGE_REL_BASED_REL32) |
There was a problem hiding this comment.
Would it make more sense for the JIT to pass addlDelta as zero?
One can imagine cases where addlDelta can be useful for other relocation types as well. They may not exist today, but they can show up in future.
There was a problem hiding this comment.
Maybe? That's what #79467 does. I don't have a strong opinion either way.
I was interested to learn when I investigated that ngen ignored addlDelta except for REL32, and I'm generally wary of ways in which the crossgen2 implementation of the JIT/EE interface diverges from the ngen implementation.
There was a problem hiding this comment.
I think #79467 is a better fix.
I would not use ngen or .NET Framework as a gold standard for JIT/EE interface implementation. The JIT/EE interface contract in .NET Framework had a lot of things in it that did not make sense and it was a lot of friction to fix them.
ghost
locked as resolved and limited conversation to collaborators
This matches what ngen did, and makes sense since only IP-relative relocations need to addlDelta to determine the address of the next instruction.
Fixes #79170