Re: [PATCH v3] gold: Fix non-deterministic behaviour of Mips gold.

[Cary Coutant <ccoutant at gmail dot com>]

>     * mips.cc (Mips_got_entry::hash): Change hash algorithm.
>     (class Mips_symbol_hash): New class.
>     (Mips_got_info::Global_got_entry_set): New type.
>     (Mips_got_info::global_got_symbols): Change return type to
>     Global_got_entry_set.
>     (Mips_got_info::global_got_symbols_): Change type to
>     Global_got_entry_set.
>     (Mips_symbol::hash): New method.
>     (Mips_output_data_la25_stub::symbols_): Change type to std::vector.
>     (Mips_output_data_mips_stubs::Mips_stubs_entry_set): New type.
>     (Mips_output_data_mips_stubs::symbols_): Change type to
>     Mips_stubs_entry_set.
>     (Mips_got_info::add_reloc_only_entries): Change type of iterator
>     to Global_got_entry_set.
>     (Mips_got_info::count_got_symbols): Likewise.
>     (Mips_output_data_la25_stub::create_la25_stub): Use push_back
>     for adding entries to symbols_.
>     (Mips_output_data_la25_stub::do_write): Change type of iterator
>     to std::vector.
>     (Mips_output_data_mips_stubs::set_lazy_stub_offsets): Change type
>     of iterator to Mips_stubs_entry_set.
>     (Mips_output_data_mips_stubs::set_needs_dynsym_value): Likewise.
>     (Mips_output_data_mips_stubs::do_write): Likewise.

     if (this->symndx_ != -1U)
+      return this->symndx_
+          ^ gold::string_hash<char>(this->object()->name().c_str())
+          ^ this->d.addend;
+
+    return this->symndx_ ^ gold::string_hash<char>(this->d.sym->name());

The long expression needs parentheses, and the ^ operator should line
up with the term above it. But I'd suggest something like this
instead:

     size_t ret = this->symndx_ ^ gold::string_hash<char>(this->d.sym->name());
     if (this->symndx_ != -1U)
       ret ^= this->d.addend;
     return ret;

This hash strategy might produce more collisions than you would like,
as small symndx values may cancel out small addends. One approach
would be to shift one or more terms so cancellations are less likely
(see Reloc_stub::hash_value() in aarch64.cc, for example). Another
would be to use iterative_hash() from libiberty.

+  typedef Unordered_set<Mips_symbol<size>*,
+      Mips_symbol_hash<size> >  Global_got_entry_set;

This should be indented to line up. Either of the following would be OK:

   typedef Unordered_set<Mips_symbol<size>*,
                         Mips_symbol_hash<size> >  Global_got_entry_set;

(Second line aligns with "Mips_symbol" above.)

   typedef Unordered_set<Mips_symbol<size>*, Mips_symbol_hash<size> >
       Global_got_entry_set;

(Indented four spaces.)

+  // Unordered set of .MIPS.stubs entries.
+  typedef Unordered_set<Mips_symbol<size>*,
+      Mips_symbol_hash<size> > Mips_stubs_entry_set;

Likewise.

-cary