cmd/compile: type safety failure in generic ssa due to representing integer signedness conversions by OpCopy

[kortschak]

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 ea3bfba87cfd7141870f975102029e2e341b4af3 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]

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]

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]

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]

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

[josharian]

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.

[randall77]

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.