Skip to content
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

Jump to bottom

cmd/asm: incorrect instruction encodings #14069

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

cmd/asm: incorrect instruction encodings #14069

rsc opened this issue Jan 22, 2016 · 8 comments
Labels
early-in-cycle A change that should be done early in the 3 month dev cycle. NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Backlog

Comments

@rsc
Copy link
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
Copy link
Contributor Author

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
Copy link

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

gopherbot pushed a commit that referenced this issue Jan 24, 2016
@rsc
cmd/asm: add generated test of amd64 instruction encodings
659c632 
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
Copy link
Contributor

bradfitz commented May 5, 2016

Is this still going to happen?

@7i 7i mentioned this issue May 8, 2016
Closed
@rsc
Copy link
Contributor Author

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 The path to resolution is known, but the work has not been done. label Oct 5, 2016
@rsc rsc modified the milestones: Go1.9Early, Go1.8 Oct 21, 2016
@gopherbot
Copy link

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

gopherbot pushed a commit that referenced this issue Apr 26, 2017
@bradfitz
cmd/internal/obj/x86: fix adcb r/mem8,reg8 encoding
92d918d 
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
Copy link

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

@dlespiau
Copy link
Contributor

dlespiau commented Apr 28, 2017 •
edited

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?

@dlespiau dlespiau mentioned this issue Apr 29, 2017
Open
gopherbot pushed a commit that referenced this issue May 1, 2017
@bradfitz
cmd/internal/obj/x86: fix ANDPS encoding
a2da210 
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 added early-in-cycle A change that should be done early in the 3 month dev cycle. and removed early-in-cycle A change that should be done early in the 3 month dev cycle. labels Jun 14, 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
Copy link
Contributor

See also #20173 .

@ianlancetaylor ianlancetaylor modified the milestones: Go1.12, Go1.13 Nov 28, 2018
@bradfitz bradfitz modified the milestones: Go1.13, Go1.14 Apr 29, 2019
@rsc rsc modified the milestones: Go1.14, Backlog Oct 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
early-in-cycle A change that should be done early in the 3 month dev cycle. NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Milestone
Backlog
Development

No branches or pull requests
6 participants
@bradfitz @dlespiau @rsc @quentinmit @ianlancetaylor @gopherbot