[PATCH] D41884: Fix thread race between SectionPiece's OutputOff and Live members

[Dimitry Andric via llvm-commits]

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.

Rui, what do you think?  This might be a nice alternative solution.

-Dimitry

> On 10 Jan 2018, at 01:47, Rafael Avila de Espindola <rafael.espindola at gmail.com> wrote:
> 
> The race is in:
> 
> 
>        if (!Sec->Pieces[I].Live)
>          continue;
>        size_t ShardId = getShardId(Sec->Pieces[I].Hash);
>        if ((ShardId & (Concurrency - 1)) == ThreadId)
>          Sec->Pieces[I].OutputOff = Shards[ShardId].add(Sec->getData(I));
> 
> Where one section can be checking Live and another is setting OutputOff,
> correct? If that is the case, why not just do hash check first:
> 
>        size_t ShardId = getShardId(Sec->Pieces[I].Hash);
>        if ((ShardId & (Concurrency - 1)) == ThreadId && Sec->Pieces[I].Live)
>          Sec->Pieces[I].OutputOff = Shards[ShardId].add(Sec->getData(I));
> 
> Cheers,
> Rafael
> 
> Dimitry Andric via Phabricator <reviews at reviews.llvm.org> writes:
> 
>> dim created this revision.
>> dim added reviewers: ruiu, rafael.
>> 
>> As reported in bug 35788, https://reviews.llvm.org/rL316280 reintroduces a race between two
>> members of SectionPiece, which share the same 64 bit memory location.
>> 
>> Reduce the Hash member by 1 bit, and move the Live member next to it,
>> instead, while making OuputOff use the full 64 bit again.
>> 
>> Adjust getShardId to cope with Hashes being 31 bits wide (I hope I
>> interpreted that function correctly... :)
>> 
>> 
>> Repository:
>>  rLLD LLVM Linker
>> 
>> https://reviews.llvm.org/D41884
>> 
>> Files:
>>  ELF/InputSection.h
>>  ELF/SyntheticSections.h
>> 
>> 
>> Index: ELF/SyntheticSections.h
>> ===================================================================
>> --- ELF/SyntheticSections.h
>> +++ ELF/SyntheticSections.h
>> @@ -710,7 +710,7 @@
>>   // If we use lower bits, it significantly increases the probability of
>>   // hash collisons.
>>   size_t getShardId(uint32_t Hash) {
>> -    return Hash >> (32 - llvm::countTrailingZeros(NumShards));
>> +    return Hash >> (31 - llvm::countTrailingZeros(NumShards));
>>   }
>> 
>>   // Section size
>> Index: ELF/InputSection.h
>> ===================================================================
>> --- ELF/InputSection.h
>> +++ ELF/InputSection.h
>> @@ -200,13 +200,13 @@
>> // be found by looking at the next one).
>> struct SectionPiece {
>>   SectionPiece(size_t Off, uint32_t Hash, bool Live)
>> -      : InputOff(Off), Hash(Hash), OutputOff(-1),
>> -        Live(Live || !Config->GcSections) {}
>> +      : InputOff(Off), Hash(Hash), Live(Live || !Config->GcSections),
>> +        OutputOff(-1) {}
>> 
>>   uint32_t InputOff;
>> -  uint32_t Hash;
>> -  int64_t OutputOff : 63;
>> -  uint64_t Live : 1;
>> +  uint32_t Hash : 31;
>> +  uint32_t Live : 1;
>> +  int64_t OutputOff;
>> };
>> 
>> static_assert(sizeof(SectionPiece) == 16, "SectionPiece is too big");
>> 
>> 
>> Index: ELF/SyntheticSections.h
>> ===================================================================
>> --- ELF/SyntheticSections.h
>> +++ ELF/SyntheticSections.h
>> @@ -710,7 +710,7 @@
>>   // If we use lower bits, it significantly increases the probability of
>>   // hash collisons.
>>   size_t getShardId(uint32_t Hash) {
>> -    return Hash >> (32 - llvm::countTrailingZeros(NumShards));
>> +    return Hash >> (31 - llvm::countTrailingZeros(NumShards));
>>   }
>> 
>>   // Section size
>> Index: ELF/InputSection.h
>> ===================================================================
>> --- ELF/InputSection.h
>> +++ ELF/InputSection.h
>> @@ -200,13 +200,13 @@
>> // be found by looking at the next one).
>> struct SectionPiece {
>>   SectionPiece(size_t Off, uint32_t Hash, bool Live)
>> -      : InputOff(Off), Hash(Hash), OutputOff(-1),
>> -        Live(Live || !Config->GcSections) {}
>> +      : InputOff(Off), Hash(Hash), Live(Live || !Config->GcSections),
>> +        OutputOff(-1) {}
>> 
>>   uint32_t InputOff;
>> -  uint32_t Hash;
>> -  int64_t OutputOff : 63;
>> -  uint64_t Live : 1;
>> +  uint32_t Hash : 31;
>> +  uint32_t Live : 1;
>> +  int64_t OutputOff;
>> };
>> 
>> static_assert(sizeof(SectionPiece) == 16, "SectionPiece is too big");

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 223 bytes
Desc: Message signed with OpenPGP
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20180110/9188181c/attachment.sig>