llvm dev - Dec 2019 - X86 Disassembler Misplaced Assignment

In the file llvm/lib/Target/X86/Disassembler/X86DisassemblerDecoder.cpp in the
function readDisplacement (line 1235),insn->consumedDisplacement is set to
true on line 1245. In the proceeding switch statement, the same variable may be
set to false, but the line immediately after the switch (line 1269) always sets
it back to true. Here’s a copy of the source:

/*
* readDisplacement - Consumes the displacement of an instruction.
*
* @param insn  - The instruction whose displacement is to be read.
* @return      - 0 if the displacement byte was successfully read; nonzero
*                otherwise.
*/
static int readDisplacement(struct InternalInstruction* insn) {
  int8_t d8;
  int16_t d16;
  int32_t d32;

  dbgprintf(insn, "readDisplacement()");

  if (insn->consumedDisplacement)
    return 0;

  insn->consumedDisplacement = true;     // The value is always set to ‘true’
here
  insn->displacementOffset = insn->readerCursor - insn->startLocation;

  switch (insn->eaDisplacement) {
  case EA_DISP_NONE:
    insn->consumedDisplacement = false;  // The value may be set to ‘false’
here
    break;                               // Control flow skips to the end of the
switch
  case EA_DISP_8:
    if (consumeInt8(insn, &d8))
      return -1;
    insn->displacement = d8;
    break;
  case EA_DISP_16:
    if (consumeInt16(insn, &d16))
      return -1;
    insn->displacement = d16;
    break;
  case EA_DISP_32:
    if (consumeInt32(insn, &d32))
      return -1;
    insn->displacement = d32;
    break;
  }

  insn->consumedDisplacement = true;     // The value is always set to ‘true’
again, reversing the change that was
                                         // made if insn->eaDisplacement ==
EA_DISP_NONE
  return 0;
}

I’m not sure if this is a bug or if it was intentional. If the logic is not
correct, what would be the appropriate fix? If it would be more appropriate to
file a formal bug report let me know and I can file one.

~ Cameron Smith

-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20191216/05d34778/attachment.html>

Based on git blame, it looks like its been like since the file was created
in 2009. Based on that we should probably just keep the one assignment
above the switch.
~Craig


On Mon, Dec 16, 2019 at 1:31 PM Smith, Cameron T via llvm-dev <
llvm-dev at lists.llvm.org> wrote:
> In the file llvm/lib/Target/X86/Disassembler/X86DisassemblerDecoder.cpp in
> the function readDisplacement (line 1235),insn->consumedDisplacement is
> set to true on line 1245. In the proceeding switch statement, the same
> variable may be set to false, but the line immediately after the switch
> (line 1269) always sets it back to true. Here’s a copy of the source:
>
>
>
> */**
>
> ** readDisplacement - Consumes the displacement of an instruction.*
>
> ***
>
> ** @param insn  - The instruction whose displacement is to be read.*
>
> ** @return      - 0 if the displacement byte was successfully read;
> nonzero*
>
> **                otherwise.*
>
> **/*
>
> static int readDisplacement(struct InternalInstruction* insn) {
>
>   int8_t d8;
>
>   int16_t d16;
>
>   int32_t d32;
>
>
>
>   dbgprintf(insn, "readDisplacement()");
>
>
>
>   *if* (insn->consumedDisplacement)
>
>     *return* 0;
>
>
>
>   insn->consumedDisplacement = true;     // The value is always set to
> ‘true’ here
>
>   insn->displacementOffset = insn->readerCursor -
insn->startLocation;
>
>
>
>   *switch* (insn->eaDisplacement) {
>
>   *case* EA_DISP_NONE:
>
>     insn->consumedDisplacement = false;  // The value may be set to
> ‘false’ here
>
>     *break*;                               // Control flow skips to the
> end of the switch
>
>   *case* EA_DISP_8:
>
>     *if* (consumeInt8(insn, &d8))
>
>       *return* -1;
>
>     insn->displacement = d8;
>
>     *break*;
>
>   *case* EA_DISP_16:
>
>     *if* (consumeInt16(insn, &d16))
>
>       *return* -1;
>
>     insn->displacement = d16;
>
>     *break*;
>
>   *case* EA_DISP_32:
>
>     *if* (consumeInt32(insn, &d32))
>
>       *return* -1;
>
>     insn->displacement = d32;
>
>     *break*;
>
>   }
>
>
>
>   insn->consumedDisplacement = true;     // The value is always set to
> ‘true’ again, reversing the change that was
>
>                                          // made if insn->eaDisplacement
> == EA_DISP_NONE
>
>   *return* 0;
>
> }
>
>
>
> I’m not sure if this is a bug or if it was intentional. If the logic is
> not correct, what would be the appropriate fix? If it would be more
> appropriate to file a formal bug report let me know and I can file one.
>
>
>
> ~ Cameron Smith
>
>
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20191216/da05458a/attachment.html>