[MIPS] Initial checkin for MIPS changes for GOLD.

[MIPS] Initial checkin for MIPS changes for GOLD.

BUG=http://code.google.com/p/nativeclient/issues/detail?id=2275 
TEST=pnacl/build.sh all

[petarj, 2012-04-27 19:06:30]

Initial checkin for MIPS changes for GOLD.

This is a cumulative patch taken from 

https://dmz-portal.mips.com/git/?p=gold.git

[robertm, 2012-04-27 19:46:15]

http://codereview.chromium.org/10252012/diff/1/gold/layout.cc
File gold/layout.cc (right):

http://codereview.chromium.org/10252012/diff/1/gold/layout.cc#newcode2546
gold/layout.cc:2546: target->fix_sections(this, symtab);
add a comment explaining what this does

http://codereview.chromium.org/10252012/diff/1/gold/output.cc
File gold/output.cc (right):

http://codereview.chromium.org/10252012/diff/1/gold/output.cc#newcode612
gold/output.cc:612: // Can't find the entry symbol, and it's not a number.  Use
This wont help with pnacl, wouldn't it be cleaner to make sure that you have
proper naming of the entry points - otherwise you are just hiding bugs

http://codereview.chromium.org/10252012/diff/1/gold/output.cc#newcode1158
gold/output.cc:1158: // R_MIPS_NONE relocation have to be first.
why ?  add comment

http://codereview.chromium.org/10252012/diff/1/gold/output.h
File gold/output.h (right):

http://codereview.chromium.org/10252012/diff/1/gold/output.h#newcode1779
gold/output.h:1779: add_absolute_null(unsigned int type, Output_data* od,
Address address)
in the spirit of keeping the deltas small - can this be avoided?

http://codereview.chromium.org/10252012/diff/1/gold/parameters.cc
File gold/parameters.cc (right):

http://codereview.chromium.org/10252012/diff/1/gold/parameters.cc#newcode229
gold/parameters.cc:229: if(this->target().machine_code() == elfcpp::EM_MIPS)
FYI: this will likely not work with pnacl

http://codereview.chromium.org/10252012/diff/1/gold/symtab.cc
File gold/symtab.cc (right):

http://codereview.chromium.org/10252012/diff/1/gold/symtab.cc#newcode2357
gold/symtab.cc:2357: bool got_offset_compare(Symbol *sym1, Symbol *sym2)
the convention seems to be to move "*" to the left when
used in a type

http://codereview.chromium.org/10252012/diff/1/gold/symtab.cc#newcode2373
gold/symtab.cc:2373: std::vector<Symbol *> externals;
are these change specific to MIPS - if yes explain why.

