New user self-registration is currently disabled. Please email eigen-core-team @ lists.tuxfamily.org if you need an account.
Bug 1152 - Data races in BLAS static initialization
Data races in BLAS static initialization
Status: RESOLVED FIXED
Product: Eigen
Classification: Unclassified
Component: General
3.3 (current stable)
All All
: Normal Unknown
Assigned To: Nobody
:
Depends on:
Blocks: 3.3
  Show dependency treegraph
 
Reported: 2016-01-25 18:26 UTC by Benoit Jacob
Modified: 2016-01-26 16:45 UTC (History)
3 users (show)



Attachments
Fix blas static initialization race (69.04 KB, patch)
2016-01-26 16:04 UTC, Benoit Jacob
no flags Details | Diff
Fix blas static initialization race (68.87 KB, patch)
2016-01-26 16:12 UTC, Benoit Jacob
no flags Details | Diff

Description Benoit Jacob 2016-01-25 18:26:44 UTC
The blas/ implementation has several instances of this idiom:

  int some_blas_func() {

    static some_type some_static_data;

    static bool init = false;
    if(!init)
    {
      initialize(some_static_data);
      init = true;
    }

    ...

  }

The problem is that such initialization code is not reentrant. This concretely leads to errors with thread-safety-checking tools such as Thread Sanitizer and Helgrind; I could share a self-contained testcase if needed, but I hope that the problem is obvious enough that we would agree that there is no need for it.

The question is, do you have a preferred way to address this issue?

What would you think of using this idiom:

  struct SomeHelper {
    some_type some_data;
    SomeHelper() {
      initialize(some_data);
    }
  };

  int some_blas_func() {
    static SomeHelper some_helper;
  }

Indeed, this would have the following advantages:
 - Compiles in plain C++98 without dependency.
 - Guaranteed in C++11 mode to be thread-safe, according to http://stackoverflow.com/questions/8102125/is-local-static-variable-initialization-thread-safe-in-c11
 - Though not guaranteed in the C++98 language, this seems to have been mandated by the C++ ABI and implemented in GCC since 2004:
https://gcc.gnu.org/ml/gcc-patches/2004-08/msg01598.html
 - GCC has nondefault -fno-threadsafe-statics, which (see GCC man page) further supports that GCC is guarding statics by default, and the man page refers to the ABI requirement...
 - for MSVC, from some googling it seems like MSVC 2010 might NOT be safe but 2013 probably is:
http://herbsutter.com/2013/09/09/visual-studio-2013-rc-is-now-available/

In short, I'd be very surprised if this idiom were unsafe on any current compiler! (Does anyone care about Eigen/BLAS on MSVC 2010?)

Or, do you have any other preferred implementation strategy?
Comment 1 Benoit Jacob 2016-01-25 18:29:10 UTC
Note: this was discovered by Ury Zhilinsky at Google using Thread Sanitizer.
Comment 2 Benoit Jacob 2016-01-25 18:42:38 UTC
Actually, seeing how simple the initialization code is, we have alternatives:

1) Do not use a helper constructor, just initialize those static arrays of pointer-sized constants.

2) Do not even use static variables at all. The cost of writing a few pointer values is probably negligible.
Comment 3 Benoit Jacob 2016-01-25 18:50:27 UTC
Seeing how it's being used, how about

3) drop the tables of function pointers and instead use a switch statement on the code.
Comment 4 Gael Guennebaud 2016-01-25 20:38:22 UTC
I'd go with either 1) or 3), but for 3) I'd check the performance on small matrices. So if you prefer 3), first fix gemm only and I'll check (I already have a suitable and reliable benchmark for that).
Comment 5 Benoit Jacob 2016-01-26 16:04:42 UTC
Created attachment 647 [details]
Fix blas static initialization race

Here's my patch following option 1), just plain static const array of function pointers. Passes blas/testing.
Comment 6 Benoit Jacob 2016-01-26 16:12:54 UTC
Created attachment 648 [details]
Fix blas static initialization race

Note You need to log in before you can comment on or make changes to this bug.