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

[Mitch Phillips via Phabricator via cfe-commits]

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
  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

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

[Mitch Phillips via Phabricator via cfe-commits]

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
  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

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

[Mitch Phillips via Phabricator via cfe-commits]

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

Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61923/new/

https://reviews.llvm.org/D61923

Files:
  cfe/trunk/runtime/CMakeLists.txt
  compiler-rt/trunk/lib/gwp_asan/CMakeLists.txt
  compiler-rt/trunk/lib/gwp_asan/mutex.h
  compiler-rt/trunk/lib/gwp_asan/platform_specific/mutex_posix.cpp
  compiler-rt/trunk/lib/gwp_asan/tests/CMakeLists.txt
  compiler-rt/trunk/lib/gwp_asan/tests/driver.cpp
  compiler-rt/trunk/lib/gwp_asan/tests/mutex_test.cpp
  compiler-rt/trunk/test/gwp_asan/CMakeLists.txt
  compiler-rt/trunk/test/gwp_asan/dummy_test.cc
  compiler-rt/trunk/test/gwp_asan/lit.cfg
  compiler-rt/trunk/test/gwp_asan/lit.site.cfg.in
  compiler-rt/trunk/test/gwp_asan/unit/lit.site.cfg.in

Index: cfe/trunk/runtime/CMakeLists.txt
===
--- cfe/trunk/runtime/CMakeLists.txt
+++ cfe/trunk/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}
Index: compiler-rt/trunk/test/gwp_asan/unit/lit.site.cfg.in
===
--- compiler-rt/trunk/test/gwp_asan/unit/lit.site.cfg.in
+++ compiler-rt/trunk/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/trunk/test/gwp_asan/lit.site.cfg.in
===
--- compiler-rt/trunk/test/gwp_asan/lit.site.cfg.in
+++ compiler-rt/trunk/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/trunk/test/gwp_asan/dummy_test.cc
===
--- compiler-rt/trunk/test/gwp_asan/dummy_test.cc
+++ compiler-rt/trunk/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/trunk/test/gwp_asan/CMakeLists.txt
===
--- compiler-rt/trunk/test/gwp_asan/CMakeLists.txt
+++ compiler-rt/trunk/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_c

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

[Mitch Phillips via Phabricator via cfe-commits]

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
  compiler-rt/lib/gwp_asan/CMakeLists.txt
  compiler-rt/lib/gwp_asan/mutex.h
  compiler-rt/lib/gwp_asan/platform_specific/mutex_posix.cpp
  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_testsui

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

[Matt Morehouse via Phabricator via cfe-commits]

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



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

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

[Mitch Phillips via Phabricator via cfe-commits]

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/runtime/CMakeLists.txt
  compiler-rt/lib/gwp_asan/CMakeLists.txt
  compiler-rt/lib/gwp_asan/mutex.h
  compiler-rt/lib/gwp_asan/platform_specific/mutex_posix.cpp
  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

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

[Mitch Phillips via Phabricator via cfe-commits]

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. `generate_compiler_rt_tests` builds each test cpp 
> > file individually, and provides no way to add `gtest_main` only during link 
> > time. 
> Then how does this driver.cpp get included in each unit test?
> 
> Maybe we should put the `main` in each test as is usually done.
Basically what happens:
```
for x in *.cpp;
  do clang++ $x -o $x.o
done
clang++ *.o -o GwpAsanUnitTests
```

The unittests are all packaged together into a single binary. Also, AFAIK, 
compiler-rt all has a single `main()` driver for unittests (at least 
ASan+MSan+Scudo do this).



