[PATCH] D37262: The issues with X86 prefixes: step 2

[Andrew Tischenko via llvm-commits]

BTW, JFYI, I found the following comment in the source:

   // FIXME:
   // Enhace prefixes integrity robustness. for example, following forms
   // are currently tolerated:
   // repz repnz <insn>    ; GAS errors for the use of two similar prefixes
   // lock addq %rax, %rbx ; Destination operand must be of memory type
   // xacquire <insn>      ; xacquire must be accompanied by 'lock'

The approach with Flag will allow to implement it.

Andrew


On 27.09.2017 12:18, Andrew Tischenko wrote:
>
> OK, I'll try to change the assembler properly but there are some 
> questions:
>
>  1. Should I do it in the same patch?
>  2. Currently if we have:
>
> repz repnz repe cmpsb
>
>     then we produce with 'llvm-mc -triple x86_64-unknown-unknown 
> -x86-asm-syntax=intel -show-encoding intel-syntax.s':
>
>         rep                             # encoding: [0xf3]
>         repne                           # encoding: [0xf2]
>         rep                             # encoding: [0xf3]
>         cmpsb   %es:(%rdi), (%rsi)      # encoding: [0xa6]
>
>     but after the change we'll get only the following:
>
>         rep                             # encoding: [0xf3]
>         cmpsb   %es:(%rdi), (%rsi)      # encoding: [0xa6]
>
>     Is it OK? (IMHO, Yes.)
>
>     If YES, do we need any warnings here? (IMHO, No.)
>
> Andrew
>
> On 26.09.2017 22:31, Rafael Avila de Espindola wrote:
>> The assembler and disassembler should use the same path.
>>
>> I would be OK with always producing 1 or N instructions, as long as both
>> the assembler and disassembler do the same. That is, it is OK to have
>> Flags, as long as the assembler uses that instead of creating a separate
>> instruction for prefixes.
>>
>> It seems that allowing the disassembler to create multiple instructions
>> would have the advantage of not needing Flags, but that is secondary
>> IMHO.
>>
>> Cheers,
>> Rafael
>>
>> Craig Topper<craig.topper at gmail.com>  writes:
>>
>>> Here's my understanding of what I think happens today.
>>>
>>> -For a very select few instructions if the AsmParser sees a repne/repe
>>> prefix it creates a special version of the instruction that has the REP
>>> bits set in TSFlags. For any other instruction it emits the repne/rep/repe
>>> as a separate MCInst.
>>> -For the disassembler if it sees a repne/repe byte at the start that it
>>> doesn't think goes with an instruction it will emit a MCInst containing
>>> just the REP.
>>> -If the disassembler encounters a repne/repe byte not at the start of the
>>> instruction that doesn't go with the instruction we drop it and don't print
>>> anything. The disassembler interface only allows us to return one
>>> instruction. So we can't return a separate repne/repe instruction and a
>>> real instruction from the same byte sequence. I don't believe the assembler
>>> can ever produce a byte sequence that hits this case, but that doesn't mean
>>> some binary couldn't contain that string of bytes created by hand. So this
>>> patch is trying to preserve the extra prefix information in the one MCInst
>>> we're allowed to emit. Maybe another option would be to allow creating
>>> multiple MCInsts from the disassembler?
>>>
>>> ~Craig
>>>
>>> On Tue, Sep 26, 2017 at 10:37 AM, Rafael Avila de Espindola <
>>> rafael.espindola at gmail.com> wrote:
>>>
>>>> The question is why it is different for disassembler than for the
>>>> assembler?
>>>>
>>>> How does the assembler handle trepne?
>>>>
>>>> Cheers,
>>>> Rafael
>>>>
>>>> Andrew Tischenko<tishenandr at xenzu.com>  writes:
>>>>
>>>>> It is not a simple flag, it's some data. And this data could be useful
>>>>> for any other component because it's some opaque info which could be
>>>>> send via MCInst from one low level target component to another one.
>>>>> Without this (additional) data MCInst loosing (potentially very useful)
>>>>> info about the given instruction.
>>>>>
>>>>> Andrew
>>>>>
>>>>> On 25.09.2017 22:05, Rafael Avila de Espindola wrote:
>>>>>> Having a flag field that is used only on disassembly seems wrong.
>>>>>>
>>>>>> Don't we support parsing our own output? I don't see trepne in any .s
>>>>>> test for example.
>>>>>>
>>>>>> Cheers,
>>>>>> Rafael
>>>>>>
>>>>>> Craig Topper via Phabricator<reviews at reviews.llvm.org>  writes:
>>>>>>
>>>>>>> craig.topper added a comment.
>>>>>>>
>>>>>>> I'm not sure I can approve growing the size of MCInst. Though I can't
>>>> see anyway around it. @rafael what do you think?
>>>>>>> https://reviews.llvm.org/D37262
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170927/5213d419/attachment.html>