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: irgen uses wrong dict param to generate code for getting dict type #51413

Closed
mdempsky opened this issue Mar 1, 2022 · 12 comments
Closed

cmd/compile: irgen uses wrong dict param to generate code for getting dict type #51413

mdempsky opened this issue Mar 1, 2022 · 12 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Go1.19

Comments

@mdempsky
Copy link
Contributor

mdempsky commented Mar 1, 2022

Since submitting https://go-review.googlesource.com/c/go/+/388534, the linux-amd64-noopt builder is failing in test/typeparam/mdempsky/13.go:

https://build.golang.org/log/e4383af359722053b9d212fc3c8c69351e0ffbd1

##### ../test
# go run run.go typeparam/mdempsky/13.go
exit status 2
# command-line-arguments
typeparam/mdempsky/13.go:38:3: internal compiler error: 'G[go.shape.interface { Abs() "".MyInt }_0,go.shape.int_1].func1': addr of undeclared ONAME .dict. declared: map[a0:v5 r0:v6 .dict:v4]

I'm not immediately able to reproduce this failure locally.

/cc @danscales @randall77
@mdempsky mdempsky added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Mar 1, 2022
@mdempsky
Copy link
Contributor Author

mdempsky commented Mar 1, 2022

Oh, I forgot the noopt builder uses -N -l, not just -N.

This is repro'able with go run -gcflags="-N -l" test/typeparam/mdempsky/13.go.

A slightly different failure message occurs with -gcflags=-l instead:

# command-line-arguments
./13.go:38:3: internal compiler error: 'G[go.shape.interface { Abs() "".MyInt }_0,go.shape.int_1].func1': Value live at entry. It shouldn't be. func G[go.shape.interface { Abs() "".MyInt }_0,go.shape.int_1].func1, node .dict, value nil

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/388894 mentions this issue: test: workaround codegen bug in typeparam/mdempsky/13.go

gopherbot pushed a commit that referenced this issue Mar 1, 2022
@mdempsky
test: workaround codegen bug in typeparam/mdempsky/13.go
f4722d8 
This test case is failing on the noopt builder, because it disables
inlining. Evidently the explicit -gcflags flag in all of our generics
tests was overriding the noopt builder's default mode.

This CL restores a noop -gcflags to get the builder green again until
the issue can be properly fixed.

Updates #51413.

Change-Id: I61d22a007105f756104ba690b73f1d68ce4be281
Reviewed-on: https://go-review.googlesource.com/c/go/+/388894
Trust: Matthew Dempsky <mdempsky@google.com>
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
Reviewed-by: Keith Randall <khr@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
@contrast-jproberts contrast-jproberts mentioned this issue Mar 2, 2022
Closed
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/391994 mentions this issue: cmd/compile: fix wrong dict param when getting dict type

@cuonglm
Copy link
Member

cuonglm commented Mar 14, 2022

It seems to me this issue should be cherry pick to 1.18 branch

cc @randall77 @danscales @mdempsky

@mdempsky
Copy link
Contributor Author

I think so too. Otherwise delve will have issues debugging generic code, right?

@cuonglm
Copy link
Member

cuonglm commented Mar 14, 2022 •
edited
Loading

I think so too. Otherwise delve will have issues debugging generic code, right?

Yeah, indeed. delve is the biggest user of -gcflags="-N -l"

@mdempsky mdempsky added this to the Go1.18 milestone Mar 14, 2022
@cuonglm cuonglm 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 Mar 14, 2022
@cuonglm
Copy link
Member

cuonglm commented Mar 14, 2022

Reopening for backport to 1.18 release branch.

@cuonglm cuonglm reopened this Mar 14, 2022
@dmitshur dmitshur mentioned this issue Mar 14, 2022
Closed
33 tasks
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/392614 mentions this issue: [release-branch.go1.18] cmd/compile: fix wrong dict param when getting dict type

@dmitshur
Copy link
Contributor

@cuonglm Thanks for flagging this issue.

We've discussed it on the release (and compiler) team. It is important for it to be fixed, but this late in the Go 1.18 release cycle we need to prioritize not to make more changes before the final release.

As a result, we'll consider this fix for backport to Go 1.18.1 (which is no more 2-3 weeks away from today) rather than blocking the final Go 1.18 release on it (scheduled for this week). Updating this issue accordingly.

@dmitshur dmitshur modified the milestones: Go1.18, Go1.19 Mar 14, 2022
@dmitshur
Copy link
Contributor

@gopherbot Please consider this for backport to 1.18. This is a serious issue—see #51460 (comment) and #51460 (comment) for detailed rationale to backport.

@gopherbot gopherbot mentioned this issue Mar 14, 2022
Closed
@gopherbot
Copy link
Contributor

Backport issue(s) opened: #51669 (for 1.18).

Remember to create the cherry-pick CL(s) as soon as the patch is submitted to master, according to https://go.dev/wiki/MinorReleases.

@cuonglm cuonglm changed the title cmd/compile: test/typeparam/mdempsky/13.go failing on noopt builder cmd/compile: irgen uses wrong dict param to generate code for getting dict type Mar 16, 2022
@cuonglm
Copy link
Member

cuonglm commented Mar 16, 2022

This issue can now be closed.

@cuonglm cuonglm closed this as completed Mar 16, 2022
@golang golang locked and limited conversation to collaborators Mar 16, 2023
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.19
Development

No branches or pull requests
4 participants
@mdempsky @dmitshur @cuonglm @gopherbot