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/link: does not support multiple TOCs in internal linking mode #21961

Open
mkumatag opened this Issue Sep 21, 2017 · 21 comments

Comments

Projects
None yet
6 participants
@mkumatag

mkumatag commented Sep 21, 2017

Please answer these questions before submitting your issue. Thanks!

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

master

root@localhost:~/go# git rev-parse HEAD
eca45997dfd6cd14a59fbdea2385f6648a0dc786
root@localhost:~/go#

Does this issue reproduce with the latest release?

yes

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

ppc64le

What did you do?

export PATH=/usr/local/go/bin/:$PATH
export GOROOT_BOOTSTRAP=go env GOROOT

root@localhost:~/go/bin# go version
go version go1.8.3 linux/ppc64le

cloned the latest code here:

root@localhost:~/go/src# pwd
/root/go/src

run make.bash
root@localhost:~/go/src# ./make.bash

Use the build go binary:

root@localhost:~/go/bin# pwd
/root/go/bin
root@localhost:~/go/bin# ./go version
fatal error: unexpected signal during runtime execution
[signal SIGSEGV: segmentation violation code=0x1 addr=0xc88320 pc=0x112c8]

runtime stack:
invalid spdelta runtime/cgo(.text).sigfillset 0x112c0 0x112c8 0x0 -1
runtime.throw(0x44825a, 0x2a)
	/root/go/src/runtime/panic.go:606 +0x68
runtime.sigpanic()
	/root/go/src/runtime/signal_unix.go:360 +0x264
invalid spdelta runtime/cgo(.text).sigfillset 0x112c0 0x112c8 0x0 -1

goroutine 1 [running]:
runtime.systemstack_switch()
	/root/go/src/runtime/asm_ppc64x.s:209 +0x10 fp=0xc420036758 sp=0xc420036738 pc=0x64e40
runtime.main()
	/root/go/src/runtime/proc.go:128 +0x60 fp=0xc4200367c0 sp=0xc420036758 pc=0x3cf80
runtime.goexit()
	/root/go/src/runtime/asm_ppc64x.s:1353 +0x4 fp=0xc4200367c0 sp=0xc4200367c0 pc=0x67464
root@localhost:~/go/bin#

If possible, provide a recipe for reproducing the error.
A complete runnable program is good.
A link on play.golang.org is best.

What did you expect to see?

Instead, I should see a golang version

What did you see instead?

I see a SIGSEGV

@mkumatag

This comment has been minimized.

mkumatag commented Sep 21, 2017

/cc @laboger

@mkumatag

This comment has been minimized.

mkumatag commented Sep 21, 2017

@mundaym

This comment has been minimized.

Member

mundaym commented Sep 21, 2017

@mkumatag which revision of Go are you using? Can you try rolling back to 101fbc2 (git checkout 101fbc2) and retrying? It looks like the ppc64le builder has been failing since 8e5ac83 (cmd/go: stop linking cgo objects together with ld -r).

@mkumatag

This comment has been minimized.

mkumatag commented Sep 21, 2017

@mundaym I'm using latest one

root@localhost:~/go# git rev-parse HEAD
eca45997dfd6cd14a59fbdea2385f6648a0dc786
root@localhost:~/go#
@mkumatag

This comment has been minimized.

mkumatag commented Sep 21, 2017

I rolled back to 101fbc2 and things are fine,

root@localhost:~/go/bin# ./go version
go version devel +101fbc2 Wed Sep 20 20:27:13 2017 +0000 linux/ppc64le
root@localhost:~/go/bin#

@mundaym @ianlancetaylor not really sure what is wrong with the latest version!

@mundaym mundaym added this to the Go1.10 milestone Sep 21, 2017

@laboger

This comment has been minimized.

Contributor

laboger commented Sep 21, 2017

Yikes I haven't been watching the build board lately and I see we have been randomly failing in runtime since e7e4a4f and this latest failure of SEGV was introduced with @ianlancetaylor 's change 8e5ac83.

segv happens here:

0x112c0 <runtime/cgo(.text).sigfillset>: std r2,24(r1)
0x112c4 <runtime/cgo(.text).sigfillset+4>: addis r12,r2,100
=> 0x112c8 <runtime/cgo(.text).sigfillset+8>: ld r12,192(r12)
0x112cc <runtime/cgo(.text).sigfillset+12>: mtctr r12
0x112d0 <runtime/cgo(.text).sigfillset+16>: bctr

In golang prior to the change to omit the ld -r where it works, this code looks like this:

=> 0x112c0 <runtime/cgo(.text).sigfillset>: std r2,24(r1)
0x112c4 <runtime/cgo(.text).sigfillset+4>: addis r12,r2,-1
0x112c8 <runtime/cgo(.text).sigfillset+8>: ld r12,32368(r12)
0x112cc <runtime/cgo(.text).sigfillset+12>: mtctr r12
0x112d0 <runtime/cgo(.text).sigfillset+16>: bctr

So my theory is that the TOCs from the individual .o files should be getting merged into a single TOC at some point, but if the external linker isn't involved in the final link, how's it going to happen except by the ld -r? My understanding is that the go tool is linked with the internal linker and not the external linker, so in that case the ld -r is still useful for ppc64le.

This brings up a different concern however, why this wasn't caught in the buildbots?

@mundaym

This comment has been minimized.

Member

mundaym commented Sep 21, 2017

You mean the trybots? They don't run on ppc64x, they only check that the compilation works.

@laboger

This comment has been minimized.

Contributor

laboger commented Sep 21, 2017

FYI... this doesn't affect ppc64 because 'go' is built statically there.

@laboger

This comment has been minimized.

Contributor

laboger commented Sep 22, 2017

