This is an archive of the discontinued LLVM Phabricator instance.

[GWP-ASan] Mutex implementation [2].
ClosedPublic

Authored by hctim on May 14 2019, 4:28 PM.

Details

Summary

See D60593 for further information.
This patch pulls out the mutex implementation and the required definitions file.

We implement our own mutex for GWP-ASan currently, because:

  1. We must be compatible with the sum of the most restrictive elements of the supporting allocator's build system. Current targets for GWP-ASan include Scudo (on Linux and Fuchsia), and bionic (on Android).
  2. Scudo specifies -nostdlib++ -nonodefaultlibs, meaning we can't use std::mutex or mtx_t.
  3. We can't use sanitizer_common's mutex, as the supporting allocators cannot afford the extra maintenance (Android, Fuchsia) and code size (Fuchsia) overheads that this would incur.

In future, we would like to implement a shared base mutex for GWP-ASan, Scudo and sanitizer_common. This will likely happen when both GWP-ASan and Scudo standalone are not in the development phase, at which point they will have stable requirements.

Diff Detail

Repository
rL LLVM

Event Timeline

hctim created this revision.May 14 2019, 4:28 PM
Herald added subscribers: llvm-commits, Restricted Project, jfb and 3 others. · View Herald TranscriptMay 14 2019, 4:28 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptMay 14 2019, 4:28 PM
jfb added a comment.May 14 2019, 4:43 PM

Tests?

Seems a shame to duplicate mutex again... Why can't use use the STL's version again? It doesn't allocate.

