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

[David Blaikie]

On Mon, Jan 26, 2015 at 10:20 AM, Yuri Gribov <tetra2005 at gmail.com> wrote:

> 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?
>

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.


>
> -Y
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150126/743e2548/attachment.html>