code review 7369057: cmd/ld: host linking support for linux/amd64

cmd/ld: host linking support for linux/amd64

Still to do: non-linux and non-amd64.
It may work on other ELF-based amd64 systems too, but untested.

"go test -ldflags -hostobj $GOROOT/misc/cgo/test" passes.

Much may yet change, but this seems a reasonable checkpoint.

[rsc, 2013-03-06 22:41:48]

Hello iant (cc: golang-dev@googlegroups.com),

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

[iant, 2013-03-07 00:18:23]

https://codereview.appspot.com/7369057/diff/13001/src/cmd/5l/obj.c
File src/cmd/5l/obj.c (right):

https://codereview.appspot.com/7369057/diff/13001/src/cmd/5l/obj.c#newcode132
src/cmd/5l/obj.c:132: 
Extraneous blank line.

https://codereview.appspot.com/7369057/diff/13001/src/cmd/6l/obj.c
File src/cmd/6l/obj.c (right):

https://codereview.appspot.com/7369057/diff/13001/src/cmd/6l/obj.c#newcode86
src/cmd/6l/obj.c:86: linkmode = LinkInternal;
Should this be LinkAuto rather than LinkInternal?  Perhaps at least a TODO?

https://codereview.appspot.com/7369057/diff/13001/src/cmd/ld/elf.c
File src/cmd/ld/elf.c (right):

https://codereview.appspot.com/7369057/diff/13001/src/cmd/ld/elf.c#newcode895
src/cmd/ld/elf.c:895: shstrtab->hide = 1;
What about other similar symbols, like: .hash, .dynsym, .dynstr, .rel?.*, .got,
etc.

https://codereview.appspot.com/7369057/diff/13001/src/cmd/ld/lib.c
File src/cmd/ld/lib.c (right):

https://codereview.appspot.com/7369057/diff/13001/src/cmd/ld/lib.c#newcode518
src/cmd/ld/lib.c:518: goto ok;
Very minor: ok is a funny label name here.  There is nothing either ok or not ok
about this branch.

https://codereview.appspot.com/7369057/diff/13001/src/cmd/ld/lib.c#newcode537
src/cmd/ld/lib.c:537: h->pn = strdup(pn);
You check the return value of realloc but not of strdup?

https://codereview.appspot.com/7369057/diff/13001/src/cmd/ld/lib.c#newcode593
src/cmd/ld/lib.c:593: }
free(p);

https://codereview.appspot.com/7369057/diff/13001/src/cmd/ld/lib.c#newcode614
src/cmd/ld/lib.c:614: argv[argc++] = "-rdynamic"; // force external symbols to
be exported for dlopen etc
Note that -rdynamic may not work on Mac OS or Windows.  I'm not sure.

https://codereview.appspot.com/7369057/diff/13001/src/cmd/ld/lib.c#newcode619
src/cmd/ld/lib.c:619: // copy host objects to temporary directory
If h->off == 0, why not try using a symlink?

https://codereview.appspot.com/7369057/diff/13001/src/cmd/ld/lib.c#newcode643
src/cmd/ld/lib.c:643: close(w);
Check for error on close.

