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: panic message for failed assertion of generic type mentions shape type, not user type #53276

Open
mdempsky opened this issue Jun 7, 2022 · 2 comments
Labels
NeedsFix
Milestone

Comments

@mdempsky
Copy link
Member

@mdempsky mdempsky commented Jun 7, 2022

https://go.dev/play/p/3Ru8nGnvTQY

The above program reports at run-time "panic: interface conversion: interface { T() go.shape.int_0 } is nil, not int".

I think the correct error is "panic: interface conversion: interface { T() int } is nil, not int".

The problem is the call to s.reflectType(src) in ssagen/ssa.go. This is too late to do dictionary lookups, so we end up using the *runtime._type for the shape type, rather than the user type.

We should push all the reflectdata.TypePtr / reflectdata.TypeLinksym calls earlier into the frontend, so they can be replaced with dictionary accesses when appropriate. Same applies to reflectdata.ITabAddr and reflectdata.ITabLSym.

/cc @randall77 @dr2chase @cuonglm

@mdempsky mdempsky added the NeedsFix label Jun 7, 2022
@mdempsky mdempsky added this to the Go1.19 milestone Jun 7, 2022
@mdempsky mdempsky removed this from the Go1.19 milestone Jun 10, 2022
@mdempsky mdempsky added this to the Go1.20 milestone Jun 10, 2022
@gopherbot
Copy link

@gopherbot gopherbot commented Jun 10, 2022

Change https://go.dev/cl/411695 mentions this issue: [dev.unified] test: add regress tests for #53276 and #53328

gopherbot pushed a commit that referenced this issue Jun 10, 2022
These two tests fail with the 1.18 compiler frontend, because of
incomplete dictionary support. This CL adds the tests for Unified IR,
which currently handles them correctly, to make sure it doesn't repeat
the same errors.

Updates #53276.
Updates #53328.

Change-Id: I9f436495d28f2bc5707a17bd2527c86abacf91f1
Reviewed-on: https://go-review.googlesource.com/c/go/+/411695
Auto-Submit: Matthew Dempsky <mdempsky@google.com>
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: David Chase <drchase@google.com>
@gopherbot
Copy link

@gopherbot gopherbot commented Jun 21, 2022

Change https://go.dev/cl/413357 mentions this issue: [dev.unified] cmd/compile: avoid reflectType in ssagen

gopherbot pushed a commit that referenced this issue Jun 21, 2022
This CL adds alternate code paths for the frontend to plumb through
rtypes to package ssagen, so the latter doesn't have to use
reflectType (which in general will only have access to shape types).

Note: This CL doesn't yet plumb through the rtypes for variables that
escape to the heap. However, those rtypes are only used for calling
runtime.newobject, and the status quo as of Go 1.18 is already to use
shape rtypes for most runtime.newobject calls. (Longer term though, I
would like to get rid of shape rtypes altogether.)

Passes toolstash -cmp.

Updates #53276.

Change-Id: I76a281eca8300de2e701fbac89ead32f8568a5f2
Reviewed-on: https://go-review.googlesource.com/c/go/+/413357
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
Reviewed-by: David Chase <drchase@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsFix
Projects
None yet
Development

No branches or pull requests

2 participants