jfb added a comment. In D61923#1502323 <https://reviews.llvm.org/D61923#1502323>, @hctim wrote:
> In D61923#1502245 <https://reviews.llvm.org/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*". In D61923#1502337 <https://reviews.llvm.org/D61923#1502337>, @eugenis wrote: > 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. ================ Comment at: compiler-rt/lib/gwp_asan/definitions.h:14 +# undef ALWAYS_INLINE +#endif // defined(ALWAYS_INLINE) +#define ALWAYS_INLINE inline __attribute__((always_inline)) ---------------- The undef above doesn't seem like a good idea. You should prefix the macro with `GWP_` if you're concerned about name clashes. ================ Comment at: compiler-rt/lib/gwp_asan/mutex.h:42 + __atomic_is_lock_free(sizeof(Locked), /*Typical Alignment*/ 0), + "gwp_asan::Mutex::Locked is not lock-free on this architecture."); +}; ---------------- You want `__atomic_always_lock_free`, it'll always be a `constexpr` (whereas the other can be a libcall). 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
