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

[Rafael Avila de Espindola via llvm-commits]

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");