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: program that requires zerorange of >=32 bytes fails to compile on aix/ppc64 #34604

Closed
danscales opened this issue Sep 30, 2019 · 5 comments
Assignees
Milestone

Comments

@danscales
Copy link

@danscales danscales commented Sep 30, 2019

What version of Go are you using (go version)?

go version devel +616c39f6a6 Thu Sep 26 20:45:09 2019 +0000 linux/amd64

Does this issue reproduce with the latest release?

Yes

What operating system and processor architecture are you using (go env)?

go env Output
$ go env

GOARCH="ppc64"
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOOS="aix"
GOPPC64="power8"

What did you do?

Cross-compilation of this program on aix/ppc64 by setting GOOS and GOARCH:

package main

func main() {
	testDuffZero()
}

var glob = 3
var globp *int64

// Compile with GOARCH=ppc64 and GOOS=aix

//go:noinline
func testDuffZero() (r, s, t, u int64) {
	defer func() {
		glob = 4
	}()
	// Force the output params to be heap allocated. The pointer to each output
	// param must be zeroed in the prologue (see plive.go:epilogue()). So we
	// will get a block of stack slots that needs to be zeroed. For size >= 32
	// and <= 1024, this leads to a obj.ADUFFZERO call on ppc64 (see
	// ppc64/ggen.go:zerorange), which then leads to a bug because of AIX's
	// use of obj9.go. It looks like progedit() changes the ADUFFZERO to have
	// a To.Type of TYPE_BRANCH, which then causes diagnostic in
	// rewriteToUseTOC()
	//
	// Error is:
	// tmp.go:19:2: do not know how to handle 00121 (tmp.go:11)        DUFFZERO        runtime.duffzero(SB) without TYPE_MEM

	globp = &r
	globp = &s
	globp = &t
	globp = &u
	return
}

What did you expect to see?

For the .go file to compile. It compiles fine on linux/amd64 architecture.

What did you see instead?

test.go:33:2: do not know how to handle 00121 (test.go:13)        DUFFZERO        runtime.duffzero(SB) without TYPE_MEM

I constructed this test case, after I ran into a similar problem when testing my open-coded defers CL https://go-review.googlesource.com/c/go/+/190098 . The open-coded defer implementation may require zeroing stack slots storing defer args, so the use of Arch.ZeroRange with larger ranges is more likely.

As described in the comments above, the pointer to each output param must be zeroed in the prologue (see plive.go:epilogue()). So we will get a block of stack slots that needs to be zeroed, and the compiler generates a call to obj.ADUFFZERO of size 32 on aix/ppc64 (see ppc64/ggen.go/zerorange). AIX uses obj9.go, which leads to the bug. It looks like progedit() changes the ADUFFZERO to have a To.Type of TYPE_BRANCH (see the first case), which then causes diagnostic in rewriteToUseTOC(). (under the 'p.To.Name == obj.NAME_EXTERN' if branch)

Seems most likely a fix would be in progedit(), but could also be in rewriteToUseTOC(), or we could change ppc64/ggen.co/zerorange to not use ADUFFZERO (use the other two zeroing methods exclusively).

@Helflym

This comment has been minimized.

Copy link
Contributor

@Helflym Helflym commented Sep 30, 2019

I've somehow missed the part with ADUFFZERO when I've adapted rewriteToUseGot into rewriteToUseTOC. I'll send a CL in order to fix it.
Note that it only appears now (almost a year after the implementation) that the ADUFFZERO instruction wasn't correctly handled. It seems that there is no tests at all, generating at least one ADUFFZERO. Is this the case or they are somehow disabled on aix/ppc64 ?

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Sep 30, 2019

Change https://golang.org/cl/197842 mentions this issue: cmd/internal/obj/ppc64: Fix ADUFFxxxx generation on aix/ppc64

@danscales

This comment has been minimized.

Copy link
Author

@danscales danscales commented Sep 30, 2019

It seems that there is no tests at all, generating at least one ADUFFZERO. Is this the case or they are somehow disabled on aix/ppc64 ?

Yes, it looks like there are no explicit tests to generate zerorange requests (which for certain ranges of sizes usually causes an ADUFFZERO call on most architectures). In current tests, we only do zerorange of 8 bytes (one pointer) If you want to make sure that your change works with my test program and check it in (after review), I can then add a bunch of small test cases for a variety of zerorange/ADUFFZERO sizes (generated similarly to my test program), probably in a file cmd/compile/internal/gc/zerorange_test.go. Alternatively, you can add the test yourself in your CL.

Thanks for the quick response to the bug!

@laboger

This comment has been minimized.

Copy link
Contributor

@laboger laboger commented Oct 1, 2019

@gopherbot gopherbot closed this in 09c9bce Oct 1, 2019
@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Oct 1, 2019

Change https://golang.org/cl/198045 mentions this issue: cmd/compile: add an explicit test for compile of arch.ZeroRange

gopherbot pushed a commit that referenced this issue Oct 2, 2019
Add a test that causes generation of arch.ZeroRange calls of various sizes 8-136
bytes in the compiler. This is to test that ZeroRanges of various sizes actually
compile on different architectures, but is not testing runtime correctness (which
is hard to do).

Updates #34604

Change-Id: I4131eb86669bdfe8d4e36f4ae5c2a7b069abd6c4
Reviewed-on: https://go-review.googlesource.com/c/go/+/198045
Run-TryBot: Dan Scales <danscales@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Cherry Zhang <cherryyz@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.