[MC] Detect out of range jumps further than 2^32 bytes

[dhoekwater]

Summary

On AArch64, object files may be greater than 2^32 bytes. If an
offset is greater than the max value of a 32-bit unsigned integer,
LLVM silently truncates the offset. Instead, make it return an
error.

[dhoekwater]

There was some discussion on https://reviews.llvm.org/D152841 about tests that have lots of `.space` in them, which will probably be relevant here.

[MaskRay]

fixup-out-of-range-64-bit.s may not be suitable as

• The test fails even without the patch.
• .space 1<<32 is very slow for MCELFStreamer. I am unsure whether we want to add a special case for -filetype=obj when -o /dev/null.

If we a test, we can craft a unittest in llvm/unittests/MC/, this may however involve a lot of boilerplate and make future refactoring of evaluateFixup difficult...

I think just fixing the code without a test is fine, but am happy to hear from others.

llvm/test/MC/AArch64/fixup-out-of-range-64-bit.s

9 ↗

(On Diff #533464)

[[@LINE+1]] is deprecated lit syntax. Use [[#@LINE+1]].

[peter.smith]

Although not specifically part of this patch, for branch relocations, exposing these to the linker like we do in AArch32 would permit branches that can reach outside the section to be range-extended by thunks. For example in the test case above distant could be fixed up at link time.

Another not specifically part of this patch, is that for AArch64 we should probably give an error message when the section size gets bigger than 4 GiB as if we are resolving branches internally and not passing them on to the linker, that is the largest we can support without giving an obtuse error message. We have seen this cause trouble with GCC with an internal code-generator that generated a single gigantic .C file, without -ffunction-sections it produced a giant 4 GiB section.

I'm inclined to agree with MaskRay about not requiring a test. We do have some 32-bit systems such as Arm AArch32 that have a build bot, I'm not sure whether they would be able to cope with this test case and I'm not sure we can turn off tests on a host rather than target basis.

[dhoekwater]

Sounds good, I'll remove the test. Testing this felt kind of gross, but I wasn't sure how strict the "any behavior-modifying change requires a test" philosophy is within LLVM.

In D153494#4440142, @peter.smith wrote:

1. Exposing these to the linker like we do in AArch32 would permit branches that can reach outside the section to be range-extended by thunks.

2. Another not specifically part of this patch, is that for AArch64 we should probably give an error message when the section size gets bigger than 4 GiB as if we are resolving branches internally and not passing them on to the linker, that is the largest we can support without giving an obtuse error message.

1. Yeah, agreed. Emitting a relocation and letting the linker handle out-of-range branches should be intended behavior. One thing to keep in mind is that lld currently doesn't handle offsets greater than 4GiB for position-independent code.

2. We shouldn't need to error out on a 4GiB+ section, right? If we're resolving branches internally (as is done in the BranchRelaxation pass when compiling C++ code), we should generate branches that are either in-range or emit relocations and can be fixed up by the linker. If we're just validating branch offsets, as is done when compiling AArch64 assembly, our current error handling should produce reasonable error output.

[MaskRay]

In D153494#4442086, @dhoekwater wrote:

Sounds good, I'll remove the test. Testing this felt kind of gross, but I wasn't sure how strict the "any behavior-modifying change requires a test" philosophy is within LLVM.

In D153494#4440142, @peter.smith wrote:

1. Exposing these to the linker like we do in AArch32 would permit branches that can reach outside the section to be range-extended by thunks.

2. Another not specifically part of this patch, is that for AArch64 we should probably give an error message when the section size gets bigger than 4 GiB as if we are resolving branches internally and not passing them on to the linker, that is the largest we can support without giving an obtuse error message.

1. Yeah, agreed. Emitting a relocation and letting the linker handle out-of-range branches should be intended behavior. One thing to keep in mind is that lld currently doesn't handle offsets greater than 4GiB for position-independent code.

This limit should be fine. https://maskray.me/blog/2023-05-14-relocation-overflow-and-code-models#aarch64-code-models

"This larger range makes it unlikely for AArch64 to encounter relocation overflow issues before the binary becomes excessively oversized for x86-64."

2. We shouldn't need to error out on a 4GiB+ section, right? If we're resolving branches internally (as is done in the BranchRelaxation pass when compiling C++ code), we should generate branches that are either in-range or emit relocations and can be fixed up by the linker. If we're just validating branch offsets, as is done when compiling AArch64 assembly, our current error handling should produce reasonable error output.

I think we do not need arbitrary restricting what we can emit in the assembly output. It's difficult to get right/test and likely not useful in practice.

" Instead, make it return an error." this sentence should be removed. Ideally add [MC] to the title of this patch.

[dhoekwater]

Add tag to commit message

llvm/lib/MC/MCAssembler.cpp

bool MCAssembler::evaluateFixup(const MCAsmLayout &Layout,
   }
 
   bool ShouldAlignPC = getBackend().getFixupKindInfo(Fixup.getKind()).Flags &
                        MCFixupKindInfo::FKF_IsAlignedDownTo32Bits;
   assert((ShouldAlignPC ? IsPCRel : true) &&
     "FKF_IsAlignedDownTo32Bits is only allowed on PC-relative fixups!");
 
   if (IsPCRel) {
-    uint32_t Offset = Layout.getFragmentOffset(DF) + Fixup.getOffset();
+    uint64_t Offset = Layout.getFragmentOffset(DF) + Fixup.getOffset();
 
     // A number of ARM fixups in Thumb mode require that the effective PC
     // address be determined as the 32-bit aligned version of the actual offset.
     if (ShouldAlignPC) Offset &= ~0x3;
     Value -= Offset;
   }
 
   // Let the backend force a relocation if needed.