Prevent out of range fixup encoding on AArch64

[dhoekwater]

Summary

The range of a 21-bit signed integer is [-1048576, 1048575],
not [-2097152, 2097151].

[efriedma]

LGTM

[MaskRay]

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

9

.space 1<<20 is 1MiB (acceptable, but best not to introduce too many tests like this) and .space (1<<27) below needs 128MiB. The large object file makes test slow and won't work on machines with limited disk space. I'd like to hear from other reviewers.

[efriedma]

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

9

I get the concern in general, but I don't think this test actually does anything expensive? It should error out before the point we actually try to expand ".space" to raw bits.

[efriedma]

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

9

Oh, hmm, I guess we don't actually abort emitting the object file even if there's an error. So that could get a bit expensive. Only in terms of time, though; it doesn't actually seem to use much memory. (And disk space is irrelevant if we're outputting to /dev/null.)

[MaskRay]

This is still going to be a bit slow. I think we should use the following scheme to minimize the number of large .space as well as testing the real range.

  FileCheck ... --implicit-check-not=error:

  .p2align 2
# CHECK: [[#@LINE+1]]:3: error: fixup value out of range
  adr x0, adr1-8
  adr x0, adr1-8
adr0:
  .space 1<<20
adr1:
  adr x0, adr0
# CHECK: [[#@LINE+1]]:3: error: fixup value out of range
  adr x0, adr0

[efriedma]

Maybe we could add a shortcut to allow testing this sort of thing without trying to pump megabytes of data into the ostream, so we don't have to worry about the compile-time. Might be sufficient to just add a codepath to llvm-mc to force all the output to a raw_null_ostream.

Funny side-note, while experimenting, I found that we don't emit an error if the distance is greater than 4GB. Probably accidentally truncating an offset somewhere.

[peter.smith]

Code changes LGTM, if you're aiming for completeness, a test for the pc-relative ldr would also be good.

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

61

The fixup fixup_aarch64_ldr_pcrel_imm19 is also affected. May be worth a test case for that as well. I think that would look like

ldr x0, ldr_low_range

[dhoekwater]

In D152841#4419129, @efriedma wrote:

Funny side-note, while experimenting, I found that we don't emit an error if the distance is greater than 4GB. Probably accidentally truncating an offset somewhere.

I actually did consider adding a check to ADRP to ensure the distance isn't greater than 4GB (because it only has 21 offset bits to encode the page), but I couldn't quite figure out how to expose that behavior here. Should I add the check? If so, what triple do I have to use to get this function to be called for ADRP fixups?

In In D152841#4419087, @MaskRay wrote:

adr x0, adr1-8

I didn't realize that adding an offset to the destination label like that was valid assembly. In that case, let's not emit any .spaces at all and just use custom offsets. I'll push an updated change in a little bit.

# CHECK: [[#@LINE+1]]:3: error: fixup value out of range
adr_self:
  adr x0, adr_self-(1<<20)-1

[dhoekwater]

Switch to use manual offsets to reduce runtime

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

9

Replaced with manual offsets, so we don't need to emit any space at all.

61

Done.

[efriedma]

In D152841#4423102, @dhoekwater wrote:

In D152841#4419129, @efriedma wrote:

Funny side-note, while experimenting, I found that we don't emit an error if the distance is greater than 4GB. Probably accidentally truncating an offset somewhere.

I actually did consider adding a check to ADRP to ensure the distance isn't greater than 4GB (because it only has 21 offset bits to encode the page), but I couldn't quite figure out how to expose that behavior here. Should I add the check? If so, what triple do I have to use to get this function to be called for ADRP fixups?

I saw the 4GB issue with other relocations. Actually, I didn't even try adrp. I don't think we ever use this codepath for adrp.

[dhoekwater]

In D152841#4423980, @efriedma wrote:

I saw the 4GB issue with other relocations. Actually, I didn't even try adrp. I don't think we ever use this codepath for adrp.

Oh, interesting! I can't recreate that behavior by manually setting large offsets in this test. What assembly did you use to expose that?

