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

hctim added a comment. Looks like this also broke an armv8 bot, due to a maximum number of threads issue. Have filed rL362163 to attempt to fix. If that doesn't work, I'll just disable the test for armv8. Repository: rL LLVM CHANGES SINCE LAST ACTION ht

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

hctim added a comment. Looks like this broke an autoconf bot . Have submitted rL362149 , which should hopefully fix the issue. Repository: rL LLVM CHANGES SINCE LAST ACTION

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

This revision was automatically updated to reflect the committed changes. Closed by commit rL362138: [GWP-ASan] Mutex implementation [2]. (authored by hctim, committed by ). Herald added a subscriber: delcypher. Changed prior to commit: https://reviews.llvm.org/D61923?vs=202275&id=202276#toc R

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

hctim updated this revision to Diff 202275. hctim added a comment. Merged with tip-of-tree in preparation for submit. 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

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

morehouse accepted this revision. morehouse added a comment. This revision is now accepted and ready to land. LGTM Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D61923/new/ https://reviews.llvm.org/D61923 _

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

hctim updated this revision to Diff 202237. hctim marked 7 inline comments as done. hctim added a comment. - Updated from Matt's comments. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D61923/new/ https://reviews.llvm.org/D61923 Files: clang/runt

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

hctim added inline comments. Comment at: compiler-rt/lib/gwp_asan/tests/driver.cpp:14 + return RUN_ALL_TESTS(); +} morehouse wrote: > hctim wrote: > > morehouse wrote: > > > Can we just link with gtest_main instead of having this? > > Unfortunately not easily. `

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

morehouse added inline comments. Comment at: compiler-rt/lib/gwp_asan/tests/driver.cpp:14 + return RUN_ALL_TESTS(); +} hctim wrote: > morehouse wrote: > > Can we just link with gtest_main instead of having this? > Unfortunately not easily. `generate_compiler_rt_

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

hctim added inline comments. Comment at: compiler-rt/lib/gwp_asan/tests/driver.cpp:14 + return RUN_ALL_TESTS(); +} morehouse wrote: > Can we just link with gtest_main instead of having this? Unfortunately not easily. `generate_compiler_rt_tests` builds each test

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

hctim updated this revision to Diff 202078. hctim marked 21 inline comments as done. hctim added a comment. - Apologies about the delay on this. Updated with Matt's comments. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D61923/new/ https://reviews.

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

morehouse added inline comments. Comment at: compiler-rt/lib/gwp_asan/mutex.h:22 + constexpr Mutex() = default; + ~Mutex() = default; + // Lock the mutex. We should probably delete copy constructor and operator= Comment at: compiler-rt/lib/g

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

hctim updated this revision to Diff 200760. hctim marked 2 inline comments as done. hctim added a comment. - Ifdef-d headers into mutex.h Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D61923/new/ https://reviews.llvm.org/D61923 Files: clang/runti

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

hctim added inline comments. Comment at: compiler-rt/lib/gwp_asan/platform_specific/mutex_posix.cpp:43 + +Mutex::Mutex() : PImpl(new Impl) {} +Mutex::~Mutex() { delete PImpl; } eugenis wrote: > This is a dependency on libc++ / libstdc++. > > I'm not sure about u

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

eugenis added inline comments. Comment at: compiler-rt/lib/gwp_asan/platform_specific/mutex_posix.cpp:43 + +Mutex::Mutex() : PImpl(new Impl) {} +Mutex::~Mutex() { delete PImpl; } This is a dependency on libc++ / libstdc++. I'm not sure about using malloc() insid

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

hctim updated this revision to Diff 200604. hctim marked an inline comment as done. hctim added a comment. - Updated to use pointer-to-impl to abstract implementation behaviour away from header files. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D6

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

eugenis added inline comments. Comment at: compiler-rt/lib/gwp_asan/mutex.h:27 +private: +#include "gwp_asan/platform_specific/mutex_members.inc" +}; What's the point of this include? You are leaking platform details into this common header anyway. We can make

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

hctim updated this revision to Diff 200600. hctim added a comment. Changed GWP-ASan to use platform specific mutexes. For now, we only target Android and Linux, and subsequently only need the pthread_mutex variant for POSIX. Kept around the mutex unittests as it's an easy assertion that the abstr

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

hctim updated this revision to Diff 200094. hctim added a comment. Merged in master after fixing up some buildbots with the previous patches. Also have pinged libcxx-dev :) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D61923/new/ https://reviews.l

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

hctim added a comment. In D61923#1503272 , @jfb wrote: > Have you asked on libcxx-dev whether a stand-alone base is something of > interest to them? No, but I will follow up on that. > Kinda... but your answer really sounds like this is a Google-only p

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

jfb added a comment. > In 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

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

hctim updated this revision to Diff 199632. hctim marked 2 inline comments as done. hctim added a comment. In D61923#1502988 , @cryptoad wrote: > Is the question why do Scudo has its own as opposed to rely on platform > specific ones? Yes, I have assump

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

cryptoad added a comment. In 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 sa

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

jfb added a comment. In D61923#1502323 , @hctim wrote: > In 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

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

eugenis added a comment. 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? ). As Mitch mentioned, we should move the implementation into a

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

hctim added a comment. In 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 (and can't make the strong guarant

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

hctim updated this revision to Diff 199541. hctim marked 10 inline comments as done. hctim added a comment. Herald added subscribers: cfe-commits, srhines. Herald added a reviewer: jfb. Herald added a project: clang. - Working copy of mutex. - >>! In D61923#1502245