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/asm: Go 1.6 asm regression for MOVWQZX table-32768(SP)(R11*2), R15" #15426

Closed
nigeltao opened this issue Apr 24, 2016 · 5 comments
Closed

cmd/asm: Go 1.6 asm regression for MOVWQZX table-32768(SP)(R11*2), R15" #15426

nigeltao opened this issue Apr 24, 2016 · 5 comments
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone
Go1.7

Comments

@nigeltao
Copy link
Contributor

nigeltao commented Apr 24, 2016 •
edited

This line of .s code:

MOVWQZX table-32768(SP)(R11*2), R15

assembles fine with Go 1.4, Go 1.5 and with Go tip, but fails on Go 1.6.2:

$ GOROOT=/home/nt/go1.6 ~/go1.6/bin/go test
# github.com/golang/snappy
asm: invalid instruction: 00228 (/home/nt/src/github.com/golang/snappy/encode_amd64.s:343)  MOVWQZX table+120(SP)(R11*2), R15
asm: asm: assembly of ./encode_amd64.s failed
FAIL    github.com/golang/snappy [build failed]

To repeat, it's fixed on Go tip, but one lead might be that the offset changed from -32768 to +120. Perhaps the assembler was confused by SP being both a physical and virtual register.

Given that it assembled properly on Go 1.4 and Go 1.5, should this regression be fixed for Go 1.6.3?

Upstream issue: golang/snappy#29

Upstream workaround: golang/snappy@ec64241

@randall77 @robpike
@robpike
Copy link
Contributor

robpike commented Apr 24, 2016

The assembler is printing that but the diagnostic is actually generated by the obj library and is in new code.

It's line 4188 of https://github.com/golang/go/blob/master/src/cmd/internal/obj/x86/asm6.go and was added by https://go-review.googlesource.com/c/18845/

Since obj is essentially a black box to cmd/asm, it makes sense to start there. Blame says it's @rsc so going there first.

I do not believe I have touched the assembler since 1.6, other than reviewing code, but the infrastructure around it has changed profoundly.

@robpike
Copy link
Contributor

robpike commented Apr 24, 2016

The question remains: what fixed it? I haven't figured that out yet.

@albhaf
Copy link

albhaf commented Apr 24, 2016

Did some digging in the history of that file and my unqualified guess is that it was fixed by eb0cff9 (merge from master to dev.ssa)

@bradfitz bradfitz added this to the Go1.7 milestone May 4, 2016
@quentinmit quentinmit added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label May 26, 2016
@quentinmit
Copy link
Contributor

Why is this in the Go1.7 milestone if it's already fixed in 1.7 and we're discussing whether it should be in 1.6.3?

@rsc
Copy link
Contributor

rsc commented May 26, 2016

I wanted to verify that it really was the correct bytes being emitted. I talked to Nigel and he says he's checked and it is correct, so we can leave it at that.

@rsc rsc closed this as completed May 26, 2016
@golang golang locked and limited conversation to collaborators May 26, 2017
@rsc rsc removed their assignment Jun 23, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Milestone
Go1.7
Development

No branches or pull requests
7 participants
@bradfitz @albhaf @rsc @quentinmit @robpike @nigeltao @gopherbot