Skip to content

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

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
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. FrozenDueToAge 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
Loading

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
Contributor

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

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
GPG key ID: 4AEE18F83AFDEB23
Expired
Verified
Learn about vigilant mode
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
@mknyszek mknyszek moved this to In Progress in Go Compiler / Runtime Jul 5, 2023
@randall77
Copy link
Contributor

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

@github-project-automation github-project-automation bot moved this from In Progress to Done in Go Compiler / Runtime Aug 9, 2023
@dmitshur
Copy link
Contributor

dmitshur commented Aug 9, 2023 •
edited
Loading

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
@golang golang locked and limited conversation to collaborators Aug 8, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@randall77 randall77

Labels
compiler/runtime Issues related to the Go compiler and/or runtime. FrozenDueToAge 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