[PATCH] D61923: [GWP-ASan] Mutex implementation [2].

[Mitch Phillips via Phabricator via cfe-commits]

hctim updated this revision to Diff 199632.
hctim marked 2 inline comments as done.
hctim added a comment.

In D61923#1502988 <https://reviews.llvm.org/D61923#1502988>, @cryptoad wrote:

> 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 <https://reviews.llvm.org/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 :)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61923/new/

https://reviews.llvm.org/D61923

Files:
  clang/runtime/CMakeLists.txt
  compiler-rt/lib/gwp_asan/CMakeLists.txt
  compiler-rt/lib/gwp_asan/definitions.h
  compiler-rt/lib/gwp_asan/mutex.h
  compiler-rt/lib/gwp_asan/tests/CMakeLists.txt
  compiler-rt/lib/gwp_asan/tests/driver.cpp
  compiler-rt/lib/gwp_asan/tests/mutex_test.cpp
  compiler-rt/test/gwp_asan/CMakeLists.txt
  compiler-rt/test/gwp_asan/dummy_test.cc
  compiler-rt/test/gwp_asan/lit.cfg
  compiler-rt/test/gwp_asan/lit.site.cfg.in
  compiler-rt/test/gwp_asan/unit/lit.site.cfg.in

-------------- next part --------------
A non-text attachment was scrubbed...
Name: D61923.199632.patch
Type: text/x-patch
Size: 14667 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20190515/ac6382eb/attachment-0001.bin>