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/compile: type safety failure in generic ssa due to representing integer signedness conversions by OpCopy #37753

Open
kortschak opened this issue Mar 9, 2020 · 6 comments
Labels
NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Milestone

Comments

@kortschak
Copy link
Contributor

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

$ go version
go version devel +ea3bfba87c Thu Feb 27 16:38:23 2020 +0000 linux/amd64

Does this issue reproduce with the latest release?

No

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"
GOINSECURE=""
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="/dev/null"
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-build838367334=/tmp/go-build -gno-record-gcc-switches"

What did you do?

Using the Go built at or after ea3bfba run the code at https://play.golang.org/p/ah0Rl1b03Li

What did you expect to see?

4 4
-4 -4

What did you see instead?

4 4
2305843009213693948 -4

Additional information

This code is used in the Gonum matrix package to detect parameter shadowing, for example here, and is the subject of the (now quite old) #12445. It would be wonderful to have some certainty on this.

/cc @josharian

@kortschak
Copy link
Contributor Author

kortschak commented Mar 9, 2020

Looking at the commit in question I can see that a workaround in Gonum for this is

func offset(a, b []float64) int {
	if &a[0] == &b[0] {
		return 0
	}
	return (int(uintptr(unsafe.Pointer(&b[0]))) - int(uintptr(unsafe.Pointer(&a[0])))) / int(unsafe.Sizeof(float64(0)))
}

but the error is something that probably should be warned about at the very least, and it's odd that it affects the unsafe-based code, but not the (at the apparently relevant point) reflect-based code since they both have uintptr by the time this calculation is done.

@josharian
Copy link
Contributor

Thanks for catching this early, @kortschak, and for the concise reproducer.

Diagnosis: On our way into SSA form, there's an OCONV node converting uintptr(unsafe.Pointer(&b[0]))-uintptr(unsafe.Pointer(&a[0]) into an int. We generate an OpCopy for that. Then the phielim pass eliminates the copy, even though it has a different type. (Aside: it looks like copyelim is re-doing a bunch of work that phielim just did.) As a result, the division by 8 looks like it can be done with just a shift, which it cannot.

I'll back out that part of the compiler change for now. (CL coming probably tomorrow, I have to sign off now.)

But this also points to another source of type-unsafety during the generic SSA stages, which @randall77 and @cherrymui and I have bumped up against a few times this cycle. We might want to try to restore some type safety there, at which point might be able to restore the optimization.

@josharian
Copy link
Contributor

Simpler reproducer (and now I'm signing off!):

package main

//go:noinline
func f(a, b uint) int {
	return int(a-b) / 8
}

func main() {
	if x := f(1, 2); x != 0 {
		panic(x)
	}
}

@gopherbot
Copy link

Change https://golang.org/cl/222620 mentions this issue: cmd/compile: use only bit patterns in isNonNegative

gopherbot pushed a commit that referenced this issue Mar 9, 2020
CL 212777 added a check to isNonNegative
to return true for unsigned values.
However, the SSA backend isn't type safe
enough for that to be sound.
The other checks in isNonNegative
look only at the pattern of bits.
Remove the type-based check.

Updates #37753

Change-Id: I059d0e86353453133f2a160dce53af299f42e533
Reviewed-on: https://go-review.googlesource.com/c/go/+/222620
Run-TryBot: Josh Bleecher Snyder <josharian@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Keith Randall <khr@golang.org>
@josharian josharian changed the title cmd/compile: ea3bfba87c breaks unsafe slice offset calculations cmd/compile: type safety failure in generic ssa due to representing integer signedness conversions by OpCopy Mar 9, 2020
@josharian
Copy link
Contributor

We are (I hope!) back to generating correct code again. The remaining question is around how to make generic ssa type-correct again. Two options I see: Introduce a conversion op specifically for converting beteeen signed and unsigned ints. (2) Changing copyelim to not eliminate copies with different types.

@toothrot toothrot added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Mar 9, 2020
@toothrot toothrot added this to the Backlog milestone Mar 9, 2020
@toothrot toothrot added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Mar 9, 2020
@gopherbot gopherbot removed the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Mar 9, 2020
@randall77
Copy link
Contributor

Yeah, I think we need some sort of reinterpret/cast opcode in the machine-independent form.

I'd rather not foist this task on OpCopy. If anything, if v.Op == OpCopy then we should have v.Type == v.Args[0].Type. Similar for OpPhi.

It might be kinda ugly to get there though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Projects
None yet
Development

No branches or pull requests

5 participants