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: compiler crashes in ssa: isNonNegative bad type #40746

Closed
kazzmir opened this issue Aug 13, 2020 · 9 comments
Closed

cmd/compile: compiler crashes in ssa: isNonNegative bad type #40746

kazzmir opened this issue Aug 13, 2020 · 9 comments
Labels
Milestone

Comments

@kazzmir
Copy link

@kazzmir kazzmir commented Aug 13, 2020

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

$ go version
go version go1.15 linux/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
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/jon/.cache/go-build"
GOENV="/home/jon/.config/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE=""
GOMODCACHE="/home/jon/.go/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/home/jon/.go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/home/jon/Downloads/go1.15"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/home/jon/Downloads/go1.15/pkg/tool/linux_amd64"
GCCGO="gccgo"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD="/home/jon/tmp/nes/go.mod"
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-build689779029=/tmp/go-build -gno-record-gcc-switches"

What did you do?

The go 1.15 compiler crashes with

isNonNegative bad type

goroutine 12 [running]:
cmd/compile/internal/ssa.Compile.func1(0xc000958f08, 0xc000911600)
        /usr/local/go/src/cmd/compile/internal/ssa/compile.go:48 +0xa5
panic(0xbd0460, 0xd42470)
        /usr/local/go/src/runtime/panic.go:969 +0x175
cmd/compile/internal/ssa.isNonNegative(0xc001050278, 0x69b814)
        /usr/local/go/src/cmd/compile/internal/ssa/prove.go:1337 +0x3eb
cmd/compile/internal/ssa.isNonNegative(0xc00104e528, 0xc001abe180)
        /usr/local/go/src/cmd/compile/internal/ssa/prove.go:1377 +0x95
cmd/compile/internal/ssa.(*factsTable).isNonNegative(0xc001abe180, 0xc00104e528, 0x8ff)
        /usr/local/go/src/cmd/compile/internal/ssa/prove.go:573 +0x2f
cmd/compile/internal/ssa.addBranchRestrictions(0xc001abe180, 0xc001084ee8, 0x1)
        /usr/local/go/src/cmd/compile/internal/ssa/prove.go:980 +0xff
cmd/compile/internal/ssa.prove(0xc000911600)
        /usr/local/go/src/cmd/compile/internal/ssa/prove.go:872 +0x11c5
cmd/compile/internal/ssa.Compile(0xc000911600)
        /usr/local/go/src/cmd/compile/internal/ssa/compile.go:93 +0x9d1
cmd/compile/internal/gc.buildssa(0xc000865340, 0x1, 0x0)
        /usr/local/go/src/cmd/compile/internal/gc/ssa.go:460 +0xd25
cmd/compile/internal/gc.compileSSA(0xc000865340, 0x1)
        /usr/local/go/src/cmd/compile/internal/gc/pgen.go:317 +0x5d
cmd/compile/internal/gc.compileFunctions.func2(0xc001462420, 0xc00048ef70, 0x1)
        /usr/local/go/src/cmd/compile/internal/gc/pgen.go:382 +0x4d
created by cmd/compile/internal/gc.compileFunctions
        /usr/local/go/src/cmd/compile/internal/gc/pgen.go:380 +0x129

What did you expect to see?

My program compiles fine with go 1.14.7, go 1.15 should work the same.

At the moment I cannot reduce my program to something small enough to include in this bug report, I am still working on that so I will update this bug with a reproducing test case. I have run git bisect on the golang git however and found the issue is due to

commit 5fac45a320561b45b52cdcae933882a70699a21d (refs/bisect/bad)
Author: Josh Bleecher Snyder <josharian@gmail.com>
Date:   Mon Mar 9 06:37:49 2020 -0700

    cmd/compile: use only bit patterns in isNonNegative
    
    CL 212777 added a check to isNonNegative
    to return true for unsigned values.
    However, the SSA backend isn't type safe
    enough for that to be sound.
    The other checks in isNonNegative
    look only at the pattern of bits.
    Remove the type-based check.
    
    Updates #37753
    
    Change-Id: I059d0e86353453133f2a160dce53af299f42e533
    Reviewed-on: https://go-review.googlesource.com/c/go/+/222620
    Run-TryBot: Josh Bleecher Snyder <josharian@gmail.com>
    TryBot-Result: Gobot Gobot <gobot@golang.org>
    Reviewed-by: Keith Randall <khr@golang.org>

diff --git a/src/cmd/compile/internal/ssa/prove.go b/src/cmd/compile/internal/ssa/prove.go
index 4788f2d803..12c2580c95 100644
--- a/src/cmd/compile/internal/ssa/prove.go
+++ b/src/cmd/compile/internal/ssa/prove.go
@@ -1305,9 +1305,9 @@ func isNonNegative(v *Value) bool {
        if !v.Type.IsInteger() {
                panic("isNonNegative bad type")
        }
-       if !v.Type.IsSigned() {
-               return true
-       }
+       // TODO: return true if !v.Type.IsSigned()
+       // SSA isn't type-safe enough to do that now (issue 37753).
+       // The checks below depend only on the pattern of bits.
@josharian
Copy link
Contributor

@josharian josharian commented Aug 13, 2020

Thanks for reporting! When you have a stand-alone test case, even if it's not fully minimized, please do share it. If you need help narrowing things down, let me know. E.g. if you modify the compiler to print v.Block.Func.Name just before the panic, that should let you get down to a single function quickly.

@kazzmir
Copy link
Author

@kazzmir kazzmir commented Aug 13, 2020

package main

type CPUState struct {
    Status byte
}

func (cpu *CPUState) setBit(bit byte, set bool){
    if set {
        cpu.Status = cpu.Status | bit
    } else {
        cpu.Status = cpu.Status & (^bit)
    }
}

func (cpu *CPUState) getBit(bit byte) bool {
    return (cpu.Status & bit) == bit
}

func (cpu *CPUState) SetZeroFlag(zero bool){
    cpu.setBit(byte(0x2), zero)
}

func (cpu *CPUState) SetCarryFlag(set bool){
    cpu.setBit(byte(0x1), set)
}

func (cpu *CPUState) GetCarryFlag() bool {
    return cpu.getBit(byte(0x1))
}

func (cpu *CPUState) SetNegativeFlag(set bool) {
    cpu.setBit(byte(1 << 7), set)
}

func (cpu *CPUState) messedup(value byte) byte {
    var carryBit byte
    if cpu.GetCarryFlag() {
        carryBit = 1
    }

    newCarry := (value & (1<<7)) == (1<<7)
    out := (value << 1) | carryBit

    cpu.SetCarryFlag(newCarry)
    cpu.SetNegativeFlag(int8(out) < 0)
    return out
}

func main(){
}

Hopefully this is small enough, but I think it can be reduced further if necessary.

@josharian
Copy link
Contributor

@josharian josharian commented Aug 13, 2020

That’s great, thanks! We can take it from here.

@ALTree ALTree added this to the Go1.16 milestone Aug 13, 2020
@ALTree ALTree changed the title go 1.15 compiler crashes in ssa: isNonNegative bad type cmd/compile: compiler crashes in ssa: isNonNegative bad type Aug 13, 2020
@gopherbot
Copy link

@gopherbot gopherbot commented Aug 13, 2020

Change https://golang.org/cl/248397 mentions this issue: cmd/compile: correct type of CvtBoolToUint8 values

@josharian
Copy link
Contributor

@josharian josharian commented Aug 13, 2020

@gopherbot please open a backport issue for 1.14. This is a compiler crash on valid input, with no obvious workaround.

@gopherbot
Copy link

@gopherbot gopherbot commented Aug 13, 2020

Backport issue(s) opened: #40772 (for 1.14).

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

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Aug 13, 2020

@josharian Does this also need to be backported to 1.15? Now that 1.15 is out, the 1.15 release branch gets separate backport issues too. Thanks.

@josharian
Copy link
Contributor

@josharian josharian commented Aug 13, 2020

Thanks. That was a mistake on my part. Only 1.15 needs it.

@gopherbot gopherbot closed this in cde5fd1 Aug 13, 2020
@gopherbot
Copy link

@gopherbot gopherbot commented Aug 13, 2020

Change https://golang.org/cl/248401 mentions this issue: cmd/compile: correct type of CvtBoolToUint8 values

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
6 participants
You can’t perform that action at this time.