This is an archive of the discontinued LLVM Phabricator instance.

[Sanitizers] Error when attempting to use `static-lsan` with `TSan` or `Asan` on darwin
ClosedPublic

Authored by dmaclach on Feb 23 2023, 1:02 PM.

Details

Summary

darwin platforms do not support static-lsan with TSan or ASan but were "silently failing" in that they would just ignore the flag and link in the dynamic runtimes instead.

This matches the pre-existing UBSan failure path.

Diff Detail

Event Timeline

dmaclach created this revision.Feb 23 2023, 1:02 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 23 2023, 1:02 PM
dmaclach requested review of this revision.Feb 23 2023, 1:02 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 23 2023, 1:02 PM
yln added a comment.Feb 23 2023, 2:14 PM

Looks like this is a follow-up to D141550.

LGTM, but please wait for a sign-off from Usama.
@usama54321 please confirm that static runtimes really aren't supported for ASan&TSan (not just UBSan).

b/llvm/llvm-project/clang/include/clang/Basic/DiagnosticDriverKinds.td
228 ↗(On Diff #499961)

Should we generalize this to "static sanitizer runtimes are not supported on Apple platforms" to avoid needing an error diagnostic for every tool?

b/llvm/llvm-project/clang/lib/Driver/ToolChains/Darwin.cpp
1431–1432 ↗(On Diff #499961)

Needs updated indentation.

1439 ↗(On Diff #499961)

This would become less code if we are okay with not repeating the tool name in the error message.

usama54321 added inline comments.Feb 23 2023, 2:32 PM
b/llvm/llvm-project/clang/include/clang/Basic/DiagnosticDriverKinds.td
228 ↗(On Diff #499961)

I would suggest replacing the name with a placeholder so we only need one definition here.

b/llvm/llvm-project/clang/include/clang/Driver/Options.td
1218 ↗(On Diff #499961)

I see asan capitalized as ASan in another help text in this file. I will suggest conforming the capitalization accordingly for the 3 names. Also, I'd suggest changing Apple platforms to Darwin platforms or just darwin.

dmaclach updated this revision to Diff 500231.Feb 24 2023, 8:59 AM

Addressed comments. PTAL

dmaclach marked 5 inline comments as done.Feb 24 2023, 9:01 AM
dmaclach added inline comments.
b/llvm/llvm-project/clang/lib/Driver/ToolChains/Darwin.cpp
1439 ↗(On Diff #499961)

Rewritten with placeholder.

dmaclach retitled this revision from [Sanitizers] Error when attempting to use `static-lsan` with `tsan` or `asan` on darwin to [Sanitizers] Error when attempting to use `static-lsan` with `TSan` or `Asan` on darwin.Feb 24 2023, 9:02 AM
dmaclach edited the summary of this revision. (Show Details)

LGTM, but I will defer to Usama as well.

usama54321 accepted this revision.Feb 24 2023, 11:02 AM
This revision is now accepted and ready to land.Feb 24 2023, 11:02 AM
usama54321 requested changes to this revision.Feb 24 2023, 11:22 AM

Sorry for this. I need to confirm that asan and tsan are not supported on darwin. Please don't merge until then. Thanks

This revision now requires changes to proceed.Feb 24 2023, 11:22 AM
dmaclach marked an inline comment as done.Feb 24 2023, 3:35 PM

Sorry for this. I need to confirm that asan and tsan are not supported on darwin. Please don't merge until then. Thanks

Probably being pedantic, but note that asan and tsan *are* supported on darwin as shared libraries, just not as static libraries. If you take a look at the llvm distro, libclang_rt.asan_osx only ships as a dylib.

dmaclach updated this revision to Diff 500942.Feb 27 2023, 3:28 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 27 2023, 3:28 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript

Updated with buildable patch.

usama54321 accepted this revision.Feb 28 2023, 11:37 AM

I was verifying that static versions of these libraries are not present on darwin. Sorry for the delay.

This revision is now accepted and ready to land.Feb 28 2023, 11:37 AM
dmaclach accepted this revision.Mar 1 2023, 10:16 AM

@usama54321 or @yln are you able to commit for me?

This revision was landed with ongoing or failed builds.Mar 1 2023, 11:07 AM
This revision was automatically updated to reflect the committed changes.
zixuw added a subscriber: zixuw.Mar 1 2023, 3:26 PM

Hi! This broke the asan replaceable_new_delete.cpp test on Darwin because it has a run line using -static-libsan. Could you take a look? Probably need to separate that check out and mark as unsupported on Darwin

usama54321 reopened this revision.Mar 1 2023, 4:38 PM

@dmaclach Can you please take a look and fix this? Thanks

This revision is now accepted and ready to land.Mar 1 2023, 4:38 PM

Yep. Apologies. Been a long time since I committed anything to LLVM. I'll try and take a look tonight/first thing tomorrow.

I have reverted the commit for now

dmaclach updated this revision to Diff 501899.Mar 2 2023, 9:40 AM

Updated with fixed tests for replaceable_new_delete.cpp.

Split replaceable_new_delete.cpp into replaceable_new_delete_shared.cpp and replaceable_new_delete_static.cpp.
Static is marked as failing on darwin.

Herald added subscribers: Restricted Project, Enna1. · View Herald TranscriptMar 2 2023, 9:40 AM
yln added inline comments.Mar 2 2023, 3:03 PM
compiler-rt/test/asan/TestCases/replaceable_new_delete_static.cpp
10–11 ↗(On Diff #501899)

This should work, right?

dmaclach added inline comments.Mar 2 2023, 3:31 PM
compiler-rt/test/asan/TestCases/replaceable_new_delete_static.cpp
10–11 ↗(On Diff #501899)

No.. darwin should fail with the -static-libsan flag. This is the test that was failing and caused the rollback.

zixuw added inline comments.Mar 2 2023, 3:34 PM
compiler-rt/test/asan/TestCases/replaceable_new_delete_static.cpp
10–11 ↗(On Diff #501899)

I think @yln is suggesting using REQUIRES: asan-static-runtime instead of XFAIL: darwin. I wasn't aware of that conditional but yeah that should be better if it works.

yln added inline comments.Mar 2 2023, 3:35 PM
compiler-rt/test/asan/TestCases/replaceable_new_delete_static.cpp
10–11 ↗(On Diff #501899)

I meant using // REQUIRES: asan-static-runtime instead of XFAIL: darwin since it seems that we already have a lit feature for it.

usama54321 added inline comments.Mar 3 2023, 4:24 AM
compiler-rt/test/asan/TestCases/replaceable_new_delete_static.cpp
10–11 ↗(On Diff #501899)

I think UNSUPPORTED: darwin makes the most sense here. I don't think lit understands that REQUIRES: asan-static-runtime should result in skipping the test on Darwin as it does not know about this dependency.

dmaclach updated this revision to Diff 502138.Mar 3 2023, 8:10 AM
dmaclach marked an inline comment as done.

Went with unsupported.

yln added inline comments.Mar 3 2023, 9:04 AM
compiler-rt/test/asan/TestCases/replaceable_new_delete_static.cpp
10–11 ↗(On Diff #501899)

I don't think lit understands that REQUIRES: asan-static-runtime should result in skipping the test on Darwin as it does not know about this dependency.

Actually, this was exactly my point. We have other tests already marked with REQUIRES: asan-static-runtime and we should double check our changes don't affect these as well.

If LIT doesn't model this dependency yet, then we should make sure it does! And this test can act as a good "canary in the coal mine".

Please use REQUIRES: asan-static-runtime and make sure we understand and deal with any fallout.

usama54321 added inline comments.Mar 3 2023, 10:33 AM
compiler-rt/test/asan/TestCases/replaceable_new_delete_static.cpp
10–11 ↗(On Diff #501899)

Sorry for my earlier comment. @yln is correct, and we should use REQUIRES: asan-static-runtime. I double checked, and this already works as expected on darwin, i.e. these tests are unsupported on darwin. So you should not need to do anything apart from adding the REQUIRES in the test.

yln accepted this revision.Mar 3 2023, 2:18 PM
yln added inline comments.
compiler-rt/test/asan/TestCases/replaceable_new_delete_static.cpp
10–11 ↗(On Diff #501899)

[...] I double checked, and this already works as expected on darwin, i.e. these tests are unsupported on darwin.

Thanks Usama!

Patch LGTM assuming we switch to using REQUIRES: asan-static-runtime. Thanks for seeing this through! :)

dmaclach updated this revision to Diff 502474.Mar 5 2023, 2:17 PM

Moved to REQUIRES: asan-static-runtime

usama54321 accepted this revision.Mar 6 2023, 3:03 AM
This revision was automatically updated to reflect the committed changes.