add tsan shared library

[ZijunZhao]

Summary

Add tsan shared library on Android. Only build tsan when minSdkVersion is above 23.

[ZijunZhao]

add tsan shared lib

[danalbert]

A couple stray pieces snuck in. Could you revert those and update the change? It looks like you also missed the mailing list, but we can fix that once the patch is fixed.

.arcconfig

2 ↗

(On Diff #367582)

Should revert this change.

libcxx/utils/merge_archives.py

11 ↗

(On Diff #367582)

And this.

[vitalybuka]

compiler-rt/lib/tsan/CMakeLists.txt

266 ↗

(On Diff #367668)

@eugenis I would say that we need to test dynamic runtime, but I noticed that we test Asan but don't hwasan and ubsan. Is there good reason for that or just overlook?

libcxx/utils/merge_archives.py

11 ↗

(On Diff #367668)

Why do we need this change?

[eugenis]

compiler-rt/lib/tsan/CMakeLists.txt

266 ↗

(On Diff #367668)

We test hwasan on android, it defaults to shared runtime there. As well as all sanitizers, I believe.

Yeah, it would be good to add such testing.

[danalbert]

compiler-rt/lib/tsan/CMakeLists.txt

266 ↗

(On Diff #367668)

We're working on testing on our end as well. TSan doesn't actually work on Android currently, but I don't think that's related to it being a shared library. TSan was working on Android as a shared library many years ago, but it bit rot because it never got used or tested.

Is there support for cross-testing in compiler-rt? If so we can add some tests here as well.

libcxx/utils/merge_archives.py

11 ↗

(On Diff #367668)

We don't. This is an accidental revert due to a merge resolution, I think.

@ZijunZhao, need to update the review to exclude this part.

[ZijunZhao]

libcxx/utils/merge_archives.py

11 ↗

(On Diff #367668)

Okay, I will delete this one.

[danalbert]

(I'd still like to hear what vitalybuka/eugenis have to say, but LGTM)

[ldionne]

I'm not sure why the libc++ group reviewer is on this patch, but I don't think it should.

[eugenis]

compiler-rt/lib/tsan/CMakeLists.txt

266 ↗

(On Diff #367668)

What do you mean by cross-testing? There is a mode where lit tests build on Linux host and execute on Android through the scripts in test/sanitizer_common/android_commands/.

This should get tested automatically here:
https://github.com/llvm/llvm-zorg/blob/main/zorg/buildbot/builders/sanitizers/buildbot_android_functions.sh#L287

On this bot:
https://lab.llvm.org/buildbot/#/builders/77/builds/9195

Need to make sure that test/tsan has all the android bits right, though.

[danalbert]

compiler-rt/lib/tsan/CMakeLists.txt

266 ↗

(On Diff #367668)

Yep, that's what I meant. Thanks!

To be clear: tsan on Android doesn't actually work currently. It used to but bit rot. We're working on it as we find each issue, but this isn't going to be the last one. We'll probably just need to disable some tests for Android, though given that they aren't currently failing, probably nothing we need to do yet?

[eugenis]

compiler-rt/lib/tsan/CMakeLists.txt

266 ↗

(On Diff #367668)

That's fine. If enough tests are passing, we could annotate remaining with XFAIL: android or REQUIRES: !android and then fix them one by one.

The zorg script can be ran locally, it's the easiest way to run android cross tests.

This is the entry point (make sure to run it in an empty directory - it may clobber the llvm checkout in $(pwd)):
https://github.com/llvm/llvm-zorg/blob/main/zorg/buildbot/builders/sanitizers/buildbot_android.sh

[vitalybuka]

compiler-rt/lib/tsan/CMakeLists.txt

266 ↗

(On Diff #367668)

My question is not just about android.
If we build this lib, we probably should be able to build and test that on any linux without cross compiling as well.

[vitalybuka]

compiler-rt/test/tsan/CMakeLists.txt

108 ↗

(On Diff #372373)

Nothing sets COMPILER_RT_TSAN_HAS_DYNAMIC_RUNTIME variable?

I guess you need to follow COMPILER_RT_ASAN_HAS_STATIC_RUNTIME
Name is weird but I believe it means that if !COMPILER_RT_ASAN_HAS_STATIC_RUNTIME then dynamic is default.
if COMPILER_RT_ASAN_HAS_STATIC_RUNTIME, static is default and we need to do additional dirs/configs to test dynamic stuff. I guess it applies to TSAN as well, so it would be nice if we replicate same approach here for consistency.

[vitalybuka]

compiler-rt/lib/tsan/tests/CMakeLists.txt

63 ↗

(On Diff #372743)

multiple places below use asan

87 ↗

(On Diff #372743)

there is no tsan for MSVC or windows

compiler-rt/test/tsan/CMakeLists.txt

45–64 ↗

(On Diff #372743)

This block is misaligned.

compiler-rt/test/tsan/lit.site.cfg.py.in

3 ↗

(On Diff #372743)

This one sets variable but nothing uses it,

More interesting stuff is in lit.cfg.py.
We need to match config.asan_dynamic from there.

We need to pass -shared-libsan into tests, in similar way as we do "-shared-libasan"
Then we need something as config.available_features.add("asan-dynamic-runtime")

[vitalybuka]

I guess testing is getting more complicated.
Maybe we should separate tests and non-test patches here? I suspect we well have several of them. Even after lit will run them as expected, they will likely just crash on start.
WDYT?

[danalbert]

In D108394#3003237, @vitalybuka wrote:

I guess testing is getting more complicated.
Maybe we should separate tests and non-test patches here? I suspect we well have several of them. Even after lit will run them as expected, they will likely just crash on start.
WDYT?

Fine with us. It seems like the CMake portion of the tests is nearly complete, so perhaps should finish up that part and then follow up with the LIT portion?

[vitalybuka]

In D108394#3003306, @danalbert wrote:

In D108394#3003237, @vitalybuka wrote:

I guess testing is getting more complicated.
Maybe we should separate tests and non-test patches here? I suspect we well have several of them. Even after lit will run them as expected, they will likely just crash on start.
WDYT?

Fine with us. It seems like the CMake portion of the tests is nearly complete, so perhaps should finish up that part and then follow up with the LIT portion?

LGTM. I assume now, without lit.cfg.py patch, it just runs default tsan tests and passes them.

[danalbert]

LGTM but we should wait for @vitalybuka.

[vitalybuka]

compiler-rt/test/tsan/CMakeLists.txt

57 ↗

(On Diff #373328)

this else branch needs to be executed always

if (COMPILER_RT_TSAN_HAS_STATIC_RUNTIME) {
  # add dynamic
}
# always add default configure_lit_site_cfg

[vitalybuka]

I suspect that test part of this patch has higher chances of revert.
I would still recommend to split in two:

1. compiler-rt/cmake/config-ix.cmake and compiler-rt/lib/tsan/CMakeLists.txt
2. the rest

compiler-rt/test/tsan/CMakeLists.txt

142 ↗

(On Diff #373713)

please add EXCLUDE_FROM_CHECK_ALL for dynamic for now and it's fine to land

[vitalybuka]

compiler-rt/test/tsan/CMakeLists.txt

130 ↗

(On Diff #373713)

TSAN_DYNAMIC_TEST_DEPS is not set
we need set(TSAN_DYNAMIC_TEST_DEPS ${TSAN_TEST_DEPS})

[vitalybuka]

compiler-rt/test/tsan/CMakeLists.txt

19 ↗

(On Diff #373713)

141–145 ↗

(On Diff #373713)

[danalbert]

compiler-rt/lib/tsan/CMakeLists.txt

250 ↗

(On Diff #375448)

Typo. ninja check-tsan is failing.

compiler-rt/lib/tsan/tests/CMakeLists.txt

83 ↗

(On Diff #375448)

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.

compiler-rt/test/tsan/CMakeLists.txt

44 ↗

(On Diff #375448)

TSAN_TEST_DYNAMIC is never used; this block isn't needed.

63 ↗

(On Diff #375448)

Also not needed.

124 ↗

(On Diff #375448)

Same here.

133 ↗

(On Diff #375448)

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.

19 ↗

(On Diff #373713)

I think this is still needed? This list is missing most of the deps.

[ZijunZhao]

compiler-rt/test/tsan/CMakeLists.txt

19 ↗

(On Diff #373713)

19 ↗

(On Diff #373713)

If there is no need for unit test, I suppose

list(APPEND TSAN_DYNAMIC_TEST_DEPS TsanDynamicUnitTests)

should be deleted as well.

[danalbert]

LGTM. @vitalybuka, do you want to have one last look, or are we clear to merge?

[vitalybuka]

Thank you. LGTM

compiler-rt/test/tsan/CMakeLists.txt

64–66 ↗

(On Diff #375967)

[ZijunZhao]

Fix "ld.lld: error: unable to find library -lc++" this bug in Fuchsia.

[danalbert]

Fixes LGTM.

[melver]

Hello,

If possible please use arc diff and amend to get the canonical commit message format (see git log for other commits).
Otherwise Phabricator won't be able to track the history of what's been happening.

I saw several instances of this committed to the LLVM repo without a commit message or link here and got a little confused:

git log --oneline | grep -i 'add tsan shar\|revert tsan'
d2b43605c96f add tsan shared lib
45d28e3a303a Revert "add tsan shared lib"
92c9b340be41 add tsan shared lib
0e8862901ca5 revert tsan part for investigation
91bfccf83733 add tsan shared library

If you don't cleanly revert a change, the next change should explain what it's fixing and why, and usually come with a new Phabricator review.
Unless you fully revert the prior change, then the original one can be reopened.

[ZijunZhao]

Hi Marco,

Thank you for reminding me! I already submit a new one with explanation!

Best regards,
Zijun Zhao

[danalbert]

OS_NAME MATCHES "Darwin|Linux|FreeBSD|Android|NetBSD" AND ANDROID_PLATFORM_LEVEL GREATER 23

I think for non-Android ANDROID_PLATFORM_LEVEL won't be set at all, so this will disable this branch for all non-Android.

[ZijunZhao]

Correct the condition and when the minSdkVersion is above 23 on Android.

[danalbert]

compiler-rt/cmake/config-ix.cmake

754

Looks like a stray whitespace change snuck in (space indentation mixed with tabs), but otherwise LGTM. (I don't actually know if the project cares but I'll assume they do)

[ZijunZhao]

remove stray whitespace

[ZijunZhao]

I copy the whitespace from other statements and paste it before each new statement.

[danalbert]

compiler-rt/cmake/config-ix.cmake

753–754

There's still a tab here where there wasn't one before.

[danalbert]

compiler-rt/cmake/config-ix.cmake

753–754

Checked the actual patch with git show and in vim... and they both agree there isn't a tab here. idk what phabricator is trying to tell me. Sorry for the noise.

compiler-rt/cmake/config-ix.cmake

 
 if (PROFILE_SUPPORTED_ARCH AND NOT LLVM_USE_SANITIZER AND
     OS_NAME MATCHES "Darwin|Linux|FreeBSD|Windows|Android|Fuchsia|SunOS|NetBSD|AIX")
   set(COMPILER_RT_HAS_PROFILE TRUE)
 else()
   set(COMPILER_RT_HAS_PROFILE FALSE)
 endif()
 
-if (COMPILER_RT_HAS_SANITIZER_COMMON AND TSAN_SUPPORTED_ARCH AND
-    OS_NAME MATCHES "Darwin|Linux|FreeBSD|Android|NetBSD")
+if (COMPILER_RT_HAS_SANITIZER_COMMON AND TSAN_SUPPORTED_ARCH)
+  if (OS_NAME MATCHES "Linux|Darwin|FreeBSD|NetBSD")
     set(COMPILER_RT_HAS_TSAN TRUE)
+  elseif (OS_NAME MATCHES "Android" AND ANDROID_PLATFORM_LEVEL GREATER 23)
+    set(COMPILER_RT_HAS_TSAN TRUE)
+  else()
+    set(COMPILER_RT_HAS_TSAN FALSE)
+  endif()
 else()
   set(COMPILER_RT_HAS_TSAN FALSE)
 endif()
 
 if (OS_NAME MATCHES "Linux|FreeBSD|Windows|NetBSD|SunOS")
   set(COMPILER_RT_TSAN_HAS_STATIC_RUNTIME TRUE)
 else()
   set(COMPILER_RT_TSAN_HAS_STATIC_RUNTIME FALSE)