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: ICE: not lowered: v58, ITab PTR64 PTR64 #22327

Closed
mdempsky opened this issue Oct 18, 2017 · 9 comments

Comments

Projects
None yet
4 participants
@mdempsky
Copy link
Member

commented Oct 18, 2017

At tip, this package fails to compile with an ICE in SSA:

package p
func f() ([]interface{}, *int) {
    return nil, nil
}
var _ = append(f())

/cc @randall77

@randall77

This comment has been minimized.

Copy link
Contributor

commented Oct 18, 2017

go1.2 reports multiple-value f() in single-value context, and doesn't ICE.
go1.4 compiles it successfully (!)
go1.[56] ICEs with cgen_wb of type interface {}
go1.[789] to current ICE with not lowered: ITab PTR64 PTR64

I think go1.2 is correct. I think if we reported that error correctly, we'd never pass the bad code to the backend, and then we wouldn't ICE.

@randall77

This comment has been minimized.

Copy link
Contributor

commented Oct 18, 2017

go1.4 generated code is "correct", in that it does convert the *int to an interface{} and appends it to a []interface{}.

@randall77

This comment has been minimized.

Copy link
Contributor

commented Oct 18, 2017

Hmmm, looks like append of the result of a multi-return function does generally work.
Maybe go1.4 is correct?

@mdempsky

This comment has been minimized.

Copy link
Member Author

commented Oct 18, 2017

Oh, yes, the code should compile correctly, like it did with go1.4.

go/types and gccgo accept the code. (Sorry, I should have mentioned that in the original report.)

@mdempsky

This comment has been minimized.

Copy link
Member Author

commented Oct 18, 2017

Looking at cmd/compile's -W output, I think maybe this is an issue in walk instead. Currently we're rewriting append(f()) into something like:

t0, t1 := f()
append([]interface{}(t0), t1)

I think what we actually mean to do is rewrite it to:

t0, t1 := f()
append(t0, interface{}(t1))

@ianlancetaylor ianlancetaylor added this to the Go1.11 milestone Mar 29, 2018

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Mar 29, 2018

Still happening with go version devel +9761a162f0 Thu Mar 29 16:37:26 2018 +0000 linux/amd64

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Jun 30, 2018

This has been broken for a while, so punting to 1.12, but marking as a release-blocker for that release.

@randall77

This comment has been minimized.

Copy link
Contributor

commented Oct 16, 2018

Looks like there is a missing CONVIFACE here.
Before order:

.   AS l(7) tc(1)
.   .   NAME-_ a(true) l(7) x(0) tc(1) SLICE-[]interface {}
.   .   APPEND l(7) tc(1) SLICE-[]interface {}
.   .   APPEND-list
.   .   .   CALLFUNC l(7) tc(1) STRUCT-([]interface {}, *int)
.   .   .   .   NAME-p.f a(true) l(3) x(0) class(PFUNC) tc(1) used FUNC-func() ([]interface {}, *int)

After order:

.   AS2FUNC l(7) tc(1)
.   AS2FUNC-list
.   .   NAME-p..autotmp_2 a(true) l(7) x(0) class(PAUTO) esc(N) tc(1) assigned used SLICE-[]interface {}

.   .   NAME-p..autotmp_3 a(true) l(7) x(0) class(PAUTO) esc(N) tc(1) assigned used PTR-*int
.   AS2FUNC-rlist
.   .   CALLFUNC l(7) tc(1) STRUCT-([]interface {}, *int)
.   .   .   NAME-p.f a(true) l(3) x(0) class(PFUNC) tc(1) used FUNC-func() ([]interface {}, *int)

.   AS2 l(7) tc(1)
.   AS2-list
.   .   NAME-p..autotmp_0 a(true) l(7) x(0) class(PAUTO) esc(N) tc(1) assigned used SLICE-[]interface {}

.   .   NAME-p..autotmp_1 a(true) l(7) x(0) class(PAUTO) esc(N) tc(1) assigned used PTR-*int
.   AS2-rlist
.   .   NAME-p..autotmp_2 a(true) l(7) x(0) class(PAUTO) esc(N) tc(1) assigned used SLICE-[]interface {}

.   .   NAME-p..autotmp_3 a(true) l(7) x(0) class(PAUTO) esc(N) tc(1) assigned used PTR-*int
.   AS l(7) tc(1)
.   .   NAME-_ a(true) l(7) x(0) tc(1) SLICE-[]interface {}
.   .   APPEND l(7) tc(1) SLICE-[]interface {}
.   .   APPEND-list
.   .   .   NAME-p..autotmp_0 a(true) l(7) x(0) class(PAUTO) esc(N) tc(1) assigned used SLICE-[]interface {}
.   .   .   NAME-p..autotmp_1 a(true) l(7) x(0) class(PAUTO) esc(N) tc(1) assigned used PTR-*int

Nowhere is there a CONVIFACE to convert the second append arugment from *int to interface{}.
Normally the typecheck pass, even before order, inserts CONVIFACE and the like ops to convert function arguments to the right types. We just don't do that here.
I think copyRet in order.go needs to use temporaries corresponding to the outer function's required argument types instead of the inner function's result types. Then the typecheck call it does should insert the conversions required.

@gopherbot

This comment has been minimized.

Copy link

commented Oct 16, 2018

Change https://golang.org/cl/142718 mentions this issue: cmd/compile: In append(f()), type convert appended items

@gopherbot gopherbot closed this in dca769d Oct 22, 2018

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.