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

[David Majnemer]

On Fri, Feb 20, 2015 at 8:23 AM, Dmitry Vyukov <dvyukov at google.com> wrote:

> On Fri, Feb 20, 2015 at 7:01 PM, Timur Iskhodzhanov <timurrrr at google.com>
> wrote:
> > [The discussion is a little bit lengthy to understand from a first read,
> so
> > don't blame me too much if I haven't understood the question]
> >
> > Unfortunately, I don't know how to reliably do linker-initialized
> > CriticalSections on Windows.  [+Dmitry, David or Reid might help]
>
> Windows does not have statically initialized mutexes.
> There is only Once (INIT_ONCE_STATIC_INIT), if you are OK with working
> only on Vista+. Having static Once you can init a mutex lazily.
>
>
Actually, Windows does have such a mutex.  You can use a SRWLOCK and
statically initialize it with SRWLOCK_INIT.

If you can't upgrade to SRWLOCK (introduced in Vista), you can resort to
this trick with CRITICAL_SECTION:
https://chromium.googlesource.com/webm/libvpx/+/1a23086bc667054a1da6942750975f53a504c1de/vp8/common/rtcd.c#32


>
> > Currently there is a hack in BlockingMutex::Lock @ sanitizer_win.cc that
> > makes global mutexes work on Windows, but I think even this
> implementation
> > fails to start sometimes.
> >
> > Also please note we DON'T yet have a public buildbot that runs ASan
> tests on
> > Windows.  I've been working on one this week and it's green, but it only
> > compiles:
> > http://lab.llvm.org:8011/builders/sanitizer-windows
> > Hopefully it will start running tests next week.
> >
> >
> > On Fri Feb 20 2015 at 8:28:36 AM Yuri Gribov <tetra2005 at gmail.com>
> wrote:
> >>
> >> On Fri, Feb 20, 2015 at 5:19 AM, Alexey Samsonov <vonosmas at gmail.com>
> >> wrote:
> >> >
> >> > On Wed, Feb 18, 2015 at 5:29 AM, Yury Gribov <y.gribov at samsung.com>
> >> > wrote:
> >> >>
> >> >> On 02/06/2015 09:10 AM, Yury Gribov wrote:
> >> >>>
> >> >>> On 02/04/2015 07:23 PM, David Blaikie wrote:
> >> >>>>
> >> >>>> On Wed, Feb 4, 2015 at 3:56 AM, Yury Gribov <tetra2005 at gmail.com>
> >> >>>> wrote:
> >> >>>>
> >> >>>>>> 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.
> >> >>>>>
> >> >>>>
> >> >>>> Perhaps there's a relatively small number of structures (probably
> >> >>>> templates) we could create to keep all of this logic (and Windows
> >> >>>> workarounds/suppressions) in one place so it could scale to all the
> >> >>>> use
> >> >>>> cases we have?
> >> >>>
> >> >>>
> >> >>> Thanks for the idea, I'll check this next week.
> >> >>
> >> >>
> >> >> Folks,
> >> >>
> >> >> Here is a draft version (not full-blown patch but rather a PoC). The
> >> >> main
> >> >> part is in sanitizer_internal_defs.h (classes LinkerInitializedArray
> >> >> and
> >> >> LinkerInitializedStructArray). I also slightly changed atomic types
> in
> >> >> sanitizer_atomic.h (volatile members can't be linker-initialized).
> Does
> >> >> this
> >> >> look sane in general?
> >> >
> >> > +Timur for Windows parts.
> >> > Overall, yes, although missing constexpr is such a pain (sigh). Why is
> >> > it
> >> > necessary to initialize all fields to 0 in LinkerInitialized
> >> > constructor?
> >>
> >> You mean some classes may want non-zero values there? That's true, I
> >> planned to address this (the patch is a draft anyway).
> >>
> >> >> I've did full rebuild and regtests on Linux (compiler-rt built by
> trunk
> >> >> Clang, GCC 4.8 and Clang 3.1). I haven't done full testing on Windows
> >> >> and
> >> >> Mac but at least relevant parts of sanitizer_internal_defs.h compiled
> >> >> fine.
> >> >>
> >> >> If all this makes sense, how should I proceed with a refactoring of
> >> >> this
> >> >> scale? Is it mandatory to verify _all_ platforms or I could to some
> >> >> extent
> >> >> rely on respected maintainers to fix errors? The former would be
> quite
> >> >> a
> >> >> burden.
> >> >
> >> > Well, at least for LLVM, we have buildbots (for Linux/Mac/Windows), so
> >> > it's
> >> > fine to test your change on available platforms, and then watch the
> >> > buildbots closely.
> >>
> >> Ok.
> >>
> >> > It would be awesome to rollout this out gradually, first by
> introducing
> >> > LinkerInitializedArray, then converting global objects to use
> constexpr
> >> > constructor one by one, and enabling -Wglobal-constructor as a final
> >> > change.
> >>
> >> Right.
> >>
> >> -Y
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150220/daa8830d/attachment.html>