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: walkConvInterface produces broken IR [1.19 backport] #56770

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

cmd/compile: walkConvInterface produces broken IR [1.19 backport] #56770

gopherbot opened this issue Nov 16, 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

Comments

@gopherbot
Copy link

gopherbot commented Nov 16, 2022

@cuonglm requested issue #56768 to be considered for backport to the next 1.19 minor release.

@gopherbot please backport this to go1.19

Some static initialization code causes the compiler to crash.

@gopherbot gopherbot added the CherryPickCandidate Used during the release process for point releases label Nov 16, 2022
@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Nov 16, 2022
@gopherbot gopherbot added this to the Go1.19.4 milestone Nov 16, 2022
@toothrot
Copy link
Contributor

@cuonglm Hello! Could you please provide rationale as per https://go.dev/wiki/MinorReleases?

@cuonglm
Copy link
Member

cuonglm commented Nov 16, 2022

Hi @toothrot, I did add the rationale in the issue description:

Some static initialization code causes the compiler to crash.

@gopherbot
Copy link
Author

Change https://go.dev/cl/451875 mentions this issue: [release-branch.go1.19] cmd/compile: fix broken IR for iface -> eface

@cherrymui
Copy link
Member

Similar to #56744 , we'd need better understanding to decide whether we should backport this. Thanks.

@aclements
Copy link
Member

Maybe other people understand this, but I'm confused by how we weren't covering this code path in walkConvInterface before. It seems like this code path would always result in an ICE, but it's been around since 57668b8 (Aug 2021). Were we actually never taking this code path until the fix for #56727, or is there something I'm missing about that fix that makes this path in walkConvInterface construct bad IR?

@cuonglm
Copy link
Member

cuonglm commented Nov 30, 2022

Maybe other people understand this, but I'm confused by how we weren't covering this code path in walkConvInterface before. It seems like this code path would always result in an ICE, but it's been around since 57668b8 (Aug 2021). Were we actually never taking this code path until the fix for #56727, or is there something I'm missing about that fix that makes this path in walkConvInterface construct bad IR?

Because after walkConvInterface produced bad IR, this bad IR is never typechecked, it's just pass to backend as-is. For OITAB, a generic pointer (*uintpr, *int32 ...) would satisfy the backend.

There're many places in the walk pass that we produce an ir.Node then call SetTypecheck(1), instead of calling typecheck.{Stmt,Expr} on generated node.

@aclements
Copy link
Member

Ah, so just to clarify further, we were taking that code path in walkConvInterface before, but only in places where where we also called SetTypecheck(1)?

@cuonglm
Copy link
Member

cuonglm commented Dec 1, 2022

Ah, so just to clarify further, we were taking that code path in walkConvInterface before, but only in places where where we also called SetTypecheck(1)?

Yes. The problem is more hidden for walkConvInterface, since the bad IR is put into init nodes, so it's harder to trigger the bug. Only until the fix to order slice literal, walkStmtList is called on init nodes, the missing typecheck bug was reveal. Then to fix the missing typecheck bug, typecheck.Stmts was called, which reveal the type mismatch in itab assignment.

@gopherbot gopherbot modified the milestones: Go1.19.4, Go1.19.5 Dec 6, 2022
@thanm
Copy link
Contributor

thanm commented Dec 14, 2022

discussed at the weekly meeting, approved

@thanm thanm added CherryPickApproved Used during the release process for point releases and removed CherryPickCandidate Used during the release process for point releases labels Dec 14, 2022
@gopherbot
Copy link
Author

Closed by merging 5758a14 to release-branch.go1.19.

gopherbot pushed a commit that referenced this issue Dec 28, 2022
For implementing interface to empty interface conversion, the compiler
generate code like:

	var res *uint8
	res = itab
	if res != nil {
		res = res.type
	}

However, itab has type *uintptr, so the assignment is broken. The
problem is not shown up, until CL 450215, which call typecheck on this
broken assignment.

To fix this, just cast itab to *uint8 when doing the conversion.

Fixes #56770

Change-Id: Id42792d18e7f382578b40854d46eecd49673792c
Reviewed-on: https://go-review.googlesource.com/c/go/+/451256
Reviewed-by: Keith Randall <khr@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Cuong Manh Le <cuong.manhle.vn@gmail.com>
Reviewed-by: Keith Randall <khr@golang.org>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
Auto-Submit: Cuong Manh Le <cuong.manhle.vn@gmail.com>
Reviewed-on: https://go-review.googlesource.com/c/go/+/451875
Auto-Submit: Dmitri Shuralyov <dmitshur@google.com>
Reviewed-by: Joedian Reid <joedian@golang.org>
@golang golang locked and limited conversation to collaborators Dec 28, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
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
Development

No branches or pull requests

6 participants