hctim updated this revision to Diff 199632. hctim marked 2 inline comments as done. hctim added a comment.
In D61923#1502988 <https://reviews.llvm.org/D61923#1502988>, @cryptoad wrote: > Is the question why do Scudo has its own as opposed to rely on platform > specific ones? Yes, I have assumptions about why, but would love to hear right from the horse's mouth :) 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++). >> 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 :) 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 compiler-rt/lib/gwp_asan/CMakeLists.txt compiler-rt/lib/gwp_asan/definitions.h compiler-rt/lib/gwp_asan/mutex.h compiler-rt/lib/gwp_asan/tests/CMakeLists.txt compiler-rt/lib/gwp_asan/tests/driver.cpp compiler-rt/lib/gwp_asan/tests/mutex_test.cpp compiler-rt/test/gwp_asan/CMakeLists.txt compiler-rt/test/gwp_asan/dummy_test.cc compiler-rt/test/gwp_asan/lit.cfg compiler-rt/test/gwp_asan/lit.site.cfg.in compiler-rt/test/gwp_asan/unit/lit.site.cfg.in
Index: compiler-rt/test/gwp_asan/unit/lit.site.cfg.in =================================================================== --- /dev/null +++ compiler-rt/test/gwp_asan/unit/lit.site.cfg.in @@ -0,0 +1,9 @@ +@LIT_SITE_CFG_IN_HEADER@ + +config.name = "GwpAsan-Unittest" +# Load common config for all compiler-rt unit tests. +lit_config.load_config(config, "@COMPILER_RT_BINARY_DIR@/unittests/lit.common.unit.configured") + +config.test_exec_root = os.path.join("@COMPILER_RT_BINARY_DIR@", + "lib", "gwp_asan", "tests") +config.test_source_root = config.test_exec_root Index: compiler-rt/test/gwp_asan/lit.site.cfg.in =================================================================== --- /dev/null +++ compiler-rt/test/gwp_asan/lit.site.cfg.in @@ -0,0 +1,11 @@ +@LIT_SITE_CFG_IN_HEADER@ + +config.name_suffix = "@GWP_ASAN_TEST_CONFIG_SUFFIX@" +config.target_arch = "@GWP_ASAN_TEST_TARGET_ARCH@" +config.target_cflags = "@GWP_ASAN_TEST_TARGET_CFLAGS@" + +# Load common config for all compiler-rt lit tests. +lit_config.load_config(config, "@COMPILER_RT_BINARY_DIR@/test/lit.common.configured") + +# Load tool-specific config that would do the real work. +lit_config.load_config(config, "@GWP_ASAN_LIT_SOURCE_DIR@/lit.cfg") Index: compiler-rt/test/gwp_asan/lit.cfg =================================================================== --- /dev/null +++ compiler-rt/test/gwp_asan/lit.cfg @@ -0,0 +1,31 @@ +# -*- Python -*- + +import os + +# Setup config name. +config.name = 'GWP-ASan' + config.name_suffix + +# Setup source root. +config.test_source_root = os.path.dirname(__file__) + +# Test suffixes. +config.suffixes = ['.c', '.cc', '.cpp', '.test'] + +# C & CXX flags. +c_flags = ([config.target_cflags]) + +# Android doesn't want -lrt. +if not config.android: + c_flags += ["-lrt"] + +cxx_flags = (c_flags + config.cxx_mode_flags + ["-std=c++11"]) + +def build_invocation(compile_flags): + return " " + " ".join([config.clang] + compile_flags) + " " + +# Add substitutions. +config.substitutions.append(("%clang ", build_invocation(c_flags))) + +# GWP-ASan tests are currently supported on Linux only. +if config.host_os not in ['Linux']: + config.unsupported = True Index: compiler-rt/test/gwp_asan/dummy_test.cc =================================================================== --- /dev/null +++ compiler-rt/test/gwp_asan/dummy_test.cc @@ -0,0 +1,4 @@ +// Exists to simply stop warnings about lit not discovering any tests here. +// RUN: %clang %s + +int main() { return 0; } Index: compiler-rt/test/gwp_asan/CMakeLists.txt =================================================================== --- compiler-rt/test/gwp_asan/CMakeLists.txt +++ compiler-rt/test/gwp_asan/CMakeLists.txt @@ -0,0 +1,45 @@ +set(GWP_ASAN_LIT_SOURCE_DIR ${CMAKE_CURRENT_SOURCE_DIR}) +set(GWP_ASAN_LIT_BINARY_DIR ${CMAKE_CURRENT_BINARY_DIR}) + +set(GWP_ASAN_TESTSUITES) + +set(GWP_ASAN_UNITTEST_DEPS) +set(GWP_ASAN_TEST_DEPS + ${SANITIZER_COMMON_LIT_TEST_DEPS} + gwp_asan) + +if (COMPILER_RT_INCLUDE_TESTS) + list(APPEND GWP_ASAN_TEST_DEPS GwpAsanUnitTests) + configure_lit_site_cfg( + ${CMAKE_CURRENT_SOURCE_DIR}/unit/lit.site.cfg.in + ${CMAKE_CURRENT_BINARY_DIR}/unit/lit.site.cfg) + add_lit_testsuite(check-gwp_asan-unit "Running GWP-ASan unit tests" + ${CMAKE_CURRENT_BINARY_DIR}/unit + DEPENDS ${GWP_ASAN_TEST_DEPS}) + set_target_properties(check-gwp_asan-unit PROPERTIES FOLDER + "Compiler-RT Tests") + list(APPEND GWP_ASAN_TEST_DEPS check-gwp_asan-unit) +endif() + +configure_lit_site_cfg( + ${CMAKE_CURRENT_SOURCE_DIR}/lit.site.cfg.in + ${CMAKE_CURRENT_BINARY_DIR}/lit.site.cfg + ) + +foreach(arch ${GWP_ASAN_SUPPORTED_ARCH}) + set(GWP_ASAN_TEST_TARGET_ARCH ${arch}) + string(TOLOWER "-${arch}" GWP_ASAN_TEST_CONFIG_SUFFIX) + get_test_cc_for_arch(${arch} GWP_ASAN_TEST_TARGET_CC GWP_ASAN_TEST_TARGET_CFLAGS) + string(TOUPPER ${arch} ARCH_UPPER_CASE) + set(CONFIG_NAME ${ARCH_UPPER_CASE}${OS_NAME}Config) + + configure_lit_site_cfg( + ${CMAKE_CURRENT_SOURCE_DIR}/lit.site.cfg.in + ${CMAKE_CURRENT_BINARY_DIR}/${CONFIG_NAME}/lit.site.cfg) + list(APPEND GWP_ASAN_TESTSUITES ${CMAKE_CURRENT_BINARY_DIR}/${CONFIG_NAME}) +endforeach() + +add_lit_testsuite(check-gwp_asan "Running the GWP-ASan tests" + ${GWP_ASAN_TESTSUITES} + DEPENDS ${GWP_ASAN_TEST_DEPS}) +set_target_properties(check-gwp_asan PROPERTIES FOLDER "Compiler-RT Misc") Index: compiler-rt/lib/gwp_asan/tests/mutex_test.cpp =================================================================== --- /dev/null +++ compiler-rt/lib/gwp_asan/tests/mutex_test.cpp @@ -0,0 +1,105 @@ +//===-- mutex_test.cpp ------------------------------------------*- C++ -*-===// +// +// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. +// See https://llvm.org/LICENSE.txt for license information. +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception +// +//===----------------------------------------------------------------------===// + +#include "gwp_asan/mutex.h" +#include "gtest/gtest.h" + +#include <atomic> +#include <mutex> +#include <thread> +#include <vector> + +using gwp_asan::Mutex; +using gwp_asan::ScopedLock; + +TEST(GwpAsanMutexTest, ConstructionTest) { + Mutex Mu; + + ASSERT_TRUE(Mu.tryLock()); + ASSERT_FALSE(Mu.tryLock()); + Mu.unlock(); + + Mu.lock(); + Mu.unlock(); + + // Ensure that the mutex actually unlocked. + ASSERT_TRUE(Mu.tryLock()); + Mu.unlock(); +} + +TEST(GwpAsanMutexTest, ScopedConstructionTest) { + Mutex Mu; + { ScopedLock L(Mu); } + // Locking will fail here if the scoped lock failed to unlock. + EXPECT_TRUE(Mu.tryLock()); + Mu.unlock(); +} + +static constexpr unsigned kNumLocksInARow = 1000; +void lockTask(std::atomic<bool> *StartingGun, Mutex *Mu) { + while (!StartingGun->load()) + ; + for (unsigned i = 0; i < kNumLocksInARow; ++i) { + ScopedLock L(*Mu); + } +} + +TEST(GwpAsanMutexTest, ThreadedConstructionTest) { + Mutex Mu; + std::atomic<bool> StartingGun{false}; + + std::thread T1(lockTask, &StartingGun, &Mu); + std::thread T2(lockTask, &StartingGun, &Mu); + + StartingGun.store(true); + T1.join(); + T2.join(); + + // Locking will fail here if the scoped lock failed to unlock. + EXPECT_TRUE(Mu.tryLock()); + Mu.unlock(); +} + +void thrashTask(std::atomic<bool> *StartingGun, Mutex *Mu, unsigned *Counter, + unsigned NumIterations) { + while (!StartingGun->load()) + ; + for (unsigned i = 0; i < NumIterations; ++i) { + ScopedLock L(*Mu); + (*Counter)++; + } +} + +void runThrashTest(unsigned NumThreads, unsigned CounterMax) { + std::vector<std::thread> Threads; + if (std::thread::hardware_concurrency() < NumThreads) { + NumThreads = std::thread::hardware_concurrency(); + } + + ASSERT_TRUE(CounterMax % NumThreads == 0); + + std::atomic<bool> StartingGun{false}; + Mutex Mu; + unsigned Counter = 0; + + for (unsigned i = 0; i < NumThreads; ++i) + Threads.emplace_back(thrashTask, &StartingGun, &Mu, &Counter, + CounterMax / NumThreads); + + StartingGun.store(true); + for (auto &T : Threads) + T.join(); + + EXPECT_EQ(CounterMax, Counter); +} + +TEST(GwpAsanMutexTest, ThrashTests) { + runThrashTest(2, 10000); + runThrashTest(4, 10000); + runThrashTest(4, 100000); +} Index: compiler-rt/lib/gwp_asan/tests/driver.cpp =================================================================== --- /dev/null +++ compiler-rt/lib/gwp_asan/tests/driver.cpp @@ -0,0 +1,14 @@ +//===-- driver.cc -----------------------------------------------*- C++ -*-===// +// +// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. +// See https://llvm.org/LICENSE.txt for license information. +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception +// +//===----------------------------------------------------------------------===// + +#include "gtest/gtest.h" + +int main(int argc, char **argv) { + testing::InitGoogleTest(&argc, argv); + return RUN_ALL_TESTS(); +} Index: compiler-rt/lib/gwp_asan/tests/CMakeLists.txt =================================================================== --- /dev/null +++ compiler-rt/lib/gwp_asan/tests/CMakeLists.txt @@ -0,0 +1,49 @@ +include(CompilerRTCompile) + +set(GWP_ASAN_UNITTEST_CFLAGS + ${COMPILER_RT_UNITTEST_CFLAGS} + ${COMPILER_RT_GTEST_CFLAGS} + -I${COMPILER_RT_SOURCE_DIR}/lib/ + -O2) + +file(GLOB GWP_ASAN_HEADERS ../*.h) +file(GLOB GWP_ASAN_UNITTESTS *.cpp) +set(GWP_ASAN_UNIT_TEST_HEADERS + ${GWP_ASAN_HEADERS}) + +add_custom_target(GwpAsanUnitTests) +set_target_properties(GwpAsanUnitTests PROPERTIES FOLDER "Compiler-RT Tests") + +set(GWP_ASAN_UNITTEST_LINK_FLAGS ${COMPILER_RT_UNITTEST_LINK_FLAGS}) +list(APPEND GWP_ASAN_UNITTEST_LINK_FLAGS --driver-mode=g++) +if(NOT WIN32) + list(APPEND GWP_ASAN_UNITTEST_LINK_FLAGS -lpthread) +endif() + +if(COMPILER_RT_DEFAULT_TARGET_ARCH IN_LIST GWP_ASAN_SUPPORTED_ARCH) + # GWP-ASan unit tests are only run on the host machine. + set(arch ${COMPILER_RT_DEFAULT_TARGET_ARCH}) + + set(GWP_ASAN_TEST_RUNTIME RTGwpAsanTest.${arch}) + + set(GWP_ASAN_TEST_RUNTIME_OBJECTS + $<TARGET_OBJECTS:RTGwpAsan.${arch}>) + + add_library(${GWP_ASAN_TEST_RUNTIME} STATIC + ${GWP_ASAN_TEST_RUNTIME_OBJECTS}) + + set_target_properties(${GWP_ASAN_TEST_RUNTIME} PROPERTIES + ARCHIVE_OUTPUT_DIRECTORY ${CMAKE_CURRENT_BINARY_DIR} + FOLDER "Compiler-RT Runtime tests") + + set(GwpAsanTestObjects) + generate_compiler_rt_tests(GwpAsanTestObjects + GwpAsanUnitTests "GwpAsan-${arch}-Test" ${arch} + SOURCES ${GWP_ASAN_UNITTESTS} ${COMPILER_RT_GTEST_SOURCE} + RUNTIME ${GWP_ASAN_TEST_RUNTIME} + DEPS gtest ${GWP_ASAN_UNIT_TEST_HEADERS} + CFLAGS ${GWP_ASAN_UNITTEST_CFLAGS} + LINK_FLAGS ${GWP_ASAN_UNITTEST_LINK_FLAGS}) + set_target_properties(GwpAsanUnitTests PROPERTIES + RUNTIME_OUTPUT_DIRECTORY ${CMAKE_CURRENT_BINARY_DIR}) +endif() Index: compiler-rt/lib/gwp_asan/mutex.h =================================================================== --- /dev/null +++ compiler-rt/lib/gwp_asan/mutex.h @@ -0,0 +1,103 @@ +//===-- mutex.h -------------------------------------------------*- C++ -*-===// +// +// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. +// See https://llvm.org/LICENSE.txt for license information. +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception +// +//===----------------------------------------------------------------------===// + +// TODO(hctim): Move this implementation and Scudo's implementation into a +// unified base implementation. We shouldn't need to have separate +// implementations for this. + +#ifndef GWP_ASAN_MUTEX_H_ +#define GWP_ASAN_MUTEX_H_ + +#include "gwp_asan/definitions.h" + +#include <cstdint> + +#ifdef __unix__ +#include <sched.h> +#endif // defined(__unix__) + +namespace gwp_asan { +class Mutex { +public: + void lock(); + bool tryLock(); + void unlock(); + +private: + // How many cycles do we yield on the processor per call to yieldProcessor(). + static constexpr unsigned kProcessorYieldCount = 10; + void yieldProcessor(); + void yieldPlatform(); + + void lockSlow(); + + bool Locked = false; + static_assert( + __atomic_always_lock_free(sizeof(Locked), /*Typical Alignment*/ 0), + "gwp_asan::Mutex::Locked is not lock-free on this architecture."); +}; + +class ScopedLock { +public: + explicit ScopedLock(Mutex &Mx) : Mu(Mx) { Mu.lock(); } + ~ScopedLock() { Mu.unlock(); } + ScopedLock(const ScopedLock &) = delete; + +private: + Mutex Μ +}; + +ALWAYS_INLINE void Mutex::lock() { + if (tryLock()) + return; + lockSlow(); +} + +ALWAYS_INLINE bool Mutex::tryLock() { + return !__atomic_exchange_n(&Locked, true, __ATOMIC_ACQUIRE); +} + +ALWAYS_INLINE void Mutex::unlock() { + __atomic_store_n(&Locked, false, __ATOMIC_RELEASE); +} + +ALWAYS_INLINE void Mutex::yieldProcessor() { + __atomic_signal_fence(__ATOMIC_SEQ_CST); +#if defined(__i386__) || defined(__x86_64__) + for (unsigned i = 0; i < kProcessorYieldCount; ++i) + asm volatile("pause"); +#elif defined(__aarch64__) || defined(__arm__) + for (unsigned i = 0; i < kProcessorYieldCount; ++i) + asm volatile("yield"); +#else +#error "GWP-ASan on this architecture is not supported." +#endif + __atomic_signal_fence(__ATOMIC_SEQ_CST); +} + +ALWAYS_INLINE void Mutex::lockSlow() { + for (uint32_t i = 0;; ++i) { + if (i < 10) + yieldProcessor(); + else + yieldPlatform(); + + if (!__atomic_load_n(&Locked, __ATOMIC_RELAXED) && tryLock()) + return; + } +} + +#ifdef __unix__ +ALWAYS_INLINE void Mutex::yieldPlatform() { sched_yield(); } +#else +#error "GWP-ASan on this platform is not supported." +#endif + +} // namespace gwp_asan + +#endif // GWP_ASAN_MUTEX_H_ Index: compiler-rt/lib/gwp_asan/definitions.h =================================================================== --- /dev/null +++ compiler-rt/lib/gwp_asan/definitions.h @@ -0,0 +1,14 @@ +//===-- gwp_asan_definitions.h ----------------------------------*- C++ -*-===// +// +// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. +// See https://llvm.org/LICENSE.txt for license information. +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception +// +//===----------------------------------------------------------------------===// + +#ifndef GWP_ASAN_DEFINITIONS_H_ +#define GWP_ASAN_DEFINITIONS_H_ + +#define ALWAYS_INLINE inline __attribute__((always_inline)) + +#endif // GWP_ASAN_DEFINITIONS_H_ Index: compiler-rt/lib/gwp_asan/CMakeLists.txt =================================================================== --- compiler-rt/lib/gwp_asan/CMakeLists.txt +++ compiler-rt/lib/gwp_asan/CMakeLists.txt @@ -7,6 +7,8 @@ ) set(GWP_ASAN_HEADERS + definitions.h + mutex.h random.h ) @@ -34,3 +36,7 @@ ADDITIONAL_HEADERS ${GWP_ASAN_HEADERS} CFLAGS ${GWP_ASAN_CFLAGS}) endif() + +if(COMPILER_RT_INCLUDE_TESTS) + add_subdirectory(tests) +endif() Index: clang/runtime/CMakeLists.txt =================================================================== --- clang/runtime/CMakeLists.txt +++ clang/runtime/CMakeLists.txt @@ -132,7 +132,7 @@ # Add top-level targets for various compiler-rt test suites. set(COMPILER_RT_TEST_SUITES check-fuzzer check-asan check-hwasan check-asan-dynamic check-dfsan check-lsan check-msan check-sanitizer check-tsan check-ubsan check-ubsan-minimal - check-profile check-cfi check-cfi-and-supported check-safestack) + check-profile check-cfi check-cfi-and-supported check-safestack check-gwp_asan) foreach(test_suite ${COMPILER_RT_TEST_SUITES}) get_ext_project_build_command(run_test_suite ${test_suite}) add_custom_target(${test_suite}
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits