[x86] [MC] fixed no error diagnostic for out-of-range jrcxz/jecxz/jcxz (PR24072)

[kbelochapka]

Summary

Fixed no diagnostic message problem for out of range fixup value for loop/loope/loopne/jcxz/jecxz/jrcxz instructions.

[craig.topper]

lib/Target/X86/MCTargetDesc/X86AsmBackend.cpp

153–165

Why is this (Size * 8) - 1 now?

[kbelochapka]

lib/Target/X86/MCTargetDesc/X86AsmBackend.cpp

153–165

signed 8 bit PC relative value

[craig.topper]

lib/Target/X86/MCTargetDesc/X86AsmBackend.cpp

153–165

To detect if something is a signed 8 bit value, isIntN expects to be passed 8. Not 7.

[kbelochapka]

lib/Target/X86/MCTargetDesc/X86AsmBackend.cpp

153–165

you are absolutely right, will correct this

[kbelochapka]

as was pointed by Craig, corrected isIntN() argument mistake

[craig.topper]

LGTM

[kbelochapka]

treat PC relative fixups as signed values and absolute fixups as unsigned values

[kbelochapka]

Reimplemented fixup overflow check, treat PC relative fixup values as signed values and absolute fixup values as unsigned values.

[kbelochapka]

Craig, can you please when you have chance, review the update that I had made for the initial fix of the bug.

[avl]

@kbelochapka Hi Konstantin, I am not sure whether you are still waiting for that review ... But if that is the case, Would you consider following comments, please ?

1. I think it would be better to limit checking for only "getFixupKindInfo(Fixup.getKind()).Flags && MCFixupKindInfo::FKF_IsPCRel" case. Since jrcxz/jecxz/jcxz instructions have only relative offset. i.e. report error for only PCRel fixups.

2. It also looks like check for offset needs to be limited to "IsResolved" case. Since if symbols are not resolved then actual offset is not known.

3. if not the #1 and #2 then use original assertion.

[kbelochapka]

In D36991#1749597, @avl wrote:

@kbelochapka Hi Konstantin, I am not sure whether you are still waiting for that review ... But if that is the case, Would you consider following comments, please ?

1. I think it would be better to limit checking for only "getFixupKindInfo(Fixup.getKind()).Flags && MCFixupKindInfo::FKF_IsPCRel" case. Since jrcxz/jecxz/jcxz instructions have only relative offset. i.e. report error for only PCRel fixups.

2. It also looks like check for offset needs to be limited to "IsResolved" case. Since if symbols are not resolved then actual offset is not known.

3. if not the #1 and #2 then use original assertion.

Something like that:

if (getFixupKindInfo(Fixup.getKind()).Flags && MCFixupKindInfo::FKF_IsPCRel && IsRelative) {
    if (!isIntN(Size * 8, Value)) {
      Asm.getContext().reportError(Fixup.getLoc(),
                                   "Value " + Twine(int64_t(Value)) +
                                       " does not fit in the Fixup field");
    }

[avl]

Hi Konstantin, yes, I had in mind something like this. Unfortunately, I noticed your answer after D70652 was integrated : https://reviews.llvm.org/rGe73f78acd34360f7450b81167d9dc858ccddc262

[kbelochapka]

In D36991#1760268, @avl wrote:

Hi Konstantin, yes, I had in mind something like this. Unfortunately, I noticed your answer after D70652 was integrated : https://reviews.llvm.org/rGe73f78acd34360f7450b81167d9dc858ccddc262

Hi Alexey,

To be honest, that is a first time when I see that happened.
Do you have any idea what needs to be done in this situation?

[avl]

Hi Konstantin, That review looked stuck for a long time.
I tried to keep all your original authority - ping original review,
waited for response for a week, added link to your review into the new review,
put your authority in the commit message.
Apologies for not waiting longer.

lib/Target/X86/MCTargetDesc/X86AsmBackend.cpp

 //===-- X86AsmBackend.cpp - X86 Assembler Backend -------------------------===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #include "MCTargetDesc/X86BaseInfo.h"
 #include "MCTargetDesc/X86FixupKinds.h"
 #include "llvm/ADT/StringSwitch.h"
 #include "llvm/BinaryFormat/ELF.h"
 #include "llvm/BinaryFormat/MachO.h"
 #include "llvm/MC/MCAsmBackend.h"
+#include "llvm/MC/MCAssembler.h"
+#include "llvm/MC/MCContext.h"
 #include "llvm/MC/MCELFObjectWriter.h"
 #include "llvm/MC/MCExpr.h"
 #include "llvm/MC/MCFixupKindInfo.h"
 #include "llvm/MC/MCInst.h"
 #include "llvm/MC/MCMachObjectWriter.h"
 #include "llvm/MC/MCObjectWriter.h"
 #include "llvm/MC/MCRegisterInfo.h"
 #include "llvm/MC/MCSectionCOFF.h"
static unsigned getFixupKindLog2Size(unsigned Kind) {
   case FK_PCRel_8:
   case FK_SecRel_8:
   case FK_Data_8:
   case X86::reloc_global_offset_table8:
     return 3;
   }
 }
 
