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

[JF Bastien via Phabricator via llvm-commits]

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