code review 9383043: cmd/ld: emit .tbss section when doing dynamic internal ...

cmd/ld: emit .tbss section when doing dynamic internal linking
Fixes issue 5200.

[minux1, 2013-05-13 14:32:30]

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

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

[minux1, 2013-05-13 14:33:00]

do we need to add a strip(1) test to run.bash?

[dave_cheney.net, 2013-05-14 00:19:44]

People are going to keep stripping binaries regardless of our advice so I think
it is important to have a test of stripping a variety of Go executables.  



On 13/05/2013, at 7:33, minux.ma@gmail.com wrote:

> do we need to add a strip(1) test to run.bash?
> 
> https://codereview.appspot.com/9383043/
> 
> -- 
> 
> ---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.
> 
> 

[iant, 2013-05-15 13:40:58]

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

https://codereview.appspot.com/9383043/diff/4001/src/cmd/ld/elf.c#newcode895
src/cmd/ld/elf.c:895: if(!debug['d'] || linkmode == LinkExternal)
I don't see a need to test debug['d'] here.  It's normal for a statically linked
program to have a .tbss section.  Does something go wrong if we simply always
add .tbss (except on OpenBSD)?

[minux1, 2013-05-16 11:43:01]

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

https://codereview.appspot.com/9383043/diff/4001/src/cmd/ld/elf.c#newcode895
src/cmd/ld/elf.c:895: if(!debug['d'] || linkmode == LinkExternal)
On 2013/05/15 13:40:58, iant wrote:
> I don't see a need to test debug['d'] here.  It's normal for a statically
linked
> program to have a .tbss section.  Does something go wrong if we simply always
> add .tbss (except on OpenBSD)?
for internal static linking (debug['d'] is true), we manage TLS slots ourselves,
so if we add .tbss section and people strip(1)/objcopy(1) the binary,
the binutils might incorrectly add PT_TLS to program header, which is of no use.

of course, if you think it's ok, i will remove that condition.

[iant, 2013-05-17 16:44:48]

LGTM

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

https://codereview.appspot.com/9383043/diff/4001/src/cmd/ld/elf.c#newcode895
src/cmd/ld/elf.c:895: if(!debug['d'] || linkmode == LinkExternal)
On 2013/05/16 11:43:01, minux wrote:
> On 2013/05/15 13:40:58, iant wrote:
> > I don't see a need to test debug['d'] here.  It's normal for a statically
> linked
> > program to have a .tbss section.  Does something go wrong if we simply
always
> > add .tbss (except on OpenBSD)?
> for internal static linking (debug['d'] is true), we manage TLS slots
ourselves,
> so if we add .tbss section and people strip(1)/objcopy(1) the binary,
> the binutils might incorrectly add PT_TLS to program header, which is of no
use.
> 
> of course, if you think it's ok, i will remove that condition.

OK, fair enough.

[minux1, 2013-05-17 18:42:01]

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

cmd/ld: emit .tbss section when doing dynamic internal linking
Fixes issue 5200.

R=iant, dave
CC=golang-dev
https://codereview.appspot.com/9383043