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

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

Comments

@mdempsky
Copy link
Member

@mdempsky mdempsky commented Jul 15, 2021

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

@cuonglm
Copy link
Member

@cuonglm cuonglm commented Jul 16, 2021

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.

Loading

@gopherbot
Copy link

@gopherbot gopherbot commented Jul 16, 2021

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

Loading

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

@gopherbot gopherbot commented Jul 17, 2021

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

Loading

@cuonglm
Copy link
Member

@cuonglm cuonglm commented Jul 17, 2021

@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.

Loading

gopherbot pushed a commit that referenced this issue Jul 21, 2021
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>
@gopherbot
Copy link

@gopherbot gopherbot commented Jul 22, 2021

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

Loading

@gopherbot
Copy link

@gopherbot gopherbot commented Jul 22, 2021

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

Loading

gopherbot pushed a commit that referenced this issue Jul 22, 2021
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>
@gopherbot gopherbot closed this in 052da57 Jul 22, 2021
steeve added a commit to znly/go that referenced this issue Aug 19, 2021
Currently, the ABI analysis assigns parameter/result offsets
to the fields of function *Type. In some cases, we may have
an ABI0 function reference and an ABIInternal reference share
the same function *Type. For example, for an ABI0 function F,
"f := F" will make f and (ABI0) F having the same *Type. But f,
as a func value, should use ABIInternal. Analyses on F and f will
collide and cause ICE.

Also, changing field offsets in ABI analysis has to be done very
carefully to avoid data races. It has been causing
trickiness/difficulty.

This CL removes the change of field offsets in ABI analysis
altogether. The analysis result is stored in ABIParamAssignment,
which is the only way to access parameter/result stack offset now.

Fixes golang#47317.
Fixes golang#47227.

Change-Id: I23a3e081a6cf327ac66855da222daaa636ed1ead
Reviewed-on: https://go-review.googlesource.com/c/go/+/336629
Trust: Cherry Mui <cherryyz@google.com>
Reviewed-by: Cuong Manh Le <cuong.manhle.vn@gmail.com>
Reviewed-by: Than McIntosh <thanm@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
3 participants