code review 5823055: cmd/ld, cmd/6l, cmd/8l: fix hidden/local symbol import for ELF systems

cmd/ld, cmd/6l, cmd/8l, cmd/5l: fix hidden/local symbol import for ELF systems
   Introduce a newsym() to cmd/lib.c to add a symbol but don't add
them to hash table.
   Introduce a new bit flag SHIDDEN and bit mask SMASK to handle hidden
and/or local symbols in ELF symbol tables. Though we still need to order
the symbol table entries correctly.
   Fix for issue 3261 comment #9.
   For CL 5822049.

[minux1, 2012-03-15 04:36:57]

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

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

[rsc, 2012-03-15 18:57:51]

LGTM but wait for iant

[iant, 2012-03-15 19:46:02]

http://codereview.appspot.com/5823055/diff/3002/src/cmd/ld/ldelf.c
File src/cmd/ld/ldelf.c (right):

http://codereview.appspot.com/5823055/diff/3002/src/cmd/ld/ldelf.c#newcode762
src/cmd/ld/ldelf.c:762: // fall through
You don't want a STB_GLOBAL symbol to fall through into code that changes the
symbol name.

http://codereview.appspot.com/5823055/diff/3002/src/cmd/ld/ldelf.c#newcode766
src/cmd/ld/ldelf.c:766: newname = smprint("%s_%d", sym->name, i);
A local symbol is unique across object files as well as within an object file. 
Nothing else is ever going to look up the symbol by name--if it did, presumably
this patch would not work.  So I don't think we need to add this symbol to the
hash table at all.  I think we should have a variant of lookup that creates the
symbol and adds it to the allsym list but does not add it to the hash table.

[minux1, 2012-03-16 10:35:57]

PTAL.
On 2012/03/15 19:46:02, iant wrote:
> http://codereview.appspot.com/5823055/diff/3002/src/cmd/ld/ldelf.c
> File src/cmd/ld/ldelf.c (right):
> 
> http://codereview.appspot.com/5823055/diff/3002/src/cmd/ld/ldelf.c#newcode762
> src/cmd/ld/ldelf.c:762: // fall through
> You don't want a STB_GLOBAL symbol to fall through into code that changes the
> symbol name.
Done. 
> http://codereview.appspot.com/5823055/diff/3002/src/cmd/ld/ldelf.c#newcode766
> src/cmd/ld/ldelf.c:766: newname = smprint("%s_%d", sym->name, i);
> A local symbol is unique across object files as well as within an object file.

> Nothing else is ever going to look up the symbol by name--if it did,
presumably
> this patch would not work.  So I don't think we need to add this symbol to the
> hash table at all.  I think we should have a variant of lookup that creates
the
> symbol and adds it to the allsym list but does not add it to the hash table.
I've added a newsym() to handle this case.
Also fix the hidden visibility symbol problem, because on Linux/386, we have to
deal with (possible duplicate) __i686.get_pc_thunk.bx.

[iant, 2012-03-16 13:55:10]

http://codereview.appspot.com/5823055/diff/11010/src/cmd/ld/data.c
File src/cmd/ld/data.c (right):

