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



_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to