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

cmd/vet: false positives for FP assembly #35264

Closed
kortschak opened this issue Oct 31, 2019 · 5 comments
Closed

cmd/vet: false positives for FP assembly #35264

kortschak opened this issue Oct 31, 2019 · 5 comments
Assignees

Comments

@kortschak
Copy link
Contributor

@kortschak kortschak commented Oct 31, 2019

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

$ go version
go version go1.13.3 linux/amd64

Does this issue reproduce with the latest release?

Yes.

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

go env Output
$ go env
GO111MODULE="on"
GOARCH="amd64"
GOBIN="/home/user/bin"
GOCACHE="/home/user/.cache/go-build"
GOENV="/home/user/.config/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/home/user"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/home/user/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/home/user/go/pkg/tool/linux_amd64"
GCCGO="gccgo"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD="/home/user/src/gonum.org/v1/gonum/go.mod"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build952532343=/tmp/go-build -gno-record-gcc-switches"

What did you do?

git clone git@github.com:gonum/gonum.git
cd gonum.org/v1/gonum/internal/asm
go vet ./...

What did you expect to see?

No complaints.

What did you see instead?

# gonum.org/v1/gonum/internal/asm/c64
c64/axpyinc_amd64.s:53:1: [amd64] AxpyInc: invalid MOVSD of alpha+0(FP); complex64 is 8-byte value containing alpha_real+0(FP) and alpha_imag+4(FP)
c64/axpyincto_amd64.s:57:1: [amd64] AxpyIncTo: invalid MOVSD of alpha+40(FP); complex64 is 8-byte value containing alpha_real+40(FP) and alpha_imag+44(FP)
c64/axpyunitary_amd64.s:48:1: [amd64] AxpyUnitary: invalid MOVSD of alpha+0(FP); complex64 is 8-byte value containing alpha_real+0(FP) and alpha_imag+4(FP)
c64/axpyunitaryto_amd64.s:49:1: [amd64] AxpyUnitaryTo: invalid MOVSD of alpha+24(FP); complex64 is 8-byte value containing alpha_real+24(FP) and alpha_imag+28(FP)
c64/dotcinc_amd64.s:159:1: [amd64] DotcInc: invalid MOVSD of sum+88(FP); complex64 is 8-byte value containing sum_real+88(FP) and sum_imag+92(FP)
c64/dotcunitary_amd64.s:207:1: [amd64] DotcUnitary: invalid MOVSD of sum+48(FP); complex64 is 8-byte value containing sum_real+48(FP) and sum_imag+52(FP)
c64/dotuinc_amd64.s:147:1: [amd64] DotuInc: invalid MOVSD of sum+88(FP); complex64 is 8-byte value containing sum_real+88(FP) and sum_imag+92(FP)
c64/dotuunitary_amd64.s:196:1: [amd64] DotuUnitary: invalid MOVSD of sum+48(FP); complex64 is 8-byte value containing sum_real+48(FP) and sum_imag+52(FP)
# gonum.org/v1/gonum/internal/asm/f64
f64/sum_amd64.s:99:1: [amd64] Sum: unknown variable sum; offset 24 is ret+24(FP)
f64/sum_amd64.s:100:1: [amd64] Sum: RET without writing to 8-byte ret+24(FP)

See gonum/gonum#79.

@randall77

This comment has been minimized.

Copy link
Contributor

@randall77 randall77 commented Oct 31, 2019

I have a fix for the complex ones. We should allow a single instruction to load both parts of a complex value.

Those last two lines are intended. The name of the return value for an anonymous return value is "ret", not "sum". You can either change the assembly to use "ret", or change the prototype to:

func Sum(x []float64) (sum float64)
@randall77 randall77 self-assigned this Oct 31, 2019
@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Oct 31, 2019

Change https://golang.org/cl/204537 mentions this issue: go/analyis/passes/asmdecl: allow loading both parts of a complex with one instruction

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Oct 31, 2019

Change https://golang.org/cl/204538 mentions this issue: cmd/vet: add test for loading complex values with a single instruction

@kortschak

This comment has been minimized.

Copy link
Contributor Author

@kortschak kortschak commented Oct 31, 2019

Thanks, @randall77. I had thought I had tried that change in Sum. Clearly not.

gopherbot pushed a commit to golang/tools that referenced this issue Nov 4, 2019
… one instruction

Update golang/go#35264

Change-Id: I4317c75cc5acc592ab739b0aab4cd85280858219
Reviewed-on: https://go-review.googlesource.com/c/tools/+/204537
Reviewed-by: Alan Donovan <adonovan@google.com>
Run-TryBot: Keith Randall <khr@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Nov 4, 2019

Change https://golang.org/cl/205240 mentions this issue: cmd: vendor in new version of x/tools

@gopherbot gopherbot closed this in 4af639a Nov 5, 2019
gopherbot pushed a commit that referenced this issue Nov 5, 2019
Actual fix will be submitted to x/tools and vendored.
This is just an end-to-end test for vet after that is done.

Update #35264

Change-Id: I1a63f607e7cfa7aafee23c2c081086c276d3c38c
Reviewed-on: https://go-review.googlesource.com/c/go/+/204538
Run-TryBot: Keith Randall <khr@golang.org>
Reviewed-by: Alan Donovan <adonovan@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.