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/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
Labels
FrozenDueToAge

Comments

@kortschak
Copy link
Contributor

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
Copy link
Contributor

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
Copy link

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

@gopherbot
Copy link

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

@kortschak
Copy link
Contributor Author

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
@randall77
go/analyis/passes/asmdecl: allow loading both parts of a complex with…
6b7b8b7 
… 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
Copy link

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

gopherbot pushed a commit that referenced this issue Nov 5, 2019
@randall77
cmd/vet: add test for loading complex values with a single instruction
376c4ce 
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>
@golang golang locked and limited conversation to collaborators Nov 4, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@kortschak @randall77 @gopherbot