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
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. release-blocker
Milestone

Comments

@bartekn
Copy link

bartekn 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 NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. release-blocker labels Jun 12, 2019
@ianlancetaylor ianlancetaylor added this to the Go1.14 milestone Jun 12, 2019
@ianlancetaylor
Copy link
Contributor

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

CC @randall77 @josharian @dr2chase @mdempsky

@mdempsky
Copy link
Member

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

Minimized:

package main

var x int32 = -1

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

@mdempsky
Copy link
Member

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

Bisected to CL 100278 (29162ec).

/cc @rasky

@mdempsky
Copy link
Member

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

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

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

@mdempsky
Copy link
Member

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

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

@ALTree ALTree added NeedsFix The path to resolution is known, but the work has not been done. and removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Jun 12, 2019
@mdempsky
Copy link
Member

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

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

cc also @aclements @zdjones for prove

@MonsieurNicolas
Copy link

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

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

Hm, interesting.

@mdempsky
Copy link
Member

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

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

@gopherbot
Copy link

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.

jefferai added a commit to hashicorp/vault that referenced this issue Jul 16, 2019
This version fixes a bug that is bad, but hard to say whether it might
affect us or not -- or more crucially, any of our dependencies. It's
almost certainly worth updating just in case. See
golang/go#32560
jefferai added a commit to hashicorp/vault that referenced this issue Jul 17, 2019
This version fixes a bug that is bad, but hard to say whether it might
affect us or not -- or more crucially, any of our dependencies. It's
almost certainly worth updating just in case. See
golang/go#32560
@golang golang locked and limited conversation to collaborators Jun 11, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. release-blocker
Projects
None yet
Development

No branches or pull requests

7 participants