-
Notifications
You must be signed in to change notification settings - Fork 17.8k
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: compile failed with "Value live at entry" #53702
Comments
This is the commit that introduced the bug: a3bb28e |
I tried to change the loop from range to by step,just like func (s *Base) Do() *int {
resp := &g_val
for i := 0; i < 8; i++ {
//for _, e := range s.elem {
s.elem[i].Wait(func() {
*resp = 0
})
}
return resp
} Compiled with Without the change, compiled with go1.17 we got
And with the change, compiled with go1.17 we got
The range is missing, and then compiled with go1.18.3 we got
So the bug maybe introduced not only a3bb28e |
Writing
is enough to trigger the bug. |
git bisect points to https://go-review.googlesource.com/c/go/+/327871 cc @mdempsky Unified IR does not have this bug, because it generates its own wrapper instead of relying on |
Change https://go.dev/cl/423434 mentions this issue: |
That's surprising. That commit should have only affect devirtualizationed, but the test case doesn't have any interface types. If we revert just the typecheck.NeedITab stuff, does it fix the issue? (We shouldn't need it for unified IR, but maybe it was important to !unified mode.) |
I think the problem comes from generating method wrapper, not interface types. After CL 327871, we now always do inlining for method wrapper after global escape analysis. Note that disable inlining will make it compiled in non-unified.
No. |
The issue is expected to be fixed when Unified IR is enabled by default, so adding a test to make sure thing works correctly. Updates #53702 Change-Id: Id9d7d7ca4506103df0d10785ed5ee170d69988ba Reviewed-on: https://go-review.googlesource.com/c/go/+/423434 TryBot-Result: Gopher Robot <[email protected]> Reviewed-by: Matthew Dempsky <[email protected]> Reviewed-by: Ian Lance Taylor <[email protected]> Run-TryBot: Cuong Manh Le <[email protected]> Auto-Submit: Cuong Manh Le <[email protected]>
Change https://go.dev/cl/423834 mentions this issue: |
The test requires inlining happens. Updates #53702 Change-Id: I0d93b5e29e271ace4098307b74c40c0e06d975e5 Reviewed-on: https://go-review.googlesource.com/c/go/+/423834 TryBot-Result: Gopher Robot <[email protected]> Reviewed-by: Keith Randall <[email protected]> Auto-Submit: Cuong Manh Le <[email protected]> Reviewed-by: Matthew Dempsky <[email protected]> Reviewed-by: Keith Randall <[email protected]> Run-TryBot: Cuong Manh Le <[email protected]>
This is now fixed at tip, could we close? @mdempsky |
@cuonglm The issue is reportedly still a regression from Go 1.17. It's also not specifically a generics issue, so I think we should try to backport fixes for Go 1.18/1.19. |
Change https://go.dev/cl/426334 mentions this issue: |
@gopherbot please backport this issue to go1.18 and go1.19 |
Backport issue(s) opened: #54725 (for 1.18), #54726 (for 1.19). Remember to create the cherry-pick CL(s) as soon as the patch is submitted to master, according to https://go.dev/wiki/MinorReleases. |
What version of Go are you using (
go version
)?Does this issue reproduce with the latest release?
Yes
What operating system and processor architecture are you using (
go env
)?go env
OutputWhat did you do?
What did you expect to see?
compile with no error
What did you see instead?
it's ok with go1.16 and go1.17
I have tried patch which didn't work:
The text was updated successfully, but these errors were encountered: