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: register conflict between external linker and duffzero on arm64 #32773

Closed
xuanjiazhen opened this issue Jun 25, 2019 · 14 comments
Closed

Comments

@xuanjiazhen
Copy link

@xuanjiazhen xuanjiazhen commented Jun 25, 2019

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

$ go version
go version go1.12.4 linux/arm64

We build our Go app on arm64. In a recent version, we encountered a segment violation error when it run. Through gdb debugging we found that the error originated from the register conflict between external linker and duffzero (I guess).

The error occurred in a line in the runtime·duffzero in duff_arm64.s.
It is found by GDB that it is in the __runtime.duffzero_veneer function before entering runtime·duffzero. This function looks like this:

Dump of assembler code fo function __runtime.duffzero_veneer:
0x0000xxx<+0>: adrp x16,0x460000<runtime.call1073741824+8>
0x0000xxx<+4>: add x16,x16,#0xbec
0x0000xxx<+8>: br x16

br x16 will jump into runtime·duffzero.

TEXT runtime·duffzero(SB), NOSPLIT|NOFRAME, $0-0
	STP.P	(ZR, ZR), 16(R16)
	STP.P	(ZR, ZR), 16(R16)
	STP.P	(ZR, ZR), 16(R16)
...

But at this time the address in x16 is the address of the code segment. Then it went wrong.

cmd/internal/obj/arm64/a.out.go:
REGRT1 = REG_R16 // ARM64 IP0, for external linker, runtime, duffzero and duffcopy

It seems that external linker and duffzero has the same register.

I don't know if I have made it clear. I want to know if this is a bug?

@cherrymui

This comment has been minimized.

Copy link
Contributor

@cherrymui cherrymui commented Jun 25, 2019

I think we could change the register used in duffzero, if necessary.

Is __runtime.duffzero_veneer a trampoline generated by the C linker? Could you share the command you used to build (link) your program?

@cherrymui

This comment has been minimized.

Copy link
Contributor

@cherrymui cherrymui commented Jun 25, 2019

Ok, the ARM64 ABI says

The A64 branch instructions are unable to reach every destination in the address space, so it may be necessary
for the linker to insert a veneer between a calling routine and a called subroutine. Veneers may also be needed to
support dynamic linking. Any veneer inserted must preserve the contents of all registers except IP0, IP1 (r16, r17)
and the condition code flags; a conforming program must assume that a veneer that alters IP0 and/or IP1 may be
inserted at any branch instruction that is exposed to a relocation that supports long branches.

Maybe we should change the register. It seems unlikely that calling duffzero needs a trampoline. I guess you are using shared libraries, where the Go runtime and the Go user code are not in the same module?

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Jun 25, 2019

Change https://golang.org/cl/183842 mentions this issue: cmd/compile, runtime: use R20, R21 in ARM64's Duff's devices

@andybons andybons added this to the Unplanned milestone Jun 25, 2019
@cherrymui cherrymui changed the title cmd/go: Register conflict between external linker and duffzero on arm64 cmd/compile: register conflict between external linker and duffzero on arm64 Jun 25, 2019
@xuanjiazhen

This comment has been minimized.

Copy link
Author

@xuanjiazhen xuanjiazhen commented Jun 26, 2019

@cherrymui
May be simple because it is too big. Our code segment has more than 100M. Is there any way to temporarily circumvent this problem?

@cherrymui

This comment has been minimized.

Copy link
Contributor

@cherrymui cherrymui commented Jun 26, 2019

The patch in https://golang.org/cl/183842 should fix this.

@xuanjiazhen

This comment has been minimized.

Copy link
Author

@xuanjiazhen xuanjiazhen commented Jun 26, 2019

@cherrymui
Thank you!
I will verify if the problem has been solved. Can I know how long this commit will be merged and which version will it be merged into?

@gopherbot gopherbot closed this in 4ea7aa7 Jun 26, 2019
@cherrymui

This comment has been minimized.

Copy link
Contributor

@cherrymui cherrymui commented Jun 26, 2019

@xuanjiazhen the patch is in. It will be in Go 1.13 release.

@xuanjiazhen

This comment has been minimized.

Copy link
Author

@xuanjiazhen xuanjiazhen commented Jun 28, 2019

@cherrymui
I got a go version based on 1.12.4 and https://golang.org/cl/183842. I tried to verify if the issue was resolved based on this version. But I got another mistake. The program has not even run to the last error. (src/runtime/malloc.go:936)

