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/link: load of "ld -r" host object fails in -newobj mode #35779

Open
thanm opened this issue Nov 22, 2019 · 9 comments
Assignees
Labels
Milestone

Comments

@thanm
Copy link
Member

@thanm thanm commented Nov 22, 2019

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

$ go version
go version devel +9c6f6409ad Wed Nov 20 15:16:17 2019 +0000 linux/amd64

Does this issue reproduce with the latest release?

Yes.

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

linux/amd64

What did you do?

Build a small program that links in a *.syso object that contains multiple static symbols that have the same name and section.

This is a bit elaborate, but here is a repro recipe:

$ cat /tmp/c1.c
static void blah() { }
int c1() { return 42; }
$ cat /tmp/c2.c
static void blah() { }
int c2() { return 42; }
$ gcc -c -o c1.o /tmp/c1.c
$ gcc -c -o c2.o /tmp/c2.c
$ ld -r -o ldr.syso c1.o c2.o 
$ objdump -t ldr.syso | fgrep blah
0000000000000000 l     F .text	0000000000000007 blah
0000000000000012 l     F .text	0000000000000007 blah
$ rm c1.o c2.o 
$ cat main.go
package main
func main() {
}
$ go build .
$ go build -asmflags=all=-newobj -gcflags=all=-newobj -ldflags=-newobj .
# ...
loadelf: $WORK/b001/_pkg_.a(ldr.syso): duplicate symbol reference: blah in both main(.text) and main(.text)
$

What did you expect to see?

Clean build with -newobj.

What did you see instead?

loadelf error.

@thanm thanm added the NeedsFix label Nov 22, 2019
@thanm thanm added this to the Go1.14 milestone Nov 22, 2019
@thanm thanm self-assigned this Nov 22, 2019
@thanm

This comment has been minimized.

Copy link
Member Author

@thanm thanm commented Nov 22, 2019

@cherrymui , @jeremyfaller just as FYI

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Nov 22, 2019

Change https://golang.org/cl/208478 mentions this issue: cmd/link: add new linker testpoint for "ld -r" w/ -newobj

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Nov 22, 2019

Change https://golang.org/cl/208479 mentions this issue: cmd/link: fix bug with -newobj and "ld -r" ELF host objects

@gopherbot gopherbot closed this in f29e53d Nov 23, 2019
gopherbot pushed a commit that referenced this issue Nov 23, 2019
This adds a new test that builds a small Go program with linked
against a *.syso file that is the result of an "ld -r" link. The
sysobj in question has multiple static symbols in the same section
with the same name, which triggered a bug in the loader in -newobj
mode.

Updates #35779.

Change-Id: Ibe1a75662dc1d49c4347279e55646ee65a81508e
Reviewed-on: https://go-review.googlesource.com/c/go/+/208478
Reviewed-by: Cherry Zhang <cherryyz@google.com>
@thanm

This comment has been minimized.

Copy link
Member Author

@thanm thanm commented Nov 23, 2019

The new testcase broke the MIPS builders -- reopening while I work on that.

@thanm thanm reopened this Nov 23, 2019
@thanm

This comment has been minimized.

Copy link
Member Author

@thanm thanm commented Nov 23, 2019

Representative failures:

https://build.golang.org/log/98d47379c6065fd00edcd68aa384451c5deae722
    elf_test.go:202: # elf_test
        loadelf: $WORK/b001/_pkg_.a(ldr.syso): malformed elf file: invalid elf symbol index
https://build.golang.org/log/ad7532a8bd9457571fe394269d7a017b82abe21e
    elf_test.go:202: # elf_test
        /tmp/gobuilder-mips64/tmp/go-build246906970/b001/_pkg_.a(ldr.syso): unknown relocation type 333831; compiled without -fpic?
@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Nov 23, 2019

Change https://golang.org/cl/208557 mentions this issue: cmd/link: disable new testpoint on mips pending investigation

gopherbot pushed a commit that referenced this issue Nov 24, 2019
Skip TestMinusRSymsWithSameName testpoint on MIPS for the time being
since it triggers failures on that arch. Will re-enable once the
problems are fixed.

Updates #35779.

Change-Id: I3e6650158ab04a2be77e3db5a5194df3bbb0859e
Reviewed-on: https://go-review.googlesource.com/c/go/+/208557
Reviewed-by: Cherry Zhang <cherryyz@google.com>
@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Nov 25, 2019

Change https://golang.org/cl/208778 mentions this issue: cmd/link: additional fixes for -newobj and "ld -r" ELF host objects

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Nov 28, 2019

Change https://golang.org/cl/209317 mentions this issue: cmd/link: fix loadelf failed on MIPS family

gopherbot pushed a commit that referenced this issue Dec 2, 2019
The previous fix for this issue (CL 208479) was not general enough;
this patch revises it to handle more cases.

The problem with the original fix was that once a sym.Symbol is
created for a given static symbol and given a bogus anonymous version
of -1, we hit problems if some other non-anonymous symbol (created by
host object loading) had relocations targeting the static symbol.

In this patch instead of assigning a fixed anonymous version of -1 to
such symbols, each time loader.Create is invoked we create a new
(unique) anonymous version for the sym.Symbol, then enter the result
into the loader's extStaticSyms map, permitting it to be found in
lookups when processing relocation targets.

NB: this code will hopefully get a lot simpler once we can move host
object loading away from early sym.Symbol creation.

Updates #35779.

Change-Id: I450ff577e17549025565d355d6707a2d28a5a617
Reviewed-on: https://go-review.googlesource.com/c/go/+/208778
Run-TryBot: Than McIntosh <thanm@google.com>
Reviewed-by: Cherry Zhang <cherryyz@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Dec 3, 2019

Change https://golang.org/cl/209658 mentions this issue: [dev.link] all: merge branch 'master' into dev.link

gopherbot pushed a commit that referenced this issue Dec 3, 2019
Bring in Than's fix of #35779.

The only merge conflict is cmd/link/internal/loadelf/ldelf.go,
with a modification-deletion conflict.

Change-Id: Id2fcfd2094a31120966a6ea9c462b4ec76646b10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.