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.

Sign up for GitHub

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

Jump to bottom

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

Comments

@kazzmir
Copy link

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

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

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

@ALTree ALTree added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Aug 13, 2020
@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

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

@josharian
Copy link
Contributor

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

@gopherbot gopherbot mentioned this issue Aug 13, 2020
Closed
@gopherbot
Copy link

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

@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

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

@gopherbot
Copy link

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

@dmitshur dmitshur 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 Aug 21, 2020
@golang golang locked and limited conversation to collaborators Aug 21, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Milestone
Go1.16
Development

No branches or pull requests
6 participants
@josharian @kazzmir @dmitshur @ianlancetaylor @ALTree @gopherbot