Go: cmd/compile: `switch` statement on a custom `int32` type with negative values behaves differently in two consecutive calls

[bartekn]

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

$ go version
go version go1.12.6 darwin/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

GOARCH="amd64"

GOBIN=""

GOCACHE="/Users/bartek/Library/Caches/go-build"

GOEXE=""

GOFLAGS=""

GOHOSTARCH="amd64"

GOHOSTOS="darwin"

GOOS="darwin"

GOPATH="/Users/bartek/gocode"

GOPROXY=""

GORACE=""

GOROOT="/Users/bartek/Downloads/go"

GOTMPDIR=""

GOTOOLDIR="/Users/bartek/Downloads/go/pkg/tool/darwin_amd64"

GCCGO="gccgo"

CC="clang"

CXX="clang++"

CGO_ENABLED="1"

GOMOD=""

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 -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/zv/kh35w1j12wn5_4md98dl9zfr0000gn/T/go-build400417808=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

https://play.golang.org/p/da7SYpb02ed

A switch statement on a custom int32 type with negative values behaves differently in two consecutive calls to the same function. What is strange is that each of the following changes make this application work correctly:

• Changing BucketEntryTypeMetaentry to a positive number.
• Commenting one or more empty case clauses.
• Adding fmt.Println("ok") between entry.MustMetaEntry() lines (lines 40 and 41).
• Adding fmt.Println("ok") anywhere inside MustMetaEntry().
• Removing MetaEntry on BucketEntry.

What did you expect to see?

No output:

What did you see instead?

panic: oh no

goroutine 1 [running]:
main.BucketEntry.MustMetaEntry(...)
    /Users/bartek/gocode/src/github.com/stellar/go/custom_type.go:31
main.main()
    /Users/bartek/gocode/src/github.com/stellar/go/custom_type.go:41 +0x81
exit status 2

Please note that function call in line 40 does not panic.

[ianlancetaylor]

Interesting. Works with Go 1.10 and gccgo, fails with Go 1.11, 1.12, and tip.

CC @randall77 @josharian @dr2chase @mdempsky

[mdempsky]

Inspecting the GOSSAFUNC=main output, it looks like the "generic deadcode" pass gets rid of the normal Return path, and leaves behind only the two panicking paths.

[mdempsky]

Minimized:

package main

var x int32 = -1

func main() {
    if x != -1 {
        panic("oh no")
    }
    if x > 0 {
        panic("oh no")
    }
}

[mdempsky]

Actually that minimized test case only fails at tip. Here's one that fails back at Go 1.11 too:

package main

var x int32 = -1

func main() {
    if x != -1 {
        panic("oh no")
    }
    if x > 0 || x != -1 {
        panic("oh no")
    }
}

[mdempsky]

Bisected to CL 100278 (29162ec9a).

/cc @rasky

[mdempsky]

And FWIW, the simpler test case bisects to CL 165617 (4a9064ef41).

It's because that CL swaps the order that the bThen and bElse blocks are created, and apparently prove.go is sensitive to that.

[mdempsky]

I think the problem is this check:

// Check if the recorded limits can prove that the value is positive
if l, has := ft.limits[v.ID]; has && (l.min >= 0 || l.umax <= math.MaxInt64) {

The l.umax <= math.MaxInt64 check doesn't make sense for int8, int16, or int32, because they still have negative values whose unsigned representation is less than MaxInt64.

[gopherbot]

Change https://golang.org/cl/181797 mentions this issue: cmd/compile: fix range analysis of small signed integers

[mdempsky]

I'm curious what folks think about this issue.

On the one hand, it seems embarrassingly bad and in need of a fix and backport. (Edit: By "embarassingly bad," I just mean because the minimal repro cases don't involve any tricky Go semantics; the underlying root cause is totally understandable though.)

On the other, it's been in stable releases for almost a year and this is the first we've heard of it?

[ianlancetaylor]

If the fix seems safe then I would vote for fixing and backporting.

[mdempsky]

@ianlancetaylor The fix seems straight forward enough to me, but it could definitely use some more eyes from the usual package ssa reviewers. This was my first time looking at prove.go. (Thanks to @josharian for the quick review and +1.)

[bartekn]

Thanks for checking this so quickly!

On the one hand, it seems embarrassingly bad and in need of a fix and backport.

One thing I'm wondering about is if it poses any security risk. Consider this code. Obviously, this is a bad code and a simple test (or even testing it manually) would catch it quickly but it shows the potential risks.

On the other, it's been in stable releases for almost a year and this is the first we've heard of it?

Actually, the original code that revealed this bug was different but I submitted the other snippet because it was isolated. The thing is, at first I really thought that it's probably something wrong with my code (not Golang) and I could easily fix this by changing entry.MustMetaEntry() to entry.MetaEntry:

    if entry.Type == BucketEntryTypeMetaentry {
        meta2 := entry.MetaEntry
        fmt.Println(meta2)
    }

But I started digging... I guess that maybe (maybe!) there were other instances of this but devs just changed their code to work thinking it's an issue with their code.

[josharian]

cc also @aclements @zdjones for prove

[MonsieurNicolas]

Is the fix enough?

I would expect the minimized test to fail with int8 and int16 but it passes for me in the playground.

[mdempsky]

I would expect the minimized test to fail with int8 and int16 but it passes for me in the playground.

Hm, interesting.

[mdempsky]

Just looking at the code, I would guess int8 and int16 don't reproduce the issue simply because prove.go only looks for 32- and 64-bit operations (eg, OpAdd64, OpAdd32, OpConst64, OpConst32).

[josharian]

@gopherbot please open backport issues. This is an important compiler correctness bug with a safe, localized, minimal fix.

[gopherbot]

Backport issue(s) opened: #32582 (for 1.11), #32583 (for 1.12).

Remember to create the cherry-pick CL(s) as soon as the patch is submitted to master, according to https://golang.org/wiki/MinorReleases.