New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cmd/asm: incorrect instruction encodings #14069

Open
rsc opened this Issue Jan 22, 2016 · 8 comments

Comments

Projects
None yet
6 participants
@rsc
Contributor

rsc commented Jan 22, 2016

I have constructed a fairly exhaustive test suite for the x86 assembler and identified some problems. The ones in this issue are long-time bugs that appear to have been present since the beginning of the Go project. We should fix them but given the history there is no need to rush the fixes into Go 1.6.

This may not be all of them: my tests don't account for some Go renamings of instructions.

I intend to fix these and check in the tests.

ADCB (BX), DL: have encoding "1013", want "1213"
   0:   10 13                   adc    %dl,(%rbx)
   0:   12 13                   adc    (%rbx),%dl

ANDPS (BX), X2: have encoding "660f5413", want "0f5413"
   0:   66 0f 54 13             andpd  (%rbx),%xmm2
   0:   0f 54 13                andps  (%rbx),%xmm2

CMOVLEQ (BX), DX: have encoding "0f4413", want "480f4e13"
   0:   0f 44 13                cmove  (%rbx),%edx
   0:   48 0f 4e 13             cmovle (%rbx),%rdx

MOVQ 0x123456789abcdef1, AX: have encoding "488b0425f1debc9a", want "48a1f1debc9a78563412"
   0:   48 8b 04 25 f1 de bc    mov    0xffffffff9abcdef1,%rax
   7:   9a 
   0:   48 a1 f1 de bc 9a 78    movabs 0x123456789abcdef1,%rax
   7:   56 34 12 

MOVQ AX, $0x123456789abcdef1: have encoding "48890425f1debc9a", want "48a3f1debc9a78563412"
   0:   48 89 04 25 f1 de bc    mov    %rax,0xffffffff9abcdef1
   7:   9a 
   0:   48 a3 f1 de bc 9a 78    movabs %rax,0x123456789abcdef1
   7:   56 34 12

@rsc rsc self-assigned this Jan 22, 2016

@rsc rsc added this to the Go1.7Early milestone Jan 22, 2016

@rsc

This comment has been minimized.

Contributor

rsc commented Jan 22, 2016

Recounting this to @aclements I figured out what is going on with CMOVLEQ. The GNU form is CMOV[cond][size] but the form added for Go is CMOV[size][cond]. So GNU's cmovleq is cmov-le-q while Go's CMOVLEQ is CMOV-L-EQ. The Intel syntax has no size suffix and is CMOVEQ/CMOVLE, but it seems clear that Go should not be inserting a size suffix in the middle of the defined opcode name. While in general I feel that we need to put up with past mistakes in our assembly definitions to avoid breaking existing assembly programs, this one seems so egregious and error-prone that I think we have no choice but to change these names for Go 1.7 to match the Intel and GNU syntax.

@gopherbot

This comment has been minimized.

gopherbot commented Jan 22, 2016

CL https://golang.org/cl/18850 mentions this issue.

gopherbot pushed a commit that referenced this issue Jan 24, 2016

cmd/asm: add generated test of amd64 instruction encodings
Generated by x86test, from https://golang.org/cl/18842
(still in progress).

The commented out lines are either missing or misspelled
or incorrectly handled instructions.

For #4816, #8037, #13822, #14068, #14069.

Change-Id: If309310c97d9d2a3c71fc64c51d4a957e9076ab7
Reviewed-on: https://go-review.googlesource.com/18850
Reviewed-by: Rob Pike <r@golang.org>
Run-TryBot: Russ Cox <rsc@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>

@bradfitz bradfitz modified the milestones: Go1.7, Go1.7Early May 5, 2016

@bradfitz

This comment has been minimized.

Member

bradfitz commented May 5, 2016

Is this still going to happen?

@rsc

This comment has been minimized.

Contributor

rsc commented May 17, 2016

Didn't get the new tables done for Go 1.7. These weren't an emergency for Go 1.6 and remain not an emergency for Go 1.7.

@rsc rsc modified the milestones: Go1.8, Go1.7 May 17, 2016

@quentinmit quentinmit added the NeedsFix label Oct 5, 2016

@rsc rsc modified the milestones: Go1.9Early, Go1.8 Oct 21, 2016

@gopherbot

This comment has been minimized.

gopherbot commented Apr 26, 2017

CL https://golang.org/cl/41857 mentions this issue.

gopherbot pushed a commit that referenced this issue Apr 26, 2017

cmd/internal/obj/x86: fix adcb r/mem8,reg8 encoding
Taken from the Intel Software Development Manual (of course, in the line
below it's ADC DST, SRC; The opposite of the commit subject).

  12 /r		ADC r8, r/m8

We need 0x12 for the corresponding ytab line, not 0x10.

  {Ymb, Ynone, Yrb, Zm_r, 1},

Updates #14069

Change-Id: Id37cbd0c581c9988c2de355efa908956278e2189
Reviewed-on: https://go-review.googlesource.com/41857
Reviewed-by: Keith Randall <khr@golang.org>
@gopherbot

This comment has been minimized.

gopherbot commented Apr 28, 2017

CL https://golang.org/cl/42090 mentions this issue.

@dlespiau

This comment has been minimized.

Contributor

dlespiau commented Apr 28, 2017

Looking at CMOVLEQ, I'm happy enough to go and change the GO assembler to use CMOV[cond][size] and break the world as the result. Is it still the behaviour what people think is best? I would tend to agree with that change and the sooner it's done the better.

I'm wondering how it'd be possible to make this less painful for people out there using the Go assembler:

  1. Error out when encountering the old CMOV[size][cond] form
  2. Provide a way to automatically convert assembly files from old to new form (I'm thinking it could just a sed command, not a full assembly rewriter)

Of course, as CMOVLEQ is ambiguous we couldn't error out on this one in 1.

Any other ideas?

gopherbot pushed a commit that referenced this issue May 1, 2017

cmd/internal/obj/x86: fix ANDPS encoding
ANDPS, like all others PS (Packed Single precision floats) instructions,
need Ym: they don't use the 0x66 prefix.

From the manual:

    NP 0F 54 /r        ANDPS xmm1, xmm2/m128

NP meaning, quoting the manual:

  NP - Indicates the use of 66/F2/F3 prefixes (beyond those already part
  of the instructions opcode) are not allowed with the instruction.

And indeed, the same instruction prefixed by 0x66 is ANDPD.

Updates #14069

Change-Id: If312a6f1e77113ab8c0febe66bdb1b4171e41e0a
Reviewed-on: https://go-review.googlesource.com/42090
Reviewed-by: Keith Randall <khr@golang.org>
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>

@bradfitz bradfitz modified the milestones: Go1.10Early, Go1.9Early May 3, 2017

@bradfitz bradfitz modified the milestones: Go1.10Early, Go1.10 Jun 14, 2017

@rsc rsc removed their assignment Nov 22, 2017

@rsc rsc modified the milestones: Go1.10, Go1.11 Nov 22, 2017

@bradfitz bradfitz modified the milestones: Go1.11, Go1.12 May 18, 2018

@ianlancetaylor ianlancetaylor added this to the Go1.12 milestone Jun 1, 2018

@ianlancetaylor

This comment has been minimized.

Contributor

ianlancetaylor commented Nov 28, 2018

See also #20173 .

@ianlancetaylor ianlancetaylor modified the milestones: Go1.12, Go1.13 Nov 28, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment