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.

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/compile: invalid instruction error for FMOVD when compiling for 387 #22429

Closed
NightTsarina opened this issue Oct 24, 2017 · 13 comments
Closed

cmd/compile: invalid instruction error for FMOVD when compiling for 387 #22429

NightTsarina opened this issue Oct 24, 2017 · 13 comments
Labels
CherryPickApproved Used during the release process for point releases FrozenDueToAge release-blocker
Milestone
Go1.9.3

Comments

@NightTsarina
Copy link

Please answer these questions before submitting your issue. Thanks!

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

go version go1.9.1 linux/386

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

GOARCH="386"
GOBIN=""
GOEXE=""
GOHOSTARCH="386"
GOHOSTOS="linux"
GOOS="linux"
GOPATH="/root/go"
GORACE=""
GOROOT="/usr/lib/go-1.9"
GOTOOLDIR="/usr/lib/go-1.9/pkg/tool/linux_386"
GCCGO="gccgo"
GO386="387"
CC="gcc"
GOGCCFLAGS="-fPIC -m32 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build059883492=/tmp/go-build -gno-record-gcc-switches"
CXX="g++"
CGO_ENABLED="1"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"

What did you do?

I did not try to isolate a minimal test case, as I have no idea what is going on. But at least one package in Debian is failing to build because of this (prometheus).

When compiling the tests, this line fails with the following error:

$ GOPATH=$PWD/build go test -c -v github.com/prometheus/prometheus/storage/local
# github.com/prometheus/prometheus/storage/local
build/src/github.com/prometheus/prometheus/storage/local/storage_test.go:2026:26: invalid instruction: 01483 (/tmp/buildd/prometheus-1.8.1+ds/build/src/github.com/prometheus/prometheus/storage/local/storage_test.go:2027) FMOVD ""..autotmp_78+176(DX)(SP*1), F0

I can't reproduce this problem with go 1.7 or 1.8 from Debian. Disabling optimisations with -gcflags=-N seems to solve the issue.

Looking at #21860, I guess that this could something else than an "invalid instruction", but I have no idea :-)
@ianlancetaylor ianlancetaylor changed the title Invalid instruction error when compiling in i386 cmd/compile: invalid instruction error for FMOVD when compiling for 386 Oct 24, 2017
@ianlancetaylor ianlancetaylor modified the milestones: Go1.10, Go1.9.3 Oct 24, 2017
@ianlancetaylor
Copy link
Contributor

CC @randall77

Tentatively marking as 1.9.3.

@randall77 randall77 self-assigned this Oct 25, 2017
@randall77
Copy link
Contributor

@TheTincho I'm having trouble reproducing this. There is no local directory in github.com/prometheus/prometheus/storage.
Could you make that available somehow?

@ALTree
Copy link
Member

ALTree commented Oct 25, 2017 •
edited

@randall77

$ go get github.com/prometheus/prometheus/cmd/...
$ cd $GOPATH/src/github.com/prometheus/prometheus
$ git checkout release-1.8
$ cat VERSION 
1.8.1  # this is the version that appears in the error log above

I can't reproduce this, anyway:

$ go version
go version go1.9.1 linux/amd64

$ GOARCH=386 go test -c -v github.com/prometheus/prometheus/storage/local

the build command succeeds and the generated test binary runs fine. Not sure what I'm doing wrong.

@randall77
Copy link
Contributor

I can't reproduce yet, but I think I know what is wrong.
The compiler is using SP as an index register in an instruction. That's a no-no in x86 world.
I've seen this before, and we have fixes for it. Maybe those fixes aren't complete for 387?

Normally this happens for ops with indexed addressing mode, and scale==1. Then the order of the base and index registers don't matter to SSA, so they can appear in either order. We have code to swap them during assembly generation, like this (from amd64/ssa.go):

		r := v.Args[0].Reg()
		i := v.Args[1].Reg()
		if i == x86.REG_SP {
			r, i = i, r
		}

I'll prepare a CL. It would be nice to be able to reproduce the OP's problem so we can be sure it is fixed.

@gopherbot
Copy link

Change https://golang.org/cl/73551 mentions this issue: cmd/compile: make sure not to use SP as an index register

@randall77
Copy link
Contributor

@TheTincho I've uploaded a patch. If you can, please try it and let us know if it fixes your issue.

@randall77
Copy link
Contributor

I concocted a stand-alone repro and added it to the patch.

@randall77 randall77 changed the title cmd/compile: invalid instruction error for FMOVD when compiling for 386 cmd/compile: invalid instruction error for FMOVD when compiling for 387 Oct 25, 2017
@randall77
Copy link
Contributor

Reopening for 1.9.3.

@randall77 randall77 reopened this Oct 26, 2017
@NightTsarina
Copy link
Author

Thank you all for looking at this so quickly!

@randall77: Like @ALTree said, this is from the 1.8 branch, that's why you were missing the package. And about the non-reproducibility, maybe something is slightly different because of our patches in Debian..

In any case, I am now rebuilding the golang-go Debian package with this patch, to see if the problem is solved.

@NightTsarina
Copy link
Author

Confirmed, this solves the issue. Thanks!!

@andybons
Copy link
Member

CL 73551 OK for Go 1.9.3.

@andybons andybons mentioned this issue Jan 18, 2018
Closed
@andybons andybons added the CherryPickApproved Used during the release process for point releases label Jan 18, 2018
This was referenced Jan 18, 2018
Closed
Closed
@gopherbot
Copy link

Change https://golang.org/cl/88317 mentions this issue: [release-branch.go1.9] cmd/compile: make sure not to use SP as an index register

gopherbot pushed a commit that referenced this issue Jan 22, 2018
@randall77 @andybons
[release-branch.go1.9] cmd/compile: make sure not to use SP as an ind…
5c08eef 
…ex register

...because that's an illegal addressing mode.

I double-checked handling of this code, and 387 is the only
place where this check is missing.

Fixes #22429

Change-Id: I2284fe729ea86251c6af2f04076ddf7a5e66367c
Reviewed-on: https://go-review.googlesource.com/73551
Run-TryBot: Keith Randall <khr@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
Reviewed-on: https://go-review.googlesource.com/88317
Run-TryBot: Andrew Bonventre <andybons@golang.org>
Reviewed-by: Keith Randall <khr@golang.org>
@andybons
Copy link
Member

go1.9.3 has been packaged and includes:

  • CL 73551 cmd/compile: make sure not to use SP as an index register

The release is posted at golang.org/dl.

— golang.org/x/build/cmd/releasebot, Jan 22 21:02:54 UTC

@golang golang locked and limited conversation to collaborators Jan 22, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
CherryPickApproved Used during the release process for point releases FrozenDueToAge release-blocker
Projects
None yet
Milestone
Go1.9.3
Development

No branches or pull requests
6 participants
@andybons @ianlancetaylor @ALTree @NightTsarina @randall77 @gopherbot