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: unexpected panic introduced with c08b01e #25776

Closed
kortschak opened this issue Jun 7, 2018 · 5 comments

Comments

Projects
None yet
4 participants
@kortschak
Copy link
Contributor

commented Jun 7, 2018

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

go version devel +c08b01e Wed Jun 6 20:35:23 2018 +0000 linux/amd64

Does this issue reproduce with the latest release?

Not in latest release. Yes in master e5f0c1f

What operating system and processor architecture are you using (go env)?

$ go env
GOARCH="amd64"
GOBIN=""
GOEXE=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOOS="linux"
GORACE=""
GOTMPDIR=""
GCCGO="gccgo"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
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-build982235567=/tmp/go-build -gno-record-gcc-switches"

What did you do?

Run the code at https://play.golang.org/p/hr84SxOns8y

package main

import "gonum.org/v1/gonum/mat"

func main() {
	ta := mat.NewTriDense(3, mat.Upper, nil)
	ta.ScaleTri(1, ta)
}

I have attempted to produce a smaller reproducer that does not depend on gonum/mat without success.

What did you expect to see?

No output.

What did you see instead?

$ go run panic.go 
panic: matrix: triangular storage mismatch

goroutine 1 [running]:
gonum.org/v1/gonum/mat.(*TriDense).reuseAs(0xc000024080, 0x3, 0x7f2f9fa82300)
	/home/daniel/src/gonum.org/v1/gonum/mat/triangular.go:263 +0x1dc
gonum.org/v1/gonum/mat.(*TriDense).ScaleTri(0xc000024080, 0x3ff0000000000000, 0x4b6c80, 0xc000024080)
	/home/daniel/src/gonum.org/v1/gonum/mat/triangular.go:427 +0x70
main.main()
	/home/daniel/panic.go:7 +0xe9
exit status 2

Additional information

Gonum version used is gonum/gonum@438fe0c.

Bisected to Go commit c08b01e.

commit c08b01ecb4488fb3a95fd5cc7baa8b31812e7b76
Author: David Chase <drchase@google.com>
Date:   Wed Jun 6 12:38:35 2018 -0400

    cmd/compile: fix panic-okay-to-inline change; adjust tests
    
    This line of the inlining tuning experiment
    https://go-review.googlesource.com/c/go/+/109918/1/src/cmd/compile/internal/gc/inl.go#347
    was incorrectly rewritten in a later patch to use the call
    cost, not the panic cost, and thus the inlining of panic
    didn't occur when it should.  I discovered this when I
    realized that tests should have failed, but didn't.
    
    Fix is to make the correct change, and also to modify the
    tests that this causes to fail.  One test now asserts the
    new normal, the other calls "ppanic" instead which is
    designed to behave like panic but not be inlined.
    
    Change-Id: I423bb7f08bd66a70d999826dd9b87027abf34cdf
    Reviewed-on: https://go-review.googlesource.com/116656
    Run-TryBot: David Chase <drchase@google.com>
    TryBot-Result: Gobot Gobot <gobot@golang.org>
    Reviewed-by: Keith Randall <khr@golang.org>

Original ref gonum/gonum#522.

/cc @dr2chase

@kortschak kortschak changed the title cmd/compile: cmd/compile: unexpected panic with introduced with c08b01e Jun 7, 2018

@kortschak kortschak changed the title cmd/compile: unexpected panic with introduced with c08b01e cmd/compile: unexpected panic introduced with c08b01e Jun 7, 2018

@agnivade agnivade added this to the Go1.11 milestone Jun 7, 2018

@agnivade

This comment has been minimized.

Copy link
Member

commented Jun 7, 2018

Thanks for the bisect.

@dr2chase dr2chase self-assigned this Jun 7, 2018

@dr2chase

This comment has been minimized.

Copy link
Contributor

commented Jun 7, 2018

I'm looking at this, I suspect but am not sure it is some latent bug tickled by changes to panic-inlining.
Compilation of (*TriDense).Triangle is funny looking; it always returns 0 for the kind.

@dr2chase

This comment has been minimized.

Copy link
Contributor

commented Jun 11, 2018

FYI, longer but self-contained reproducer (and not-reproducer). It's delicate:

package main

const (
	Upper       = true
	blas_Upper  = 121
	badTriangle = "bad triangle"
)

