Page MenuHomePhabricator

[TSan][Darwin] Remove unnecessary lit substitution
ClosedPublic

Authored by yln on Aug 11 2020, 6:28 PM.

Details

Summary

We don't test on very old versions of Apple platforms anymore. This lit
substitution concerning the minimum deployment target for ARC support
can be safely removed.

%darwin_min_target_with_full_runtime_arc_support -> 10.11

Diff Detail

Event Timeline

yln created this revision.Aug 11 2020, 6:28 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 11 2020, 6:28 PM
Herald added a subscriber: Restricted Project. · View Herald Transcript
yln requested review of this revision.Aug 11 2020, 6:28 PM

@yln Are you sure this actually works for all platforms, in particular the watchOS simulator? The code you link (https://github.com/llvm/llvm-project/blob/master/compiler-rt/lib/tsan/CMakeLists.txt#L122) is not the minimum deploy target. That code just checks the SDK version used to build which is different from the minimum deploy target used.

The minimum deployment target for macOS is actually 10.10 I think (https://github.com/llvm/llvm-project/blob/master/compiler-rt/cmake/config-ix.cmake#L400). Other code sets the minimum deployment targets (https://github.com/llvm/llvm-project/blob/master/compiler-rt/cmake/config-ix.cmake#L363) for other platforms.

yln added a comment.Aug 13 2020, 4:56 PM

@yln Are you sure this actually works for all platforms, in particular the watchOS simulator? The code you link (https://github.com/llvm/llvm-project/blob/master/compiler-rt/lib/tsan/CMakeLists.txt#L122) is not the minimum deploy target. That code just checks the SDK version used to build which is different from the minimum deploy target used.

The minimum deployment target for macOS is actually 10.10 I think (https://github.com/llvm/llvm-project/blob/master/compiler-rt/cmake/config-ix.cmake#L400). Other code sets the minimum deployment targets (https://github.com/llvm/llvm-project/blob/master/compiler-rt/cmake/config-ix.cmake#L363) for other platforms.

You are completely right. The minimum deployment target for sanitizers is 10.10. For TSan, we require SDK 10.12+ to build. I confused myself here when I was writing the description for the patch. Let me try again:

The purpose of this patch is to cleanup our tests and deployment target lit substitutions as much as possible under the assumption that test execution itself does not happen on these very old versions anymore. We don't want to do anything that would intentionally break these old versions or raise the deployment target without justification, but testing them is not a priority anymore (we have stopped to do so), so our testing infrastructure doesn't need to support this anymore.

I will run ninja check-tsan check-tsan-iossim-x86_64 check-tsan-watchossim-x86_64 and adapt this patch accordingly.

yln updated this revision to Diff 285533.Aug 13 2020, 5:34 PM

Keep %darwin_min_target_with_tls_support -> 10.12. TLS requires watchOS 3+ simulator.

yln retitled this revision from [TSan][Darwin] Remove obsolete lit substitutions to [TSan][Darwin] Remove unnecessary lit substitution.Aug 13 2020, 5:35 PM
yln edited the summary of this revision. (Show Details)
delcypher accepted this revision.Aug 20 2020, 12:20 PM
This revision is now accepted and ready to land.Aug 20 2020, 12:20 PM
This revision was automatically updated to reflect the committed changes.