compiler-rt/lib/gwp_asan/definitions.h
15 ↗(On Diff #199533)

You're not using this.

compiler-rt/lib/gwp_asan/mutex.h
32 ↗(On Diff #199533)

uint8_t seems easy to inadvertently truncate. If you want to restrict size, make it a template and refuse to compile above a certain size. It's always 10 anyways.

37 ↗(On Diff #199533)

You should static_assert that this is always lock free.

42 ↗(On Diff #199533)

You should take a reference here, not a pointer.

47 ↗(On Diff #199533)

ScopedLock(const ScopedLock&) = delete;

65 ↗(On Diff #199533)

Seems like this leading compiler barrier should be above the #if.
You also might as well use something like __atomic_signal_fence instead.

72 ↗(On Diff #199533)

You should hard-error on other architectures.

81 ↗(On Diff #199533)

This won't compile if __unix__ isn't defined.

hctim updated this revision to Diff 199541.May 14 2019, 5:59 PM
hctim marked 10 inline comments as done.
Herald added a project: Restricted Project. · View Herald Transcript
hctim added a comment.May 14 2019, 5:59 PM
In D61923#1502245, @jfb wrote:

Seems a shame to duplicate mutex again... Why can't use use the STL's version again? It doesn't allocate.

We can't use std::mutex as we must be C-compatible (and can't make the strong guarantee that std::mutex is header-only), see https://reviews.llvm.org/D60593#inline-537276.
We also are trying to stay independent of pthread for platform-independentness.
We also can't use a sanitizer_common variant as we don't want to pull in the entirety sanitizer_common as a dependency.

Basically, the roadmap is to create a shared mutex library for sanitizer_common, gwp_asan and scudo to all use.

In D61923#1502245, @jfb wrote:

Tests?

Added some unit tests.

compiler-rt/lib/gwp_asan/mutex.h
32 ↗(On Diff #199533)

Seeming as this is an entirely-internal number, I've made it a global instead.

81 ↗(On Diff #199533)

Should be guarded by if (COMPILER_RT_HAS_GWP_ASAN) at lib/gwp_asan/CMakeLists.txt:20, but have added explicit error condition here.

I think the idea is that implementing our own spinlock is not much harder than having 3 platform-specific implementations (posix, window, fuchsia).
Also, scudo_standalone is doing the same ( @cryptoad, why? ).

As Mitch mentioned, we should move the implementation into a common directory. Why not do this now?

jfb added a comment.May 14 2019, 9:02 PM
In D61923#1502245, @jfb wrote:

Seems a shame to duplicate mutex again... Why can't use use the STL's version again? It doesn't allocate.

We can't use std::mutex as we must be C-compatible

Can you clarify what you mean by this? I don't understand.

(and can't make the strong guarantee that std::mutex is header-only), see https://reviews.llvm.org/D60593#inline-537276.

Have you asked on libcxx-dev? Is there interest in the libc++ community for a stand-alone base?

We also are trying to stay independent of pthread for platform-independentness.

The code I see is clearly not platform independent. Please clarify your reasoning. I can understand if you don't want pthread for a valid reason, but "platform-independence" doesn't make sense here.

We also can't use a sanitizer_common variant as we don't want to pull in the entirety sanitizer_common as a dependency.

What's the restriction that makes it unacceptable?

Basically, the roadmap is to create a shared mutex library for sanitizer_common, gwp_asan and scudo to all use.

I'm asking all these question because they should be in the commit message, and would hopefully have surfaced from the RFC about GWP / Scudo. If those weren't in the RFC that's fine, they should still be in the commit message and you'll want to update the RFC with "while reviewing code we realized *stuff*".

I think the idea is that implementing our own spinlock is not much harder than having 3 platform-specific implementations (posix, window, fuchsia).
Also, scudo_standalone is doing the same ( @cryptoad, why? ).

Your concerns are backwards. Implementation is a one-time thing, maintenance is an ongoing concern and it way overshadows implementation concerns. In particular, a variety of multicore code that does its own locking is much harder to tune at the system-wide level compared to a single thing that's tuned for each application. I'm not saying a separate mutex doesn't make sense, what I'm saying is that experience shows your above reasoning to be generally invalid. I'm totally willing to believe that there's a very good reason to have your own mutex here, but you gotta explain it.

compiler-rt/lib/gwp_asan/definitions.h
14 ↗(On Diff #199541)

The undef above doesn't seem like a good idea. You should prefix the macro with GWP_ if you're concerned about name clashes.

compiler-rt/lib/gwp_asan/mutex.h
42 ↗(On Diff #199541)

You want __atomic_always_lock_free, it'll always be a constexpr (whereas the other can be a libcall).

I think the idea is that implementing our own spinlock is not much harder than having 3 platform-specific implementations (posix, window, fuchsia).
Also, scudo_standalone is doing the same ( @cryptoad, why? ).

As Mitch mentioned, we should move the implementation into a common directory. Why not do this now?

Is the question why do Scudo has its own as opposed to rely on platform specific ones?

hctim updated this revision to Diff 199632.May 15 2019, 9:59 AM
hctim marked 2 inline comments as done.

Is the question why do Scudo has its own as opposed to rely on platform specific ones?

Yes, I have assumptions about why, but would love to hear right from the horse's mouth :)

In D61923#1502404, @jfb wrote:

We can't use std::mutex as we must be C-compatible

Can you clarify what you mean by this? I don't understand.
Have you asked on libcxx-dev? Is there interest in the libc++ community for a stand-alone base?

Sorry, poor choice of wording. Our archive must be able to be statically linked into the supporting allocators. At this stage, the plans are to implement GWP-ASan into Scudo and Android's bionic libc. This means we have to be compatible with the the most restrictive aspects of each allocator's build environment, simultaneously.

In practice, we can't use std::mutex because Scudo specifies -nostdlib++ -nodefaultlibs, and if any part of the implementation of std::mutex (and likewise mtx_t) falls outside of the header file, we will fail to link (as is the case with libc++).

We also are trying to stay independent of pthread for platform-independentness.

The code I see is clearly not platform independent. Please clarify your reasoning. I can understand if you don't want pthread for a valid reason, but "platform-independence" doesn't make sense here.

My view was that small stubs to implement the architecture-dependent and OS-dependent functionality is much easier to manage. If we use pthread on unix, and CreateMutex on Windows, we still have to have OS-dependent ifdefs, but we then pass on the requirement to the supporting allocator to ensure our dependencies are there (which could be a huge pain point both for Scudo+fuchsia and Android).

We also can't use a sanitizer_common variant as we don't want to pull in the entirety sanitizer_common as a dependency.

What's the restriction that makes it unacceptable?

Scudo (standalone) can't pull in the entirety of sanitizer_common (code size + maintainability for fuchsia). It's also a much easier track to get Android supported if we don't have to pull in and build the entirety of sanitizer_common.

Basically, the roadmap is to create a shared mutex library for sanitizer_common, gwp_asan and scudo to all use.

I'm asking all these question because they should be in the commit message, and would hopefully have surfaced from the RFC about GWP / Scudo. If those weren't in the RFC that's fine, they should still be in the commit message and you'll want to update the RFC with "while reviewing code we realized *stuff*".

Ack.

I think the idea is that implementing our own spinlock is not much harder than having 3 platform-specific implementations (posix, window, fuchsia).

Your concerns are backwards. Implementation is a one-time thing, maintenance is an ongoing concern and it way overshadows implementation concerns. In particular, a variety of multicore code that does its own locking is much harder to tune at the system-wide level compared to a single thing that's tuned for each application. I'm not saying a separate mutex doesn't make sense, what I'm saying is that experience shows your above reasoning to be generally invalid. I'm totally willing to believe that there's a very good reason to have your own mutex here, but you gotta explain it.

Hopefully above helps to clarify :)

hctim edited the summary of this revision. (Show Details)May 15 2019, 10:06 AM
jfb added a comment.May 15 2019, 10:08 AM
In D61923#1502404, @jfb wrote:

We can't use std::mutex as we must be C-compatible

Can you clarify what you mean by this? I don't understand.
Have you asked on libcxx-dev? Is there interest in the libc++ community for a stand-alone base?

Sorry, poor choice of wording. Our archive must be able to be statically linked into the supporting allocators. At this stage, the plans are to implement GWP-ASan into Scudo and Android's bionic libc. This means we have to be compatible with the the most restrictive aspects of each allocator's build environment, simultaneously.

In practice, we can't use std::mutex because Scudo specifies -nostdlib++ -nodefaultlibs, and if any part of the implementation of std::mutex (and likewise mtx_t) falls outside of the header file, we will fail to link (as is the case with libc++).

Have you asked on libcxx-dev whether a stand-alone base is something of interest to them?

We also are trying to stay independent of pthread for platform-independentness.

The code I see is clearly not platform independent. Please clarify your reasoning. I can understand if you don't want pthread for a valid reason, but "platform-independence" doesn't make sense here.

My view was that small stubs to implement the architecture-dependent and OS-dependent functionality is much easier to manage. If we use pthread on unix, and CreateMutex on Windows, we still have to have OS-dependent ifdefs, but we then pass on the requirement to the supporting allocator to ensure our dependencies are there (which could be a huge pain point both for Scudo+fuchsia and Android).

We also can't use a sanitizer_common variant as we don't want to pull in the entirety sanitizer_common as a dependency.

What's the restriction that makes it unacceptable?

Scudo (standalone) can't pull in the entirety of sanitizer_common (code size + maintainability for fuchsia). It's also a much easier track to get Android supported if we don't have to pull in and build the entirety of sanitizer_common.

Basically, the roadmap is to create a shared mutex library for sanitizer_common, gwp_asan and scudo to all use.

I'm asking all these question because they should be in the commit message, and would hopefully have surfaced from the RFC about GWP / Scudo. If those weren't in the RFC that's fine, they should still be in the commit message and you'll want to update the RFC with "while reviewing code we realized *stuff*".

Ack.

I think the idea is that implementing our own spinlock is not much harder than having 3 platform-specific implementations (posix, window, fuchsia).

Your concerns are backwards. Implementation is a one-time thing, maintenance is an ongoing concern and it way overshadows implementation concerns. In particular, a variety of multicore code that does its own locking is much harder to tune at the system-wide level compared to a single thing that's tuned for each application. I'm not saying a separate mutex doesn't make sense, what I'm saying is that experience shows your above reasoning to be generally invalid. I'm totally willing to believe that there's a very good reason to have your own mutex here, but you gotta explain it.

Hopefully above helps to clarify :)

Kinda... but your answer really sounds like this is a Google-only project, with intended uses only for some Google applications. That's not necessarily a bad thing, but it's really weird: my impression was that GWP and Scudo were intended to be generally usable elsewhere. Is that not the case?

Not that I expect *you* to do any porting work, but the direction you're setting just sounds like "well, we're our only users and we'll take the path that'll work for us". None of the reasoning seems to be truly technical, it's more "these projects we want to integrate into build this way, so we should fit in that way". i.e. it's more about implementation convenience than anything else. Is that the case? Implementation convenience is sometimes a good reason, but as I've outlined above I think you need to provide more thoughtful reasoning.

In D61923#1503272, @jfb wrote:

Have you asked on libcxx-dev whether a stand-alone base is something of interest to them?

No, but I will follow up on that.

Kinda... but your answer really sounds like this is a Google-only project, with intended uses only for some Google applications. That's not necessarily a bad thing, but it's really weird: my impression was that GWP and Scudo were intended to be generally usable elsewhere. Is that not the case?

Not that I expect *you* to do any porting work, but the direction you're setting just sounds like "well, we're our only users and we'll take the path that'll work for us". None of the reasoning seems to be truly technical, it's more "these projects we want to integrate into build this way, so we should fit in that way". i.e. it's more about implementation convenience than anything else. Is that the case? Implementation convenience is sometimes a good reason, but as I've outlined above I think you need to provide more thoughtful reasoning.

I definitely don't want that to be the case, or to have it come across that way. We want a reference implementation that's available for any allocator to drop in and use, with minor tweaking. As part of that, our goal is to make the requirements to support GWP-ASan as low as possible, preferably as simple as "merge this into your codebase somewhere, and you're good to go". We inherit the restrictions of each allocator's build system, which means more restrictive environment for us, but we can be drop-in supported pretty much everywhere. Our plans for this reference implementation is to use it in Android and Fuchsia, but we would love to collaborate with other interested projects to hear about what their requirements would be as well.

We discussed at length whether to have GWP-ASan be a standalone GitHub project (similar to Scudo standalone), or as part of compiler-rt. We decided to put it into compiler-rt because we can then exercise LLVM's extensive build/test/review infrastructure and open-source licensing.

hctim updated this revision to Diff 200094.May 17 2019, 2:08 PM

Merged in master after fixing up some buildbots with the previous patches.

Also have pinged libcxx-dev :)

hctim updated this revision to Diff 200600.May 21 2019, 3:25 PM

Changed GWP-ASan to use platform specific mutexes. For now, we only
target Android and Linux, and subsequently only need the pthread_mutex
variant for POSIX.

Kept around the mutex unittests as it's an easy assertion that the
abstraction over the platform specifics is implemented correctly.

eugenis added inline comments.May 21 2019, 3:45 PM
compiler-rt/lib/gwp_asan/mutex.h
27 ↗(On Diff #200600)

What's the point of this include? You are leaking platform details into this common header anyway.

We can make the interface C-only; or use pImpl to hide the implementation; or just move the entire declaration of Mutex to the platform header.

hctim updated this revision to Diff 200604.May 21 2019, 4:03 PM
hctim marked an inline comment as done.
  • Updated to use pointer-to-impl to abstract implementation behaviour away from header files.
eugenis added inline comments.May 21 2019, 4:58 PM
compiler-rt/lib/gwp_asan/platform_specific/mutex_posix.cpp
43 ↗(On Diff #200604)

This is a dependency on libc++ / libstdc++.

I'm not sure about using malloc() inside mutex constructor, either.

We will probably want the mutex to be linker-initialized, too. AFAIK, gwp-asan would not have any clear initialization entry point, and would need to do this lazily in malloc, which can there be several of, concurrently.

hctim updated this revision to Diff 200760.May 22 2019, 8:30 AM
hctim marked 2 inline comments as done.
  • Ifdef-d headers into mutex.h
compiler-rt/lib/gwp_asan/platform_specific/mutex_posix.cpp
43 ↗(On Diff #200604)

As per offline discussion, have ifdef-d the platform-specific headers into this file. Allows us to have linker initialisation (at least for POSIX).

morehouse added inline comments.May 22 2019, 11:20 AM
compiler-rt/lib/gwp_asan/mutex.h
22 ↗(On Diff #200760)

We should probably delete copy constructor and operator=

40 ↗(On Diff #200760)

We should also delete operator=.

compiler-rt/lib/gwp_asan/platform_specific/mutex_posix.cpp
19 ↗(On Diff #200760)

Unnecessary parens (also below)

compiler-rt/lib/gwp_asan/tests/driver.cpp
14 ↗(On Diff #200760)

Can we just link with gtest_main instead of having this?

compiler-rt/lib/gwp_asan/tests/mutex_test.cpp
20 ↗(On Diff #200760)

Nit: ConstructionTest doesn't seem to describe what this test does.

Something like LockUnlockTest seems more descriptive.

(Same for other "Construction" tests)

37 ↗(On Diff #200760)

We should trylock inside the scope, and maybe also do an unlock+lock.

43 ↗(On Diff #200760)

This doesn't need to be a global.

44 ↗(On Diff #200760)

static

45 ↗(On Diff #200760)

Can we use the simpler syntax while (!StartingGun) instead?

46 ↗(On Diff #200760)

Format seems weird. Does clang-format let us do while (...) {} on one line?

49 ↗(On Diff #200760)

Nit: curly braces unnecessary

59 ↗(On Diff #200760)

Can we use simpler StartingGun = true?

68 ↗(On Diff #200760)

static

82 ↗(On Diff #200760)

I don't think we should care about the number of cores at all. Time-sliced threads are just fine for this test.

101 ↗(On Diff #200760)

How are any of these "thrash" tests?

I'd prefer something more descriptive, like SynchronizedCounterTest.

102 ↗(On Diff #200760)

This test case seems to test a superset of ThreadedConstructionTest, so do we really need that one?

104 ↗(On Diff #200760)

4 threads isn't very much. Can we test with more threads, say ~1000?

compiler-rt/test/gwp_asan/CMakeLists.txt
1 ↗(On Diff #200760)

Can the lit test stuff go in another patch when we actually have tests?

hctim updated this revision to Diff 202078.May 29 2019, 4:29 PM
hctim marked 21 inline comments as done.
  • Apologies about the delay on this. Updated with Matt's comments.
compiler-rt/lib/gwp_asan/tests/driver.cpp
14 ↗(On Diff #200760)

Unfortunately not easily. generate_compiler_rt_tests builds each test cpp file individually, and provides no way to add gtest_main only during link time.

compiler-rt/lib/gwp_asan/tests/mutex_test.cpp
46 ↗(On Diff #200760)

CF changes it to

while (!StartingGun) {
}
compiler-rt/test/gwp_asan/CMakeLists.txt
1 ↗(On Diff #200760)

Not AFAIK. The unit tests are actually invoked through the lit configurations in this directory.

morehouse added inline comments.May 29 2019, 6:22 PM
compiler-rt/lib/gwp_asan/tests/driver.cpp
14 ↗(On Diff #200760)

Then how does this driver.cpp get included in each unit test?

Maybe we should put the main in each test as is usually done.

compiler-rt/lib/gwp_asan/tests/mutex_test.cpp
37 ↗(On Diff #200760)

No, I didn't mean that way.

Something like this:

{
  ScopedLock L(Mu);
  EXPECT_FALSE(Mu.tryLock());  // Test that the ctor did in fact lock
  Mu.unlock()
  EXPECT_TRUE(Mu.tryLock());  // Test that manual unlock overrides ScopedLock
}
EXPECT_TRUE(Mu.tryLock());  // Test that dtor did in fact unlock
46 ↗(On Diff #200760)

Not a big deal, but might be more readable if we do:

while (!StartingGun) {
  // Wait for starting gun.
}
59 ↗(On Diff #200760)

Can we use simpler StartingGun = true?

102 ↗(On Diff #200760)

This test case seems to test a superset of ThreadedConstructionTest, so do we really need that one?

hctim updated this revision to Diff 202237.May 30 2019, 10:34 AM
hctim marked 7 inline comments as done.
  • Updated from Matt's comments.
compiler-rt/lib/gwp_asan/tests/driver.cpp
14 ↗(On Diff #200760)

Basically what happens:

for x in *.cpp;
  do clang++ $x -o $x.o
done
clang++ *.o -o GwpAsanUnitTests

The unittests are all packaged together into a single binary. Also, AFAIK, compiler-rt all has a single main() driver for unittests (at least ASan+MSan+Scudo do this).

compiler-rt/lib/gwp_asan/tests/mutex_test.cpp
102 ↗(On Diff #200760)

Deleted ThreadedLockUnlockTest.

This revision is now accepted and ready to land.May 30 2019, 11:50 AM
hctim updated this revision to Diff 202275.May 30 2019, 12:41 PM

Merged with tip-of-tree in preparation for submit.

This revision was automatically updated to reflect the committed changes.
hctim added a comment.May 30 2019, 1:48 PM

Looks like this broke an autoconf bot. Have submitted rL362149, which should hopefully fix the issue.

hctim added a comment.May 30 2019, 2:57 PM

Looks like this also broke an armv8 bot, due to a maximum number of threads issue. Have filed rL362163 to attempt to fix. If that doesn't work, I'll just disable the test for armv8.