// Triangular represents a triangular matrix. Triangular matrices are always square.
type Triangular interface {
	// Triangular returns the number of rows/columns in the matrix and its
	// orientation.
	Tryangle() (mmmm int, kynd bool)
	Triangle() (mmmm int, kynd bool)
}

// blas64_Triangular represents a triangular matrix using the conventional storage scheme.
type blas64_Triangular struct {
	Stride int
	Uplo   int
}

// TriDense represents an upper or lower triangular matrix in dense storage
// format.
type TriDense struct {
	mat blas64_Triangular
}

func NewTriDense() *TriDense {
	return &TriDense{
		mat: blas64_Triangular{
			Stride: 3,
			Uplo:   blas_Upper,
		},
	}
}

func (t *TriDense) isUpper() bool {
	return isUpperUplo(t.mat.Uplo)
}

func (t *TriDense) triKind() bool {
	return isUpperUplo(t.mat.Uplo)
}

func isUpperUplo(u int) bool {
	switch u {
	case blas_Upper:
		return true
	default:
		panic(badTriangle)
	}
}

func (t *TriDense) IsZero() bool {
	return t.mat.Stride == 0
}

//go:noinline
func (t *TriDense) ScaleTri(f float64, a Triangular) {
	n, kind := a.Triangle()
	println("ScaleTri n, kind=", n, ", ", kind)
}

//go:noinline
func (t *TriDense) ScaleTry(f float64, a Triangular) {
	n, kind := a.Tryangle()
	println("ScaleTry n, kind=", n, ", ", kind)
}

// Triangle fails
func (t *TriDense) Triangle() (nnnn int, kind bool) {
	return 3, !t.IsZero() && t.triKind()
}

// Tryangle works -- difference is not-named output parameters.
func (t *TriDense) Tryangle() (int, bool) {
	return 3, !t.IsZero() && t.triKind()
}

func main() {
	ta := NewTriDense()
	n, kind := ta.Triangle()
	println("    main n, kind=", n, ", ", kind)
	ta.ScaleTri(1, ta)
	ta.ScaleTry(1, ta)
}

The inlined switch is rewritten incorrectly in walk, resulting in a switch body that begins with a break.
Still haven't figured out why it does this:

.   .   .   .   .   SWITCH l(47) tc(1) int
.   .   .   .   .   SWITCH-body
.   .   .   .   .   .   BREAK l(47)

.   .   .   .   .   .   AS l(47) tc(1)
.   .   .   .   .   .   .   NAME-main..autotmp_10 a(true) l(47) x(0) class(PAUTO) esc(N) tc(1) assigned used int
.   .   .   .   .   .   .   NAME-main.u a(true) l(46) x(0) class(PAUTO) tc(1) assigned used int
@gopherbot

This comment has been minimized.

Copy link

commented Jun 12, 2018

Change https://golang.org/cl/118318 mentions this issue: cmd/compile: ensure that operand of ORETURN is not double-walked

@gopherbot gopherbot closed this in c359d75 Jun 14, 2018

@gopherbot

This comment has been minimized.

Copy link

commented Dec 17, 2018

Change https://golang.org/cl/154497 mentions this issue: cmd/compile: prevent double-walk of switch for OPRINT/OPRINTN

gopherbot pushed a commit that referenced this issue Dec 17, 2018

cmd/compile: prevent double-walk of switch for OPRINT/OPRINTN
When a println arg contains a call to an inlineable function
that itself contains a switch, that switch statement will be
walked twice, once by the walkexprlist formerly in the
OPRINT/OPRINTN case, then by walkexprlistcheap in walkprint.

Remove the first walkexprlist, it is not necessary.
walkexprlist =
		s[i] = walkexpr(s[i], init)
walkexprlistcheap = {
		s[i] = cheapexpr(n, init)
		s[i] = walkexpr(s[i], init)
}

Seems like this might be possible in other places, i.e.,
calls to inlineable switch-containing functions.

See also #25776.
Fixes #29220.

Change-Id: I3781e86aad6688711597b8bee9bc7ebd3af93601
Reviewed-on: https://go-review.googlesource.com/c/154497
Run-TryBot: David Chase <drchase@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Keith Randall <khr@golang.org>
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.