Inconsistent order of x86 instruction prefixes with -via-file-asm

[Quuxplusone]

Bugzilla Link	PR23594
Status	NEW
Importance	P normal
Reported by	Russell Gallop (russell_gallop@sn.scee.net)
Reported on	2015-05-20 11:37:24 -0700
Last modified on	2016-01-15 07:40:57 -0800
Version	trunk
Hardware	PC Linux
CC	artem.tamazov@amd.com, dschuff@google.com, grosbach@apple.com, jvoung@google.com, llvm-bugs@lists.llvm.org, llvm-bugzilla@jfbastien.com, paul_robinson@playstation.sony.com, rafael@espindo.la
Fixed by commit(s)
Attachments
Blocks
Blocked by
See also

The order of x86 instruction prefixes is inconsistent if an asm file is written
out. Tested with revision 237107.

$ cat test.cpp
bool test() {
    unsigned short val;
    return __sync_bool_compare_and_swap_2(&val, 0, 0);
}

$ clang -c test.cpp && objdump -d test.o

test.o:     file format elf64-x86-64
...
   c:   66 f0 0f b1 4d fe       lock cmpxchg %cx,-0x2(%rbp)

$ clang -via-file-asm -c test.cpp && objdump -d test.o

test.o:     file format elf64-x86-64
...
   c:   f0 66 0f b1 4d fe       lock cmpxchg %cx,-0x2(%rbp)

[Quuxplusone]

IL testcase:

define void @f(i16* %x) {
  cmpxchg i16* %x, i16 0, i16 0 seq_cst seq_cst
  ret void
}

[Quuxplusone]

I'm seeing this with trunk and 3.6 but not 3.5. If that helps.

[Quuxplusone]

I emailed a patch already.

[Quuxplusone]

My patch was incorrect. The real issue is that

    lock        cmpxchgw    %cx, (%rdi)
    lock
    cmpxchgw    %cx, (%rdi)

should assemble to

   0:   66 f0 0f b1 0f          lock cmpxchg %cx,(%rdi)
   5:   f0 66 0f b1 0f          lock cmpxchg %cx,(%rdi)

I had a quick look at how to handle this in the parser, but it is not trivial.

[Quuxplusone]

I had a chat with Jim Grosbach on IRC about this. He says that with the current infrastructure the less horrible way to handle this would be for X86AsmParser::ParseInstruction to handle

lock cmpxchgw %cx, (%rdi)

by returning 4 operands: [cmpxchgw, %cx, (%rdi), lock].

That is somewhat non trivial since X86AsmParser::ParseInstruction is not in a position to convert cmpxchgw to lower case: it has no place to hang the ownership.

The order was first changed in r224283 to "simplify tooling that operates on the instruction's byte sequence (such as NaCl's validator)".

Given the complexity of supporting both orders on our side, it might be worth checking with the NaCl folks how hard it is to change the validator and change llc back to printing the lock first (and also revert r238232).

[Quuxplusone]

Changing the NaCl validator isn't trivial and generates quite a bit more code.

I think the bigger issue is consistency with GCC tooling, and expectations of derived tooling such as the NaCl validator: I'd rather not have LLVM be inconsistent in this respect.

It's not an entirely random choice of encoding on GCC's part (though ISA-wise both are equivalent). As you outlined in your examples the assembly is different in both cases.

[Quuxplusone]

If they're equivalent, why should we care about being bit for bit identical to gas? We don't, and shouldn't, make any guarantees about that.

Put another way, why is the NaCl verifier tightly coupled to the implementation of gas? That seems problematic.

[Quuxplusone]

It's desirable to follow a similar ordering so that tools which rely on having
compilers emit similar code can be checked against one another for correctness.
E.g. it's much easier to verify that different assemblers and disassemblers are
correct when they agree to use the same order for silly things like
interchangeable prefixes.

It's not just useful from a security perspective (which is what NaCl cares
about): it's also useful from a simple bugfinding perspective. This found quite
a few bugs in different tools when NaCl was initially being brought up.

I'm not overly attached to this particular change, it just seems unfortunate
and wasteful to disagree with GCC on something silly like this.

I just realized that this bug is missing the link to the CL:
  http://reviews.llvm.org/D6630

The binutils code from tc-i386.c says:
/* Prefixes will be emitted in the order defined below.
   WAIT_PREFIX must be the first prefix since FWAIT is really is an
   instruction, and so must come before any prefixes.
   The preferred prefix order is SEG_PREFIX, ADDR_PREFIX, DATA_PREFIX,
   REP_PREFIX/HLE_PREFIX, LOCK_PREFIX.  */
#define WAIT_PREFIX 0
#define SEG_PREFIX  1
#define ADDR_PREFIX 2
#define DATA_PREFIX 3
#define REP_PREFIX  4
#define HLE_PREFIX  REP_PREFIX
#define BND_PREFIX  REP_PREFIX
#define LOCK_PREFIX 5
#define REX_PREFIX  6       /* must come last.  */
#define MAX_PREFIXES    7   /* max prefixes per opcode */