+static bool isFixupRelative(unsigned Kind) {
+  switch (Kind) {
+  default:
+    llvm_unreachable("invalid fixup kind!");
+  case FK_Data_1:
+  case FK_Data_2:
+  case FK_Data_4:
+  case FK_Data_8:
+  case X86::reloc_signed_4byte:
+  case X86::reloc_signed_4byte_relax:
+  case X86::reloc_global_offset_table8:
+  case X86::reloc_global_offset_table:
+    return false;
+  case FK_PCRel_1:
+  case FK_SecRel_1:
+  case FK_PCRel_2:
+  case FK_SecRel_2:
+  case FK_PCRel_4:
+  case FK_SecRel_4:
+  case X86::reloc_riprel_4byte:
+  case X86::reloc_riprel_4byte_relax:
+  case X86::reloc_riprel_4byte_relax_rex:
+  case X86::reloc_riprel_4byte_movq_load:
+  case FK_PCRel_8:
+  case FK_SecRel_8:
+    return true;
+  }
+}
+
 namespace {
 
 class X86ELFObjectWriter : public MCELFObjectTargetWriter {
 public:
   X86ELFObjectWriter(bool is64Bit, uint8_t OSABI, uint16_t EMachine,
                      bool HasRelocationAddend, bool foobar)
     : MCELFObjectTargetWriter(is64Bit, OSABI, EMachine, HasRelocationAddend) {}
 };
void applyFixup(const MCAssembler &Asm, const MCFixup &Fixup,
     unsigned Size = 1 << getFixupKindLog2Size(Fixup.getKind());
 
     assert(Fixup.getOffset() + Size <= Data.size() && "Invalid fixup offset!");
 
     // Check that uppper 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.
-    assert(isIntN(Size * 8 + 1, Value) &&
-           "Value does not fit in the Fixup field");
+    if (isFixupRelative(Fixup.getKind())) {
+      if (!isIntN(Size * 8, Value)) {
+        Asm.getContext().reportError(Fixup.getLoc(),
+                                     "Value " + Twine(int64_t(Value)) +
+                                         " does not fit in the Fixup field");
+      }
+    } else {
+      if (!isIntN((Size * 8) + 1, Value)) {
+        Asm.getContext().reportError(Fixup.getLoc(),
+                                     "Value " + Twine(Value) +
+                                         " does not fit in the Fixup field");
+      }
+    }
 
     for (unsigned i = 0; i != Size; ++i)
       Data[Fixup.getOffset() + i] = uint8_t(Value >> (i * 8));
   }
 
   bool mayNeedRelaxation(const MCInst &Inst) const override;
 
   bool fixupNeedsRelaxation(const MCFixup &Fixup, uint64_t Value,

test/MC/MachO/reloc.s

 
 L0:
         .long L0
         .long L1
 
         .text
 _f0:
 L1:
-        jmp 0xbabecafe
+        jmp 0x7abecafe
         jmp L0
         jmp L1
         ret
 
         .objc_class_name_A=0
   .globl .objc_class_name_A
 
         .text

test/MC/X86/x86-jcxz-loop-fixup.s

+# RUN: not llvm-mc -filetype=obj -triple=x86_64-linux-gnu %s 2>&1 | FileCheck %s
+
+       .balign 256
+label00:
+// CHECK: Value 509 does not fit in the Fixup field
+  jecxz   label01
+// CHECK: Value 507 does not fit in the Fixup field
+  jrcxz   label01
+// CHECK: Value 505 does not fit in the Fixup field
+  loop  label01
+// CHECK: Value 503 does not fit in the Fixup field
+  loope label01
+// CHECK: Value 501 does not fit in the Fixup field
+  loopne  label01
+        .balign 512
+label01:
+// CHECK: Value -515 does not fit in the Fixup field
+  jecxz   label00
+// CHECK: Value -517 does not fit in the Fixup field
+  jrcxz   label00
+// CHECK: Value -519 does not fit in the Fixup field
+  loop  label00
+// CHECK: Value -521 does not fit in the Fixup field
+  loope label00
+// CHECK: Value -523 does not fit in the Fixup field
+  loopne  label00
+
+
+