We do use this codepath for adrp, but I guess only on Windows. case AArch64::fixup_aarch64_pcrel_adrp_imm21 handles the fixup, and it's tested in llvm/test/MC/AArch64/fixup-out-of-range.s.

[dhoekwater]

Rebase onto origin/main

[dhoekwater]

Add a newline at the end of the file, for my sanity

[efriedma]

In D152841#4426366, @dhoekwater wrote:

In D152841#4423980, @efriedma wrote:

I saw the 4GB issue with other relocations. Actually, I didn't even try adrp. I don't think we ever use this codepath for adrp.

Oh, interesting! I can't recreate that behavior by manually setting large offsets in this test. What assembly did you use to expose that?

printf ".Lfoo:\n.space (1<<32)\ntbz x0, #1, .Lfoo\n" | llvm-mc -triple=aarch64 -filetype=obj -o  /dev/null

We do use this codepath for adrp, but I guess only on Windows. case AArch64::fixup_aarch64_pcrel_adrp_imm21 handles the fixup, and it's tested in llvm/test/MC/AArch64/fixup-out-of-range.s.

Oh, right; in that case, the offset isn't the offset relative to the instruction; it's the offset relative to the specified symbol, which needs to be encoded in a 21-bit field. On ELF, we can encode arbitrary offsets (it'll just fail to link if the resolved address isn't close enough).

[dhoekwater]

In D152841#4426615, @efriedma wrote:

printf ".Lfoo:\n.space (1<<32)\ntbz x0, #1, .Lfoo\n" | llvm-mc -triple=aarch64 -filetype=obj -o /dev/null

This looks like it's caused by // uint32_t Offset = Layout.getFragmentOffset(DF) + Fixup.getOffset(); in MCAssembler.cpp:276, which should be replaced with a uint64_t. I can't find a good way to test it without writing .space 1 << 32, which isn't preferable for the reasons listed above. I'm actually not sure where it should be fixed/tested, as it's not AArch64-specific.

[efriedma]

I'm actually not sure where it should be fixed/tested, as it's not AArch64-specific.

MC-level tests are all target-dependent to some extent, so all tests specify some target. We don't have a way to specify "any ELF target". Given that, just pick a target and write a test.

llvm/lib/Target/AArch64/MCTargetDesc/AArch64AsmBackend.cpp

 #include "llvm/MC/MCRegisterInfo.h"
 #include "llvm/MC/MCSectionELF.h"
 #include "llvm/MC/MCSectionMachO.h"
 #include "llvm/MC/MCSubtargetInfo.h"
 #include "llvm/MC/MCTargetOptions.h"
 #include "llvm/MC/MCValue.h"
 #include "llvm/MC/TargetRegistry.h"
 #include "llvm/Support/ErrorHandling.h"
