Skip to content

Skip reloc handling for IMAGE_REL_BASED_DIR64 if already handled #105522

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

Merged
merged 1 commit into from
Jul 26, 2024
Merged

Skip reloc handling for IMAGE_REL_BASED_DIR64 if already handled #105522

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions src/coreclr/tools/superpmi/superpmi-shared/compileresult.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -894,8 +894,8 @@ void CompileResult::applyRelocs(RelocContext* rc, unsigned char* block1, ULONG b
}
break;

case IMAGE_REL_ARM64_SECREL_HIGH12A:
case IMAGE_REL_ARM64_SECREL_LOW12A:
case IMAGE_REL_ARM64_SECREL_HIGH12A: // TLSDESC ADD for High-12 Add
case IMAGE_REL_ARM64_SECREL_LOW12A: // TLSDESC ADD for Low-12 Add
case IMAGE_REL_AARCH64_TLSDESC_LD64_LO12:
case IMAGE_REL_AARCH64_TLSDESC_ADD_LO12: // TLSDESC ADD for corresponding ADRP
case IMAGE_REL_AARCH64_TLSDESC_CALL:
Expand All @@ -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))
Copy link
Contributor

@amanasifkhalid amanasifkhalid Jul 26, 2024

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.

Copy link
Contributor Author

@kunalspathak kunalspathak Jul 26, 2024

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).

Copy link
Contributor

@amanasifkhalid amanasifkhalid Jul 26, 2024

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!

{
DWORDLONG fixupLocation = tmp.location;

Expand Down