[PATCH] [ASan] Make BlockingMutex really linker initialized.

[Yuri Gribov]

On Mon, Jan 26, 2015 at 7:21 PM, David Blaikie <dblaikie at gmail.com> wrote:
>
>
> On Mon, Jan 26, 2015 at 12:52 AM, Yury Gribov <tetra2005 at gmail.com> wrote:
>>
>> Hi kcc, samsonov,
>>
>> 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.
>>
>> REPOSITORY
>>   rL LLVM
>>
>> http://reviews.llvm.org/D7171
>>
>> Files:
>>   lib/sanitizer_common/sanitizer_linux.cc
>>   lib/sanitizer_common/sanitizer_mac.cc
>>   lib/sanitizer_common/sanitizer_mutex.h
>>
>> Index: lib/sanitizer_common/sanitizer_linux.cc
>> ===================================================================
>> --- lib/sanitizer_common/sanitizer_linux.cc
>> +++ lib/sanitizer_common/sanitizer_linux.cc
>> @@ -493,15 +493,10 @@
>>    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;
>> Index: lib/sanitizer_common/sanitizer_mac.cc
>> ===================================================================
>> --- lib/sanitizer_common/sanitizer_mac.cc
>> +++ lib/sanitizer_common/sanitizer_mac.cc
>> @@ -217,13 +217,7 @@
>>    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_));
>> Index: lib/sanitizer_common/sanitizer_mutex.h
>> ===================================================================
>> --- lib/sanitizer_common/sanitizer_mutex.h
>> +++ lib/sanitizer_common/sanitizer_mutex.h
>> @@ -73,14 +73,18 @@
>>
>>  class BlockingMutex {
>>   public:
>> +#if SANITIZER_WINDOWS
>
>
> Should this be instead based on detecting constexpr support?

Actually this one isn't related to constexpr. Windows implementation
of BlockingMutex does not really support LinkerInitialized (due to
some MS linker weirdness - see comments in sanitizer_win.cc). It has a
completely different constructor so I decided to leave as is.

>>    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] = {};
>
>
> I imagine these might not compile on the oldest version of MSVC we support
> (not sure about other compilers in the support matrix)... might be worth
> checking their respective documentation (linked from the LLVM coding style
> guide)

Indeed: http://stackoverflow.com/questions/18518761/visual-studio-2012-2013-in-class-member-initialization

I could throw in more preprocessing or perhaps you see a better solution?

-Y