This is an archive of the discontinued LLVM Phabricator instance.

[ASan] Make BlockingMutex really linker initialized.
ClosedPublic

Authored by ygribov on Jan 26 2015, 12:52 AM.

Details

Summary

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.

Diff Detail

Repository
rL LLVM

Event Timeline

ygribov updated this revision to Diff 18743.Jan 26 2015, 12:52 AM
ygribov retitled this revision from to [ASan] Make BlockingMutex really linker initialized..
ygribov updated this object.
ygribov edited the test plan for this revision. (Show Details)
ygribov added reviewers: kcc, samsonov.
ygribov set the repository for this revision to rL LLVM.
ygribov added subscribers: Unknown Object (MLST), dblaikie, kubamracek.

Tested on Linux x64, both with and without LLVM_BUILD_EXTERNAL_COMPILER_RT=ON.

ygribov updated this revision to Diff 18803.Jan 27 2015, 1:13 AM

Updated based on review.

samsonov accepted this revision.Jan 28 2015, 2:57 PM
samsonov edited edge metadata.

LGTM. Yes, I think enabling -Wglobal-constructors for ASan (and for all the rest sanitizers) will be great. Thank you!

lib/sanitizer_common/sanitizer_linux.cc
501

owner_ isn't used in any other BlockingMutex functions on Linux. Let's just delete this line.

This revision is now accepted and ready to land.Jan 28 2015, 2:57 PM
ygribov added inline comments.Jan 29 2015, 1:28 AM
lib/sanitizer_common/sanitizer_linux.cc
501

Then the field would become unused causing build errors. And removing it just for Linux would cause more #ifdefs. BTW what's the reason for different treatment of owner_ on Linux and Mac? Looks like Mac check got strengthened at some point.

Good point!

lib/sanitizer_common/sanitizer_linux.cc
501

owner_ field was previously used on Linux, but BlockingMutex implementation was rewritten in r172380. I'd suggest to consistently use owner_ to store GetThreadSelf() on all platforms (we already do this on Mac and Windows).

This revision was automatically updated to reflect the committed changes.

Landed in 227560. I'll submit a pthread_self patch shortly.

I'll submit a pthread_self patch shortly.

Ok, it turned out to be more interesting than I expected. We clone a lock-owning thread in StopTheWorld and then call CheckLocked in the clone (assuming that it "inherited" the lock). This obviously fails because owner_ field does not match. So looks like the whole idea of owner_ conflicts with our lock-passing semantics. I wonder if it's better to remove it altogether (at least on Linux and Mac, Windows uses it for some other purpose).

Ok, it turned out to be more interesting than I expected. We clone a lock-owning thread in StopTheWorld and then call CheckLocked in the clone (assuming that it "inherited" the lock). This obviously fails because owner_ field does not match. So looks like the whole idea of owner_ conflicts with our lock-passing semantics. I wonder if it's better to remove it altogether (at least on Linux and Mac, Windows uses it for some other purpose).

+gilder

Fun. I'm fine with removing owner_ field on Linux and Mac, but we'll need to implement BlockingMutex::CheckLocked() in this case somehow.

glider edited edge metadata.Feb 2 2015, 9:48 AM

Note that we do not need to remove owner_ on Mac, since this weird ownership passing is Linux-specific.
Off the top of my head I don't have an idea how to do that, but I think it should be possible to fix StopTheWorld somehow.

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.