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

[Rui Ueyama via llvm-commits]

Ah, I think Rafael's suggestion is fantastic. That's better aligned with
the existing code than reducing the hash by one bit.

On Wed, Jan 10, 2018 at 2:46 AM, Dimitry Andric <dimitry at andric.com> wrote:

> 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 --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20180110/5c4bed8f/attachment.html>