[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

124–128

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

[kbelochapka]

lib/Target/X86/MCTargetDesc/X86AsmBackend.cpp

124–128

signed 8 bit PC relative value

[craig.topper]

lib/Target/X86/MCTargetDesc/X86AsmBackend.cpp

124–128

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

124–128

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"
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 (!isIntN(Size * 8, Value)) {
+      Asm.getContext().reportError(Fixup.getLoc(),
+                                   "Value " + Twine(int64_t(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/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
+
+
+