This is the mail archive of the
binutils@sourceware.org
mailing list for the binutils project.
Re: [PATCH v3] gold: Fix non-deterministic behaviour of Mips gold.
- From: Cary Coutant <ccoutant at gmail dot com>
- To: Vladimir Radosavljevic <Vladimir dot Radosavljevic at imgtec dot com>
- Cc: "binutils at sourceware dot org" <binutils at sourceware dot org>, Petar Jovanovic <Petar dot Jovanovic at imgtec dot com>
- Date: Tue, 29 Mar 2016 08:26:36 -0700
- Subject: Re: [PATCH v3] gold: Fix non-deterministic behaviour of Mips gold.
- Authentication-results: sourceware.org; auth=none
- References: <3060420525346945A0ADBD567348A91723740CBE at BADAG02 dot ba dot imgtec dot org>
> * 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