-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Skip reloc handling for IMAGE_REL_BASED_DIR64 if already handled #105522
Conversation
@dotnet/jit-contrib |
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
/azp run runtime-coreclr superpmi-diffs |
Azure Pipelines could not run because the pipeline triggers exclude this branch/path. |
@@ -918,7 +918,7 @@ void CompileResult::applyRelocs(RelocContext* rc, unsigned char* block1, ULONG b | |||
|
|||
if (IsSpmiTarget64Bit()) | |||
{ | |||
if (relocType == IMAGE_REL_BASED_DIR64) | |||
if (!wasRelocHandled && (relocType == IMAGE_REL_BASED_DIR64)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I might be missing something here, but if relocType == IMAGE_REL_BASED_DIR64
, shouldn't wasRelocHandled
always be false? I don't see this reloc type handled in any of the above switch statements, and those are the only places where wasRelocHandled
can be set to true.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, that's because IMAGE_REL_BASED_DIR64
and IMAGE_REL_ARM64_SECREL_HIGH12A
share the same enum value 0x9
. IMAGE_REL_BASED_DIR64
is used in xarch, while IMAGE_REL_ARM64_SECREL_HIGH12A
is for arm64. So yes, wasRelocHandled
will mostly be false
, except for arm64/IMAGE_REL_ARM64_SECREL_HIGH12A
in which case we should not handle it here. If we do, it just overwrites the codestream leading to "decoding failures". Ideally, this handling should be just for SPMI_TARGET_ARCHITECTURE_X86
and SPMI_TARGET_ARCHITECTURE_AMD64
, but I was not sure if there was any historic reason to do so. Wanted to do minimal change to address the underlying issue for arm64.
See the discussion in #104282 (comment).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it, thank you for explaining this!
Resolves #104584 and resolves #104441