Change CALL_ONCE_INITIALIZATION implementation to use static initialization

[mehdi_amini]

Summary

The previous implementation was breaking TSAN.

[kcc]

include/llvm/PassSupport.h

35

Mmm. I am probably missing something trivial (c++11-ish), but how is this going to work?
For me it looks like you define a static bool function, but never call it.

[kcc]

include/llvm/PassSupport.h

35

Aaaah, init_once is a bool variable, not a function. Oh, how much this is confusing...
Will this work with the versions of Visual C++ we use for LLVM?
I remember that static initialization was broken there (until recently?)

Why not std::call_once?

[mehdi_amini]

include/llvm/PassSupport.h

35

Yeah it would deserve a comment probably :)
I don't know if static initialization is safe in MSVC 2013 (I know it was announced to be "fixed", but don't know which version).

std::call_once require <mutex>, and I was worried of portability (I had trouble last time I tried to include it), and because of that we have an abstraction of mutex in libSupport I believe).

[kcc]

Reid, are function scope statics thread-safe with VC++ 2013?

include/llvm/PassSupport.h

35

If you can make it

static bool initialized_once = ...

it will be much more clear and not require a comment.

[mehdi_amini]

Improve readability

[rnk]

Initialization of local statics is not thread safe in VS 2013, so no, this approach doesn't work. There was some other mingw issue with std::call_once. Ask Chris about this, it's horrible.

These problems will go away when our support floor is VS 2015.

[mehdi_amini]

I don't think we have a plan to get rid of MSVC2013 on the short term. We may use a mutex to protect the initialization on MSVC2013 only?

[kcc]

We may also leave the current code under ifdef VC++ and use your new code for everything else.
If others agree, this will work for me too.

[rnk]

If we're going to suffer ifdefs, we should just use InitOnceExecuteOnce on the Windows side. I trust it more than this code.

We try not to let LLVM headers transitively pollute the world with windows.h macros, so we would need to have our own definitions of union _RTL_RUN_ONCE and the InitOnceExecuteOnce prototype.

[mehdi_amini]

I'm hesitating: the InitiOnceExecuteOnce seems "complex" to setup, knowing that it is *only* for MSVC2013, the tradeoff of leaving the existing code dying with the 2013 support is tempting.

[rnk]

I already don't trust the existing code, and tsan apparently says it is broken, so if you agree we don't want to use it on Unix, then I *really* don't want to use it on Windows either. :) Feel free to add the ifdef and make it my problem, though.

[kcc]

In D19271#406788, @joker.eph wrote:

I'm hesitating: the InitiOnceExecuteOnce seems "complex" to setup, knowing that it is *only* for MSVC2013, the tradeoff of leaving the existing code dying with the 2013 support is tempting.

Yea, I agree that we don't want very intrusive changes here.
Reid, what's wrong with leaving the existing code for VC++?

[mehdi_amini]

Yeah if we have multi-threaded client on Windows, you're right it should be fixed.
I'll give a try!

[mehdi_amini]

Chris B. pointed me to: http://reviews.llvm.org/D5922 which introduced llvm::call_once

It was reverted because of a bug in std::call_once for "aarch64 debian". Anyone knows if we are still bound by this "bug" two years after? The revert didn't give much information, but I guess I can recommit the original revision and see if it breaks again.

[rnk]

In D19271#410844, @joker.eph wrote:

Chris B. pointed me to: http://reviews.llvm.org/D5922 which introduced llvm::call_once

It was reverted because of a bug in std::call_once for "aarch64 debian". Anyone knows if we are still bound by this "bug" two years after? The revert didn't give much information, but I guess I can recommit the original revision and see if it breaks again.

Technically it's been 17 months (11/2014 to 4/2016). Given that it affected PPC and AArch64, I doubt the problem has been fixed. If you're willing to argue with people using busted platforms, then feel free to use std::call_once. If you just want this issue to go away and never bother you again, I'd stick with the thread safe static local. :)

[mehdi_amini]

I'm tempted to commit it just to get a correct bug report...

[kcc]

In D19271#410997, @joker.eph wrote:

I'm tempted to commit it just to get a correct bug report...

IMHO this is a good idea.
If one of the toolchains we depend on does not support proper C++ we need to have it documented (and then, fixed).

[beanz]

I’m completely in favor of re-landing the patch. I spent a lot of time working on it with Reid and David Majnemer. Having a call_once mechanism is a good way to avoid certain types of static initialization, so it would be great to have this support in LLVM.

At the time I didn’t have the ability to reproduce the issue that was reported (still probably don’t), and I was pulled off onto another project shortly after.

-Chris

[krytarowski]

This commit broke NetBSD as noted in http://reviews.llvm.org/D5922

[llvm-commits]

Can you investigate? So that we can have a correct implementation to propose instead.

Thanks.

[krytarowski]

OK, for now please revert to unbreak the bot.

[mehdi_amini]

r269584.

[krytarowski]

It looks odd.

gdb --args ../../bin/llvm-tblgen -gen-attrs -I /public/llvm/lib/IR -I /public/llvm/lib/Target -I /public/llvm/include /public/llvm/lib/IR/AttributesCompatFunc.td -o /tmp/lib/IR/AttributesCompatFunc.inc.tmp

(gdb) b main
Breakpoint 1 at 0x4e0758: file /public/llvm/utils/TableGen/TableGen.cpp, line 182.
(gdb) r
The program being debugged has been started already.
Start it from the beginning? (y or n) y
Starting program: /tmp/bin/llvm-tblgen -gen-attrs -I /public/llvm/lib/IR -I /public/llvm/lib/Target -I /public/llvm/include /public/llvm/lib/IR/AttributesCompatFunc.td -o /tmp/lib/IR/AttributesCompatFunc.inc.tmp

