<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Mon, Jan 26, 2015 at 10:20 AM, Yuri Gribov <span dir="ltr"><<a href="mailto:tetra2005@gmail.com" target="_blank">tetra2005@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5">On Mon, Jan 26, 2015 at 7:21 PM, David Blaikie <<a href="mailto:dblaikie@gmail.com">dblaikie@gmail.com</a>> wrote:<br>
><br>
><br>
> On Mon, Jan 26, 2015 at 12:52 AM, Yury Gribov <<a href="mailto:tetra2005@gmail.com">tetra2005@gmail.com</a>> wrote:<br>
>><br>
>> Hi kcc, samsonov,<br>
>><br>
>> This patch adds constexpr to BlockingMutex which instructs compiler to<br>
>> never generate global constructors (and thus fixes -Wglobal-constructors<br>
>> warnings about BlockingMutex globals in sanitizer_common). I could do the<br>
>> same fixes to other LinkerInitialized classes (Allocator, etc.) and enable<br>
>> -Wglobal-constructors for ASan but I wasn't sure whether anyone needs this.<br>
>><br>
>> REPOSITORY<br>
>>   rL LLVM<br>
>><br>
>> <a href="http://reviews.llvm.org/D7171" target="_blank">http://reviews.llvm.org/D7171</a><br>
>><br>
>> Files:<br>
>>   lib/sanitizer_common/sanitizer_linux.cc<br>
>>   lib/sanitizer_common/sanitizer_mac.cc<br>
>>   lib/sanitizer_common/sanitizer_mutex.h<br>
>><br>
>> Index: lib/sanitizer_common/sanitizer_linux.cc<br>
>> ===================================================================<br>
>> --- lib/sanitizer_common/sanitizer_linux.cc<br>
>> +++ lib/sanitizer_common/sanitizer_linux.cc<br>
>> @@ -493,15 +493,10 @@<br>
>>    MtxSleeping = 2<br>
>>  };<br>
>><br>
>> -BlockingMutex::BlockingMutex(LinkerInitialized) {<br>
>> -  CHECK_EQ(owner_, 0);<br>
>> -}<br>
>> -<br>
>> -BlockingMutex::BlockingMutex() {<br>
>> -  internal_memset(this, 0, sizeof(*this));<br>
>> -}<br>
>> +BlockingMutex::BlockingMutex() {}<br>
>><br>
>>  void BlockingMutex::Lock() {<br>
>> +  CHECK_EQ(owner_, 0);<br>
>>    atomic_uint32_t *m = reinterpret_cast<atomic_uint32_t<br>
>> *>(&opaque_storage_);<br>
>>    if (atomic_exchange(m, MtxLocked, memory_order_acquire) == MtxUnlocked)<br>
>>      return;<br>
>> Index: lib/sanitizer_common/sanitizer_mac.cc<br>
>> ===================================================================<br>
>> --- lib/sanitizer_common/sanitizer_mac.cc<br>
>> +++ lib/sanitizer_common/sanitizer_mac.cc<br>
>> @@ -217,13 +217,7 @@<br>
>>    return sysconf(_SC_PAGESIZE);<br>
>>  }<br>
>><br>
>> -BlockingMutex::BlockingMutex(LinkerInitialized) {<br>
>> -  // We assume that OS_SPINLOCK_INIT is zero<br>
>> -}<br>
>> -<br>
>> -BlockingMutex::BlockingMutex() {<br>
>> -  internal_memset(this, 0, sizeof(*this));<br>
>> -}<br>
>> +BlockingMutex::BlockingMutex() {}<br>
>><br>
>>  void BlockingMutex::Lock() {<br>
>>    CHECK(sizeof(OSSpinLock) <= sizeof(opaque_storage_));<br>
>> Index: lib/sanitizer_common/sanitizer_mutex.h<br>
>> ===================================================================<br>
>> --- lib/sanitizer_common/sanitizer_mutex.h<br>
>> +++ lib/sanitizer_common/sanitizer_mutex.h<br>
>> @@ -73,14 +73,18 @@<br>
>><br>
>>  class BlockingMutex {<br>
>>   public:<br>
>> +#if SANITIZER_WINDOWS<br>
><br>
><br>
> Should this be instead based on detecting constexpr support?<br>
<br>
</div></div>Actually this one isn't related to constexpr. Windows implementation<br>
of BlockingMutex does not really support LinkerInitialized (due to<br>
some MS linker weirdness - see comments in sanitizer_win.cc). It has a<br>
completely different constructor so I decided to leave as is.<br>
<span class=""><br>
>>    explicit BlockingMutex(LinkerInitialized);<br>
>> +#else<br>
>> +  explicit constexpr BlockingMutex(LinkerInitialized) {}<br>
>> +#endif<br>
>>    BlockingMutex();<br>
>>    void Lock();<br>
>>    void Unlock();<br>
>>    void CheckLocked();<br>
>>   private:<br>
>> -  uptr opaque_storage_[10];<br>
>> -  uptr owner_;  // for debugging<br>
>> +  uptr opaque_storage_[10] = {};<br>
><br>
><br>
> I imagine these might not compile on the oldest version of MSVC we support<br>
> (not sure about other compilers in the support matrix)... might be worth<br>
> checking their respective documentation (linked from the LLVM coding style<br>
> guide)<br>
<br>
</span>Indeed: <a href="http://stackoverflow.com/questions/18518761/visual-studio-2012-2013-in-class-member-initialization" target="_blank">http://stackoverflow.com/questions/18518761/visual-studio-2012-2013-in-class-member-initialization</a><br>
<br>
I could throw in more preprocessing or perhaps you see a better solution?<br></blockquote><div><br>Nope, I don't know of any particularly better solution. I imagine just conditionalizing this on the same property as the dtor is all there is to do here.<br> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<span class="HOEnZb"><font color="#888888"><br>
-Y<br>
</font></span></blockquote></div><br></div></div>