<div dir="ltr">Ah, I think Rafael's suggestion is fantastic. That's better aligned with the existing code than reducing the hash by one bit.</div><div class="gmail_extra"><br><div class="gmail_quote">On Wed, Jan 10, 2018 at 2:46 AM, Dimitry Andric <span dir="ltr"><<a href="mailto:dimitry@andric.com" target="_blank">dimitry@andric.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Yes, that also fixes the race.  It does require calling getShardId() even if the piece is not live, but it does not look like getShardId() is a very expensive function.<br>
<br>
Rui, what do you think?  This might be a nice alternative solution.<br>
<span class="HOEnZb"><font color="#888888"><br>
-Dimitry<br>
</font></span><div class="HOEnZb"><div class="h5"><br>
> On 10 Jan 2018, at 01:47, Rafael Avila de Espindola <<a href="mailto:rafael.espindola@gmail.com">rafael.espindola@gmail.com</a>> wrote:<br>
><br>
> The race is in:<br>
><br>
><br>
>        if (!Sec->Pieces[I].Live)<br>
>          continue;<br>
>        size_t ShardId = getShardId(Sec->Pieces[I].<wbr>Hash);<br>
>        if ((ShardId & (Concurrency - 1)) == ThreadId)<br>
>          Sec->Pieces[I].OutputOff = Shards[ShardId].add(Sec-><wbr>getData(I));<br>
><br>
> Where one section can be checking Live and another is setting OutputOff,<br>
> correct? If that is the case, why not just do hash check first:<br>
><br>
>        size_t ShardId = getShardId(Sec->Pieces[I].<wbr>Hash);<br>
>        if ((ShardId & (Concurrency - 1)) == ThreadId && Sec->Pieces[I].Live)<br>
>          Sec->Pieces[I].OutputOff = Shards[ShardId].add(Sec-><wbr>getData(I));<br>
><br>
> Cheers,<br>
> Rafael<br>
><br>
> Dimitry Andric via Phabricator <<a href="mailto:reviews@reviews.llvm.org">reviews@reviews.llvm.org</a>> writes:<br>
><br>
>> dim created this revision.<br>
>> dim added reviewers: ruiu, rafael.<br>
>><br>
>> As reported in bug 35788, <a href="https://reviews.llvm.org/rL316280" rel="noreferrer" target="_blank">https://reviews.llvm.org/<wbr>rL316280</a> reintroduces a race between two<br>
>> members of SectionPiece, which share the same 64 bit memory location.<br>
>><br>
>> Reduce the Hash member by 1 bit, and move the Live member next to it,<br>
>> instead, while making OuputOff use the full 64 bit again.<br>
>><br>
>> Adjust getShardId to cope with Hashes being 31 bits wide (I hope I<br>
>> interpreted that function correctly... :)<br>
>><br>
>><br>
>> Repository:<br>
>>  rLLD LLVM Linker<br>
>><br>
>> <a href="https://reviews.llvm.org/D41884" rel="noreferrer" target="_blank">https://reviews.llvm.org/<wbr>D41884</a><br>
>><br>
>> Files:<br>
>>  ELF/InputSection.h<br>
>>  ELF/SyntheticSections.h<br>
>><br>
>><br>
>> Index: ELF/SyntheticSections.h<br>
>> ==============================<wbr>==============================<wbr>=======<br>
>> --- ELF/SyntheticSections.h<br>
>> +++ ELF/SyntheticSections.h<br>
>> @@ -710,7 +710,7 @@<br>
>>   // If we use lower bits, it significantly increases the probability of<br>
>>   // hash collisons.<br>
>>   size_t getShardId(uint32_t Hash) {<br>
>> -    return Hash >> (32 - llvm::countTrailingZeros(<wbr>NumShards));<br>
>> +    return Hash >> (31 - llvm::countTrailingZeros(<wbr>NumShards));<br>
>>   }<br>
>><br>
>>   // Section size<br>
>> Index: ELF/InputSection.h<br>
>> ==============================<wbr>==============================<wbr>=======<br>
>> --- ELF/InputSection.h<br>
>> +++ ELF/InputSection.h<br>
>> @@ -200,13 +200,13 @@<br>
>> // be found by looking at the next one).<br>
>> struct SectionPiece {<br>
>>   SectionPiece(size_t Off, uint32_t Hash, bool Live)<br>
>> -      : InputOff(Off), Hash(Hash), OutputOff(-1),<br>
>> -        Live(Live || !Config->GcSections) {}<br>
>> +      : InputOff(Off), Hash(Hash), Live(Live || !Config->GcSections),<br>
>> +        OutputOff(-1) {}<br>
>><br>
>>   uint32_t InputOff;<br>
>> -  uint32_t Hash;<br>
>> -  int64_t OutputOff : 63;<br>
>> -  uint64_t Live : 1;<br>
>> +  uint32_t Hash : 31;<br>
>> +  uint32_t Live : 1;<br>
>> +  int64_t OutputOff;<br>
>> };<br>
>><br>
>> static_assert(sizeof(<wbr>SectionPiece) == 16, "SectionPiece is too big");<br>
>><br>
>><br>
>> Index: ELF/SyntheticSections.h<br>
>> ==============================<wbr>==============================<wbr>=======<br>
>> --- ELF/SyntheticSections.h<br>
>> +++ ELF/SyntheticSections.h<br>
>> @@ -710,7 +710,7 @@<br>
>>   // If we use lower bits, it significantly increases the probability of<br>
>>   // hash collisons.<br>
>>   size_t getShardId(uint32_t Hash) {<br>
>> -    return Hash >> (32 - llvm::countTrailingZeros(<wbr>NumShards));<br>
>> +    return Hash >> (31 - llvm::countTrailingZeros(<wbr>NumShards));<br>
>>   }<br>
>><br>
>>   // Section size<br>
>> Index: ELF/InputSection.h<br>
>> ==============================<wbr>==============================<wbr>=======<br>
>> --- ELF/InputSection.h<br>
>> +++ ELF/InputSection.h<br>
>> @@ -200,13 +200,13 @@<br>
>> // be found by looking at the next one).<br>
>> struct SectionPiece {<br>
>>   SectionPiece(size_t Off, uint32_t Hash, bool Live)<br>
>> -      : InputOff(Off), Hash(Hash), OutputOff(-1),<br>
>> -        Live(Live || !Config->GcSections) {}<br>
>> +      : InputOff(Off), Hash(Hash), Live(Live || !Config->GcSections),<br>
>> +        OutputOff(-1) {}<br>
>><br>
>>   uint32_t InputOff;<br>
>> -  uint32_t Hash;<br>
>> -  int64_t OutputOff : 63;<br>
>> -  uint64_t Live : 1;<br>
>> +  uint32_t Hash : 31;<br>
>> +  uint32_t Live : 1;<br>
>> +  int64_t OutputOff;<br>
>> };<br>
>><br>
>> static_assert(sizeof(<wbr>SectionPiece) == 16, "SectionPiece is too big");<br>
<br>
</div></div></blockquote></div><br></div>