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

[Dmitry Vyukov]

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.


> 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