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

Jump to bottom

cmd/compile: internal compiler error: missing typecheck [1.19 backport] #56744

Closed
gopherbot opened this issue Nov 15, 2022 · 10 comments
Closed

cmd/compile: internal compiler error: missing typecheck [1.19 backport] #56744

gopherbot opened this issue Nov 15, 2022 · 10 comments
Labels
CherryPickApproved Used during the release process for point releases compiler/runtime Issues related to the Go compiler and/or runtime. FrozenDueToAge
Milestone
Go1.19.5

Comments

@gopherbot
Copy link

@randall77 requested issue #56727 to be considered for backport to the next 1.19 minor release.

@gopherbot please open a backport issue for 1.19.
@gopherbot gopherbot added the CherryPickCandidate Used during the release process for point releases label Nov 15, 2022
@gopherbot gopherbot mentioned this issue Nov 15, 2022
Closed
@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Nov 15, 2022
@gopherbot gopherbot added this to the Go1.19.4 milestone Nov 15, 2022
@gopherbot
Copy link
Author

Change https://go.dev/cl/451155 mentions this issue: [release-branch.go1.19] cmd/compile: fix missing typecheck for static initialization slice

@dmitshur
Copy link
Contributor

dmitshur commented Nov 16, 2022 •
edited

https://go.dev/wiki/MinorReleases includes:

The issue should include [...] and a short rationale about why the backport might be needed.

@randall77 Can you please add a rationale for why this backport should be made? It's needed for the cherry-pick review process to make progress. Thanks.

@randall77
Copy link
Contributor

Some static initialization code causes the compiler to crash.
(This issue is a fix-to-the-fix for #56105, which has already been backported.)

@cherrymui
Copy link
Member

We discussed with the release team, and we think we don't know exactly the importance and safety for this backport. Could the compiler team give more input to make the decision? Thanks.

This was referenced Nov 23, 2022
Closed
Closed
@randall77
Copy link
Contributor

This backport I would describe as mildly important. Without it there would be some static initialization code that causes the compiler to crash. It isn't common, but it isn't fuzz-generated either. Real people have run into it. I suspect it is not too hard to work around.
The fix for #56105 was for the same underlying issue, but was incomplete. This issue+CL fixes that incompleteness.

The backport is very safe. It just does some extra typechecking.

@cherrymui
Copy link
Member

Thanks.

As #56727 (comment) mentioned, this issue was not in Go 1.19.2 but caused by a backport of #56105 in 1.19.3. I guess another option would be reverting that backport. The input in #56105 does look a bit contrived. The concern here is that the backport of #56105 has caused a chain of more issues and backports. How would that sound? Thanks.

@randall77
Copy link
Contributor

Yes, the other option is to revert the fix for #56106. That seems ok to me. The test case for that issue is certainly more contrived than this one.

There's an outside chance that reverting the fix for #56106 leaves open the possibility of incorrectly compiled code instead of an ICE. But if it is possible it is certainly very rare.

@cuonglm
Copy link
Member

cuonglm commented Nov 24, 2022

I'm still leaning to backport theses.

Isn't that without #56106, people won't be able to use fuzzing?

@aclements
Copy link
Member

I think we should go ahead with the backport. The backport fix is very safe (doing an unnecessary typecheck is a no-op), and can't make the situation worse. Whereas, as Keith pointed out, there's some chance that reverting the original fix reintroduces a miscompilation, which is much worse than an ICE. On balance, I support landing the backport.

@gopherbot gopherbot modified the milestones: Go1.19.4, Go1.19.5 Dec 6, 2022
@prattmic prattmic added the CherryPickApproved Used during the release process for point releases label Dec 7, 2022
@gopherbot gopherbot removed the CherryPickCandidate Used during the release process for point releases label Dec 7, 2022
@gopherbot
Copy link
Author

Closed by merging 926ffba to release-branch.go1.19.

gopherbot pushed a commit that referenced this issue Dec 9, 2022
@cuonglm @prattmic
[release-branch.go1.19] cmd/compile: fix missing typecheck for static…
926ffba 
… initialization slice

CL 440455 fixed missing walk pass for static initialization slice.
However, slicelit may produce un-typechecked node, thus we need to do
typecheck for sinit before calling walkStmtList.

Fixes #56744

Change-Id: I40730cebcd09f2be4389d71c5a90eb9a060e4ab7
Reviewed-on: https://go-review.googlesource.com/c/go/+/450215
Reviewed-by: Keith Randall <khr@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Cherry Mui <cherryyz@google.com>
Auto-Submit: Cuong Manh Le <cuong.manhle.vn@gmail.com>
Reviewed-by: Keith Randall <khr@google.com>
Run-TryBot: Cuong Manh Le <cuong.manhle.vn@gmail.com>
Reviewed-on: https://go-review.googlesource.com/c/go/+/451155
Reviewed-by: Joedian Reid <joedian@golang.org>
@golang golang locked and limited conversation to collaborators Dec 9, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
CherryPickApproved Used during the release process for point releases compiler/runtime Issues related to the Go compiler and/or runtime. FrozenDueToAge
Projects
None yet
Milestone
Go1.19.5
Development

No branches or pull requests
7 participants
@prattmic @dmitshur @aclements @cuonglm @randall77 @gopherbot @cherrymui