This bugzilla service is closed. All entries have been migrated to https://gitlab.com/libeigen/eigen

Bug 1152

Summary: Data races in BLAS static initialization
Product: Eigen Reporter: Benoit Jacob <jacob.benoit.1>
Component: GeneralAssignee: Nobody <eigen.nobody>
Status: RESOLVED FIXED    
Severity: Unknown CC: chtz, gael.guennebaud, jacob.benoit.1
Priority: Normal    
Version: 3.3 (current stable)   
Hardware: All   
OS: All   
Whiteboard:
Bug Depends on:    
Bug Blocks: 558    
Attachments:
Description Flags
Fix blas static initialization race
none
Fix blas static initialization race none

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
Comment 8 Nobody 2019-12-04 15:22:09 UTC
-- GitLab Migration Automatic Message --

This bug has been migrated to gitlab.com's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.com/libeigen/eigen/issues/1152.