[AArch64] Better errors for out-of-range fixups

[olista01]

Summary

When a fixup that can be resolved by the assembler is out of range, we should report an error in the source, rather than crashing.

[rengolin]

lib/Target/AArch64/MCTargetDesc/AArch64AsmBackend.cpp

271 ↗

(On Diff #51434)

As it is, in through this call, none of the errors will be caught or reported.

[olista01]

adjustFixupValue will always be called twice, first from processFixupValue and then from applyFixup. The first one will report the errors (because the MCContext is available there), so the second one does not have to. The ARM version of this code works the exact same way.

[rengolin]

Sorry, this fell out of my radar. How can you guarantee the fixup and the value are the exact same? What if some process (even if later on), does change it?

I think if the ARM one works in the same way, then both are wrong...

I'll defer to Tim and James to accept this, as I'm not comfortable with the idea.

Reporting an error is better than crashing, but crashing is better than silent codegen errors.

cheers,
--renato

[t.p.northover]

I think this is a good idea. MachO has been doing similar things for a while and I don't think we've had any complaints about misplaced diagnostics. We certainly used to get complaints about ones with missing locations though!

Tim.

[asb]

Hi all. Sorry for digging up an old review, but I've been looking at reporting fixup overflow errors for RISC-V, and this seems the most appropriate place to discuss how it's currently handled in AArch64. I agree with @rengolin's concerns with this approach - or at least, I think we can do this better. Right now, adjustFixupValue is always called twice. The second time it's called, the MCContext argument is null and the code relies on the fact that any errors should have been caught the first time round, and so this null pointer will never be dereferenced [EDIT: sorry, that's not true - it does consistently check that Ctx is non-null]. I like having error checking alongside the value adjustments, but I propose that instead we achieve this by:

• Adding an MCContext argument to MCAsmBackend::applyFixup, and document that this function should report any errors in applying the fixup (should as the value being out of range).. This just requires a simple update to MCAssembler.cpp so it passes the context argument, and of course modifying FootgtAsmBackend for all targets to reflect the new argument
• For backends that report errors using adjustFixupValue, stop calling it from processFixupValue and instead just modify the applyFixup implementation to pass an MCContext rather than a nullptr

I just thought I'd quickly check in case I'm missing something obviously wrong with this approach.

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

 //===-- AArch64AsmBackend.cpp - AArch64 Assembler Backend -----------------===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #include "AArch64.h"
 #include "AArch64RegisterInfo.h"
 #include "MCTargetDesc/AArch64FixupKinds.h"
 #include "llvm/ADT/Triple.h"
 #include "llvm/MC/MCAsmBackend.h"
+#include "llvm/MC/MCContext.h"
 #include "llvm/MC/MCDirectives.h"
 #include "llvm/MC/MCELFObjectWriter.h"
 #include "llvm/MC/MCFixupKindInfo.h"
 #include "llvm/MC/MCObjectWriter.h"
 #include "llvm/MC/MCSectionELF.h"
 #include "llvm/MC/MCSectionMachO.h"
 #include "llvm/MC/MCValue.h"
 #include "llvm/Support/ErrorHandling.h"

 }
 
 static unsigned AdrImmBits(unsigned Value) {
   unsigned lo2 = Value & 0x3;
   unsigned hi19 = (Value & 0x1ffffc) >> 2;
   return (hi19 << 5) | (lo2 << 29);
 }
 
-static uint64_t adjustFixupValue(unsigned Kind, uint64_t Value) {
+static uint64_t adjustFixupValue(const MCFixup &Fixup, uint64_t Value,
+                                 MCContext *Ctx) {
+  unsigned Kind = Fixup.getKind();
   int64_t SignedValue = static_cast<int64_t>(Value);
   switch (Kind) {
   default:
     llvm_unreachable("Unknown fixup kind!");
   case AArch64::fixup_aarch64_pcrel_adr_imm21:
-    if (SignedValue > 2097151 || SignedValue < -2097152)
-      report_fatal_error("fixup value out of range");
+    if (Ctx && (SignedValue > 2097151 || SignedValue < -2097152))
+      Ctx->reportError(Fixup.getLoc(), "fixup value out of range");
     return AdrImmBits(Value & 0x1fffffULL);
   case AArch64::fixup_aarch64_pcrel_adrp_imm21:
     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)
-      report_fatal_error("fixup value out of range");
+      if (Ctx) Ctx->reportError(Fixup.getLoc(), "fixup value out of range");
+    if (Ctx && (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:
     // Unsigned 12-bit immediate
-    if (Value >= 0x1000)
-      report_fatal_error("invalid imm12 fixup value");
+    if (Ctx && Value >= 0x1000)
+      Ctx->reportError(Fixup.getLoc(), "fixup value out of range");
     return Value;
   case AArch64::fixup_aarch64_ldst_imm12_scale2:
     // Unsigned 12-bit immediate which gets multiplied by 2
-    if (Value & 1 || Value >= 0x2000)
-      report_fatal_error("invalid imm12 fixup value");
+    if (Ctx && (Value >= 0x2000))
+      Ctx->reportError(Fixup.getLoc(), "fixup value out of range");
+    if (Ctx && (Value & 0x1))
+      Ctx->reportError(Fixup.getLoc(), "fixup must be 2-byte aligned");
     return Value >> 1;
   case AArch64::fixup_aarch64_ldst_imm12_scale4:
     // Unsigned 12-bit immediate which gets multiplied by 4
-    if (Value & 3 || Value >= 0x4000)
-      report_fatal_error("invalid imm12 fixup value");
+    if (Ctx && (Value >= 0x4000))
+      Ctx->reportError(Fixup.getLoc(), "fixup value out of range");
+    if (Ctx && (Value & 0x3))
+      Ctx->reportError(Fixup.getLoc(), "fixup must be 4-byte aligned");
     return Value >> 2;
   case AArch64::fixup_aarch64_ldst_imm12_scale8:
     // Unsigned 12-bit immediate which gets multiplied by 8
-    if (Value & 7 || Value >= 0x8000)
-      report_fatal_error("invalid imm12 fixup value");
+    if (Ctx && (Value >= 0x8000))
+      Ctx->reportError(Fixup.getLoc(), "fixup value out of range");
+    if (Ctx && (Value & 0x7))
+      Ctx->reportError(Fixup.getLoc(), "fixup must be 8-byte aligned");
     return Value >> 3;
   case AArch64::fixup_aarch64_ldst_imm12_scale16:
     // Unsigned 12-bit immediate which gets multiplied by 16
-    if (Value & 15 || Value >= 0x10000)
-      report_fatal_error("invalid imm12 fixup value");
+    if (Ctx && (Value >= 0x10000))
+      Ctx->reportError(Fixup.getLoc(), "fixup value out of range");
+    if (Ctx && (Value & 0xf))
+      Ctx->reportError(Fixup.getLoc(), "fixup must be 16-byte aligned");
     return Value >> 4;
   case AArch64::fixup_aarch64_movw:
-    report_fatal_error("no resolvable MOVZ/MOVK fixups supported yet");
+    if (Ctx)
+      Ctx->reportError(Fixup.getLoc(),
+                       "no resolvable MOVZ/MOVK fixups supported yet");
     return Value;
   case AArch64::fixup_aarch64_pcrel_branch14:
     // Signed 16-bit immediate
-    if (SignedValue > 32767 || SignedValue < -32768)
-      report_fatal_error("fixup value out of range");
+    if (Ctx && (SignedValue > 32767 || SignedValue < -32768))
+      Ctx->reportError(Fixup.getLoc(), "fixup value out of range");
     // Low two bits are not encoded (4-byte alignment assumed).
-    if (Value & 0x3)
-      report_fatal_error("fixup not sufficiently aligned");
+    if (Ctx && (Value & 0x3))
+      Ctx->reportError(Fixup.getLoc(), "fixup not sufficiently aligned");
     return (Value >> 2) & 0x3fff;
   case AArch64::fixup_aarch64_pcrel_branch26:
   case AArch64::fixup_aarch64_pcrel_call26:
     // Signed 28-bit immediate
-    if (SignedValue > 134217727 || SignedValue < -134217728)
-      report_fatal_error("fixup value out of range");
+    if (Ctx && (SignedValue > 134217727 || SignedValue < -134217728))
+      Ctx->reportError(Fixup.getLoc(), "fixup value out of range");
     // Low two bits are not encoded (4-byte alignment assumed).
-    if (Value & 0x3)
-      report_fatal_error("fixup not sufficiently aligned");
+    if (Ctx && (Value & 0x3))
+      Ctx->reportError(Fixup.getLoc(), "fixup not sufficiently aligned");
     return (Value >> 2) & 0x3ffffff;
   case FK_Data_1:
   case FK_Data_2:
   case FK_Data_4:
   case FK_Data_8:
     return Value;
   }
 }

 void AArch64AsmBackend::applyFixup(const MCFixup &Fixup, char *Data,
                                    unsigned DataSize, uint64_t Value,
                                    bool IsPCRel) const {
   unsigned NumBytes = getFixupKindNumBytes(Fixup.getKind());
   if (!Value)
     return; // Doesn't change encoding.
   MCFixupKindInfo Info = getFixupKindInfo(Fixup.getKind());
   // Apply any target-specific value adjustments.
-  Value = adjustFixupValue(Fixup.getKind(), Value);
+  Value = adjustFixupValue(Fixup, Value, nullptr);
 
   // Shift the value into position.
   Value <<= Info.TargetOffset;
 
   unsigned Offset = Fixup.getOffset();
   assert(Offset + NumBytes <= DataSize && "Invalid fixup offset!");
 
   // Used to point to big endian bytes.
void ELFAArch64AsmBackend::processFixupValue(
   //
   // If the ADRP occurs at address 0xffc then "there" will be at 0x1000 and
   // we'll need that as an offset. At any other address "there" will be in the
   // same page as the ADRP and the instruction should encode 0x0. Assuming the
   // section isn't 0x1000-aligned, we therefore need to delegate this decision
   // to the linker -- a relocation!
   if ((uint32_t)Fixup.getKind() == AArch64::fixup_aarch64_pcrel_adrp_imm21)
     IsResolved = false;
+
+  // Try to get the encoded value for the fixup as-if we're mapping it into
+  // the instruction. This allows adjustFixupValue() to issue a diagnostic
+  // if the value is invalid.
+  if (IsResolved)
+    (void)adjustFixupValue(Fixup, Value, &Asm.getContext());
 }
 
 }
 
 MCAsmBackend *llvm::createAArch64leAsmBackend(const Target &T,
                                               const MCRegisterInfo &MRI,
                                               const Triple &TheTriple,
                                               StringRef CPU) {

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

+// RUN: not llvm-mc -triple aarch64--none-eabi -filetype obj < %s -o /dev/null 2>&1 | FileCheck %s
+
+// CHECK: :[[@LINE+1]]:{{[0-9]+}}: error: fixup value out of range
+  adr x0, distant
+
+// CHECK: :[[@LINE+1]]:{{[0-9]+}}: error: fixup value out of range
+  ldr x0, distant
+
+// CHECK: :[[@LINE+1]]:{{[0-9]+}}: error: fixup not sufficiently aligned
+  ldr x0, unaligned
+
+// CHECK: :[[@LINE+1]]:{{[0-9]+}}: error: fixup value out of range
+  b.eq distant
+
+// CHECK: :[[@LINE+1]]:{{[0-9]+}}: error: fixup not sufficiently aligned
+  b.eq unaligned
+
+// CHECK: :[[@LINE+1]]:{{[0-9]+}}: error: fixup value out of range
+  ldr x0, [x1, distant-.]
+
+// CHECK: :[[@LINE+1]]:{{[0-9]+}}: error: fixup must be 8-byte aligned
+  ldr x0, [x1, unaligned-.]
+
+// CHECK: :[[@LINE+1]]:{{[0-9]+}}: error: fixup value out of range
+  ldr w0, [x1, distant-.]
+
+// CHECK: :[[@LINE+1]]:{{[0-9]+}}: error: fixup must be 4-byte aligned
+  ldr w0, [x1, unaligned-.]
+
+// CHECK: :[[@LINE+1]]:{{[0-9]+}}: error: fixup value out of range
+  ldrh w0, [x1, distant-.]
+
+// CHECK: :[[@LINE+1]]:{{[0-9]+}}: error: fixup must be 2-byte aligned
+  ldrh w0, [x1, unaligned-.]
+
+// CHECK: :[[@LINE+1]]:{{[0-9]+}}: error: fixup value out of range
+  ldrb w0, [x1, distant-.]
+
+// CHECK: :[[@LINE+1]]:{{[0-9]+}}: error: fixup value out of range
+  ldr q0, [x1, distant-.]
+
+// CHECK: :[[@LINE+1]]:{{[0-9]+}}: error: fixup must be 16-byte aligned
+  ldr q0, [x1, unaligned-.]
+
+// CHECK: :[[@LINE+1]]:{{[0-9]+}}: error: fixup value out of range
+  tbz x0, #1, distant
+
+// CHECK: :[[@LINE+1]]:{{[0-9]+}}: error: fixup not sufficiently aligned
+  tbz x0, #1, unaligned
+
+// CHECK: :[[@LINE+1]]:{{[0-9]+}}: error: fixup value out of range
+  b distant
+
+// CHECK: :[[@LINE+1]]:{{[0-9]+}}: error: fixup not sufficiently aligned
+  b unaligned
+
+  .byte 0
+unaligned:
+  .byte 0
+
+  .space 1<<27
+  .balign 8
+distant:
+  .word 0

llvm/trunk/test/MC/AArch64/ldr-pseudo-obj-errors.s

 //RUN: not llvm-mc -triple=aarch64-linux -filetype=obj %s -o %t1 2> %t2
 //RUN: cat %t2 | FileCheck %s
 
 //These tests look for errors that should be reported for invalid object layout
 //with the ldr pseudo. They are tested separately from parse errors because they
 //only trigger when the file has successfully parsed and the object file is about
 //to be written out.
 
 .text
 foo:
+// CHECK: :[[@LINE+1]]:{{[0-9]+}}: error: fixup value out of range
   ldr x0, =0x10111
   .space 0xdeadb0
-// CHECK: LVM ERROR: fixup value out of range