[llvm] r239566 - [Support] Fix a race initializing a static local in MSVC

[David Majnemer]

On Fri, Jun 12, 2015 at 10:06 AM, Mehdi Amini <mehdi.amini at apple.com> wrote:

> Thanks for the answer.
>
> On Jun 12, 2015, at 9:42 AM, David Majnemer <david.majnemer at gmail.com>
> wrote:
>
>
>
> On Fri, Jun 12, 2015 at 7:56 AM, Mehdi Amini <mehdi.amini at apple.com>
> wrote:
>
>> Hi Reid,
>>
>> Can you explain how the change makes it thread safe? It’s not obvious to
>> me right now.
>>
>
> In the MSVC world, loads marked volatile have acquire semantics and stores
> marked volatile have release semantics.  We actually don't need such strong
> semantics here, just a monotonic guarantee on the loads and stores which
> volatile will more or less give us on GCC (for mingw builds and the like).
>
> It contains a benign race but we are OK with that (two threads might both
> compute the allocation granularity but they will both produce the same
> result.)
>
>
> I see, we’re fine with a race on the initialization in the sense that it
> can happen multiple times (since I guess getAllocationGranularity() is
> pure) but it has to be visible in each thread.
>
> Also I read online that MSVC 2013 should support thread-safe local static
> initialization (see:
> http://herbsutter.com/2013/09/09/visual-studio-2013-rc-is-now-available/ )
> Isn’t MSVC2013 the current minimal version for LLVM?
>

That blog post didn't make things entirely clear.  Herb is referring to the
VS "14" CTP which has this support.  The first stable version to support
this will be MSVC 2015.


>
> Thanks,
>
> —
> Mehdi
>
>
>
>
>>
>> Thanks,
>>
>> Mehdi
>>
>> > On Jun 11, 2015, at 3:22 PM, Reid Kleckner <reid at kleckner.net> wrote:
>> >
>> > Author: rnk
>> > Date: Thu Jun 11 17:22:45 2015
>> > New Revision: 239566
>> >
>> > URL: http://llvm.org/viewvc/llvm-project?rev=239566&view=rev
>> > Log:
>> > [Support] Fix a race initializing a static local in MSVC
>> >
>> > static local initialization isn't thread safe with MSVC and a race was
>> > reported in PR23817. We can't use std::atomic because it's not trivially
>> > constructible, so instead do some lame volatile global integer
>> > manipulation.
>> >
>> > Modified:
>> >    llvm/trunk/lib/Support/Windows/Memory.inc
>> >
>> > Modified: llvm/trunk/lib/Support/Windows/Memory.inc
>> > URL:
>> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Support/Windows/Memory.inc?rev=239566&r1=239565&r2=239566&view=diff
>> >
>> ==============================================================================
>> > --- llvm/trunk/lib/Support/Windows/Memory.inc (original)
>> > +++ llvm/trunk/lib/Support/Windows/Memory.inc Thu Jun 11 17:22:45 2015
>> > @@ -78,7 +78,15 @@ MemoryBlock Memory::allocateMappedMemory
>> >   // While we'd be happy to allocate single pages, the Windows
>> allocation
>> >   // granularity may be larger than a single page (in practice, it is
>> 64K)
>> >   // so mapping less than that will create an unreachable fragment of
>> memory.
>> > -  static const size_t Granularity = getAllocationGranularity();
>> > +  // Avoid using one-time initialization of static locals here, since
>> they
>> > +  // aren't thread safe with MSVC.
>> > +  static volatile size_t GranularityCached;
>> > +  size_t Granularity = GranularityCached;
>> > +  if (Granularity == 0) {
>> > +    Granularity = getAllocationGranularity();
>> > +    GranularityCached = Granularity;
>> > +  }
>> > +
>> >   const size_t NumBlocks = (NumBytes+Granularity-1)/Granularity;
>> >
>> >   uintptr_t Start = NearBlock ?
>> reinterpret_cast<uintptr_t>(NearBlock->base()) +
>> >
>> >
>> > _______________________________________________
>> > llvm-commits mailing list
>> > llvm-commits at cs.uiuc.edu
>> > http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>>
>>
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150612/4a067b30/attachment.html>