[X86] Check if signed value is too large for fixup under some conditions

[Heath123]

Fixes #116899. Consistent with other architectures, we don't perform any additional range checks (other than the existing assert) on generic fixup types, as this will break anything that emits a signed value for a generic fixup type, for example (as can happen when setting a symbol to a negative value). Instead, we only check architecture-specific fixup types, which in the case of x86 are always signed. This is slightly different to the previous logic as it also uses the fixup type to determine whether to perform the check, rather than just the PC-relative flag.

[llvmbot]

@llvm/pr-subscribers-backend-x86

@llvm/pr-subscribers-mc

Author: circuit10 (Heath123)

Changes

Fixes #116899. Consistent with other architectures, we don't perform any additional range checks (other than the existing assert) on generic fixup types, as this will break anything that emits a signed value for a generic fixup type, for example (as can happen when setting a symbol to a negative value). Instead, we only check architecture-specific fixup types, which in the case of x86 are always signed. This is slightly different to the previous logic as it also uses the fixup type to determine whether to perform the check, rather than just the PC-relative flag.

---

Full diff: https://github.com/llvm/llvm-project/pull/150976.diff

2 Files Affected:

• (modified) llvm/lib/Target/X86/MCTargetDesc/X86AsmBackend.cpp (+45-3)
• (added) llvm/test/MC/X86/fixup-range-check.s (+8)

diff --git a/llvm/lib/Target/X86/MCTargetDesc/X86AsmBackend.cpp b/llvm/lib/Target/X86/MCTargetDesc/X86AsmBackend.cpp
index e213923ccf38e..bb0722351c4dd 100644
--- a/llvm/lib/Target/X86/MCTargetDesc/X86AsmBackend.cpp
+++ b/llvm/lib/Target/X86/MCTargetDesc/X86AsmBackend.cpp
@@ -21,6 +21,7 @@
 #include "llvm/MC/MCELFObjectWriter.h"
 #include "llvm/MC/MCELFStreamer.h"
 #include "llvm/MC/MCExpr.h"
+#include "llvm/MC/MCFixup.h"
 #include "llvm/MC/MCInst.h"
 #include "llvm/MC/MCInstrInfo.h"
 #include "llvm/MC/MCObjectStreamer.h"
@@ -721,15 +722,56 @@ void X86AsmBackend::applyFixup(const MCFragment &F, const MCFixup &Fixup,
   assert(Fixup.getOffset() + Size <= Data.size() && "Invalid fixup offset!");
 
   int64_t SignedValue = static_cast<int64_t>(Value);
-  if (IsResolved && Fixup.isPCRel()) {
-    // check that PC relative fixup fits into the fixup size.
+
+  // If the fixup is resolved to an absolute value, and is explictly signed
+  // (either by its type or by being marked as PC-relative), we should check
+  // that it fits.
+  // Otherwise, be conservative because some users may rely on overflow, or
+  // being able to use a signed value where an unsigned value would normally be
+  // expected, so strict checks may break things.
+  bool CheckSignedVal = false;
+  if (IsResolved) {
+    switch (Fixup.getKind()) {
+    default:
+      llvm_unreachable("invalid fixup kind!");
+    case FK_NONE:
+    case FK_Data_1:
+    case FK_Data_2:
+    case FK_Data_4:
+    case FK_Data_8:
+    case FK_SecRel_1:
+    case FK_SecRel_2:
+    case FK_SecRel_4:
+    case FK_SecRel_8:
+      CheckSignedVal = false;
+      break;
+    case X86::reloc_riprel_4byte:
+    case X86::reloc_riprel_4byte_relax:
+    case X86::reloc_riprel_4byte_relax_rex:
+    case X86::reloc_riprel_4byte_relax_rex2:
+    case X86::reloc_riprel_4byte_movq_load:
+    case X86::reloc_riprel_4byte_movq_load_rex2:
+    case X86::reloc_riprel_4byte_relax_evex:
+    case X86::reloc_signed_4byte:
+    case X86::reloc_signed_4byte_relax:
+    case X86::reloc_global_offset_table:
+    case X86::reloc_branch_4byte_pcrel:
+      CheckSignedVal = true;
+      break;
+    }
+
+    if (Fixup.isPCRel())
+      CheckSignedVal = true;
+  }
+
+  if (CheckSignedVal) {
     if (Size > 0 && !isIntN(Size * 8, SignedValue))
       getContext().reportError(Fixup.getLoc(),
                                "value of " + Twine(SignedValue) +
                                    " is too large for field of " + Twine(Size) +
                                    ((Size == 1) ? " byte." : " bytes."));
   } else {
-    // Check that uppper bits are either all zeros or all ones.
+    // Check that upper bits are either all zeros or all ones.
     // Specifically ignore overflow/underflow as long as the leakage is
     // limited to the lower bits. This is to remain compatible with
     // other assemblers.
diff --git a/llvm/test/MC/X86/fixup-range-check.s b/llvm/test/MC/X86/fixup-range-check.s
new file mode 100644
index 0000000000000..7586ed528d7f8
--- /dev/null
+++ b/llvm/test/MC/X86/fixup-range-check.s
@@ -0,0 +1,8 @@
+// RUN: not llvm-mc -filetype=obj -o /dev/null -triple x86_64-unknown-unknown %s 2>&1 | FileCheck %s
+
+.intel_syntax noprefix
+.code64
+test_case:
+// CHECK: error: value of -9223372036854775808 is too large for field of 4 bytes.
+  mov rcx, EXAMPLE
+.set EXAMPLE, (1ULL<<63ULL)

[phoebewang]

Why checking isPCRel since we listed all kinds above?

[Heath123]

Sometimes a PC-relative fixup uses one of the generic fixup types. I think this is because all of the architecture-specific ones are 4-byte, so for example a 1-byte PC-relative fixup will be emitted as a generic fixup, but with the PC-relative flag set to true

[MaskRay]

In X86AsmBackend::applyFixup, the assert in the else branch is reachable for out-of-range fixup values and should be fixed.
FK_Data_[1248] may have true of false PCRel, while other fixup kinds have a known PCRel value.

When IsResolved, we should add the reportError check for more fixup kinds, but should ensure every new case has test coverage.