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.

Sign up for GitHub

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/objdump: order of CMP's arguments doesn't match go tool compile #60920

Closed
eliben opened this issue Jun 21, 2023 · 4 comments
Closed

cmd/objdump: order of CMP's arguments doesn't match go tool compile #60920

eliben opened this issue Jun 21, 2023 · 4 comments
Assignees
@randall77
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Go1.21

Comments

@eliben
Copy link
Member

eliben commented Jun 21, 2023 •
edited

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

$ go version
go version go1.20.1 linux/amd64

Does this issue reproduce with the latest release?

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

go env Output
$ go env
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/eliben/.cache/go-build"
GOENV="/home/eliben/.config/go/env"
GOEXE=""
GOEXPERIMENT=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE=""
GOMODCACHE="/home/eliben/go/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/home/eliben/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org"
GOROOT="/usr/local/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/local/go/pkg/tool/linux_amd64"
GOVCS=""
GOVERSION="go1.20.1"
GCCGO="gccgo"
GOAMD64="v1"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD="/home/eliben/eli/eliben-code/go/assembly/go.mod"
GOWORK=""
CGO_CFLAGS="-O2 -g"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-O2 -g"
CGO_FFLAGS="-O2 -g"
CGO_LDFLAGS="-O2 -g"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -Wl,--no-gc-sections -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build2840069826=/tmp/go-build -gno-record-gcc-switches"

What did you do?

Consider this Go code:

package assembly

func mycond(n int) int {
	if n > 15 {
		return 25
	} else {
		return 9
	}
}

I compile this code and emit assembly using two approaches:

  1. go tool compile -S
  2. go build followed by go tool objdump

What did you expect to see?

The same order of arguments in CMP instructions

What did you see instead?

In the output of go tool compile -S I see:

CMPQ	AX, $15
JLE	<label for return 9>

In the output of go tool objdump I see:

CMPQ $0xf, AX		
JLE <label for return 9>

The order of arguments is different. In assembly input (for example https://cs.opensource.google/go/go/+/master:src/hash/crc32/crc32_amd64.s), the order matches what go tool compile -S produces:

CMPQ CX, $8
JL less_than_8

it seems like the issue is with go tool objdump
@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Jun 21, 2023
@dmitshur dmitshur added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Jun 22, 2023
@dmitshur dmitshur added this to the Backlog milestone Jun 22, 2023
@gopherbot
Copy link

Change https://go.dev/cl/505375 mentions this issue: x86asm: disassemble CMP instruction's arguments in the opposite order

@dmitshur dmitshur added NeedsFix The path to resolution is known, but the work has not been done. and removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Jun 23, 2023
gopherbot pushed a commit to golang/arch that referenced this issue Jul 3, 2023
@randall77
x86asm: disassemble CMP instruction's arguments in the opposite order
060bf14 
That way it matches what the compiler's -S flag generates, and what we write
in assembly.

  CMP AX, $16
  JLE foo

should get to foo if AX <= 16. Without this CL, the disassembly looks like

  CMP $16, AX
  JLE foo

which reads like we should get to foo if 16 <= AX, which is not what these
two instructions actually do.

It was originally this way because the CMP instruction parallels the SUB
instruction, except it throws away the non-flags result. We write that
subtraction as

  SUB $16, AX  // AX <- AX-16

but we don't need to match the SUB's disassembly order, as CMP is not
writing to a register output.

Update golang/go#60920
(This fixes the underlying issue, but the actual "fixes" comment needs to go
on the CL that vendors x/arch containing this CL into the main branch.)

Change-Id: Ifa8d3878453d6e33ae144bfdb01b34171c2106a1
Reviewed-on: https://go-review.googlesource.com/c/arch/+/505375
Reviewed-by: Cherry Mui <cherryyz@google.com>
Run-TryBot: Keith Randall <khr@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Keith Randall <khr@google.com>
@randall77
Copy link
Contributor

The fix has been submitted to x/arch. I think at this point it is best to wait for 1.22 to vendor it into the stdlib.
(It has been broken ~forever, so no hurry.)

@dmitshur dmitshur modified the milestones: Backlog, Go1.22 Jul 3, 2023
@randall77
Copy link
Contributor

The fix has been pulled into the stdlib, so closing.

@dmitshur
Copy link
Contributor

dmitshur commented Aug 9, 2023 •
edited

CL 505375 fixed this in x/arch and its footer said:

Update golang/go#60920
(This fixes the underlying issue, but the actual "fixes" comment needs to go
on the CL that vendors x/arch containing this CL into the main branch.)

The fix was pulled into the main Go repo in CL 509099, which was included in Go 1.21 starting with RC 3. So moving this to Go1.21 milestone.

@dmitshur dmitshur modified the milestones: Go1.22, Go1.21 Aug 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@randall77 randall77

Labels
compiler/runtime Issues related to the Go compiler and/or runtime. NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Milestone
Go1.21
Development

No branches or pull requests
4 participants
@eliben @dmitshur @randall77 @gopherbot