func modulesinit() {
	modules := new([]*moduledata)  // crash !

If I change back to unmodified go1.12.4, the program will pass normally here.
I can't be completely sure if there is a problem with the modification of the register or if there is a problem with the version of Go I compiled. So can you please provide a go release version on the arm64 platform with patched 1.12.x?This looks a bit beyond your scope of work. But if you can, I hope you can help me.....Thank you ! I am trying to solve this problem.

@xuanjiazhen

This comment has been minimized.

Copy link
Author

@xuanjiazhen xuanjiazhen commented Jun 28, 2019

@cherrymui
I am very depressed to find out that the current go may not be able to compile a binary with a code segment of more than 128M on arm64, which can run smoothly. Register conflicts may be just one of the cases, and it has not been determined that the conflict has been resolved. Should open a new issue to track the BL instruction jump problem over 128M.

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Jun 28, 2019

Handling long jumps should be a separate issue, yes. Thanks.

@cherrymui

This comment has been minimized.

Copy link
Contributor

@cherrymui cherrymui commented Jun 28, 2019

I don't think there is a Go 1.12 release with the patch. You can try Go 1.13 beta, which includes the patch. https://golang.org/dl/#go1.13beta1

That said, I realized that CL 183842 is not complete. If there is a linker trampoline involved, R16 and R17 may be clobbered. We moved away from them in Duff's devices, but the register allocator still needs to know they are clobbered. I can send a CL to fix this part. But I still don't know how linker trampoline is involved here. @xuanjiazhen could you share the command you used to build the binary? If you use go build, you can pass -x -ldflags=-v for extra information, which would be helpful.

As @ianlancetaylor said, handling long jumps should be a separate issue. I agree we don't have that support. But it shouldn't be hard, given that we already have the infrastructure for this on ARM32 and PPC64.

@xuanjiazhen

This comment has been minimized.

Copy link
Author

@xuanjiazhen xuanjiazhen commented Jun 29, 2019

@cherrymui
There is nothing special when building the binary. Just you need to make the binary text section built over 128M.

I constructed an example to reproduce the problem over 128M. You can generate a main_generate.go using the following code. Then generate a bunch of method files from main_generate.go. Finally, call them in another main method. You should also add a CGO hello world to make it use the external linker.

package main

import (
	"bytes"
	"fmt"
	"io/ioutil"
	"log"
	"strconv"
)

func main() {
	gen()
}

func gen() {
	var buf bytes.Buffer

	fmt.Fprintln(&buf, "// Code generated by mkduff.go; DO NOT EDIT.")
	fmt.Fprintln(&buf, "// Run go generate to update")
	fmt.Fprintln(&buf, "package main")
	fmt.Fprintln(&buf, "import (")
	fmt.Fprintln(&buf, "	\"bytes\"")
	fmt.Fprintln(&buf, "	\"fmt\"")
	fmt.Fprintln(&buf, "	\"io\"")
	fmt.Fprintln(&buf, "	\"io/ioutil\"")
	fmt.Fprintln(&buf, "	\"log\"")
	fmt.Fprintln(&buf, "	\"strconv\"")
	fmt.Fprintln(&buf, ")")
	fmt.Fprintln(&buf)
	fmt.Fprintln(&buf)
	fmt.Fprintln(&buf, "func main() {")
	for i := 0; i < 24; i++ {
		fmt.Fprintln(&buf, "gen(\""+strconv.FormatInt(int64(i), 10)+"_arm64\", notags, zeroAMD64"+strconv.FormatInt(int64(i), 10)+")")
	}
	fmt.Fprintln(&buf, "}")
	fmt.Fprintln(&buf)
	fmt.Fprintln(&buf, "func notags(w io.Writer) { fmt.Fprintln(w) }")

	fmt.Fprintln(&buf, "	func gen(arch string, tags, zero func(io.Writer)) {")
	fmt.Fprintln(&buf, "		var buf bytes.Buffer")
	fmt.Fprintln(&buf, "fmt.Fprintln(&buf, \"// Code generated by mkduff.go; DO NOT EDIT.\")")
	fmt.Fprintln(&buf, "		fmt.Fprintln(&buf, \"// Run go generate to update\")")
	fmt.Fprintln(&buf, "		tags(&buf)")
	fmt.Fprintln(&buf, "		fmt.Fprintln(&buf, \"package generate\")")
	fmt.Fprintln(&buf, "		fmt.Fprintln(&buf)")
	fmt.Fprintln(&buf, "		zero(&buf)")
	fmt.Fprintln(&buf, "		fmt.Fprintln(&buf)")
	fmt.Fprintln(&buf, "		if err := ioutil.WriteFile(\"duff_\"+arch+\".go\", buf.Bytes(), 0644); err != nil {")
	fmt.Fprintln(&buf, "			log.Fatalln(err)")
	fmt.Fprintln(&buf, "		}")
	fmt.Fprintln(&buf, "	}")
	for i := 0; i < 24; i++ {
		{
			fmt.Fprintln(&buf, "func zeroAMD64"+strconv.FormatInt(int64(i), 10)+"(w io.Writer) {")
			fmt.Fprintln(&buf, "fmt.Fprintln(w, \"func PrintForDuff"+strconv.FormatInt(int64(i), 10)+"() int {\")")
			fmt.Fprintln(&buf, "for i := 0; i < 200000; i++ {")
			fmt.Fprintln(&buf, "	fmt.Fprintln(w, \"println(\\\""+strconv.FormatInt(int64(i), 10)+"\\\")\")")
			fmt.Fprintln(&buf, "}")
			fmt.Fprintln(&buf, "fmt.Fprintln(w)")
			fmt.Fprintln(&buf, "fmt.Fprintln(w, \"\t return 0 }\")")
			fmt.Fprintln(&buf, "}")
		}
	}
	if err := ioutil.WriteFile("main_generate.go", buf.Bytes(), 0644); err != nil {
		log.Fatalln(err)
	}
}

@cherrymui

This comment has been minimized.

Copy link
Contributor

@cherrymui cherrymui commented Jul 1, 2019

I cannot reproduce the failure with your example. Anyway, I'll send my CL in case it helps. You can apply it on top of Go 1.13 beta1.

a CGO hello world to make it use the external linker

You can pass -ldflags=-linkmode=external to force external linking without cgo.

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Jul 1, 2019

Change https://golang.org/cl/184437 mentions this issue: cmd/compile: mark R16, R17 clobbered for non-standard calls on ARM64

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.