This is an archive of the discontinued LLVM Phabricator instance.

[AArch64] Fix the upper limit for folded address offsets for COFF
ClosedPublic

Authored by mstorsjo on Apr 5 2022, 2:49 PM.

Details

Summary

In COFF, the immediates in IMAGE_REL_ARM64_PAGEBASE_REL21 relocations are limited to 21 bit signed, i.e. the offset has to be less than
(1 << 20). The previous limit did intend to cover for this case, but had missed that the 21 bit field was signed.

This fixes issue https://github.com/llvm/llvm-project/issues/54753.

Diff Detail

Unit TestsFailed

Event Timeline

mstorsjo created this revision.Apr 5 2022, 2:49 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 5 2022, 2:49 PM
mstorsjo requested review of this revision.Apr 5 2022, 2:49 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 5 2022, 2:49 PM
efriedma added a subscriber: pcc.Apr 5 2022, 3:32 PM

Please add a testcase.

Do you agree we should make this dependent on the target OS (or more precisely, object file format), or should we just reduce the universal limit and keep it object file format agnostic?

I'd like to keep the limits of specific formats documented, so future readers can understand why the specific number was chosen. I don't really care whether that's code or a comment; it's unlikely to matter much in practice.

I'm not actually sure where "1 << 21" comes from, though; I think ELF is unlimited, and MachO is limited to "1 << 23".

pcc added a comment.Apr 5 2022, 5:06 PM

IIRC 1 << 21 was from COFF (missed that the addend was signed), so it wouldn't make sense to have this be 1 << 21 on other object formats if the actual limit is higher.

I think it's unlikely that people will be hitting this limit in practice, so I'd probably just change this to 20 and document that it comes from COFF. I don't think this needs to be conditional but if it were it should be based on the object format and not the operating system because there may be non-Windows COFF users (UEFI comes to mind).

I'm not actually sure where "1 << 21" comes from, though; I think ELF is unlimited, and MachO is limited to "1 << 23".

IIRC 1 << 21 was from COFF (missed that the addend was signed), so it wouldn't make sense to have this be 1 << 21 on other object formats if the actual limit is higher.

I think it's unlikely that people will be hitting this limit in practice, so I'd probably just change this to 20 and document that it comes from COFF. I don't think this needs to be conditional but if it were it should be based on the object format and not the operating system because there may be non-Windows COFF users (UEFI comes to mind).

Awesome, thanks - that clarifies the existing intent and makes it clearer what kind of test to add for this (just adjusting the existing one).

mstorsjo updated this revision to Diff 420781.Apr 6 2022, 4:21 AM

Updated the patch to only adjust the existing universal limit from 1 << 21 to 1 << 20, and clarified the comment to explain why this limit is chosen. Updated the existing testcase which tests the boundary conditions of this limit.

mstorsjo retitled this revision from [AArch64] Limit folded address offsets to 2^20 for COFF to [AArch64] Fix the upper limit for folded address offsets for COFF.Apr 6 2022, 4:23 AM
mstorsjo edited the summary of this revision. (Show Details)
This revision is now accepted and ready to land.Apr 6 2022, 11:12 AM
This revision was landed with ongoing or failed builds.Apr 6 2022, 12:55 PM
This revision was automatically updated to reflect the committed changes.