https://codereview.appspot.com/7369057/diff/13001/src/cmd/ld/lib.c#newcode667
src/cmd/ld/lib.c:667: /*
Extraneous.

[rsc, 2013-03-07 03:04:20]

PTAL

https://codereview.appspot.com/7369057/diff/13001/src/cmd/5l/obj.c
File src/cmd/5l/obj.c (right):

https://codereview.appspot.com/7369057/diff/13001/src/cmd/5l/obj.c#newcode132
src/cmd/5l/obj.c:132: 
On 2013/03/07 00:18:23, iant wrote:
> Extraneous blank line.

Done.

https://codereview.appspot.com/7369057/diff/13001/src/cmd/6l/obj.c
File src/cmd/6l/obj.c (right):

https://codereview.appspot.com/7369057/diff/13001/src/cmd/6l/obj.c#newcode86
src/cmd/6l/obj.c:86: linkmode = LinkInternal;
On 2013/03/07 00:18:23, iant wrote:
> Should this be LinkAuto rather than LinkInternal?  Perhaps at least a TODO?

Done.

https://codereview.appspot.com/7369057/diff/13001/src/cmd/ld/elf.c
File src/cmd/ld/elf.c (right):

https://codereview.appspot.com/7369057/diff/13001/src/cmd/ld/elf.c#newcode895
src/cmd/ld/elf.c:895: shstrtab->hide = 1;
On 2013/03/07 00:18:23, iant wrote:
> What about other similar symbols, like: .hash, .dynsym, .dynstr, .rel?.*,
.got,
> etc.

I'm always a little hazy on which ones need to be present at run time. This was
enough to make things start working so I stopped there.

I removed this line and changed genasmsym (in lib.c) to treat all symbols with
first letter '.' as hidden.

https://codereview.appspot.com/7369057/diff/13001/src/cmd/ld/lib.c
File src/cmd/ld/lib.c (right):

https://codereview.appspot.com/7369057/diff/13001/src/cmd/ld/lib.c#newcode518
src/cmd/ld/lib.c:518: goto ok;
On 2013/03/07 00:18:23, iant wrote:
> Very minor: ok is a funny label name here.  There is nothing either ok or not
ok
> about this branch.

Done.

https://codereview.appspot.com/7369057/diff/13001/src/cmd/ld/lib.c#newcode537
src/cmd/ld/lib.c:537: h->pn = strdup(pn);
On 2013/03/07 00:18:23, iant wrote:
> You check the return value of realloc but not of strdup?

Went through and changed all the realloc and strdup to call checking wrappers.

https://codereview.appspot.com/7369057/diff/13001/src/cmd/ld/lib.c#newcode593
src/cmd/ld/lib.c:593: }
On 2013/03/07 00:18:23, iant wrote:
> free(p);

Done.

https://codereview.appspot.com/7369057/diff/13001/src/cmd/ld/lib.c#newcode614
src/cmd/ld/lib.c:614: argv[argc++] = "-rdynamic"; // force external symbols to
be exported for dlopen etc
On 2013/03/07 00:18:23, iant wrote:
> Note that -rdynamic may not work on Mac OS or Windows.  I'm not sure.

Done.

https://codereview.appspot.com/7369057/diff/13001/src/cmd/ld/lib.c#newcode619
src/cmd/ld/lib.c:619: // copy host objects to temporary directory
On 2013/03/07 00:18:23, iant wrote:
> If h->off == 0, why not try using a symlink?

Never going to happen. These are always .o files found inside .a files. The only
allowed command-line argument is main.a or main.6. Everything else is found via
import, which names a .a or .6 file.

https://codereview.appspot.com/7369057/diff/13001/src/cmd/ld/lib.c#newcode643
src/cmd/ld/lib.c:643: close(w);
On 2013/03/07 00:18:23, iant wrote:
> Check for error on close.

Done.

https://codereview.appspot.com/7369057/diff/13001/src/cmd/ld/lib.c#newcode667
src/cmd/ld/lib.c:667: /*
On 2013/03/07 00:18:23, iant wrote:
> Extraneous.

Done.

[iant, 2013-03-07 05:41:59]

LGTM

[rsc, 2013-03-07 14:19:05]

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

cmd/ld: host linking support for linux/amd64

Still to do: non-linux and non-amd64.
It may work on other ELF-based amd64 systems too, but untested.

"go test -ldflags -hostobj $GOROOT/misc/cgo/test" passes.

Much may yet change, but this seems a reasonable checkpoint.

R=iant
CC=golang-dev
https://codereview.appspot.com/7369057

[bsiegert, 2013-03-07 17:10:36]

This change broke cgo on Windows and the BSDs.

--Benny.

On Thu, Mar 7, 2013 at 3:19 PM,  <rsc@golang.org> wrote:
> *** Submitted as
> https://code.google.com/p/go/source/detail?r=fa13899da667 ***
>
>
> cmd/ld: host linking support for linux/amd64
>
> Still to do: non-linux and non-amd64.
> It may work on other ELF-based amd64 systems too, but untested.
>
> "go test -ldflags -hostobj $GOROOT/misc/cgo/test" passes.
>
> Much may yet change, but this seems a reasonable checkpoint.
>
> R=iant
> CC=golang-dev
> https://codereview.appspot.com/7369057
>
>
>
> https://codereview.appspot.com/7369057/
>
> --
>
> ---You received this message because you are subscribed to the Google Groups
> "golang-dev" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to golang-dev+unsubscribe@googlegroups.com.
> For more options, visit https://groups.google.com/groups/opt_out.
>
>



--
The first essential in chemistry is that you should perform practical
work and conduct experiments, for he who performs not practical work
nor makes experiments will never attain the least degree of mastery.
        -- Abu Musa Jabir ibn Hayyan (721-815)

[rsc, 2013-03-07 17:17:36]

Of course it did. :-(

[elias.naur, 2013-03-07 18:18:45]

The TLS section doesn't seem to be generated when using -hostobj, making use of
__thread variables from Cgo crash. You may already be aware of this, but I added
a (currently failing) test for this at:

https://codereview.appspot.com/7578043

Use or apply at your convenience.