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

[JF Bastien via Phabricator via cfe-commits]

jfb added a comment.

> 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++).

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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61923