This is an archive of the discontinued LLVM Phabricator instance.

Differential D139652

Add the thread sanitizer support for X86_64 WatchOS simulators
ClosedPublic

Authored by yfyang on Dec 8 2022, 10:22 AM.

Details

Reviewers
jyknight
yln
Commits
rG0073fd8d0dda: Add the thread sanitizer support for X86_64 WatchOS simulators
Summary

Allow TSan in clang driver for X86_64 WatchOS simulator.

It was already functional, and Apple's downstream fork of clang allows it, but that change had not made it upstream yet.

Diff Detail

Event Timeline

yfyang created this revision.Dec 8 2022, 10:22 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 8 2022, 10:22 AM
Herald added a subscriber: pengfei. · View Herald Transcript
yfyang requested review of this revision.Dec 8 2022, 10:22 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 8 2022, 10:22 AM
Herald added subscribers: cfe-commits, MaskRay. · View Herald Transcript
jyknight accepted this revision.Dec 8 2022, 10:39 AM
jyknight added a subscriber: yln.

Thanks for the change.

The description is a bit misleading, this would be better:

Allow TSan in clang driver for X86_64 WatchOS simulator.

It was already functional, and Apple's downstream fork of clang allows it, but that change had not made it upstream yet.

cc+=yln FYI, though this change seems obvious enough I'm comfortable to approve it directly.

This revision is now accepted and ready to land.Dec 8 2022, 10:41 AM
yfyang edited the summary of this revision. (Show Details)Dec 8 2022, 10:43 AM
yln accepted this revision.EditedDec 8 2022, 4:09 PM
yln added a subscriber: rsundahl.

Thank you! LGTM, with one nit.

@rsundahl: can you keep an eye on this and resolve any potential Apple-internal conflicts? Thanks!

clang/lib/Driver/ToolChains/Darwin.cpp
3279–3284

Yes, for watchOS simulator. I think we can also drop the constraint on x86_64 and then merge the if. TSan should be supported for simulators on Apple Silicon. Can you add additional tests for these 3 cases? Thanks!

Harbormaster completed remote builds in B202022: Diff 481353.Dec 8 2022, 5:25 PM
rsundahl added a comment.Dec 9 2022, 7:35 AM
In D139652#3983079, @yln wrote:

@rsundahl: can you keep an eye on this and resolve any potential Apple-internal conflicts? Thanks!

Will do @yln.

yfyang updated this revision to Diff 481679.Dec 9 2022, 10:10 AM

Addressing comments.

Harbormaster completed remote builds in B202256: Diff 481679.Dec 9 2022, 10:11 AM
yfyang updated this revision to Diff 481682.Dec 9 2022, 10:18 AM

Formatting

Harbormaster completed remote builds in B202258: Diff 481682.Dec 9 2022, 10:18 AM
yfyang updated this revision to Diff 481684.Dec 9 2022, 10:25 AM
  • Add TSAN support for Apple arm64 simulators
  • Formatting
This revision was landed with ongoing or failed builds.Dec 9 2022, 10:33 AM
Closed by commit rG0073fd8d0dda: Add the thread sanitizer support for X86_64 WatchOS simulators (authored by yfyang, committed by oontvoo). · Explain Why
This revision was automatically updated to reflect the committed changes.
oontvoo added a commit: rG0073fd8d0dda: Add the thread sanitizer support for X86_64 WatchOS simulators.
Harbormaster completed remote builds in B202261: Diff 481684.Dec 9 2022, 11:22 AM

Revision Contents

PathSize
clang/
lib/
Driver/
ToolChains/
6 lines

Diff 481682

clang/lib/Driver/ToolChains/Darwin.cpp

//===--- Darwin.cpp - Darwin Tool and ToolChain Implementations -*- C++ -*-===// //===--- Darwin.cpp - Darwin Tool and ToolChain Implementations -*- C++ -*-===//
Lint: Lint
Inline Actions

clang-format not found in user’s local PATH; not linting file.

Lint: Lint: clang-format not found in user’s local PATH; not linting file.
// //
// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
// See https://llvm.org/LICENSE.txt for license information. // See https://llvm.org/LICENSE.txt for license information.
// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
// //
//===----------------------------------------------------------------------===// //===----------------------------------------------------------------------===//
▲ Show 20 Lines • Show All 3,262 Lines • ▼ Show 20 LinesSanitizerMask Darwin::getSupportedSanitizers() const {
Res |= SanitizerKind::ObjCCast; Res |= SanitizerKind::ObjCCast;
// Prior to 10.9, macOS shipped a version of the C++ standard library without // Prior to 10.9, macOS shipped a version of the C++ standard library without
// C++11 support. The same is true of iOS prior to version 5. These OS'es are // C++11 support. The same is true of iOS prior to version 5. These OS'es are
// incompatible with -fsanitize=vptr. // incompatible with -fsanitize=vptr.
if (!(isTargetMacOSBased() && isMacosxVersionLT(10, 9)) && if (!(isTargetMacOSBased() && isMacosxVersionLT(10, 9)) &&
!(isTargetIPhoneOS() && isIPhoneOSVersionLT(5, 0))) !(isTargetIPhoneOS() && isIPhoneOSVersionLT(5, 0)))
Res |= SanitizerKind::Vptr; Res |= SanitizerKind::Vptr;
if ((IsX86_64 || IsAArch64) && (isTargetMacOSBased() || if ((IsX86_64 || IsAArch64) &&
isTargetIOSSimulator() || isTargetTvOSSimulator() || (isTargetMacOSBased() || isTargetIOSSimulator() ||
isTargetWatchOSSimulator()) { isTargetTvOSSimulator() || isTargetWatchOSSimulator())) {
Res |= SanitizerKind::Thread; Res |= SanitizerKind::Thread;
} }
ylnUnsubmitted
Not Done
ReplyInline Actions
Res |= SanitizerKind::Vptr;
- if ((IsX86_64 || IsAArch64) && isTargetMacOSBased()) {
- Res |= SanitizerKind::Thread;
- } else if (isTargetIOSSimulator() || isTargetTvOSSimulator() ||
- isTargetWatchOSSimulator()) {
- if (IsX86_64)
- Res |= SanitizerKind::Thread;
+ if ((IsX86_64 || IsAArch64) &&
+ (isTargetMacOSBased() || isTargetIOSSimulator() ||
+ isTargetTvOSSimulator() || isTargetWatchOSSimulator())) {
+ Res |= SanitizerKind::Thread;
}
return Res;

Yes, for watchOS simulator. I think we can also drop the constraint on x86_64 and then merge the if. TSan should be supported for simulators on Apple Silicon. Can you add additional tests for these 3 cases? Thanks!

yln: Yes, for watchOS simulator. I think we can also drop the constraint on x86_64 and then merge…
return Res; return Res;
} }
void Darwin::printVerboseInfo(raw_ostream &OS) const { void Darwin::printVerboseInfo(raw_ostream &OS) const {
CudaInstallation.print(OS); CudaInstallation.print(OS);
RocmInstallation.print(OS); RocmInstallation.print(OS);
} }