This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by kbelochapka on Aug 21 2017, 4:54 PM.

Details

Reviewers
craig.topper
Summary

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

Diff Detail

Event Timeline

kbelochapka created this revision.Aug 21 2017, 4:54 PM
craig.topper added inline comments.Aug 24 2017, 3:14 PM
lib/Target/X86/MCTargetDesc/X86AsmBackend.cpp
124

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

kbelochapka added inline comments.Aug 24 2017, 3:19 PM
lib/Target/X86/MCTargetDesc/X86AsmBackend.cpp
124

signed 8 bit PC relative value

craig.topper added inline comments.Aug 24 2017, 3:36 PM
lib/Target/X86/MCTargetDesc/X86AsmBackend.cpp
124

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

kbelochapka added inline comments.Aug 24 2017, 3:57 PM
lib/Target/X86/MCTargetDesc/X86AsmBackend.cpp
124

you are absolutely right, will correct this

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

This revision is now accepted and ready to land.Aug 24 2017, 6:28 PM

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

kbelochapka requested review of this revision.Sep 15 2017, 2:59 PM
kbelochapka edited edge metadata.

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

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

avl added a subscriber: avl.Nov 18 2019, 3:39 AM

@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.
  1. 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.
  1. if not the #1 and #2 then use original assertion.
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.
  1. 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.
  1. 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 added a comment.Nov 26 2019, 7:48 AM

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

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 added a comment.Nov 26 2019, 10:43 PM

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.