http://codereview.appspot.com/5823055/diff/11010/src/cmd/ld/data.c#newcode162
src/cmd/ld/data.c:162: if(r->sym != S && (r->sym->type == 0 || r->sym->type ==
SXREF) && !(r->sym->type & SHIDDEN)) {
I don't understand this change.

http://codereview.appspot.com/5823055/diff/11010/src/cmd/ld/ldelf.c
File src/cmd/ld/ldelf.c (right):

http://codereview.appspot.com/5823055/diff/11010/src/cmd/ld/ldelf.c#newcode760
src/cmd/ld/ldelf.c:760: // symbol hash table, but mark them as hidden and dupok
Hidden symbols aren't permitted to be duplicated any more than any other defined
symbol is allowed to be duplicated.  Perhaps this approach is OK for now but the
comment needs to be fixed.

The reason that __i686.get_pc_thunk.bx is permitted to be duplicated is that the
function is defined in a section group.  The linker keeps only one copy of a
section group that has the GRP_COMDAT flag set.  Symbol definitions in a copy
that is discarded are treated as undefined symbols.  Thus the linker only ever
sees one definition of the symbol.

http://codereview.appspot.com/5823055/diff/11010/src/cmd/ld/symtab.c
File src/cmd/ld/symtab.c (right):

http://codereview.appspot.com/5823055/diff/11010/src/cmd/ld/symtab.c#newcode110
src/cmd/ld/symtab.c:110: bind = (ver || (x->type & SHIDDEN)) ? STB_LOCAL :
STB_GLOBAL;
Tecnically ELF requires that all STB_LOCAL symbols precede all STB_GLOBAL and
STB_WEAK symbols in the symbol table.  I guess we haven't been doing that so
far, though.

[minux1, 2012-03-17 08:52:43]

PTAL.
On 2012/03/16 13:55:10, iant wrote:
> http://codereview.appspot.com/5823055/diff/11010/src/cmd/ld/data.c#newcode162
> src/cmd/ld/data.c:162: if(r->sym != S && (r->sym->type == 0 || r->sym->type ==
> SXREF) && !(r->sym->type & SHIDDEN)) {
> I don't understand this change.
Sorry, this code is wrong. It's left over of old code.
> 
> http://codereview.appspot.com/5823055/diff/11010/src/cmd/ld/ldelf.c#newcode760
> src/cmd/ld/ldelf.c:760: // symbol hash table, but mark them as hidden and
dupok
> Hidden symbols aren't permitted to be duplicated any more than any other
defined
> symbol is allowed to be duplicated.  Perhaps this approach is OK for now but
the
> comment needs to be fixed.
> 
> The reason that __i686.get_pc_thunk.bx is permitted to be duplicated is that
the
> function is defined in a section group.  The linker keeps only one copy of a
> section group that has the GRP_COMDAT flag set.  Symbol definitions in a copy
> that is discarded are treated as undefined symbols.  Thus the linker only ever
> sees one definition of the symbol.
Fixed the comment.
 
>
http://codereview.appspot.com/5823055/diff/11010/src/cmd/ld/symtab.c#newcode110
> src/cmd/ld/symtab.c:110: bind = (ver || (x->type & SHIDDEN)) ? STB_LOCAL :
> STB_GLOBAL;
> Tecnically ELF requires that all STB_LOCAL symbols precede all STB_GLOBAL and
> STB_WEAK symbols in the symbol table.  I guess we haven't been doing that so
> far, though.
OK, should I just disable the STB_LOCAL generation all together or add a TODO?
(I've tried to do a two pass symbol table generation, but I think it will change
too much
code at this stage.)

If we violate this ELF requirement, will anything bad happen?

[iant, 2012-03-18 04:24:18]

LGTM

Thanks for bearing with me.

Violating the ELF requirement that all STB_LOCAL symbols precede
STB_GLOBAL/STB_WEAK symbols is not too serious when generating an executable. 
It may break something but nothing important, and in any case nobody has
complained about it so far.

Getting it wrong when generating an object file would break linking that object
file, but that doesn't apply here.

[minux1, 2012-03-18 07:15:36]

On 2012/03/18 04:24:18, iant wrote:
> Thanks for bearing with me.
I'd like to thank you not only for pointing out issues in my code, but also
providing so much valuable background information.
> Violating the ELF requirement that all STB_LOCAL symbols precede
> STB_GLOBAL/STB_WEAK symbols is not too serious when generating an executable. 
> It may break something but nothing important, and in any case nobody has
> complained about it so far.
> 
> Getting it wrong when generating an object file would break linking that
object
> file, but that doesn't apply here.
OK. Because this is a slightly bigger change on the linker before Go 1, I'd like
to
play safe and wait for one more green light before submitting.

[rsc, 2012-03-26 19:35:31]

Hi,

Please do not submit this before Go 1.
It would have been fine to submit eight days ago,
and then it would have gotten some use during the
past week, but now it is too late to make such a
substantial change to the linker.  Sorry.

Russ

[minux1, 2012-04-06 07:32:49]

ping.

[rsc, 2012-04-10 20:14:58]

It's fine to submit this now.  I did not re-review the code.

[minux1, 2012-04-21 19:22:30]

Found a new bug in this approach.

When testing this CL on Linux/ARM with the cgo support patches.
I noticed that the gcc 4.6.1/binutils 2.21.53 I used generate local function
symbol
that is used by relocations.

Due to our implementation in ldelf(), we first load relocations, which calls
readsym()
to read the respective symbol and then loads sub symbols for each sections which
agains calls readsym(). In effect, each used local symbol is readsym()'ed twice,
for
normal symbol, this will be fine, because they are in hash table. But for this
CL, they
will end up in two Syms that aren't in the symbol hash table, and this leads to
trouble
when doing the relocations.

We have two options here:
1, always enter function symbols and object symbols into the hash table (we must
ignore ElfSymTypeNone symbols, because of .LC0's)
2, reverse the order of relocation loading and loading of symbols, and keep a
array
of loaded symbols, so that when loading relocation, we don't need to re-read the
referenced symbols.

IMO, both approach are acceptable, any suggestions?

[minux1, 2012-04-21 19:39:15]

On 2012/04/21 19:22:30, minux wrote:
> We have two options here:
> 1, always enter function symbols and object symbols into the hash table (we
must
> ignore ElfSymTypeNone symbols, because of .LC0's)
Also found relocations for .LC0 symbol, so this option is no longer viable.
> 2, reverse the order of relocation loading and loading of symbols, and keep a
array
> of loaded symbols, so that when loading relocation, we don't need to re-read
the
> referenced symbols.

Maybe the best approach is to revert back to make each local symbol a unique but
deterministic name (for example, append symbol index to its name), and always
add them into the hash table.

[minux1, 2012-04-21 20:22:32]

PTAL.

Reversed the order of relocation loading and sub-symbol loading.
Tested on Linux/amd64, Linux/386, Linux/ARM and FreeBSD/amd64.

[iant, 2012-05-02 00:36:31]

LGTM

[minux1, 2012-05-22 18:32:39]

*** Submitted as http://code.google.com/p/go/source/detail?r=046668c6315e ***

cmd/ld, cmd/6l, cmd/8l, cmd/5l: fix hidden/local symbol import for ELF systems
   Introduce a newsym() to cmd/lib.c to add a symbol but don't add
them to hash table.
   Introduce a new bit flag SHIDDEN and bit mask SMASK to handle hidden
and/or local symbols in ELF symbol tables. Though we still need to order
the symbol table entries correctly.
   Fix for issue 3261 comment #9.
   For CL 5822049.

R=iant, rsc
CC=golang-dev
http://codereview.appspot.com/5823055