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

Wed Sep 27 04:08:17 PDT 2017

It was added when I’ve start poking around with prefixes, to implement the proper recognition of xaquire/xrelease (rL311309<https://reviews.llvm.org/rL311309>).
I can suggest some additional views to the matter at hand:
Enhancing prefix digestion by the parser is highly recommended – aforementioned FIXME note describes those issues I’ve found on a brief exploring, surly there’s more.
Currently it is emitted as a standalone instruction, which I don’t see much sense in.
IMHO, we should aggregate prefixes till we have consumed an ‘actual’ instruction, and then make queries about whether they form a legal combination, and I deem it to be the right course for the disassembler as well, unless we don’t care about tolerating nonsense in disassembly (how does others disassemblers handle such phenomena?)
Personally, I see prefixes as a part of a particular instruction, so at least on the concept level I’m in favor of ‘Flags’.
More generally, whether producing multiple MCInsts, using flag or whatever other approach – it’s technicalities.
Agreement should be first reached on what would be considered as a proper handling for both ends (parser, disassembler).

p.s. can you guyz please use Phab for further discussion? Hard (for me) to keep track on mail correspondence.

From: Andrew Tischenko [mailto:tishenandr at xenzu.com]
Sent: Wednesday, September 27, 2017 12:56
To: Rafael Avila de Espindola <rafael.espindola at gmail.com>; Craig Topper <craig.topper at gmail.com>
Cc: reviews+D37262+public+6b8fd5b17a169c12 at reviews.llvm.org; Eric Christopher <echristo at gmail.com>; Simon Pilgrim <llvm-dev at redking.me.uk>; Davide Italiano <dccitaliano at gmail.com>; Dinar Temirbulatov <dtemirbulatov at gmail.com>; Tayree, Coby <coby.tayree at intel.com>; llvm-commits <llvm-commits at lists.llvm.org>
Subject: Re: [PATCH] D37262: The issues with X86 prefixes: step 2

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><mailto: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<mailto: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><mailto: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><mailto: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

---------------------------------------------------------------------
Intel Israel (74) Limited

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170927/20795015/attachment.html>