+#include "llvm/Support/MathExtras.h"
 #include "llvm/TargetParser/Triple.h"
 using namespace llvm;
 
 namespace {
 
 class AArch64AsmBackend : public MCAsmBackend {
   static const unsigned PCRelFlagVal =
       MCFixupKindInfo::FKF_IsAlignedDownTo32Bits | MCFixupKindInfo::FKF_IsPCRel;

 static uint64_t adjustFixupValue(const MCFixup &Fixup, const MCValue &Target,
                                  uint64_t Value, MCContext &Ctx,
                                  const Triple &TheTriple, bool IsResolved) {
   int64_t SignedValue = static_cast<int64_t>(Value);
   switch (Fixup.getTargetKind()) {
   default:
     llvm_unreachable("Unknown fixup kind!");
   case AArch64::fixup_aarch64_pcrel_adr_imm21:
-    if (SignedValue > 2097151 || SignedValue < -2097152)
+    if (!isInt<21>(SignedValue))
       Ctx.reportError(Fixup.getLoc(), "fixup value out of range");
     return AdrImmBits(Value & 0x1fffffULL);
   case AArch64::fixup_aarch64_pcrel_adrp_imm21:
     assert(!IsResolved);
     if (TheTriple.isOSBinFormatCOFF()) {
       if (!isInt<21>(SignedValue))
         Ctx.reportError(Fixup.getLoc(), "fixup value out of range");
       return AdrImmBits(Value & 0x1fffffULL);
     }
     return AdrImmBits((Value & 0x1fffff000ULL) >> 12);
   case AArch64::fixup_aarch64_ldr_pcrel_imm19:
   case AArch64::fixup_aarch64_pcrel_branch19:
-    // Signed 21-bit immediate
-    if (SignedValue > 2097151 || SignedValue < -2097152)
+    // Signed 19-bit immediate which gets multiplied by 4
+    if (!isInt<21>(SignedValue))
       Ctx.reportError(Fixup.getLoc(), "fixup value out of range");
     if (Value & 0x3)
       Ctx.reportError(Fixup.getLoc(), "fixup not sufficiently aligned");
     // Low two bits are not encoded.
     return (Value >> 2) & 0x7ffff;
   case AArch64::fixup_aarch64_add_imm12:
   case AArch64::fixup_aarch64_ldst_imm12_scale1:
     if (TheTriple.isOSBinFormatCOFF() && !IsResolved)

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

+// RUN: not llvm-mc -triple aarch64--none-eabi -filetype obj < %s -o /dev/null 2>&1 | FileCheck %s
+
+// COM: Edge case testing for branches and ADR[P]
+// CHECK-LABEL: :{{[0-9]+}}:{{[0-9]+}}: error: fixup value out of range
+// CHECK-NEXT:  adr x0, adr_lower_range
+  .p2align 2
+adr_lower_range:
+  .word 0
+  .space 1<<20
+  adr x0, adr_lower_range
+
+// CHECK-LABEL: :{{[0-9]+}}:{{[0-9]+}}: error: fixup value out of range
+// CHECK-NEXT:  adr x0, adr_upper_range
+  .p2align 2
+  adr x0, adr_upper_range
+  .space (1<<20)-4
+adr_upper_range:
+  .word 0
+
+// CHECK-LABEL: :{{[0-9]+}}:{{[0-9]+}}: error: fixup value out of range
+// CHECK-NEXT:  b b_lower_range
+  .p2align 2
+b_lower_range:
+  .word 0
+  .space (1<<27)
+  b b_lower_range
+
+// CHECK-LABEL: :{{[0-9]+}}:{{[0-9]+}}: error: fixup value out of range
+// CHECK-NEXT:  b b_upper_range
+  .p2align 2
+  b b_upper_range
+  .space (1<<27)-4
+  .p2align 2
+b_upper_range:
+  .word 0
+
+// CHECK-LABEL: :{{[0-9]+}}:{{[0-9]+}}: error: fixup value out of range
+// CHECK-NEXT:  beq beq_lower_range
+  .p2align 2
+beq_lower_range:
+  .word 0
+  .space (1<<20)
+  beq beq_lower_range
+  .p2align 2
+
+// CHECK-LABEL: :{{[0-9]+}}:{{[0-9]+}}: error: fixup value out of range
+// CHECK-NEXT:  beq beq_upper_range
+  beq beq_upper_range
+  .space (1<<20)-4
+  .p2align 2
+beq_upper_range:
+  .word 0
+
+// CHECK-LABEL: :{{[0-9]+}}:{{[0-9]+}}: error: fixup value out of range
+// CHECK-NEXT:  tbz x0, #1, tbz_lower_range
+  .p2align 2
+tbz_lower_range:
+  .word 0
+  .space (1<<15)
+  tbz x0, #1, tbz_lower_range
+
+// CHECK-LABEL: :{{[0-9]+}}:{{[0-9]+}}: error: fixup value out of range
+// CHECK-NEXT:  tbz x0, #1, tbz_upper_range
+  .p2align 2
+  tbz x0, #1, tbz_upper_range
+  .space (1<<15)-4
+  .p2align 2
+tbz_upper_range:
+  .word 0