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: dev.typeparams commit 0a0e3a3 broke dev.boringcrypto; fixed again at a72a499 #47227

Closed
mdempsky opened this issue Jul 15, 2021 · 6 comments
Closed

cmd/compile: dev.typeparams commit 0a0e3a3 broke dev.boringcrypto; fixed again at a72a499 #47227

mdempsky opened this issue Jul 15, 2021 · 6 comments
Labels
FrozenDueToAge help wanted NeedsFix The path to resolution is known, but the work has not been done. Suggested Issues that may be good for new contributors looking for work to do.
Milestone
Unplanned

Comments

@mdempsky
Copy link
Member

Merging 0a0e3a3 into dev.boringcrypto causes make.bash to fail due to:

Building Go toolchain2 using go_bootstrap and Go toolchain1.
# crypto/internal/boring
<autogenerated>:1: internal compiler error: offset for r1 at $WORK/b060/_cgo_gotypes.go:842:75 changed from 8 to 0

This build failure was fixed a few commits later at a72a499.

That later commit was just an optimization, so it shouldn't be essential to correctly handling compiling this code. It would be good to get a minimal, standalone reproducer for what failed and add it as a compiler regress test.

To reproduce the issue:

  1. Checkout dev.boringcrypto.
  2. Run git merge 0a0e3a3
  3. Run ./make.bash in src.

/cc @cherrymui @cuonglm
@mdempsky mdempsky added Suggested Issues that may be good for new contributors looking for work to do. help wanted NeedsFix The path to resolution is known, but the work has not been done. labels Jul 15, 2021
@mdempsky mdempsky added this to the Unplanned milestone Jul 15, 2021
@cuonglm
Copy link
Member

cuonglm commented Jul 16, 2021 •
edited

I haven't looked deeper, but here's a simple reproducer:

package p

// static void f(int i) {}
import "C"

func f() {
	defer C.f(C.int(1))
}

Just checkout commit 0a0e3a3 on dev.typeparams.

@gopherbot
Copy link

Change https://golang.org/cl/334882 mentions this issue: [dev.typeparams] test: add regression test for go/defer wrapper

@cuonglm cuonglm self-assigned this Jul 16, 2021
@gopherbot
Copy link

Change https://golang.org/cl/334883 mentions this issue: [dev.typeparams] cmd/compile: make ssagen analyze ABI correctly for static function value

@cuonglm
Copy link
Member

cuonglm commented Jul 17, 2021 •
edited

@mdempsky @cherrymui So the problem is not related to 0a0e3a3 and a72a499. This failed to compile even with current dev.typeparams tip:

$ cat _cgo_gotypes.go
package p

//go:cgo_unsafe_args
func g(*int) (r1 struct{}) {
	return
}

func f() {
	x := g
	defer x(new(int))
}

The problem is that ssagen/ssa.go:state.call does't handle direct function value, so it use the wrong ABI to set the offset, which causes offset changed.

gopherbot pushed a commit that referenced this issue Jul 21, 2021
@cuonglm
[dev.typeparams] test: add regression test for go/defer wrapper
d5f6ba9 
CL 330330 moved logic for wrapping go/defer from order to esacpe
analysis. It introduced a bug involves go/defer statement with ABI0
functions.

Consider this following code:

	package p

	//go:cgo_unsafe_args
	func g(*int) (r1 struct{}) {
		return
	}

	func f() {
		defer g(new(int))
	}

g is a cgo-like generated function with ABI0. While compiling g, we set
the offset per ABI0.

The function f is rewritten into:

	func f() {
		_0, _1 := g, new(int)
		defer func() { _0(_1) }()
	}

The temporary _0 hold function value with the same type as g, but with
class PAUTO. Thus ssagen/ssa.go:state.call cannot handle it and use
ABIDefault to set the offset, causes the offset of r1 changed

CL 330332 intended to optimize code generated for wrapping function, by
rewriting the wrapper function into:

	func f() {
		_0 := new(int)
		defer func() { g(_0) }()
	}

So it fixed the bug unintentionally.

This CL add regression test for this bug, and also add a comment to
explain while not wrapping declared function is important.

Updates #47227

Change-Id: I75c83d1d9cc7fd4699e6b218a295d0c0a10ef471
Reviewed-on: https://go-review.googlesource.com/c/go/+/334882
Trust: Cuong Manh Le <cuong.manhle.vn@gmail.com>
Run-TryBot: Cuong Manh Le <cuong.manhle.vn@gmail.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
@cherrymui cherrymui mentioned this issue Jul 21, 2021
Closed
@gopherbot
Copy link

Change https://golang.org/cl/336629 mentions this issue: cmd/compile: do not change field offset in ABI analysis

@gopherbot
Copy link

Change https://golang.org/cl/336610 mentions this issue: [dev.typeparams] cmd/compile: remove outdate TODO in escape analysis

gopherbot pushed a commit that referenced this issue Jul 22, 2021
@cuonglm
[dev.typeparams] cmd/compile: remove outdate TODO in escape analysis
73162a5 
We now understand the root cause of #47227, it will be fixed in #47317.

Change-Id: Ifcd44f887a0bd3195818df33e409bd3e818e0b27
Reviewed-on: https://go-review.googlesource.com/c/go/+/336610
Trust: Cuong Manh Le <cuong.manhle.vn@gmail.com>
Run-TryBot: Cuong Manh Le <cuong.manhle.vn@gmail.com>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
@rsc rsc unassigned cuonglm Jun 23, 2022
@golang golang locked and limited conversation to collaborators Jun 23, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge help wanted NeedsFix The path to resolution is known, but the work has not been done. Suggested Issues that may be good for new contributors looking for work to do.
Projects
None yet
Milestone
Unplanned
Development

No branches or pull requests
3 participants
@mdempsky @cuonglm @gopherbot