code review 93110044: x86asm: instruction decoder

x86asm: instruction decoder

[rsc, 2014-05-08 03:34:54]

Hello crawshaw,

I'd like you to review this change to
https://code.google.com/p/rsc.x86/

[rsc, 2014-05-08 03:35:38]

Should be obvious but decode.go is the only interesting file.

[crawshaw, 2014-05-08 11:39:48]

LGTM

Strangely like programming for a web browser.

https://codereview.appspot.com/93110044/diff/20001/x86asm/decode.go
File x86asm/decode.go (right):

https://codereview.appspot.com/93110044/diff/20001/x86asm/decode.go#newcode247
x86asm/decode.go:247: var (
If you pushed this block of variables into a struct and made decode1 a method,
you could easily split out some parts of this function. E.g. the initial prefix
reading, the repeated backwards search under "// Check to see if earlier prefix
F3/F2 is present", and maybe even the xCondPrefix instruction.

https://codereview.appspot.com/93110044/diff/20001/x86asm/decode.go#newcode373
x86asm/decode.go:373: case 0x67:
I think there will be some extra complexity here, the 0x67 prefix interacts with
REX.W. (See the table in vol 1, 3.6.1)

https://codereview.appspot.com/93110044/diff/20001/x86asm/decode.go#newcode433
x86asm/decode.go:433: if opshift >= 0 {
Isn't it an error for opshift < 0 when writing inst.Opcode? Return errInternal
instead of continuing?

https://codereview.appspot.com/93110044/diff/20001/x86asm/decode.go#newcode983
x86asm/decode.go:983: // TODO(rsc): Can address be 64 bits?
Looks like yes, intel manual mentions MOV RAX,moffs64 (REX.W + A1)

[rsc, 2014-05-08 13:55:24]

*** Submitted as
https://code.google.com/p/rsc/source/detail?r=074e342086e6&repo=x86 ***

x86asm: instruction decoder

LGTM=crawshaw
R=crawshaw
https://codereview.appspot.com/93110044

[rsc, 2014-05-08 14:00:16]

https://codereview.appspot.com/93110044/diff/20001/x86asm/decode.go
File x86asm/decode.go (right):

https://codereview.appspot.com/93110044/diff/20001/x86asm/decode.go#newcode247
x86asm/decode.go:247: var (
On 2014/05/08 11:39:48, crawshaw wrote:
> If you pushed this block of variables into a struct and made decode1 a method,
> you could easily split out some parts of this function. E.g. the initial
prefix
> reading, the repeated backwards search under "// Check to see if earlier
prefix
> F3/F2 is present", and maybe even the xCondPrefix instruction.

Yes, that's been nagging at me. I might do it in a followup but not here.

https://codereview.appspot.com/93110044/diff/20001/x86asm/decode.go#newcode373
x86asm/decode.go:373: case 0x67:
On 2014/05/08 11:39:48, crawshaw wrote:
> I think there will be some extra complexity here, the 0x67 prefix interacts
with
> REX.W. (See the table in vol 1, 3.6.1)

Yes, indeed. Thanks for the pointer, I hadn't found that section yet.

https://codereview.appspot.com/93110044/diff/20001/x86asm/decode.go#newcode433
x86asm/decode.go:433: if opshift >= 0 {
On 2014/05/08 11:39:48, crawshaw wrote:
> Isn't it an error for opshift < 0 when writing inst.Opcode? Return errInternal
> instead of continuing?

If you ended up with a 5-byte opcode (or really just 4 + modrm) then it would
end up less than zero. Weird perhaps but who knows what is possible. My thinking
was better safe than sorry. The VEX stuff may end up that way, I'm not sure.

[rsc, 2014-05-08 14:00:39]

On Thu, May 8, 2014 at 7:39 AM, <crawshaw@golang.org> wrote:

> Strangely like programming for a web browser.
>

ouch (but accurate)

[rsc, 2014-05-08 14:18:56]

https://codereview.appspot.com/93110044/diff/20001/x86asm/decode.go
File x86asm/decode.go (right):

https://codereview.appspot.com/93110044/diff/20001/x86asm/decode.go#newcode373
x86asm/decode.go:373: case 0x67:
On 2014/05/08 11:39:48, crawshaw wrote:
> I think there will be some extra complexity here, the 0x67 prefix interacts
with
> REX.W. (See the table in vol 1, 3.6.1)

Actually it looks like it doesn't, thank goodness. The table entries for address
size are REX.W-invariant.

[crawshaw, 2014-05-08 14:26:38]

https://codereview.appspot.com/93110044/diff/20001/x86asm/decode.go
File x86asm/decode.go (right):

https://codereview.appspot.com/93110044/diff/20001/x86asm/decode.go#newcode373
x86asm/decode.go:373: case 0x67:
On 2014/05/08 14:18:56, rsc wrote:
> On 2014/05/08 11:39:48, crawshaw wrote:
> > I think there will be some extra complexity here, the 0x67 prefix interacts
> with
> > REX.W. (See the table in vol 1, 3.6.1)
> 
> Actually it looks like it doesn't, thank goodness. The table entries for
address
> size are REX.W-invariant.

Isn't it the operand size that's invariant? It looks like 0x67+REX.W and
0x66+0x67+REX.W imply 32-bit addresses.

[rsc, 2014-05-08 16:47:26]

https://codereview.appspot.com/93110044/diff/20001/x86asm/decode.go
File x86asm/decode.go (right):

https://codereview.appspot.com/93110044/diff/20001/x86asm/decode.go#newcode373
x86asm/decode.go:373: case 0x67:
At least on the copy I'm reading, the row for address size is
64,32,64,32,64,32,64,32. The first four are REX.W=0 and the last are REX.W=1.
Both are 64,32,64,32, so REX.W doesn't actually influence address size.

[crawshaw, 2014-05-08 16:51:22]

https://codereview.appspot.com/93110044/diff/20001/x86asm/decode.go
File x86asm/decode.go (right):

https://codereview.appspot.com/93110044/diff/20001/x86asm/decode.go#newcode373
x86asm/decode.go:373: case 0x67:
On 2014/05/08 16:47:26, rsc wrote:
> At least on the copy I'm reading, the row for address size is
> 64,32,64,32,64,32,64,32. The first four are REX.W=0 and the last are REX.W=1.
> Both are 64,32,64,32, so REX.W doesn't actually influence address size.

Ack, I've been misreading the table. Phew.