[PATCH] D19412: [libcxx] Refactor pthread usage - II

[Asiri Rathnayake via cfe-commits]

rmaprath updated this revision to Diff 55557.
rmaprath added a comment.

In http://reviews.llvm.org/D19412#416183, @EricWF wrote:

> OK. I *think* this is my last round of review comments except for one specific issue. I'm still looking into the changes to the static mutex's and condition_variables in `memory.cpp` and `algorithm.cpp`. In these cases I don't want to go from compile-time initialization to run-time initialization.  This could introduce the static initialization order fiasco.


So, as pointed out earlier by @bcraig, there won't be a runtime overhead for those compilers supporting `constexr`. For those that don't, this is still a trivial enough constructor call that is very likely to get optimized away. To elaborate, we can simplify this scenario to something like:

**thread.h**

  struct thread_t {
    int state1;
    int *state2;
    bool state3;
  };
  
  #define THREAD_INITIALIZER  {0, 0, false}

**test.cpp**

  #include "thread.h"
  
  class Foo {
  private:
    thread_t __t_;
  public:
    Foo() {__t_ = (thread_t) THREAD_INITIALIZER;}
  };
  
  Foo f();
  
  int main() {
    return 0;
  }

Compiling this with either `clang` or `gcc` in `-std=c++03` does **not** introduce an entry in the `.init_array` section. This can be observed with:

  > clang++ -std=c++03 -c test.cpp
  > objdump -j .init_array -dxs test.o

On the other hand, if you slightly change the constructor to take a parameter, or print something into `std::cout`, there will be an entry in the `.init_array`.

Moreover, even if some compiler naively emits a runtime initialization code for these constructors (`mutex` and `condition_variable`), I don't see how that can lead to problems related to static initialization order - all these constructors do is simply copy some memory, at worse, the compiler may emit a call to `memcpy` - but `memcpy` does not (AFAIK) depend on any other library state. In other words, these constructors don't require any other parts of the library having being initialized first.

I hope that clears it?

I have addressed the rest of you comments. Two minor nits:

- Can we keep `__libcpp_mutex_t` (used in `__threading_support`) and `__mutex_type` (used in `__mutex_base`) separate? I feel it's cleaner to make a distinction between the types of the threading API and the rest of the library. I don't have a strong opinion though, let me know if you'd like it changed still.
- I'd rather not move `__libcpp_recursive_mutex_init` back into `recursive_mutex.cpp`, as this would introduce pthread dependencies (`pthread_mutexattr_xxx`) back into the library sources. It's possible to workaround that by adding further `__libcpp_mutexattr_xxx` functions to the threading API, but those seem to be bit too pthread-specific. Again, no strong preference.

Thanks!

/ Asiri


http://reviews.llvm.org/D19412

Files:
  include/__config
  include/__mutex_base
  include/__threading_support
  include/mutex
  include/thread
  src/algorithm.cpp
  src/condition_variable.cpp
  src/memory.cpp
  src/mutex.cpp
  src/thread.cpp

-------------- next part --------------
A non-text attachment was scrubbed...
Name: D19412.55557.patch
Type: text/x-patch
Size: 24183 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20160429/4bd6a9e8/attachment-0001.bin>