@mundaym This breaks the 'go' tool so I was somewhat thinking that it should fail during the build. But I see now that go_bootstrap is built as static but the final go tool is built as a dynamic binary containing cgo using the internal linker which is the case that is broken by the removal of ld -r.

@mkumatag

This comment has been minimized.

mkumatag commented Sep 25, 2017

Any updates? builds are still failing with the latest code. @ianlancetaylor any comments?

@laboger

This comment has been minimized.

Contributor

laboger commented Sep 25, 2017

@ianlancetaylor I don't know what your thoughts are on this. This regression makes the go tool unusable on the golang build so we would like to get this fixed.

I can create a fix to back out your change for ppc64le, i.e., continue to use the ld -r on ppc64le but not on other platforms.

I believe the other option is to fix the internal linker to correctly handle merging of the TOCs, but that might take me a while because I don't know the internal linker well enough.

@ianlancetaylor

This comment has been minimized.

Contributor

ianlancetaylor commented Sep 25, 2017

What happens if we simply add sys.PPC64 to this list in cmd/link/internal/ld/config.go?

	if iscgo && SysArch.InFamily(sys.ARM64, sys.MIPS64, sys.MIPS) {
		return true, objabi.GOARCH + " does not support internal cgo"
	}

That seems to me like the right temporary fix unless and until somebody fixes the internal linker. The downside is that we won't generate fully statically linked binaries for pure Go programs on ppc64le. Is that acceptable?

@laboger

This comment has been minimized.

Contributor

laboger commented Sep 25, 2017

Isn't this going to force external linking only when iscgo is true? So pure Go programs on ppc64le should still be truly statically linked with this change.

I just tried it, and that's what happened with a simple hello.go without cgo, which is pure Go AFAIK?

I think this is the correct temporary fix, but this means the 'go' tool is linked with the external linker and not the internal linker. In both cases it is a dynamic binary. I'm not sure what the downside might be for using a different linker.

@ianlancetaylor

This comment has been minimized.

Contributor

ianlancetaylor commented Sep 25, 2017

Sorry, what I wrote was a little confusing. I think this will change the behavior of any pure Go program that imports the net or os/user packages. I think that currently they will use internal linking, and with my suggestion they will use external linking. You're right that they will be dynamically linked either way.

I think the only significant difference is that with this change a pure Go program (that imports the net or os/user package) will only be buildable if the C linker is installed on the system where the Go program is being built. Without this change such a program would be buildable even if the C linker was not installed at all. On GNU/Linux I doubt that is worth worrying about (on Windows, on the other hand, it would be a problem).

Does the change actually fix the problem?

@laboger

This comment has been minimized.

Contributor

laboger commented Sep 26, 2017

Yes this fixes the problem, sorry I wasn't explicit on that.

I don't have a good feel for how many programs might be affected or who would care. Obviously the building of the 'go' tool requires the 'ld -r' so it must have the C linker on the system. I think we fix it and see who complains.

@ianlancetaylor

This comment has been minimized.

Contributor

ianlancetaylor commented Sep 26, 2017

You're right that clearly invoking ld -r means that you have the C linker. I must be misremembering something about this.

@ianlancetaylor ianlancetaylor changed the title from SIGSEGV while running go on ppc64le to cmd/link: does not support multiple TOCs in internal linking mode Sep 26, 2017

@gopherbot

This comment has been minimized.

gopherbot commented Sep 26, 2017

Change https://golang.org/cl/66270 mentions this issue: cmd/link: don't use internal linking mode for cgo on PPC64

gopherbot pushed a commit that referenced this issue Sep 26, 2017

cmd/link: don't use internal linking mode for cgo on PPC64
The internal linker doesn't know how to handle multiple TOC sections
in internal linking mode. This used to work because before CL 64793 we
invoked ld -r on multiple objects, and that merged the TOC sections
for us.

Updates #21961

Change-Id: I48260a7195be660016f2f358ebc8cb79652210ab
Reviewed-on: https://go-review.googlesource.com/66270
Run-TryBot: Ian Lance Taylor <iant@golang.org>
Reviewed-by: Lynn Boger <laboger@linux.vnet.ibm.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
@mwhudson

This comment has been minimized.

Contributor

mwhudson commented Oct 27, 2017

This is related to #15409 is it not?

From my POV I'd be fine with deleting all the code related to internal linking support for cgo but that might not be terribly friendly.

@ianlancetaylor

This comment has been minimized.

Contributor

ianlancetaylor commented Oct 27, 2017

Yes, it seems likely that fixing this would fix #15409.

Internal linking is quite important on Windows but I agree that it is less interesting on GNU/Linux systems. Still, in general, it is a goal that people can install a Go distribution and, as long as they don't use cgo, use it with no C compiler or linker installed. With the current standard library that means that the internal linker must support at least the code generated for the net and os/user packages.

@laboger

This comment has been minimized.

Contributor

laboger commented Jun 14, 2018

The recent breakage in the ppc64x builds and the discussion about cross builds made me curious, so I did a cross build of golang for ppc64le and found that the go binary created is statically (and therefore internally) linked. I moved the cross built golang over to a ppc64le machine and the tests all passed. So I think that means an installation of a Go distribution for ppc64le from a cross build on an x86 could be installed with no C compiler or linker.

At this point I don't understand why the go binary is built differently when doing a cross. It seems that it should be built the same way whether cross or natively built. It should still have multiple TOCs in both cases?

@ianlancetaylor

This comment has been minimized.

Contributor

ianlancetaylor commented Jun 14, 2018

By default when cross-compiling we assume that we don't have a C cross-compiler available, so we build without cgo support. You should be able to get the same result cross and native, assuming you have a C cross-compiler, if you set CGO_ENABLED=1 in the environment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment