[PATCH] D108394: add tsan shared library

[Dan Albert via Phabricator via llvm-commits]

danalbert requested changes to this revision.
danalbert added inline comments.
This revision now requires changes to proceed.


================
Comment at: compiler-rt/lib/tsan/CMakeLists.txt:250
       ARCHS ${arch}
-      SOURCES ${TSAN_SOURCES} ${TSAN_ASM_SOURCES}
+      SOURCES ${TSAN_SOURCES} ${TSAN_ASM_SOURCES} ${TSAN_PREINIT_SOURCE}
               $<TARGET_OBJECTS:RTInterception.${arch}>
----------------
Typo. `ninja check-tsan` is failing.


================
Comment at: compiler-rt/lib/tsan/tests/CMakeLists.txt:83
+    LINK_FLAGS ${TSAN_UNITTEST_INSTRUMENTED_LINK_FLAGS}
+    SOURCES ${TSAN_INST_TEST_SOURCES}
+    CFLAGS ${TSAN_UNITTEST_INSTRUMENTED_CFLAGS} ${TEST_CFLAGS})
----------------
This is undefined. `ninja check-tsan` is broken.

Do we even need this block or the one on L131? The tests were already present, you just need to add the dynamic variant of them, which I think is covered by `compiler-rt/test/tsan/CMakeLists.txt`? I think the tests in //this// file are just unit tests, and dynamic vs static isn't relevant here? Maybe @vitalybuka can confirm.


================
Comment at: compiler-rt/test/tsan/CMakeLists.txt:44
 
+  if(ANDROID OR APPLE)
+    set(TSAN_TEST_DYNAMIC True)
----------------
`TSAN_TEST_DYNAMIC` is never used; this block isn't needed.


================
Comment at: compiler-rt/test/tsan/CMakeLists.txt:63
+    string(TOLOWER "-${arch}-${OS_NAME}-dynamic" TSAN_TEST_CONFIG_SUFFIX)
+    set(TSAN_TEST_DYNAMIC True)
+    set(CONFIG_NAME ${ARCH_UPPER_CASE}${OS_NAME}DynamicConfig)
----------------
Also not needed.


================
Comment at: compiler-rt/test/tsan/CMakeLists.txt:124
+  if(COMPILER_RT_TSAN_HAS_STATIC_RUNTIME)
+    set(TSAN_TEST_DYNAMIC True)
+    configure_lit_site_cfg(
----------------
Same here.


================
Comment at: compiler-rt/test/tsan/CMakeLists.txt:133
+    list(APPEND TSAN_DYNAMIC_TEST_DEPS TsanDynamicUnitTests)
+    set(TSAN_DYNAMIC_TEST_DEPS ${TSAN_TEST_DEPS})
+    list(APPEND TSAN_DYNAMIC_TESTSUITES ${CMAKE_CURRENT_BINARY_DIR}/Unit/dynamic)
----------------
Ah, that's where this went. I think you want //this line// up above in place of L20. As-is you're appending `TsanDynamicUnitTests` twice and then clobbering it.


================
Comment at: compiler-rt/test/tsan/CMakeLists.txt:19
 endif()
 
 set(TSAN_TESTSUITES)
----------------
vitalybuka wrote:
> 
I think this is still needed? This list is missing most of the deps.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108394