[SystemZ] Add range checks for PC-relative fixups

[jonpa]

Summary

The AsmParser checks for the range of a pc-relative immediate operand, but when that operand is a specified label there was previously no such check performed.

This patch adds checks for these operands in applyFixup(), at which point the actual offset is known.

[jonpa]

Not sure what to do with the testing: should we do something similar as a 'Large' folder as is already done for the branch relaxation tests?

So far only one test - for the 12-bit fixup.

[uweigand]

In D114194#3141215, @jonpa wrote:

Not sure what to do with the testing: should we do something similar as a 'Large' folder as is already done for the branch relaxation tests?

The 'Large' folder is only separate because files in there take a long time to build, and therefore shouldn't be executed in each normal "make check".

I think it should be straightforward to create tests with large distances between symbols (e.g. using .rep) that don't take any longer than usual unit tests, so they don't need to be treated specially.

[jonpa]

I ran into a bit of problems trying to test the 32-bit PCRel range. Since this is encoded as halfwords, 32 bits will actually overflow, and unfortunately common-code has currently some 32-bit types for offsets. I could for now make the test pass with some common-code changes, but the (SmallVector) Contents vector should probably also be fixed then to have a 64-bit template type. This is the vector that holds the encoded bytes.

The 32-bit PCRel test kind of exploded in compile time (~10 mins). It seems the main part of this is in the table-generated SystemZAsmParser::MatchInstructionImp. The 24-bit test takes 2-3 seconds.

Not sure if/how we should proceed with the 32-bit case...

GAS is faster, but seems to print broken diagnostics with the big test case ('between 0 and -2' is weird):

time as ./llvm/test/MC/SystemZ/fixups-out-of-range-02.s -o out.o -march=zEC12
./llvm/test/MC/SystemZ/fixups-out-of-range-02.s: Assembler messages:
./llvm/test/MC/SystemZ/fixups-out-of-range-02.s:15: Error: operand out of range (4096 not between -4096 and 4094)
./llvm/test/MC/SystemZ/fixups-out-of-range-02.s:22: Error: operand out of range (-4098 not between -4096 and 4094)
./llvm/test/MC/SystemZ/fixups-out-of-range-02.s:31: Error: operand out of range (16777220 not between -16777216 and 16777214)
./llvm/test/MC/SystemZ/fixups-out-of-range-02.s:38: Error: operand out of range (-16777222 not between -16777216 and 16777214)
./llvm/test/MC/SystemZ/fixups-out-of-range-02.s:47: Error: operand out of range (65540 not between -65536 and 65534)
./llvm/test/MC/SystemZ/fixups-out-of-range-02.s:54: Error: operand out of range (-65542 not between -65536 and 65534)
./llvm/test/MC/SystemZ/fixups-out-of-range-02.s:63: Error: operand out of range (4294967300 not between 0 and -2)
./llvm/test/MC/SystemZ/fixups-out-of-range-02.s:63: Error: value of 4294967300 too large for field of 4 bytes at 16846898
./llvm/test/MC/SystemZ/fixups-out-of-range-02.s:70: Error: operand out of range (-4294967302 not between 0 and -2)
./llvm/test/MC/SystemZ/fixups-out-of-range-02.s:70: Error: value of -4294967302 too large for field of 4 bytes at 4311814206

real	2m32.105s
user	2m23.702s
sys	0m8.365s

[jonpa]

.space directive works better - compile time issue eliminated :-)

handlePCRelFixupValue(32) still in the patch, but the test for it removed in fixups-out-of-range-02.s. I think it will catch some positive out-of-range values, but not any negative ones (out of range). In the future if 64-bit offsets become supported, this should then work as expected, hopefully.

I have now made sure the SMLoc is passed to all created MCFixups in SystemZMCCodeEmitter.cpp. This improves the diagnostic message (line number and instruction printed).

I copied the diagnostic message from GAS in fixups-out-of-range-02.s, but have not done so for the displacements (fixups-out-of-range-01.s). I am not quite sure what is best: should the displacements also get the fuller diagnostic, or is it better with a simplistic message?

[uweigand]

The code changes LGTM.

I copied the diagnostic message from GAS in fixups-out-of-range-02.s, but have not done so for the displacements (fixups-out-of-range-01.s). I am not quite sure what is best: should the displacements also get the fuller diagnostic, or is it better with a simplistic message?

I think it's better to be explicit, so the message for displacements should mirror the one here.

As to the test, I don't really like the checking for explicit line:column numbers - this will make those test awkward to change in the future. Isn't it enough to identify a particular line by the label number used in it (assuming you make label uses unique, which can be easily done by sometimes putting two labels on the same location).

[jonpa]

Updated per review.

I think it's better to be explicit, so the message for displacements should mirror the one here.

ok - updated patch and tests for this.

As to the test, I don't really like the checking for explicit line:column numbers - this will make those test awkward to change in the future. Isn't it enough to identify a particular line by the label number used in it (assuming you make label uses unique, which can be easily done by sometimes putting two labels on the same location).

Removed line numbers - I think that should work ok with the CHECK-NEXT:s and the final 'CHECK-NOT'...

[uweigand]

One final comment inline, otherwise this LGTM. Thanks!

llvm/lib/Target/SystemZ/MCTargetDesc/SystemZMCAsmBackend.cpp

43

Hmm, this scenario could be triggered by wrong assembler input (e.g. something simple like j .+1), so I don't think this should result in an assert (internal compiler error).

GCC/binutils currently seems to simply ignore that case (and round down), which also isn't ideal.

I think this really should be a user-level error message, not an assert.

[jonpa]

llvm/lib/Target/SystemZ/MCTargetDesc/SystemZMCAsmBackend.cpp

43