http://codereview.chromium.org/10252012/diff/1/gold/symtab.cc#newcode2388
gold/symtab.cc:2388: else if((!sym->has_dynsym_index() &&
sym->has_got_offset(0))
This condition is complex enough that it might be better to move it into its own
function, e.g.

some-meaningful-name(sym, is_pie)

http://codereview.chromium.org/10252012/diff/1/gold/symtab.cc#newcode2415
gold/symtab.cc:2415: std::sort(externals.begin(), externals.end(),
got_offset_compare);
mention all the extra work you do here in the function comment

http://codereview.chromium.org/10252012/diff/1/gold/symtab.cc#newcode3641
gold/symtab.cc:3641: Output_data* od, Value_type value,
indentation is off, also convention seems to be one param per line

http://codereview.chromium.org/10252012/diff/1/gold/symtab.cc#newcode3654
gold/symtab.cc:3654: Output_data* od, Value_type value,
here too

http://codereview.chromium.org/10252012/diff/1/gold/symtab.h
File gold/symtab.h (right):

http://codereview.chromium.org/10252012/diff/1/gold/symtab.h#newcode1900
gold/symtab.h:1900: // Index of the first global symbol that has .got entry.
if this is only used for MIPS, it maybe worth to discuss it a little more in a
comment

http://codereview.chromium.org/10252012/diff/1/gold/target.h
File gold/target.h (right):

http://codereview.chromium.org/10252012/diff/1/gold/target.h#newcode415
gold/target.h:415: // Return address of .text section. Some architecutres needs
it as entry
I would prefer not have this change and the stuff that 
depends on it in this CL.
It wont help for pnacl, and it seems to be one of these changes that add only a
little bit of convenience which is probably outweight by the complexity it
introduces.

[Aleksandar Simeonov, 2012-05-08 15:21:42]

Here are my replies to comments before I make the changes in Gold source code.

http://codereview.chromium.org/10252012/diff/1/gold/layout.cc
File gold/layout.cc (right):

http://codereview.chromium.org/10252012/diff/1/gold/layout.cc#newcode2546
gold/layout.cc:2546: target->fix_sections(this, symtab);
On 2012/04/27 19:46:15, robertm wrote:
> add a comment explaining what this does

Sets data with values that are only known after sections are finalized. In this
concrete case it is needed to set flag if there are more then 0x10000 symbols in
dynamic symbol table.

Will add that as a comment in the source.

http://codereview.chromium.org/10252012/diff/1/gold/output.cc
File gold/output.cc (right):

http://codereview.chromium.org/10252012/diff/1/gold/output.cc#newcode612
gold/output.cc:612: // Can't find the entry symbol, and it's not a number.  Use
On 2012/04/27 19:46:15, robertm wrote:
> This wont help with pnacl, wouldn't it be cleaner to make sure that you have
> proper naming of the entry points - otherwise you are just hiding bugs

MIPS port of Gold is made to work the same way as it LD does. So it first search
for -e entry command line option, then __start symbol, then address of first
byte of .text section and if none found, the address 0.

http://codereview.chromium.org/10252012/diff/1/gold/output.cc#newcode1158
gold/output.cc:1158: // R_MIPS_NONE relocation have to be first.
On 2012/04/27 19:46:15, robertm wrote:
> why ?  add comment

On IRIX n64 rtld would ignore all dynamic relocations before an R_MIPS_NONE
entry.

http://sourceware.org/ml/binutils/2006-10/msg00186.html

http://codereview.chromium.org/10252012/diff/1/gold/output.h
File gold/output.h (right):

http://codereview.chromium.org/10252012/diff/1/gold/output.h#newcode1779
gold/output.h:1779: add_absolute_null(unsigned int type, Output_data* od,
Address address)
On 2012/04/27 19:46:15, robertm wrote:
> in the spirit of keeping the deltas small - can this be avoided?

Since R_MIPS_NONE entry with address 0 has to be first in .rel.dyn section this
function is needed to create it (there is no function that can do that in
original Gold).

We can avoid this in PNaCl patch, but will have to be added to main GOLD
repository.

http://codereview.chromium.org/10252012/diff/1/gold/parameters.cc
File gold/parameters.cc (right):

http://codereview.chromium.org/10252012/diff/1/gold/parameters.cc#newcode229
gold/parameters.cc:229: if(this->target().machine_code() == elfcpp::EM_MIPS)
On 2012/04/27 19:46:15, robertm wrote:
> FYI: this will likely not work with pnacl 
>  

MIPS has __start as default entry symbol (other architectures usally has _start)
in Linux. This is needed for GOLD. We can resolve any PNaCl issue on a different
place.

http://codereview.chromium.org/10252012/diff/1/gold/symtab.cc
File gold/symtab.cc (right):

http://codereview.chromium.org/10252012/diff/1/gold/symtab.cc#newcode2357
gold/symtab.cc:2357: bool got_offset_compare(Symbol *sym1, Symbol *sym2)
On 2012/04/27 19:46:15, robertm wrote:
> the convention seems to be to move "*" to the left when
> used in a type

Will be done.

http://codereview.chromium.org/10252012/diff/1/gold/symtab.cc#newcode2373
gold/symtab.cc:2373: std::vector<Symbol *> externals;
On 2012/04/27 19:46:15, robertm wrote:
> are these change specific to MIPS - if yes explain why.

The dynamic symbol table is divided into local and global parts.  The global
part of the dynamic symbol table is further divided into two parts: symbols that
do not have GOT entries associated with them and symbols that do have GOT
entries associated with them.

http://codereview.chromium.org/10252012/diff/1/gold/symtab.cc#newcode2415
gold/symtab.cc:2415: std::sort(externals.begin(), externals.end(),
got_offset_compare);
On 2012/04/27 19:46:15, robertm wrote:
> mention all the extra work you do here in the function comment

Will be done.

http://codereview.chromium.org/10252012/diff/1/gold/symtab.cc#newcode3641
gold/symtab.cc:3641: Output_data* od, Value_type value,
On 2012/04/27 19:46:15, robertm wrote:
> indentation is off, also convention seems to be one param per line

It is not sure what is convention since before these declarations one standard
is used and after completely different.
We can change it though.

http://codereview.chromium.org/10252012/diff/1/gold/symtab.cc#newcode3654
gold/symtab.cc:3654: Output_data* od, Value_type value,
On 2012/04/27 19:46:15, robertm wrote:
> here too

Same as above.

http://codereview.chromium.org/10252012/diff/1/gold/symtab.h
File gold/symtab.h (right):

http://codereview.chromium.org/10252012/diff/1/gold/symtab.h#newcode1900
gold/symtab.h:1900: // Index of the first global symbol that has .got entry.
On 2012/04/27 19:46:15, robertm wrote:
> if this is only used for MIPS, it maybe worth to discuss it a little more in a
> comment

The dynamic symbol table is divided into local and global parts. The global part
of the dynamic symbol table is further divided into two parts: symbols that do
not have GOT entries associated with them and symbols that do have GOT entries
associated with them. This is index of the first global symbol that has .got
entry.

http://codereview.chromium.org/10252012/diff/1/gold/target.h
File gold/target.h (right):

http://codereview.chromium.org/10252012/diff/1/gold/target.h#newcode415
gold/target.h:415: // Return address of .text section. Some architecutres needs
it as entry
On 2012/04/27 19:46:15, robertm wrote:
> I would prefer not have this change and the stuff that 
> depends on it in this CL.
> It wont help for pnacl, and it seems to be one of these changes that add only
a
> little bit of convenience which is probably outweight by the complexity it
> introduces.

This part is added to have same behavior as ld. We can remove it from this
patch, but will be needed for GOLD main trunk submission.

[petarj, 2012-05-09 15:58:07]

Here is a new patch which includes the changes requested by Robert's review.
The code has also been rebased (changeset 20:2a4cafa72ca0).

[robertm, 2012-05-11 21:50:50]

A few more suggestions otherwise LGTM (= looks good to me)

Let me know when you have made the changes and I will commit
the patch for you. 

Just a small word of caution: we are not MIPS experts 
and wont be able to maintain your changes beyond a "best effort". It would best
if you a continuous build set up
on your side so that you know right away when something
gets broken.

http://codereview.chromium.org/10252012/diff/1/gold/layout.cc
File gold/layout.cc (right):

http://codereview.chromium.org/10252012/diff/1/gold/layout.cc#newcode2546
gold/layout.cc:2546: target->fix_sections(this, symtab);
On 2012/05/08 15:21:42, Aleksandar Simeonov wrote:
> On 2012/04/27 19:46:15, robertm wrote:
> > add a comment explaining what this does
> 
> Sets data with values that are only known after sections are finalized. In
this
> concrete case it is needed to set flag if there are more then 0x10000 symbols
in
> dynamic symbol table.
> 
> Will add that as a comment in the source.

The description you give here is much better than the comment you put in. Is
this something mips specific or a general fix to scons?

http://codereview.chromium.org/10252012/diff/10002/gold/symtab.cc
File gold/symtab.cc (right):

http://codereview.chromium.org/10252012/diff/10002/gold/symtab.cc#newcode2377
gold/symtab.cc:2377: std::vector<Symbol *> externals;
move "*" to left

http://codereview.chromium.org/10252012/diff/10002/gold/symtab.cc#newcode2392
gold/symtab.cc:2392: else if((!sym->has_dynsym_index() &&
sym->has_got_offset(0))
This condition is complex enough that it might be better to move it into its own
function, e.g.

some-meaningful-name(sym, is_pie)

http://codereview.chromium.org/10252012/diff/10002/gold/symtab.h
File gold/symtab.h (right):

http://codereview.chromium.org/10252012/diff/10002/gold/symtab.h#newcode1905
gold/symtab.h:1905: unsigned int global_got_index_;
it might be nice to also explain here why this was added, e.g.:  MIPS needs this
because ....

[petarj, 2012-05-15 14:27:45]

The style issues pointed out in the previous review have been fixed.
As of maintenance and correctness of MIPS-GOLD, our buildbots will be checking
it regularly, and we will be updating it with the already undergoing and future
work (mainly for mips.cc).

[robertm, 2012-06-08 20:16:50]

I have just committed the change

On 2012/05/15 14:27:45, petarj wrote:
> The style issues pointed out in the previous review have been fixed.
> As of maintenance and correctness of MIPS-GOLD, our buildbots will be checking
> it regularly, and we will be updating it with the already undergoing and
future
> work (mainly for mips.cc).