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
Closed

cmd/compile: ICE: not lowered: v58, ITab PTR64 PTR64 #22327

mdempsky opened this issue Oct 18, 2017 · 9 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. release-blocker
Milestone

Comments

@mdempsky
Copy link
Member

mdempsky 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
Copy link
Contributor

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
Copy link
Contributor

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

@randall77
Copy link
Contributor

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

@mdempsky
Copy link
Member Author

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
Copy link
Member Author

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 the NeedsFix The path to resolution is known, but the work has not been done. label Mar 29, 2018
@ianlancetaylor ianlancetaylor added this to the Go1.11 milestone Mar 29, 2018
@ianlancetaylor
Copy link
Contributor

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

@ianlancetaylor
Copy link
Contributor

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

@randall77
Copy link
Contributor

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

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

@golang golang locked and limited conversation to collaborators Oct 22, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. release-blocker
Projects
None yet
Development

No branches or pull requests

4 participants