[PATCH] Removing the static initializer in ManagedStatic.cpp by using llvm_call_once to initialize the ManagedStatic mutex.

[David Majnemer]

On Mon, Oct 27, 2014 at 1:52 PM, Chris Bieneman <beanz at apple.com> wrote:

> A few notes.
>
> According to Chandler's related RFC (
> http://lists.cs.uiuc.edu/pipermail/llvmdev/2014-September/077081.html) we
> do not have a requirement that all platforms support <atomic>.
>

I quickly chatted with Chandler and he mentioned that <atomic> is fine so
long as we are sticking to std::atomic<integer-type>.
He wanted to convey that we want to use <atomic> more, not that we can't
use it at all today.


>
> >>! In D5922#11, @majnemer wrote:
> > I'm afraid not.  Because your implementation uses operations which are
> not atomic, it has undefined behavior in the eyes of C++11.
> >
> > C++11 [intro.multithread]p21:
> > The execution of a program contains a data race if it contains two
> conflicting actions in different threads, at least one of which is not
> atomic, and neither happens before the other. Any such data race results in
> undefined behavior.
>
> I think you're misconstruing what this means. Your statement would seem to
> imply that until C++11 nobody ever managed to write thread safe code--
> which is absurd.


I'm not implying that but relying on undefined behavior isn't great either.

At best, I dislike declaring the variable "volatile".  I would instead
prefer casting to volatile and loading, this would make things more
explicit.


> There are certainly undefined behaviors in the implementation I've
> provided, but they are known, and appropriately handled.
>

I don't think you can "handle undefined behavior" by definition.


>
> Both the MemFences, and the tmp in the else branch of the original patch
> can be safely removed, and the code can be re-written into a while loop for
> performance. An example implementation would be:
>
> ```
> enum InitStatus {
>   Done = -1,
>   Uninitialized = 0,
>   Wait = 1
> };
>
> void llvm::call_once(once_flag& flag, void (*UserFn)(void)) {
>   while (flag != Done) {
>     if (flag == Wait) {
>

I would much rather prefer you do a single load of flag instead.  Also, you
don't have a fence on the Wait => Done side; I believe this can lead to
unfortunate reorderings.


>       ::Sleep(1);
>       continue;
>     }
>
>     sys::cas_flag old_val = sys::CompareAndSwap(&flag, Wait,
> Uninitialized);
>     if (old_val == Uninitialized) {
>       UserFn();
>       sys::MemoryFence();
>       flag = Done;
>     }
>   }
> }
> ```
>
> I (and some of my colleagues) believe that this is perfectly safe code,
> and it eliminates unnecessary MemFences (which you raised concerns about).
> Do you see a problem with this implementation (other than the fact that it
> doesn't use std::atomic)?
>
> I don't have any particular opposition to using std::atomic, however since
> we don't have an enforced requirement I'm not sure we can assume we have
> it. Please keep in mind I think we still support deploying back to Windows
> XP.
>
> http://reviews.llvm.org/D5922
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20141027/6dfb8530/attachment.html>