llvm dev - Dec 2016 - A potential race on StaticList in RegisterManagedStatic

Ptr member of ManagedStaticBase is now atomic.
In ManagedStaticBase::RegisterManagedStatic we have such code:

      void *Tmp = Creator();

      Ptr.store(Tmp, std::memory_order_release);
      DeleterFn = Deleter;

      // Add to list of managed statics.
      Next = StaticList;
      StaticList = this;


StaticList is not atomic and not guarded by any fence.
The same applies to the members DeleterFn and Next.

Doesn't it seem reasonable to change the code to

      DeleterFn = Deleter;

      // Add to list of managed statics.
      Next = StaticList;
      StaticList = this;
      Ptr.store(Tmp, std::memory_order_release);
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20161226/addc40f4/attachment.html>

Though it won't actually help to guard this: while (StaticList) - a fence
here is needed...

On Mon, Dec 26, 2016 at 9:20 AM, Viacheslav Nikolaev <
viacheslav.nikolaev at gmail.com> wrote:
> Ptr member of ManagedStaticBase is now atomic.
> In ManagedStaticBase::RegisterManagedStatic we have such code:
>
>       void *Tmp = Creator();
>
>       Ptr.store(Tmp, std::memory_order_release);
>       DeleterFn = Deleter;
>
>       // Add to list of managed statics.
>       Next = StaticList;
>       StaticList = this;
>
>
> StaticList is not atomic and not guarded by any fence.
> The same applies to the members DeleterFn and Next.
>
> Doesn't it seem reasonable to change the code to
>
>       DeleterFn = Deleter;
>
>       // Add to list of managed statics.
>       Next = StaticList;
>       StaticList = this;
>       Ptr.store(Tmp, std::memory_order_release);
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20161226/b50e927c/attachment.html>

What about the lock that is taken?

MutexGuard Lock(*getManagedStaticMutex());


— 
Mehdi

> On Dec 25, 2016, at 11:14 PM, Viacheslav Nikolaev via llvm-dev <llvm-dev
at lists.llvm.org> wrote:
> 
> Though it won't actually help to guard this: while (StaticList) - a
fence here is needed...
> 
> On Mon, Dec 26, 2016 at 9:20 AM, Viacheslav Nikolaev
<viacheslav.nikolaev at gmail.com <mailto:viacheslav.nikolaev at
gmail.com>> wrote:
> Ptr member of ManagedStaticBase is now atomic.
> In ManagedStaticBase::RegisterManagedStatic we have such code:
> 
>       void *Tmp = Creator();
> 
>       Ptr.store(Tmp, std::memory_order_release);
>       DeleterFn = Deleter;
>       
>       // Add to list of managed statics.
>       Next = StaticList;
>       StaticList = this;
> 
> 
> StaticList is not atomic and not guarded by any fence.
> The same applies to the members DeleterFn and Next.
> 
> Doesn't it seem reasonable to change the code to
> 
>       DeleterFn = Deleter;
>       
>       // Add to list of managed statics.
>       Next = StaticList;
>       StaticList = this;
>       Ptr.store(Tmp, std::memory_order_release);
> 
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20161225/4640195e/attachment.html>