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/internal/obj: rewriting MOVQ to XOR causes bugs and confusion #20986

Closed
hdevalence opened this issue Jul 11, 2017 · 7 comments
Closed

cmd/internal/obj: rewriting MOVQ to XOR causes bugs and confusion #20986

hdevalence opened this issue Jul 11, 2017 · 7 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Go1.10

Comments

@hdevalence
Copy link
Contributor

Please answer these questions before submitting your issue. Thanks!

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

go version go1.8.3 linux/amd64

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

GOARCH="amd64"
GOBIN=""
GOEXE=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOOS="linux"
GOPATH="/home/hdevalence/go"
GORACE=""
GOROOT="/usr/local/go"
GOTOOLDIR="/usr/local/go/pkg/tool/linux_amd64"
GCCGO="gccgo"
CC="gcc"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build126311645=/tmp/go-build -gno-record-gcc-switches"
CXX="g++"
CGO_ENABLED="1"
PKG_CONFIG="pkg-config"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"

What did you do?

I put MOVQ $0, AX in a Go assembly file.

What did you expect to see?

A move instruction, such as

48 b8 00 00 00 00 00 	movabs rax,0x0
00 00 00

or any other equivalent instruction.

What did you see instead?

An XOR, which clobbers the carry flags, breaking the program.

31 c0                	xor    eax,eax

This is essentially a duplicate of #12405, which is now locked. That issue notes that MOV is now "marked as clobbering flags" (presumably in the compiler internals?) but this fact seems not to be documented in either the Go assembly documentation or in the Plan 9 assembler documentation.
@mvdan
Copy link
Member

mvdan commented Jul 11, 2017

Have you tried on tip or 1.9beta2?

@bradfitz bradfitz changed the title Go assembler applies wrong optimizations to MOVQ cmd/compile: Go assembler applies wrong optimizations to MOVQ Jul 12, 2017
@bradfitz bradfitz added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Jul 12, 2017
@bradfitz bradfitz added this to the Go1.10 milestone Jul 12, 2017
@bradfitz
Copy link
Contributor

/cc @randall77 @josharian @mdempsky

@josharian
Copy link
Contributor

I don't think there's anything to say beyond what was said in #12405. Unless Russ has changed his mind, this is a documentation issue.

@ALTree ALTree mentioned this issue Aug 9, 2017
Closed
@ALTree ALTree mentioned this issue Oct 18, 2017
Closed
@aclements
Copy link
Member

More information about the pain this "optimization" has caused: #22325.

FWIW, I think @rsc has changed his mind at this point and agrees that we should just take this out of obj.

@aclements aclements 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 Oct 18, 2017
@aclements aclements changed the title cmd/compile: Go assembler applies wrong optimizations to MOVQ cmd/internal/obj: rewriting MOVQ to XOR causes bugs and confusion Oct 18, 2017
@aclements
Copy link
Member

I just chatted about this with several runtime/compiler people and we all agreed that this should be removed from obj (and done explicitly in the compiler where it makes sense). Now someone just has to do it. :)

@gopherbot
Copy link

Change https://golang.org/cl/73072 mentions this issue: cmd/internal/obj/x86: stop rewriting MOV to XOR

@gopherbot
Copy link

Change https://golang.org/cl/73073 mentions this issue: cmd/internal/obj/x86: remove PRESERVEFLAGS

@golang golang locked and limited conversation to collaborators Oct 24, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Milestone
Go1.10
Development

No branches or pull requests
6 participants
@bradfitz @hdevalence @josharian @aclements @mvdan @gopherbot