code review 125140043: liblink: use pc-relative addressing for all memory refe...

liblink: use pc-relative addressing for all memory references in amd64 code

[rsc, 2014-08-13 15:21:34]

Hello golang-codereviews@googlegroups.com (cc: iant, r, rminnich),

I'd like you to review this change to
https://code.google.com/p/go/

[rsc, 2014-08-13 15:25:21]

After this change, I can build an ELF hello world linked at a large address
that works fine on Linux using:

    go build -ldflags '-T 0xff00000c00' ../test/helloworld.go

I don't know if everything Ron needs is here, but I think that demonstrates
that it's pretty close.

[rminnich, 2014-08-13 15:48:42]

This did the trick, my tests work as well.

LGTM

[iant, 2014-08-13 17:46:13]

LGTM

Probably should be mentioned in doc/go1.4.txt.

[rsc, 2014-08-19 01:07:01]

*** Submitted as https://code.google.com/p/go/source/detail?r=3910cbf15646 ***

liblink: use pc-relative addressing for all memory references in amd64 code

LGTM=rminnich, iant
R=golang-codereviews, rminnich, iant
CC=golang-codereviews, r
https://codereview.appspot.com/125140043

[dave_cheney.net, 2014-08-19 01:27:46]

On 2014/08/19 01:07:01, rsc wrote:
> *** Submitted as https://code.google.com/p/go/source/detail?r=3910cbf15646 ***
> 
> liblink: use pc-relative addressing for all memory references in amd64 code
> 
> LGTM=rminnich, iant
> R=golang-codereviews, rminnich, iant
> CC=golang-codereviews, r
> https://codereview.appspot.com/125140043

Windows and solaris are displeased by this change,

windows:

# runtime
doasm: notfound from=79 to=10 00028
(c:\gobuilder\windows-amd64-d9b3cbdfc09f\go\src\pkg\runtime\sys_windows_amd64.s:231)	SUBQ	$runtime.callbackasm+0(SB),AX
doasm: notfound from=79 to=10 00028
(c:\gobuilder\windows-amd64-d9b3cbdfc09f\go\src\pkg\runtime\sys_windows_amd64.s:231)	SUBQ	$runtime.callbackasm+0(SB),AX

[rsc, 2014-08-19 01:33:04]

the generic fix is to MOVQ $whatever into a register and then use that
register in the non-MOVQ instruction

[gobot, 2014-08-19 01:55:56]

This CL appears to have broken the windows-amd64 builder.
See http://build.golang.org/log/16f128df0aa6475536cb40f8d3a3a0f5868c7e93

[rsc, 2014-08-19 02:14:27]

I submitted a fix for the Windows breakage. I don't know what's wrong with
Solaris. It's up to the people working on the Solaris port to fix that,
sorry.

[dave_cheney.net, 2014-08-19 02:15:50]

Thanks, we'll get it fixed.

On Tue, Aug 19, 2014 at 12:14 PM, Russ Cox <rsc@golang.org> wrote:
> I submitted a fix for the Windows breakage. I don't know what's wrong with
> Solaris. It's up to the people working on the Solaris port to fix that,
> sorry.
>

[aram, 2014-08-19 10:30:48]

I'm still in vacation until September 1st or so; I don't think I can fix it
before that, but I will fix it when I come back if nobody else fixes it by then.

I've only taken a very cursory look. I believe the problem lies in the way we
access libc.so functions. In particular we access symbols from C this way:

    #pragma dynimport libc·sysconf sysconf "libc.so"
    extern uintptr libc·sysconf;

The loader knows how to generate correct relocations (R_AMD64_GLOB_DATA, .rela)
and everything else when we access symbols like that. Btw, the support is
ELF-specific, not Solaris-specific (although it's enabled only on Solaris for
now) so in principle it could be used on other platforms too. A good use case
would be to generate Linux binaries that link with glibc for name resolution
even when cross-compiling, but that's a different story.

We don't see this problem on Windows because on Windows the C code is different.

    #pragma dynimport runtime·WriteFile WriteFile "kernel32.dll"
    extern void *runtime·WriteFile;

Of course all the loader code is very different too.

If I were to guess what's wrong, I'd guess that this change made a
(R_AMD64_JUMP_SLOT, .rela.plt) relocation. Of course, we are not prepared for
this.

Of course, I just looked at the change, I haven't looked at the failure in
detail (I'm away from all my machines right now) so I might be wrong.

[aram, 2014-08-20 17:53:34]

https://codereview.appspot.com/125140043/diff/100001/src/liblink/asm6.c
File src/liblink/asm6.c (right):

https://codereview.appspot.com/125140043/diff/100001/src/liblink/asm6.c#newco...
src/liblink/asm6.c:2290: r->type = R_PCREL;
I've taken another look at Solaris today. The problem is that we don't want to
do this (R_PCREL) when the symbol is a dynamic symbol (i.e. external symbols
from libc.so, not external symbols in our own object files). We should be still
doing R_ADDR. This works fine (at least with the internal linker) because we use
the GOT to find these symbols, which preserves the position-independent
property. See go/src/cmd/6l/asm.c:/^adddynrel.

Unfortunately, I have no idea how to differentiate dynamic symbols from our own
usual external symbols in liblink. In particular, most LSym fields are not
initialized (they are initialized in the linker proper). In particular type,
cgoexport, dynid, dynimplib seem to be uninitialized:

  name=libc.sysconf, extname=libc.sysconf, type=0, external=0, cgoexport=0,
dynid=-1, dynimplib=<nil>
  name=libc.sysconf, extname=libc.sysconf, type=0, external=0, cgoexport=0,
dynid=-1, dynimplib=<nil>

So, any tips on how to make this distinction?

[rsc, 2014-08-24 03:57:15]

On 2014/08/20 17:53:34, aram wrote:
> https://codereview.appspot.com/125140043/diff/100001/src/liblink/asm6.c
> File src/liblink/asm6.c (right):
> 
>
https://codereview.appspot.com/125140043/diff/100001/src/liblink/asm6.c#newco...
> src/liblink/asm6.c:2290: r->type = R_PCREL;
> I've taken another look at Solaris today. The problem is that we don't want to
> do this (R_PCREL) when the symbol is a dynamic symbol (i.e. external symbols
> from libc.so, not external symbols in our own object files). We should be
still
> doing R_ADDR. This works fine (at least with the internal linker) because we
use
> the GOT to find these symbols, which preserves the position-independent
> property. See go/src/cmd/6l/asm.c:/^adddynrel.
> 
> Unfortunately, I have no idea how to differentiate dynamic symbols from our
own
> usual external symbols in liblink. In particular, most LSym fields are not
> initialized (they are initialized in the linker proper). In particular type,
> cgoexport, dynid, dynimplib seem to be uninitialized:
> 
>   name=libc.sysconf, extname=libc.sysconf, type=0, external=0, cgoexport=0,
> dynid=-1, dynimplib=<nil>
>   name=libc.sysconf, extname=libc.sysconf, type=0, external=0, cgoexport=0,
> dynid=-1, dynimplib=<nil>
> 
> So, any tips on how to make this distinction?

I sent you CL 129540043.