Comment at: compiler-rt/lib/gwp_asan/tests/mutex_test.cpp:102
+TEST(GwpAsanMutexTest, ThrashTests) {
+  runThrashTest(2, 1);
+  runThrashTest(4, 1);

morehouse wrote:
> morehouse wrote:
> > This test case seems to test a superset of ThreadedConstructionTest, so do 
> > we really need that one?
> This test case seems to test a superset of ThreadedConstructionTest, so do we 
> really need that one?
Deleted ThreadedLockUnlockTest.


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

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

[Matt Morehouse via Phabricator via cfe-commits]

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_tests` builds each test cpp 
> file individually, and provides no way to add `gtest_main` only during link 
> time. 
Then how does this driver.cpp get included in each unit test?

Maybe we should put the `main` in each test as is usually done.



Comment at: compiler-rt/lib/gwp_asan/tests/mutex_test.cpp:37
+  Mutex Mu;
+  { ScopedLock L(Mu); }
+  // Locking will fail here if the scoped lock failed to unlock.

morehouse wrote:
> We should trylock inside the scope, and maybe also do an unlock+lock.
No, I didn't mean that way.

Something like this:
```
{
  ScopedLock L(Mu);
  EXPECT_FALSE(Mu.tryLock());  // Test that the ctor did in fact lock
  Mu.unlock()
  EXPECT_TRUE(Mu.tryLock());  // Test that manual unlock overrides ScopedLock
}
EXPECT_TRUE(Mu.tryLock());  // Test that dtor did in fact unlock
```



Comment at: compiler-rt/lib/gwp_asan/tests/mutex_test.cpp:46
+  while (!StartingGun->load())
+;
+  for (unsigned i = 0; i < kNumLocksInARow; ++i) {

hctim wrote:
> morehouse wrote:
> > Format seems weird.  Does clang-format let us do `while (...) {}` on one 
> > line?
> CF changes it to 
> ```
> while (!StartingGun) {
> }
> ```
Not a big deal, but might be more readable if we do:

```
while (!StartingGun) {
  // Wait for starting gun.
}
```



Comment at: compiler-rt/lib/gwp_asan/tests/mutex_test.cpp:59
+
+  StartingGun.store(true);
+  T1.join();

morehouse wrote:
> Can we use simpler `StartingGun = true`?
Can we use simpler `StartingGun = true`?



Comment at: compiler-rt/lib/gwp_asan/tests/mutex_test.cpp:102
+TEST(GwpAsanMutexTest, ThrashTests) {
+  runThrashTest(2, 1);
+  runThrashTest(4, 1);

morehouse wrote:
> This test case seems to test a superset of ThreadedConstructionTest, so do we 
> really need that one?
This test case seems to test a superset of ThreadedConstructionTest, so do we 
really need that one?


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

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

[Mitch Phillips via Phabricator via cfe-commits]

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 cpp 
file individually, and provides no way to add `gtest_main` only during link 
time. 



Comment at: compiler-rt/lib/gwp_asan/tests/mutex_test.cpp:46
+  while (!StartingGun->load())
+;
+  for (unsigned i = 0; i < kNumLocksInARow; ++i) {

morehouse wrote:
> Format seems weird.  Does clang-format let us do `while (...) {}` on one line?
CF changes it to 
```
while (!StartingGun) {
}
```



Comment at: compiler-rt/test/gwp_asan/CMakeLists.txt:1
+set(GWP_ASAN_LIT_SOURCE_DIR ${CMAKE_CURRENT_SOURCE_DIR})
+set(GWP_ASAN_LIT_BINARY_DIR ${CMAKE_CURRENT_BINARY_DIR})

morehouse wrote:
> Can the lit test stuff go in another patch when we actually have tests?
Not AFAIK. The unit tests are actually invoked through the lit configurations 
in this directory.


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

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

[Mitch Phillips via Phabricator via cfe-commits]

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.llvm.org/D61923

Files:
  clang/runtime/CMakeLists.txt
  compiler-rt/lib/gwp_asan/CMakeLists.txt
  compiler-rt/lib/gwp_asan/mutex.h
  compiler-rt/lib/gwp_asan/platform_specific/mutex_posix.cpp
  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_BIN

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

[Matt Morehouse via Phabricator via cfe-commits]

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/gwp_asan/mutex.h:40
+  ~ScopedLock() { Mu.unlock(); }
+  ScopedLock(const ScopedLock &) = delete;
+

We should also delete operator=.



Comment at: compiler-rt/lib/gwp_asan/platform_specific/mutex_posix.cpp:19
+  // Remove warning for non-debug builds.
+  (void)(Status);
+}

Unnecessary parens (also below)



Comment at: compiler-rt/lib/gwp_asan/tests/driver.cpp:14
+  return RUN_ALL_TESTS();
+}

Can we just link with gtest_main instead of having this?



Comment at: compiler-rt/lib/gwp_asan/tests/mutex_test.cpp:20
+
+TEST(GwpAsanMutexTest, ConstructionTest) {
+  Mutex Mu;

Nit: ConstructionTest doesn't seem to describe what this test does.

Something like LockUnlockTest seems more descriptive.

(Same for other "Construction" tests)



Comment at: compiler-rt/lib/gwp_asan/tests/mutex_test.cpp:37
+  Mutex Mu;
+  { ScopedLock L(Mu); }
+  // Locking will fail here if the scoped lock failed to unlock.

We should trylock inside the scope, and maybe also do an unlock+lock.



Comment at: compiler-rt/lib/gwp_asan/tests/mutex_test.cpp:43
+
+static constexpr unsigned kNumLocksInARow = 1000;
+void lockTask(std::atomic *StartingGun, Mutex *Mu) {

This doesn't need to be a global.



Comment at: compiler-rt/lib/gwp_asan/tests/mutex_test.cpp:44
+static constexpr unsigned kNumLocksInARow = 1000;
+void lockTask(std::atomic *StartingGun, Mutex *Mu) {
+  while (!StartingGun->load())

static



Comment at: compiler-rt/lib/gwp_asan/tests/mutex_test.cpp:45
+void lockTask(std::atomic *StartingGun, Mutex *Mu) {
+  while (!StartingGun->load())
+;

Can we use the simpler syntax `while (!StartingGun)` instead?



Comment at: compiler-rt/lib/gwp_asan/tests/mutex_test.cpp:46
+  while (!StartingGun->load())
+;
+  for (unsigned i = 0; i < kNumLocksInARow; ++i) {

Format seems weird.  Does clang-format let us do `while (...) {}` on one line?



Comment at: compiler-rt/lib/gwp_asan/tests/mutex_test.cpp:49
+ScopedLock L(*Mu);
+  }
+}

Nit: curly braces unnecessary



Comment at: compiler-rt/lib/gwp_asan/tests/mutex_test.cpp:59
+
+  StartingGun.store(true);
+  T1.join();

Can we use simpler `StartingGun = true`?



Comment at: compiler-rt/lib/gwp_asan/tests/mutex_test.cpp:68
+
+void thrashTask(std::atomic *StartingGun, Mutex *Mu, unsigned *Counter,
+unsigned NumIterations) {

static



Comment at: compiler-rt/lib/gwp_asan/tests/mutex_test.cpp:82
+NumThreads = std::thread::hardware_concurrency();
+  }
+

I don't think we should care about the number of cores at all.  Time-sliced 
threads are just fine for this test.



Comment at: compiler-rt/lib/gwp_asan/tests/mutex_test.cpp:101
+
+TEST(GwpAsanMutexTest, ThrashTests) {
+  runThrashTest(2, 1);

How are any of these "thrash" tests?

I'd prefer something more descriptive, like SynchronizedCounterTest.



Comment at: compiler-rt/lib/gwp_asan/tests/mutex_test.cpp:102
+TEST(GwpAsanMutexTest, ThrashTests) {
+  runThrashTest(2, 1);
+  runThrashTest(4, 1);

This test case seems to test a superset of ThreadedConstructionTest, so do we 
really need that one?



Comment at: compiler-rt/lib/gwp_asan/tests/mutex_test.cpp:104
+  runThrashTest(4, 1);
+  runThrashTest(4, 10);
+}

4 threads isn't very much.  Can we test with more threads, say ~1000?



Comment at: compiler-rt/test/gwp_asan/CMakeLists.txt:1
+set(GWP_ASAN_LIT_SOURCE_DIR ${CMAKE_CURRENT_SOURCE_DIR})
+set(GWP_ASAN_LIT_BINARY_DIR ${CMAKE_CURRENT_BINARY_DIR})

Can the lit test stuff go in another patch when we actually have tests?


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

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

[Mitch Phillips via Phabricator via cfe-commits]

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/runtime/CMakeLists.txt
  compiler-rt/lib/gwp_asan/CMakeLists.txt
  compiler-rt/lib/gwp_asan/mutex.h
  compiler-rt/lib/gwp_asan/platform_specific/mutex_posix.cpp
  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(

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

[Mitch Phillips via Phabricator via cfe-commits]

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 using malloc() inside mutex constructor, either.
> 
> We will probably want the mutex to be linker-initialized, too. AFAIK, 
> gwp-asan would not have any clear initialization entry point, and would need 
> to do this lazily in malloc, which can there be several of, concurrently.
> 
As per offline discussion, have ifdef-d the platform-specific headers into this 
file. Allows us to have linker initialisation (at least for POSIX).


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

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

[Evgenii Stepanov via Phabricator via cfe-commits]

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() inside mutex constructor, either.

We will probably want the mutex to be linker-initialized, too. AFAIK, gwp-asan 
would not have any clear initialization entry point, and would need to do this 
lazily in malloc, which can there be several of, concurrently.



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

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

[Mitch Phillips via Phabricator via cfe-commits]

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/D61923/new/

https://reviews.llvm.org/D61923

Files:
  clang/runtime/CMakeLists.txt
  compiler-rt/lib/gwp_asan/CMakeLists.txt
  compiler-rt/lib/gwp_asan/mutex.h
  compiler-rt/lib/gwp_asan/platform_specific/mutex_posix.cpp
  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_TES

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

[Evgenii Stepanov via Phabricator via cfe-commits]

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 the interface C-only; or use pImpl to hide the implementation; or 
just move the entire declaration of Mutex to the platform header.



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

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

[Mitch Phillips via Phabricator via cfe-commits]

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
abstraction over the platform specifics is implemented correctly.


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/mutex.h
  compiler-rt/lib/gwp_asan/platform_specific/mutex_include.h
  compiler-rt/lib/gwp_asan/platform_specific/mutex_members.inc
  compiler-rt/lib/gwp_asan/platform_specific/mutex_posix.cpp
  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

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

[Mitch Phillips via Phabricator via cfe-commits]

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.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_NAM

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

[Mitch Phillips via Phabricator via cfe-commits]

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 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.

I definitely don't want that to be the case, or to have it come across that 
way. We want a reference implementation that's available for any allocator to 
drop in and use, with minor tweaking. As part of that, our goal is to make the 
requirements to support GWP-ASan as low as possible, preferably as simple as 
"merge this into your codebase somewhere, and you're good to go". We inherit 
the restrictions of each allocator's build system, which means more restrictive 
environment for us, but we can be drop-in supported pretty much everywhere. Our 
plans for this reference implementation is to use it in Android and Fuchsia, 
but we would love to collaborate with other interested projects to hear about 
what their requirements would be as well.

We discussed at length whether to have GWP-ASan be a standalone GitHub project 
(similar to Scudo standalone), or as part of compiler-rt. We decided to put it 
into compiler-rt because we can then exercise LLVM's extensive 
build/test/review infrastructure and open-source licensing.


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

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

[JF Bastien via Phabricator via cfe-commits]

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 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

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

[Mitch Phillips via Phabricator via cfe-commits]

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 assumptions about why, but would love to hear right from the 
horse's mouth :)

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 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
==

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

[Kostya Kortchinsky via Phabricator via cfe-commits]

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 same (  @cryptoad, why? ).
>
> As Mitch mentioned, we should move the implementation into a common 
> directory. Why not do this now?


Is the question why do Scudo has its own as opposed to rely on platform 
specific ones?


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

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

[JF Bastien via Phabricator via cfe-commits]

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 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 , @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

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

[Evgenii Stepanov via Phabricator via cfe-commits]

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 common directory. 
Why not do this now?


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

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

[Mitch Phillips via Phabricator via cfe-commits]

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 
guarantee that `std::mutex` is header-only), see 
https://reviews.llvm.org/D60593#inline-537276.
We also are trying to stay independent of `pthread` for 
platform-independentness.
We also can't use a `sanitizer_common` variant as we don't want to pull in the 
entirety `sanitizer_common` as a dependency.

Basically, the roadmap is to create a shared mutex library for 
`sanitizer_common`, `gwp_asan` and `scudo` to all use.

In D61923#1502245 , @jfb wrote:

> Tests?


Added some unit tests.




Comment at: compiler-rt/lib/gwp_asan/mutex.h:32
+private:
+  void yieldProcessor(uint8_t Count);
+  void yieldPlatform();

jfb wrote:
> `uint8_t` seems easy to inadvertently truncate. If you want to restrict size, 
> make it a template and refuse to compile above a certain size. It's always 
> `10` anyways.
Seeming as this is an entirely-internal number, I've made it a global instead.



Comment at: compiler-rt/lib/gwp_asan/mutex.h:81
+else
+  yieldPlatform();
+

jfb wrote:
> This won't compile if `__unix__` isn't defined.
Should be guarded by `if (COMPILER_RT_HAS_GWP_ASAN)` at 
`lib/gwp_asan/CMakeLists.txt:20`, but have added explicit error condition here.


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

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

[Mitch Phillips via Phabricator via cfe-commits]

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 , @jfb wrote:


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.si