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

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

Closed
TheTincho opened this Issue Oct 24, 2017 · 13 comments

Comments

Projects
None yet
6 participants
@TheTincho
Copy link

TheTincho commented Oct 24, 2017

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

This comment has been minimized.

Copy link
Contributor

ianlancetaylor commented Oct 24, 2017

CC @randall77

Tentatively marking as 1.9.3.

@randall77 randall77 self-assigned this Oct 25, 2017

@randall77

This comment has been minimized.

Copy link
Contributor

randall77 commented Oct 25, 2017

@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

This comment has been minimized.

Copy link
Member

ALTree commented Oct 25, 2017

@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

This comment has been minimized.

Copy link
Contributor

randall77 commented Oct 25, 2017

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

This comment has been minimized.

Copy link

gopherbot commented Oct 25, 2017

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

@randall77

This comment has been minimized.

Copy link
Contributor

randall77 commented Oct 25, 2017

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

@randall77

This comment has been minimized.

Copy link
Contributor

randall77 commented Oct 25, 2017

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

@gopherbot gopherbot closed this in 40649e6 Oct 26, 2017

@randall77

This comment has been minimized.

Copy link
Contributor

randall77 commented Oct 26, 2017

Reopening for 1.9.3.

@randall77 randall77 reopened this Oct 26, 2017

@TheTincho

This comment has been minimized.

Copy link
Author

TheTincho commented Oct 26, 2017

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.

@TheTincho

This comment has been minimized.

Copy link
Author

TheTincho commented Oct 26, 2017

Confirmed, this solves the issue. Thanks!!

@andybons

This comment has been minimized.

Copy link
Member

andybons commented Jan 18, 2018

CL 73551 OK for Go 1.9.3.

@gopherbot

This comment has been minimized.

Copy link

gopherbot commented Jan 18, 2018

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

[release-branch.go1.9] cmd/compile: make sure not to use SP as an ind…
…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

This comment has been minimized.

Copy link
Member

andybons commented Jan 22, 2018

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

@andybons andybons closed this Jan 22, 2018

@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.