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: `switch` statement on a custom `int32` type with negative values behaves differently in two consecutive calls #32560

Closed
bartekn opened this issue Jun 11, 2019 · 18 comments

Comments

Projects
None yet
7 participants
@bartekn
Copy link

commented Jun 11, 2019

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 ianlancetaylor changed the title `switch` statement on a custom `int32` type with negative values behaves differently in two consecutive calls cmd/compile: `switch` statement on a custom `int32` type with negative values behaves differently in two consecutive calls Jun 12, 2019

@ianlancetaylor ianlancetaylor added this to the Go1.14 milestone Jun 12, 2019

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Jun 12, 2019

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

CC @randall77 @josharian @dr2chase @mdempsky

@mdempsky

This comment has been minimized.

Copy link
Member

commented Jun 12, 2019

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

This comment has been minimized.

Copy link
Member

commented Jun 12, 2019

Minimized:

package main

var x int32 = -1

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

This comment has been minimized.

Copy link
Member

commented Jun 12, 2019

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

This comment has been minimized.

Copy link
Member

commented Jun 12, 2019

Bisected to CL 100278 (29162ec).

/cc @rasky

@mdempsky

This comment has been minimized.

Copy link
Member

commented Jun 12, 2019

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

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

This comment has been minimized.

Copy link
Member

commented Jun 12, 2019

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

This comment has been minimized.

Copy link

commented Jun 12, 2019

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

@mdempsky

This comment has been minimized.

Copy link
Member

commented Jun 12, 2019

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

This comment has been minimized.

Copy link
Contributor

commented Jun 12, 2019

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

@ALTree ALTree added NeedsFix and removed NeedsInvestigation labels Jun 12, 2019

@mdempsky

This comment has been minimized.

Copy link
Member

commented Jun 12, 2019

@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

This comment has been minimized.

Copy link
Author

commented Jun 12, 2019

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

This comment has been minimized.

Copy link
Contributor

commented Jun 12, 2019

cc also @aclements @zdjones for prove

@MonsieurNicolas

This comment has been minimized.

Copy link

commented Jun 12, 2019

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

This comment has been minimized.

Copy link
Member

commented Jun 12, 2019

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

Hm, interesting.

@mdempsky

This comment has been minimized.

Copy link
Member

commented Jun 12, 2019

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).

@gopherbot gopherbot closed this in f44404e Jun 12, 2019

@josharian

This comment has been minimized.

Copy link
Contributor

commented Jun 12, 2019

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

@gopherbot

This comment has been minimized.

Copy link

commented Jun 12, 2019

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.