[PATCH] D38786: Fix for Bug 30718 - Failure to disassemble certain MOV with rex.R

[Andrew V. Tischenko via Phabricator via llvm-commits]

avt77 added inline comments.


================
Comment at: lib/Target/X86/Disassembler/X86DisassemblerDecoder.cpp:1487
+      index = regFromModRM(insn->modRM);                  \
       if (index > 5)                                      \
         *valid = 0;                                       \
----------------
craig.topper wrote:
> avt77 wrote:
> > craig.topper wrote:
> > > I think the correct fix is to change this to "if ((index & 0x7) > 5)" We shouldn't be resampling modRM.
> > I thought about such a variant but on the other hand all other values of modRM could fail segment index. If you insist I'll use your variant of the fix but please say it again.
> There are really two different versions of this function, one called with regFromModRM and one called with rmFromModRM. While I don't think the rmFromModRM version will ever be called with SEGMENT_REG, it feels wrong to assume which field of ModRM we should be using inside the routine.
> 
> We either need to mask the index inside the method, or the caller should mask it before the call.
I tried your version but it does not work because there is the folowing code to invoke the function:

    insn->reg = (Reg)fixupRegValue(insn,
                                   (OperandType)op->type,
                                   insn->reg - insn->regBase,
                                   &valid);
 
It means we can't simply mask index here: we could check TYPE_SEGMENTREG at call time or we can use my approach. What's better? As I understand you'd prefer the caller masks the index properly, right?


https://reviews.llvm.org/D38786