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: SDYNIMPORT with external linking produces unhandled/incorrect relocations on openbsd/amd64 #42671

Open
4a6f656c opened this issue Nov 17, 2020 · 4 comments

Comments

@4a6f656c
Copy link
Contributor

@4a6f656c 4a6f656c commented Nov 17, 2020

When using SDYNIMPORT with external linking on openbsd/amd64, unhandled/incorrect relocations are produced:

$ go build -ldflags=-linkmode=external -o /tmp/b cmd/link/internal/ld/testdata/issue39256
main.trampoline: unhandled relocation for libc_getpid (type 44 (SDYNIMPORT) rtype 8 (R_CALL))
main.trampoline: unhandled relocation for libc_kill (type 44 (SDYNIMPORT) rtype 8 (R_CALL))
main.trampoline: unhandled relocation for libc_open (type 44 (SDYNIMPORT) rtype 8 (R_CALL))
main.trampoline: unhandled relocation for libc_close (type 44 (SDYNIMPORT) rtype 8 (R_CALL))

The following diff "fixes" the issue:

diff --git a/src/cmd/link/internal/ld/data.go b/src/cmd/link/internal/ld/data.go
index 00130044ab..9ac390e3f1 100644
--- a/src/cmd/link/internal/ld/data.go
+++ b/src/cmd/link/internal/ld/data.go
@@ -207,7 +207,7 @@ func (st *relocSymState) relocsym(s loader.Sym, P []byte) {
 
                // We need to be able to reference dynimport symbols when linking against
                // shared libraries, and Solaris, Darwin and AIX need it always
-               if !target.IsSolaris() && !target.IsDarwin() && !target.IsAIX() && rs != 0 && rst == sym.SDYNIMPORT && !target.IsDynlinkingGo() && !ldr.AttrSubSymbol(rs) {
+               if !target.IsSolaris() && !target.IsDarwin() && !target.IsOpenbsd() && !target.IsAIX() && rs != 0 && rst == sym.SDYNIMPORT && !target.IsDynlinkingGo() && !ldr.AttrSubSymbol(rs) {
                        if !(target.IsPPC64() && target.IsExternal() && ldr.SymName(rs) == ".TOC.") {
                                st.err.Errorf(s, "unhandled relocation for %s (type %d (%s) rtype %d (%s))", ldr.SymName(rs), rst, rst, rt, sym.RelocName(target.Arch, rt))
                        }
diff --git a/src/cmd/link/internal/ld/go.go b/src/cmd/link/internal/ld/go.go
index a6cd4c0541..f99af17b47 100644
--- a/src/cmd/link/internal/ld/go.go
+++ b/src/cmd/link/internal/ld/go.go
@@ -364,7 +364,7 @@ func (ctxt *Link) addexport() {
                        relocs := ctxt.loader.Relocs(s)
                        for i := 0; i < relocs.Count(); i++ {
                                if rs := relocs.At(i).Sym(); rs != 0 {
-                                       if ctxt.loader.SymType(rs) == sym.Sxxx && !ctxt.loader.AttrLocal(rs) {
+                                       if (ctxt.loader.SymType(rs) == sym.Sxxx || ctxt.loader.SymType(rs) == sym.SDYNIMPORT) && !ctxt.loader.AttrLocal(rs) {
                                                // sanity check
                                                if len(ctxt.loader.Data(rs)) != 0 {
                                                        panic("expected no data on undef symbol")

And allows us to produce a working binary:

$ objdump -R /tmp/b
...
DYNAMIC RELOCATION RECORDS
OFFSET           TYPE              VALUE 
00000000002c55c0 R_X86_64_GLOB_DAT  _Jv_RegisterClasses
00000000002c55e8 R_X86_64_JUMP_SLOT  _csu_finish
00000000002c55f0 R_X86_64_JUMP_SLOT  exit
00000000002c55f8 R_X86_64_JUMP_SLOT  _Jv_RegisterClasses
00000000002c5600 R_X86_64_JUMP_SLOT  atexit
00000000002c5608 R_X86_64_JUMP_SLOT  getpid
00000000002c5610 R_X86_64_JUMP_SLOT  kill
00000000002c5618 R_X86_64_JUMP_SLOT  open
00000000002c5620 R_X86_64_JUMP_SLOT  close

Without the second chunk we end up producing a binary with:

$ objdump -R /tmp/b 
...
DYNAMIC RELOCATION RECORDS
OFFSET           TYPE              VALUE 
00000000002c5540 R_X86_64_GLOB_DAT  _Jv_RegisterClasses
00000000002c5550 R_X86_64_GLOB_DAT  getpid
00000000002c5558 R_X86_64_GLOB_DAT  kill
00000000002c5560 R_X86_64_GLOB_DAT  open
00000000002c5568 R_X86_64_GLOB_DAT  close
00000000002c5588 R_X86_64_JUMP_SLOT  _csu_finish
00000000002c5590 R_X86_64_JUMP_SLOT  exit
00000000002c5598 R_X86_64_JUMP_SLOT  _Jv_RegisterClasses
00000000002c55a0 R_X86_64_JUMP_SLOT  atexit

And the intermediate linker go.o differs as follows:

-000000000005befc R_X86_64_GOTPCREL  getpid+0xfffffffffffffffc
-000000000005bf01 R_X86_64_GOTPCREL  kill+0xfffffffffffffffc
-000000000005bf06 R_X86_64_GOTPCREL  open+0xfffffffffffffffc
-000000000005bf0b R_X86_64_GOTPCREL  close+0xfffffffffffffffc
+000000000005befc R_X86_64_PC32     getpid+0xfffffffffffffffc
+000000000005bf01 R_X86_64_PC32     kill+0xfffffffffffffffc
+000000000005bf06 R_X86_64_PC32     open+0xfffffffffffffffc
+000000000005bf0b R_X86_64_PC32     close+0xfffffffffffffffc

In otherwords, using SUNDEF results in R_X86_64_PC32 that the external linker turns into R_X86_64_JUMP_SLOT. Without it we end up with R_X86_64_GOTPCREL and in turn R_X86_64_GLOB_DAT. I'm not sure if there is a better fix here - possibly an additional relocation type that needs to be handled in the amd64 linker?

@4a6f656c
Copy link
Contributor Author

@4a6f656c 4a6f656c commented Nov 17, 2020

@cherrymui
Copy link
Contributor

@cherrymui cherrymui commented Nov 17, 2020

Thanks.

https://tip.golang.org/src/cmd/link/internal/amd64/asm.go#L419
This code looks rather weird. I'm not sure when/why it makes sense to emit an R_X86_64_GOTPCREL for a CALL. Try changing it to always emit R_X86_64_PLT32?

@4a6f656c
Copy link
Contributor Author

@4a6f656c 4a6f656c commented Nov 18, 2020

Thanks.

https://tip.golang.org/src/cmd/link/internal/amd64/asm.go#L419
This code looks rather weird. I'm not sure when/why it makes sense to emit an R_X86_64_GOTPCREL for a CALL. Try changing it to always emit R_X86_64_PLT32?

Building with the following does indeed fix the issue:

diff --git a/src/cmd/link/internal/amd64/asm.go b/src/cmd/link/internal/amd64/asm.go
index 360c5338ba..2d09a6160a 100644
--- a/src/cmd/link/internal/amd64/asm.go
+++ b/src/cmd/link/internal/amd64/asm.go
@@ -413,11 +413,7 @@ func elfreloc1(ctxt *ld.Link, out *ld.OutBuf, ldr *loader.Loader, s loader.Sym,
        case objabi.R_CALL:
                if siz == 4 {
                        if ldr.SymType(r.Xsym) == sym.SDYNIMPORT {
-                               if ctxt.DynlinkingGo() {
-                                       out.Write64(uint64(elf.R_X86_64_PLT32) | uint64(elfsym)<<32)
-                               } else {
-                                       out.Write64(uint64(elf.R_X86_64_GOTPCREL) | uint64(elfsym)<<32)
-                               }
+                               out.Write64(uint64(elf.R_X86_64_PLT32) | uint64(elfsym)<<32)
                        } else {
                                out.Write64(uint64(elf.R_X86_64_PC32) | uint64(elfsym)<<32)
                        }

Not sure if something else relies on GOTPCREL though (as you said, it seems strange).

How should we proceed here?

@cherrymui
Copy link
Contributor

@cherrymui cherrymui commented Nov 18, 2020

Currently the other ELF system that uses SDYNIMPORT is Solaris. If you can try the change above on Solaris and it works well, then I think we can do that. Or you can send a CL and we can try it on the builder. Thanks.

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
2 participants
You can’t perform that action at this time.