[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/include/llvm/MC/MCFixup.h

 /// be encoded on the target).
 class MCFixup {
   /// The value to put into the fixup location. The exact interpretation of the
   /// expression is target dependent, usually it will be one of the operands to
   /// an instruction or an assembler directive.
   const MCExpr *Value = nullptr;
 
   /// The byte index of start of the relocation inside the MCFragment.
-  uint32_t Offset = 0;
+  uint64_t Offset = 0;
 
   /// The target dependent kind of fixup item this is. The kind is used to
   /// determine how the operand value should be encoded into the instruction.
   MCFixupKind Kind = FK_NONE;
 
   /// The source location which gave rise to the fixup, if any.
   SMLoc Loc;
 public:
-  static MCFixup create(uint32_t Offset, const MCExpr *Value,
+  static MCFixup create(uint64_t Offset, const MCExpr *Value,
                         MCFixupKind Kind, SMLoc Loc = SMLoc()) {
     assert(Kind <= MaxFixupKind && "Kind out of range!");
     MCFixup FI;
     FI.Value = Value;
     FI.Offset = Offset;
     FI.Kind = Kind;
     FI.Loc = Loc;
     return FI;
   }
 
   MCFixupKind getKind() const { return Kind; }
 
   unsigned getTargetKind() const { return Kind; }
 
-  uint32_t getOffset() const { return Offset; }
-  void setOffset(uint32_t Value) { Offset = Value; }
+  uint64_t getOffset() const { return Offset; }
+  void setOffset(uint64_t Value) { Offset = Value; }
 
   const MCExpr *getValue() const { return Value; }
 
   /// Return the generic fixup kind for a value with the given size. It
   /// is an error to pass an unsupported size.
   static MCFixupKind getKindForSize(unsigned Size, bool IsPCRel) {
     switch (Size) {
     default: llvm_unreachable("Invalid generic fixup size!");

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.

llvm/lib/MC/MCObjectStreamer.cpp

void MCObjectStreamer::emitGPRel64Value(const MCExpr *Value) {
   flushPendingLabels(DF, DF->getContents().size());
 
   DF->getFixups().push_back(
       MCFixup::create(DF->getContents().size(), Value, FK_GPRel_4));
   DF->getContents().resize(DF->getContents().size() + 8, 0);
 }
 
 static Optional<std::pair<bool, std::string>>
-getOffsetAndDataFragment(const MCSymbol &Symbol, uint32_t &RelocOffset,
+getOffsetAndDataFragment(const MCSymbol &Symbol, uint64_t &RelocOffset,
                          MCDataFragment *&DF) {
   if (Symbol.isVariable()) {
     const MCExpr *SymbolExpr = Symbol.getVariableValue();
     MCValue OffsetVal;
     if(!SymbolExpr->evaluateAsRelocatable(OffsetVal, nullptr, nullptr))
       return std::make_pair(false,
                             std::string("symbol in .reloc offset is not "
                                         "relocatable"));
MCObjectStreamer::emitRelocDirective(const MCExpr &Offset, StringRef Name,
   }
   if (OffsetVal.getSymB())
     return std::make_pair(false,
                           std::string(".reloc offset is not representable"));
 
   const MCSymbolRefExpr &SRE = cast<MCSymbolRefExpr>(*OffsetVal.getSymA());
   const MCSymbol &Symbol = SRE.getSymbol();
   if (Symbol.isDefined()) {
-    uint32_t SymbolOffset = 0;
+    uint64_t SymbolOffset = 0;
     Optional<std::pair<bool, std::string>> Error;
     Error = getOffsetAndDataFragment(Symbol, SymbolOffset, DF);
 
     if (Error != None)
       return Error;
 
     DF->getFixups().push_back(
         MCFixup::create(SymbolOffset + OffsetVal.getConstant(),

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 handlePCRelFixupValue = [&Value, &Ctx, &Fixup](unsigned W) -> uint64_t {
+    assert(Value % 2 == 0 && "Non-even PC relative offset.");
+    int64_t NumHalfWords = (int64_t)Value / 2;
+    int64_t Max = maxIntN(W);
+    int64_t Min = minIntN(W);
+    if (NumHalfWords < Min || NumHalfWords > Max) {
+      Ctx.reportError(Fixup.getLoc(), "operand out of range (" +
+                      Twine((int64_t) Value) + " not between " + Twine(Min * 2)
+                      + " and " + Twine(Max * 2) + ")");
+      return 0;
+    }
+    return NumHalfWords;
+  };
+
   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");
       return 0;
     }
     return Value;
 

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

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,
+                                   MI.getLoc()));
 
   // 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));
   }
   return 0;

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:      15:9: error: operand out of range (4096 not between -4096 and 4094)
+# CHECK-NEXT:         bprp 0, .Lab1, 0
+# CHECK-NEXT:         ^
+# CHECK-NEXT: 22:9: 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
+        .rept 1021 ; nop ; .endr
+.Lab1:
+  nopr
+        bprp 0, .Lab0, 0
+        bprp 0, .Lab0, 0
+
+# 24-bit
+# CHECK-NEXT: 31:9: error: operand out of range (16777220 not between -16777216 and 16777214)
+# CHECK-NEXT:         bprp 0, 0, .Lab3
+# CHECK-NEXT:         ^
+# CHECK-NEXT: 38:9: 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
+        .rept 4194302 ; nop ; .endr
+.Lab3:
+  nopr
+        bprp 0, 0, .Lab2
+        bprp 0, 0, .Lab2
+
+# 16-bit
+# CHECK-NEXT: 47:9: error: operand out of range (65540 not between -65536 and 65534)
+# CHECK-NEXT:         cij %r1, 0, 0, .Lab5
+# CHECK-NEXT:         ^
+# CHECK-NEXT: 54:9: 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
+        .rept 16382 ; nop ; .endr
+.Lab5:
+  nopr
+        cij %r1, 0, 0, .Lab4
+        cij %r1, 0, 0, .Lab4
+
+# 32-bit
+# CHECK-NEXT: 63:2: error: operand out of range (4294967300 not between -4294967296 and 4294967294)
+# CHECK-NEXT:         larl %r1, .Lab7
+# CHECK-NEXT:         ^
+# CHECK-NEXT: 70:2: error: operand out of range (-4294967302 not between -4294967296 and 4294967294)
+# CHECK-NEXT:         larl %r1, .Lab6
+# CHECK-NEXT:         ^
+  larl %r1, .Lab7
+.Lab6:
+  larl %r1, .Lab7
+        .rept 1073741822 ; nop ; .endr
+.Lab7:
+  nopr
+  larl %r1, .Lab6
+  larl %r1, .Lab6
+        
+# CHECK-NOT: error