Program received signal SIGSEGV, Segmentation fault.
[Switching to LWP 1]
0x0000000000000000 in ?? ()
(gdb)

I'm investigating.

[krytarowski]

OK, std::call_once() itself is broken on NetBSD HEAD (I upgraded the buildbot to the snapshot from yesterday to focus on LLDB porting efforts for the upcoming release NetBSD-8).. I will move it to the other NetBSD developers.

CC: @joerg

[krytarowski]

It is also broken on 7.0, the latest stable release.

[krytarowski]

Reported in the NetBSD gnats as lib/51139. Once will be squashed I will let you know and stop holding on with it!

Thanks in advance!

[mehdi_amini]

Thanks for the quick investigation!

[chandlerc]

Mehdi, your patch used generic code for the non-'std::call_once' path. How about re-committing, but rather than putting it behind an Windows ifdef, instead just put it behind an "else". Then you can change the guard for using the std variant to just exclude NetBSD until the fix lands. That lets all other platforms make forward progress (including allowing Reid to work on a more reliable Windows-specific implementation if desired).

(I have the same motivation as you I suspect -- my TSan users are complaining. =])

[mehdi_amini]

Kamil: what's up on the NetBSD side by the way? If there is no plan to fix the combination of NetBSD + stdlibc++, we'll have to drop support for it at some point.

[krytarowski]

In D19271#447193, @mehdi_amini wrote:

Kamil: what's up on the NetBSD side by the way? If there is no plan to fix the combination of NetBSD + stdlibc++, we'll have to drop support for it at some point.

It's a bug that affects only libstdc++ in dynamic linking, static works. Also std::call_once libc++ works.

Dropping a platform for little reason isn't reasonable. Maybe we can prepare a CMake test detecting if it works, if so than enable it. After that libstc++ on NetBSD won't be holding back others.

[mehdi_amini]

In D19271#447313, @krytarowski wrote:

In D19271#447193, @mehdi_amini wrote:

Kamil: what's up on the NetBSD side by the way? If there is no plan to fix the combination of NetBSD + stdlibc++, we'll have to drop support for it at some point.

It's a bug that affects only libstdc++ in dynamic linking, static works. Also std::call_once libc++ works.

Dropping a platform for little reason isn't reasonable. Maybe we can prepare a CMake test detecting if it works, if so than enable it. After that libstc++ on NetBSD won't be holding back others.

"Little reason" is very subjective, so I'll be fine disagreeing with you: if this is an important configuration to support (dynamic link + NetBSD + libstdc++), then I expect someone that maintain this platform to make sure that there is a path forward (i.e. the problem is reported upstream to gcc developer, etc.). If a platform does not support correctly the C++ standard, dropping support for this platform is something that is reasonable to me, and I believe this is how we evaluated every platform/configuration drop in the past (we dropped MSVC2012 for this reason).

(I use platform as the combination of the OS+stdlib+host compiler+...)

[krytarowski]

Please set some deadline. Debugging it isn't that simple. Probably there is some symbol conflict.

CC: @joerg

[mehdi_amini]

There is no deadline in this case, because we'll work around it with macro as long as it is not too bad. I see it more a matter of principle: making sure someone is following up so that it gets resolved "at some point". I don't really like issues that are not tracked : they are less likely to ever be resolved...

[krytarowski]

I'm declaring myself responsible for this bug. gdb(1) so far isn't helping at all, it disregards setting breakpoints even in main() of:

$ cat test.cpp                                                                                                                                      
#include <iostream>
#include <thread>
#include <mutex>

std::once_flag flag;

int main()
{
    std::call_once(flag, [](){ std::cout << "Simple example: called once\n"; });
}

I will keep you posted.

include/llvm/PassSupport.h

 #include "llvm/PassInfo.h"
 #include "llvm/PassRegistry.h"
 #include "llvm/Support/Atomic.h"
 
 namespace llvm {
 
 class TargetMachine;
 
-#define CALL_ONCE_INITIALIZATION(function) \
-  static volatile sys::cas_flag initialized = 0; \
-  sys::cas_flag old_val = sys::CompareAndSwap(&initialized, 1, 0); \
-  if (old_val == 0) { \
-    function(Registry); \
-    sys::MemoryFence(); \
-    TsanIgnoreWritesBegin(); \
-    TsanHappensBefore(&initialized); \
-    initialized = 2; \
-    TsanIgnoreWritesEnd(); \
-  } else { \
-    sys::cas_flag tmp = initialized; \
-    sys::MemoryFence(); \
-    while (tmp != 2) { \
-      tmp = initialized; \
-      sys::MemoryFence(); \
-    } \
-  } \
-  TsanHappensAfter(&initialized);
+#define CALL_ONCE_INITIALIZATION(function)                                     \
+  static bool initialized_once = [&] {                                         \
+    function(Registry);                                                        \
+    return true;                                                               \
+  }();                                                                         \
+  (void) initialized_once;
 
 #define INITIALIZE_PASS(passName, arg, name, cfg, analysis) \
   static void* initialize##passName##PassOnce(PassRegistry &Registry) { \
     PassInfo *PI = new PassInfo(name, arg, & passName ::ID, \
       PassInfo::NormalCtor_t(callDefaultCtor< passName >), cfg, analysis); \
     Registry.registerPass(*PI, true); \
     return PI; \
   } \