This is an archive of the discontinued LLVM Phabricator instance.

add tsan shared library
ClosedPublic

Authored by ZijunZhao on Aug 19 2021, 10:57 AM.

Details

Summary

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

Diff Detail

Event Timeline

ZijunZhao created this revision.Aug 19 2021, 10:57 AM
ZijunZhao requested review of this revision.Aug 19 2021, 10:57 AM

add tsan shared lib

ZijunZhao edited the summary of this revision. (Show Details)Aug 19 2021, 12:10 PM
ZijunZhao added reviewers: danalbert, srhines, enh, pirama.
ZijunZhao changed the edit policy from "All Users" to "ZijunZhao (Zijun Zhao)".
ZijunZhao changed the edit policy from "ZijunZhao (Zijun Zhao)" to "All Users".Aug 19 2021, 1:03 PM
danalbert requested changes to this revision.Aug 19 2021, 1:23 PM

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
10–11

And this.

This revision now requires changes to proceed.Aug 19 2021, 1:23 PM
ZijunZhao updated this revision to Diff 367620.Aug 19 2021, 2:40 PM
ZijunZhao added a reviewer: eugenis.
ZijunZhao added a subscriber: llvm-commits.
Herald added a project: Restricted Project. · View Herald TranscriptAug 19 2021, 2:40 PM
Herald added a subscriber: Restricted Project. · View Herald Transcript
ZijunZhao updated this revision to Diff 367625.Aug 19 2021, 2:46 PM
ZijunZhao marked an inline comment as done.
ZijunZhao updated this revision to Diff 367626.Aug 19 2021, 2:49 PM
ZijunZhao updated this revision to Diff 367644.Aug 19 2021, 4:06 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 19 2021, 4:06 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
ZijunZhao updated this revision to Diff 367656.Aug 19 2021, 4:29 PM
ZijunZhao updated this revision to Diff 367668.Aug 19 2021, 5:44 PM
vitalybuka added inline comments.Sep 1 2021, 12:06 PM
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

Why do we need this change?

eugenis added inline comments.Sep 1 2021, 12:10 PM
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 added inline comments.Sep 1 2021, 12:37 PM
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

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 added inline comments.Sep 1 2021, 2:14 PM
libcxx/utils/merge_archives.py
11

Okay, I will delete this one.

ZijunZhao updated this revision to Diff 370113.Sep 1 2021, 4:40 PM
danalbert accepted this revision.Sep 1 2021, 4:47 PM

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

ldionne removed a reviewer: Restricted Project.Sep 2 2021, 10:11 AM
ldionne added a subscriber: ldionne.

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

This revision is now accepted and ready to land.Sep 2 2021, 10:11 AM
eugenis added inline comments.Sep 2 2021, 10:23 AM
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 added inline comments.Sep 2 2021, 12:57 PM
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 added inline comments.Sep 2 2021, 1:02 PM
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 added inline comments.Sep 2 2021, 1:12 PM
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.

ZijunZhao updated this revision to Diff 372373.Sep 13 2021, 5:56 PM
vitalybuka requested changes to this revision.Sep 14 2021, 11:09 AM
vitalybuka added inline comments.
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.

This revision now requires changes to proceed.Sep 14 2021, 11:09 AM
vitalybuka added inline comments.Sep 15 2021, 10:36 PM
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")

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?

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?

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.

ZijunZhao updated this revision to Diff 373328.Sep 17 2021, 2:02 PM
ZijunZhao marked 4 inline comments as done.
danalbert accepted this revision.Sep 17 2021, 2:10 PM

LGTM but we should wait for @vitalybuka.

vitalybuka added inline comments.Sep 20 2021, 8:14 AM
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
ZijunZhao updated this revision to Diff 373713.Sep 20 2021, 2:31 PM
vitalybuka accepted this revision.Sep 20 2021, 3:49 PM

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

This revision is now accepted and ready to land.Sep 20 2021, 3:49 PM
vitalybuka added inline comments.Sep 20 2021, 4:52 PM
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 added inline comments.Sep 20 2021, 4:57 PM
compiler-rt/test/tsan/CMakeLists.txt
19 ↗(On Diff #373713)
141–145 ↗(On Diff #373713)
ZijunZhao updated this revision to Diff 375448.Sep 27 2021, 6:01 PM
ZijunZhao marked 4 inline comments as done.
danalbert requested changes to this revision.Sep 28 2021, 1:49 PM
danalbert added inline comments.
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.

This revision now requires changes to proceed.Sep 28 2021, 1:49 PM
ZijunZhao marked 13 inline comments as done.Sep 29 2021, 10:41 AM
ZijunZhao added inline comments.
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.

ZijunZhao marked 2 inline comments as done.
danalbert accepted this revision.Sep 29 2021, 2:08 PM

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

This revision is now accepted and ready to land.Sep 29 2021, 2:08 PM
vitalybuka accepted this revision.Sep 30 2021, 11:41 AM

Thank you. LGTM

compiler-rt/test/tsan/CMakeLists.txt
64–66 ↗(On Diff #375967)
ZijunZhao edited the summary of this revision. (Show Details)Sep 30 2021, 11:52 AM
ZijunZhao edited the summary of this revision. (Show Details)Sep 30 2021, 12:05 PM
ZijunZhao updated this revision to Diff 384915.Nov 4 2021, 5:15 PM

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

danalbert accepted this revision.Nov 8 2021, 12:05 PM

Fixes LGTM.

ZijunZhao updated this revision to Diff 391520.Dec 2 2021, 6:11 PM
ZijunZhao edited the summary of this revision. (Show Details)
ZijunZhao added a reviewer: jamesfarrell.
ZijunZhao updated this revision to Diff 391524.Dec 2 2021, 6:41 PM
melver added a subscriber: melver.Dec 3 2021, 7:43 AM

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.

Hi Marco,

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

Best regards,
Zijun Zhao

danalbert requested changes to this revision.Jan 10 2022, 12:37 PM

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.

This revision now requires changes to proceed.Jan 10 2022, 12:37 PM
ZijunZhao updated this revision to Diff 401061.Jan 18 2022, 5:34 PM
ZijunZhao edited the summary of this revision. (Show Details)

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

danalbert added inline comments.Jan 18 2022, 6:17 PM
compiler-rt/cmake/config-ix.cmake
754 ↗(On Diff #401061)

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)

remove stray whitespace

ZijunZhao marked an inline comment as done.Jan 19 2022, 10:14 AM

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

danalbert added inline comments.Jan 19 2022, 12:55 PM
compiler-rt/cmake/config-ix.cmake
753–754 ↗(On Diff #401313)

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

danalbert accepted this revision.Jan 19 2022, 4:13 PM
danalbert added inline comments.
compiler-rt/cmake/config-ix.cmake
753–754 ↗(On Diff #401313)

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.

This revision is now accepted and ready to land.Jan 19 2022, 4:13 PM
This revision was automatically updated to reflect the committed changes.