[ASan] Make BlockingMutex really linker initialized.

[ygribov]

Summary

This patch adds constexpr to BlockingMutex which instructs compiler to never generate global constructors (and thus fixes -Wglobal-constructors warnings about BlockingMutex globals in sanitizer_common). I could do the same fixes to other LinkerInitialized classes (Allocator, etc.) and enable -Wglobal-constructors for ASan but I wasn't sure whether anyone needs this.

[ygribov]

Tested on Linux x64, both with and without LLVM_BUILD_EXTERNAL_COMPILER_RT=ON.

[ygribov]

Updated based on review.

[samsonov]

LGTM. Yes, I think enabling -Wglobal-constructors for ASan (and for all the rest sanitizers) will be great. Thank you!

lib/sanitizer_common/sanitizer_linux.cc

499

owner_ isn't used in any other BlockingMutex functions on Linux. Let's just delete this line.

[ygribov]

lib/sanitizer_common/sanitizer_linux.cc

499

Then the field would become unused causing build errors. And removing it just for Linux would cause more #ifdefs. BTW what's the reason for different treatment of owner_ on Linux and Mac? Looks like Mac check got strengthened at some point.

[samsonov]

Good point!

lib/sanitizer_common/sanitizer_linux.cc

499

owner_ field was previously used on Linux, but BlockingMutex implementation was rewritten in r172380. I'd suggest to consistently use owner_ to store GetThreadSelf() on all platforms (we already do this on Mac and Windows).

[ygribov]

Landed in 227560. I'll submit a pthread_self patch shortly.

[ygribov]

I'll submit a pthread_self patch shortly.

Ok, it turned out to be more interesting than I expected. We clone a lock-owning thread in StopTheWorld and then call CheckLocked in the clone (assuming that it "inherited" the lock). This obviously fails because owner_ field does not match. So looks like the whole idea of owner_ conflicts with our lock-passing semantics. I wonder if it's better to remove it altogether (at least on Linux and Mac, Windows uses it for some other purpose).

[samsonov]

Ok, it turned out to be more interesting than I expected. We clone a lock-owning thread in StopTheWorld and then call CheckLocked in the clone (assuming that it "inherited" the lock). This obviously fails because owner_ field does not match. So looks like the whole idea of owner_ conflicts with our lock-passing semantics. I wonder if it's better to remove it altogether (at least on Linux and Mac, Windows uses it for some other purpose).

+gilder

Fun. I'm fine with removing owner_ field on Linux and Mac, but we'll need to implement BlockingMutex::CheckLocked() in this case somehow.

[glider]

Note that we do not need to remove owner_ on Mac, since this weird ownership passing is Linux-specific.
Off the top of my head I don't have an idea how to do that, but I think it should be possible to fix StopTheWorld somehow.

[ygribov]

Yes, I think enabling -Wglobal-constructors for ASan (and for all the rest sanitizers) will be great.

FYI it was relatively easy to get this working on Linux (with ~500 lines of changes). Unfortunately Windows compiler lacks too many necessary features: explicit initialization of array members, constexpr, unrestricted unions (all still missing in VS2013 and we still use VS2012). Having #if WINDOWS all over the place isn't an option as well so I'm afraid we are out of luck.

lib/sanitizer_common/sanitizer_linux.cc

 #endif  // SANITIZER_GO
 
 enum MutexState {
   MtxUnlocked = 0,
   MtxLocked = 1,
   MtxSleeping = 2
 };
 
-BlockingMutex::BlockingMutex(LinkerInitialized) {
-  CHECK_EQ(owner_, 0);
-}
-
-BlockingMutex::BlockingMutex() {
-  internal_memset(this, 0, sizeof(*this));
-}
+BlockingMutex::BlockingMutex() {}
 
 void BlockingMutex::Lock() {
+  CHECK_EQ(owner_, 0);
   atomic_uint32_t *m = reinterpret_cast<atomic_uint32_t *>(&opaque_storage_);
   if (atomic_exchange(m, MtxLocked, memory_order_acquire) == MtxUnlocked)
     return;
   while (atomic_exchange(m, MtxSleeping, memory_order_acquire) != MtxUnlocked) {
 #if SANITIZER_FREEBSD
     _umtx_op(m, UMTX_OP_WAIT_UINT, MtxSleeping, 0, 0);
 #else
     internal_syscall(SYSCALL(futex), (uptr)m, FUTEX_WAIT, MtxSleeping, 0, 0, 0);

lib/sanitizer_common/sanitizer_mac.cc

void PrepareForSandboxing(__sanitizer_sandbox_arguments *args) {
   (void)args;
   // Nothing here for now.
 }
 
 uptr GetPageSize() {
   return sysconf(_SC_PAGESIZE);
 }
 
-BlockingMutex::BlockingMutex(LinkerInitialized) {
-  // We assume that OS_SPINLOCK_INIT is zero
-}
-
-BlockingMutex::BlockingMutex() {
-  internal_memset(this, 0, sizeof(*this));
-}
+BlockingMutex::BlockingMutex() {}
 
 void BlockingMutex::Lock() {
   CHECK(sizeof(OSSpinLock) <= sizeof(opaque_storage_));
   CHECK_EQ(OS_SPINLOCK_INIT, 0);
   CHECK_NE(owner_, (uptr)pthread_self());
   OSSpinLockLock((OSSpinLock*)&opaque_storage_);
   CHECK(!owner_);
   owner_ = (uptr)pthread_self();

lib/sanitizer_common/sanitizer_mutex.h

 
  private:
   SpinMutex(const SpinMutex&);
   void operator=(const SpinMutex&);
 };
 
 class BlockingMutex {
  public:
+#if SANITIZER_WINDOWS
   explicit BlockingMutex(LinkerInitialized);
+#else
+  explicit constexpr BlockingMutex(LinkerInitialized) {}
+#endif
   BlockingMutex();
   void Lock();
   void Unlock();
   void CheckLocked();
  private:
-  uptr opaque_storage_[10];
-  uptr owner_;  // for debugging
+  uptr opaque_storage_[10] = {};
+  uptr owner_ = 0;  // for debugging
 };
 
 // Reader-writer spin mutex.
 class RWMutex {
  public:
   RWMutex() {
     atomic_store(&state_, kUnlocked, memory_order_relaxed);
   }