Changed this like:

   auto handlePCRelFixupValue = [&](unsigned W) -> uint64_t {
-    assert(Value % 2 == 0 && "Non-even PC relative offset.");
+    if (Value % 2 != 0)
+      Ctx.reportError(Fixup.getLoc(), "Non-even PC relative offset.");
     if (!checkFixupInRange(minIntN(W) * 2, maxIntN(W) * 2))
       return 0;
     return (int64_t)Value / 2;

llvm/lib/Target/SystemZ/MCTargetDesc/SystemZMCAsmBackend.cpp

 // Value is a fully-resolved relocation value: Symbol + Addend [- Pivot].
 // Return the bits that should be installed in a relocation field for
 // fixup kind Kind.
 static uint64_t extractBitsForFixup(MCFixupKind Kind, uint64_t Value,
                                     const MCFixup &Fixup, MCContext &Ctx) {
   if (Kind < FirstTargetFixupKind)
     return Value;
 
+  auto checkFixupInRange = [&](int64_t Min, int64_t Max) -> bool {
+    int64_t SVal = int64_t(Value);
+    if (SVal < Min || SVal > Max) {
+      Ctx.reportError(Fixup.getLoc(), "operand out of range (" + Twine(SVal) +
+                                          " not between " + Twine(Min) +
+                                          " and " + Twine(Max) + ")");
+      return false;
+    }
+    return true;
+  };
+
+  auto handlePCRelFixupValue = [&](unsigned W) -> uint64_t {
+    if (Value % 2 != 0)
+      Ctx.reportError(Fixup.getLoc(), "Non-even PC relative offset.");
+    if (!checkFixupInRange(minIntN(W) * 2, maxIntN(W) * 2))
+      return 0;
+    return (int64_t)Value / 2;
+  };
+
   switch (unsigned(Kind)) {
   case SystemZ::FK_390_PC12DBL:
+    return handlePCRelFixupValue(12);
   case SystemZ::FK_390_PC16DBL:
+    return handlePCRelFixupValue(16);
   case SystemZ::FK_390_PC24DBL:
+    return handlePCRelFixupValue(24);
   case SystemZ::FK_390_PC32DBL:
-    return (int64_t)Value / 2;
+    return handlePCRelFixupValue(32);
 
   case SystemZ::FK_390_12:
-    if (!isUInt<12>(Value)) {
-      Ctx.reportError(Fixup.getLoc(), "displacement exceeds uint12");
+    if (!checkFixupInRange(0, maxUIntN(12)))
       return 0;
-    }
     return Value;
 
   case SystemZ::FK_390_20: {
-    if (!isInt<20>(Value)) {
-      Ctx.reportError(Fixup.getLoc(), "displacement exceeds int20");
+    if (!checkFixupInRange(minIntN(20), maxIntN(20)))
       return 0;
-    }
     // The high byte of a 20 bit displacement value comes first.
     uint64_t DLo = Value & 0xfff;
     uint64_t DHi = (Value >> 12) & 0xff;
     return (DLo << 8) | DHi;
   }
 
   case SystemZ::FK_390_TLS_CALL:
     return 0;

llvm/lib/Target/SystemZ/MCTargetDesc/SystemZMCCodeEmitter.cpp

getDispOpValue(const MCInst &MI, unsigned OpNum,
                SystemZ::FixupKind Kind) const {
   const MCOperand &MO = MI.getOperand(OpNum);
   if (MO.isImm())
     return static_cast<uint64_t>(MO.getImm());
   if (MO.isExpr()) {
     // All instructions follow the pattern where the first displacement has a
     // 2 bytes offset, and the second one 4 bytes.
     unsigned ByteOffs = Fixups.size() == 0 ? 2 : 4;
-    Fixups.push_back(MCFixup::create(ByteOffs, MO.getExpr(), (MCFixupKind)Kind));
+    Fixups.push_back(MCFixup::create(ByteOffs, MO.getExpr(), (MCFixupKind)Kind,
+                                     MI.getLoc()));
     assert(Fixups.size() <= 2 && "More than two memory operands in MI?");
     return 0;
   }
   llvm_unreachable("Unexpected operand type!");
 }
 
 uint64_t SystemZMCCodeEmitter::
 getBDAddr12Encoding(const MCInst &MI, unsigned OpNum,
getBDVAddr12Encoding(const MCInst &MI, unsigned OpNum,
   return (Index << 16) | (Base << 12) | Disp;
 }
 
 uint64_t
 SystemZMCCodeEmitter::getPCRelEncoding(const MCInst &MI, unsigned OpNum,
                                        SmallVectorImpl<MCFixup> &Fixups,
                                        unsigned Kind, int64_t Offset,
                                        bool AllowTLS) const {
+  SMLoc Loc = MI.getLoc();
   const MCOperand &MO = MI.getOperand(OpNum);
   const MCExpr *Expr;
   if (MO.isImm())
     Expr = MCConstantExpr::create(MO.getImm() + Offset, Ctx);
   else {
     Expr = MO.getExpr();
     if (Offset) {
       // The operand value is relative to the start of MI, but the fixup
       // is relative to the operand field itself, which is Offset bytes
       // into MI.  Add Offset to the relocation value to cancel out
       // this difference.
       const MCExpr *OffsetExpr = MCConstantExpr::create(Offset, Ctx);
       Expr = MCBinaryExpr::createAdd(Expr, OffsetExpr, Ctx);
     }
   }
-  Fixups.push_back(MCFixup::create(Offset, Expr, (MCFixupKind)Kind));
+  Fixups.push_back(MCFixup::create(Offset, Expr, (MCFixupKind)Kind, Loc));
 
   // Output the fixup for the TLS marker if present.
   if (AllowTLS && OpNum + 1 < MI.getNumOperands()) {
     const MCOperand &MOTLS = MI.getOperand(OpNum + 1);
-    Fixups.push_back(MCFixup::create(0, MOTLS.getExpr(),
-                                     (MCFixupKind)SystemZ::FK_390_TLS_CALL));
+    Fixups.push_back(MCFixup::create(
+        0, MOTLS.getExpr(), (MCFixupKind)SystemZ::FK_390_TLS_CALL, Loc));
   }
   return 0;
 }
 
 #define ENABLE_INSTR_PREDICATE_VERIFIER
 #include "SystemZGenMCCodeEmitter.inc"
 
 MCCodeEmitter *llvm::createSystemZMCCodeEmitter(const MCInstrInfo &MCII,
                                                 const MCRegisterInfo &MRI,
                                                 MCContext &Ctx) {
   return new SystemZMCCodeEmitter(MCII, Ctx);
 }

llvm/test/MC/SystemZ/fixups-out-of-range-01.s

 # RUN: not llvm-mc -triple s390x-unknown-unknown -filetype=obj %s 2>&1 | FileCheck %s
 
   .text
 
-# CHECK: error: displacement exceeds uint12
+# CHECK:      error: operand out of range (4096 not between 0 and 4095)
+# CHECK-NEXT:        la %r1, b-a(%r1)
+# CHECK-NEXT:        ^
   la    %r1, b-a(%r1)
 
-# CHECK: error: displacement exceeds int20
+# CHECK-NEXT: error: operand out of range (524288 not between -524288 and 524287)
+# CHECK-NEXT:        lay   %r1, d-c(%r1)
+# CHECK-NEXT:        ^
         lay   %r1, d-c(%r1)
 
 # CHECK-NOT: error
         lay   %r1, b-a(%r1)
 
   .type a,@object
   .local  a
   .comm a,4096

llvm/test/MC/SystemZ/fixups-out-of-range-02.s

+# RUN: not llvm-mc -triple s390x-unknown-unknown -filetype=obj -mcpu=zEC12 \
+# RUN:   -o /dev/null %s 2>&1 | FileCheck %s
+
+  .text
+
+# Test fixup ranges, which are encoded as half-words.
+
+# 12-bit
+# CHECK:      error: operand out of range (4096 not between -4096 and 4094)
+# CHECK-NEXT:        bprp 0, .Lab1, 0
+# CHECK-NEXT:        ^
+# CHECK-NEXT: error: operand out of range (-4098 not between -4096 and 4094)
+# CHECK-NEXT:        bprp 0, .Lab0, 0
+# CHECK-NEXT:        ^
+        bprp 0, .Lab1, 0
+.Lab0:
+        bprp 0, .Lab1, 0
+        .space 4084
+.Lab1:
+  nopr
+        bprp 0, .Lab0, 0
+        bprp 0, .Lab0, 0
+
+# 24-bit
+# CHECK-NEXT: error: operand out of range (16777220 not between -16777216 and 16777214)
+# CHECK-NEXT:        bprp 0, 0, .Lab3
+# CHECK-NEXT:        ^
+# CHECK-NEXT: error: operand out of range (-16777222 not between -16777216 and 16777214)
+# CHECK-NEXT:        bprp 0, 0, .Lab2
+# CHECK-NEXT:        ^
+        bprp 0, 0, .Lab3
+.Lab2:
+        bprp 0, 0, .Lab3
+        .space 16777208
+.Lab3:
+  nopr
+        bprp 0, 0, .Lab2
+        bprp 0, 0, .Lab2
+
+# 16-bit
+# CHECK-NEXT: error: operand out of range (65540 not between -65536 and 65534)
+# CHECK-NEXT:        cij %r1, 0, 0, .Lab5
+# CHECK-NEXT:        ^
+# CHECK-NEXT: error: operand out of range (-65542 not between -65536 and 65534)
+# CHECK-NEXT:        cij %r1, 0, 0, .Lab4
+# CHECK-NEXT:        ^
+        cij %r1, 0, 0, .Lab5
+.Lab4:
+        cij %r1, 0, 0, .Lab5
+        .space 65528
+.Lab5:
+  nopr
+        cij %r1, 0, 0, .Lab4
+        cij %r1, 0, 0, .Lab4
+
+# 32-bit
+# Unfortunately there is no support for offsets greater than 32 bits, so we have
+# to for now assume they are in range.
+
+# CHECK-NOT: error