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/internal/obj/amd64: BSWAPW assembly instruction gives incorrect encoding on AMD64 #29167

Open
omion opened this Issue Dec 10, 2018 · 5 comments

Comments

Projects
None yet
5 participants
@omion
Copy link

omion commented Dec 10, 2018

What version of Go are you using (go version)?

go version go1.11 windows/amd64

Does this issue reproduce with the latest release?

yes (according to the current amd64enc.s file)

What operating system and processor architecture are you using (go env)?

go env Output
set GOARCH=amd64
set GOBIN=
set GOCACHE=C:\Users\Omion\AppData\Local\go-build
set GOEXE=.exe
set GOFLAGS=
set GOHOSTARCH=amd64
set GOHOSTOS=windows
set GOOS=windows
set GOPATH=D:\Documents\ownCloud\go
set GOPROXY=
set GORACE=
set GOROOT=E:\Programs\Go
set GOTMPDIR=
set GOTOOLDIR=E:\Programs\Go\pkg\tool\windows_amd64
set GCCGO=gccgo
set CC=gcc
set CXX=g++
set CGO_ENABLED=1
set GOMOD=
set CGO_CFLAGS=-g -O2
set CGO_CPPFLAGS=
set CGO_CXXFLAGS=-g -O2
set CGO_FFLAGS=-g -O2
set CGO_LDFLAGS=-g -O2
set PKG_CONFIG=pkg-config
set GOGCCFLAGS=-m64 -mthreads -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=C:\Users\Omion\AppData\Local\Temp\go-build669993050=/tmp/go-build -gno-record-gcc-switches

What did you do?

Use the BSWAPW assembly instruction

What did you expect to see?

The x86 XCHG instruction

What did you see instead?

The x86 BSWAP instruction

Essentially, the problem is that the BSWAP instruction is undefined on 16-bit registers. Intel's architecture programming manual (page 3-112) says to use the XCHG instruction instead.
According to the Go asm testdata file amd64enc.s, the instruction "BSWAPW DX" uses the encoding 0x660fca. 0x66 is a size override and 0x0fca is the BSWAP instruction itself. XCHG exchanges the contents of two registers, which could be the 8-bit DH and DL registers for swapping DX.

@agnivade

This comment has been minimized.

Copy link
Member

agnivade commented Dec 10, 2018

@Quasilyte

This comment has been minimized.

Copy link
Contributor

Quasilyte commented Dec 10, 2018

From the encoder point of view, 660fca is a valid instruction and it's a BSWAP.

./xed -64 -d  '660fca'
660FCA
ICLASS: BSWAP   CATEGORY: DATAXFER   EXTENSION: BASE  IFORM: BSWAP_GPRv   ISA_SET: I486REAL
SHORT: bswap dx

So I think this issue is not about incorrect encoding but about transforming valid, but not very useful instruction form into something that is semantically the same and operates correctly.

It makes sense to emit something like XCHGB DH, DL for BSWAPW DX (I believe it breaks backwards compatibility), but I remember the shift to more straightforward assembler, when no things like MOV $0, dst->XOR dst, dst is performed, so it's closer to traditional assemblers.

GAS does not permit BSWAP with 16-bit arguments. Maybe instead of transforming BSWAP into XCHG we can forbid that form as well, forcing programmers to use XCHGB directly.

@omion

This comment has been minimized.

Copy link

omion commented Dec 10, 2018

Regarding breaking backward compatibility, the current form results in undefined behavior per Intel. I don't think changing something from "could do anything" to "does a specific thing" would be an issue for backward compatibility.

I suppose you are right that it's not really the encoding that's the problem. I don't really know much of the terminology, but I meant that it produces an instruction which is not specified by the documentation. Intel's manual (linked to previously) says BSWAP "Reverses the byte order of a 32-bit or 64-bit (destination) register", and goes on saying to use XCHG for 16-bit.

Since the XCHGB assembly instruction exists I think forbidding BSWAPW completely sounds reasonable. On my computer BSWAPW doesn't actually do anything, so I doubt there are many people who actually use it.

@ALTree ALTree changed the title BSWAPW assembly instruction gives incorrect encoding on AMD64 cmd/internal/obj/amd64: BSWAPW assembly instruction gives incorrect encoding on AMD64 Dec 12, 2018

@ALTree ALTree added this to the Go1.13 milestone Dec 12, 2018

@cameron-elliott

This comment has been minimized.

Copy link

cameron-elliott commented Jan 19, 2019

Even though it looks decided, I would like to add some support and quote the Intel Architecture Manual:
[The bold and italics are mine]

Reverses the byte order of a 32-bit or 64-bit (destination) register. This instruction is provided for converting little-endian values to big-endian format and vice versa. To swap bytes in a word value (16-bit register), use the XCHG instruction. When the BSWAP instruction references a 16-bit register, the result is undefined.

@Quasilyte

This comment has been minimized.

Copy link
Contributor

Quasilyte commented Jan 19, 2019

CC @cherrymui.

I can send a CL as soon as we decide on the exact action.

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