-
Notifications
You must be signed in to change notification settings - Fork 17.9k
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
Comments
switch
statement on a custom int32
type with negative values behaves differently in two consecutive callsswitch
statement on a custom int32
type with negative values behaves differently in two consecutive calls
Interesting. Works with Go 1.10 and gccgo, fails with Go 1.11, 1.12, and tip. |
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. |
Minimized:
|
Actually that minimized test case only fails at tip. Here's one that fails back at Go 1.11 too:
|
And FWIW, the simpler test case bisects to CL 165617 (4a9064e). It's because that CL swaps the order that the |
I think the problem is this check:
The |
Change https://golang.org/cl/181797 mentions this issue: |
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? |
If the fix seems safe then I would vote for fixing and backporting. |
@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.) |
Thanks for checking this so quickly!
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.
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 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. |
cc also @aclements @zdjones for prove |
Is the fix enough? I would expect the minimized test to fail with |
Hm, interesting. |
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 please open backport issues. This is an important compiler correctness bug with a safe, localized, minimal fix. |
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. |
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
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
What version of Go are you using (
go version
)?Does this issue reproduce with the latest release?
Yes.
What operating system and processor architecture are you using (
go env
)?go env
OutputWhat did you do?
https://play.golang.org/p/da7SYpb02ed
A
switch
statement on a customint32
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:BucketEntryTypeMetaentry
to a positive number.case
clauses.fmt.Println("ok")
betweenentry.MustMetaEntry()
lines (lines 40 and 41).fmt.Println("ok")
anywhere insideMustMetaEntry()
.MetaEntry
onBucketEntry
.What did you expect to see?
No output:
What did you see instead?
Please note that function call in line 40 does not panic.
The text was updated successfully, but these errors were encountered: