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

[dmaclach]

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.

[yln]

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]

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]

Addressed comments. PTAL

[dmaclach]

b/llvm/llvm-project/clang/lib/Driver/ToolChains/Darwin.cpp

1439 ↗

(On Diff #499961)

Rewritten with placeholder.

[trybka]

LGTM, but I will defer to Usama as well.

[usama54321]

LGTM

[usama54321]

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

[dmaclach]

In D144672#4151146, @usama54321 wrote:

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 with buildable patch.

[usama54321]

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

[dmaclach]

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

[zixuw]

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]

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

[dmaclach]

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

[usama54321]

I have reverted the commit for now

[dmaclach]

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.

[yln]

compiler-rt/test/asan/TestCases/replaceable_new_delete_static.cpp

10–11

This should work, right?

[dmaclach]

compiler-rt/test/asan/TestCases/replaceable_new_delete_static.cpp

10–11

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

[zixuw]

compiler-rt/test/asan/TestCases/replaceable_new_delete_static.cpp

10–11

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]

compiler-rt/test/asan/TestCases/replaceable_new_delete_static.cpp

10–11

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

[usama54321]

compiler-rt/test/asan/TestCases/replaceable_new_delete_static.cpp

10–11

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]

Went with unsupported.

[yln]

compiler-rt/test/asan/TestCases/replaceable_new_delete_static.cpp

10–11

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]

compiler-rt/test/asan/TestCases/replaceable_new_delete_static.cpp

10–11

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]

compiler-rt/test/asan/TestCases/replaceable_new_delete_static.cpp

10–11

[...] 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]

Moved to REQUIRES: asan-static-runtime

clang/include/clang/Basic/DiagnosticDriverKinds.td

 def err_drv_malformed_sanitizer_ignorelist : Error<
   "malformed sanitizer ignorelist: '%0'">;
 def err_drv_malformed_sanitizer_coverage_allowlist : Error<
   "malformed sanitizer coverage allowlist: '%0'">;
 def err_drv_malformed_sanitizer_coverage_ignorelist : Error<
   "malformed sanitizer coverage ignorelist: '%0'">;
 def err_drv_malformed_sanitizer_metadata_ignorelist : Error<
   "malformed sanitizer metadata ignorelist: '%0'">;
-def err_drv_unsupported_static_ubsan_darwin : Error<
-  "static UndefinedBehaviorSanitizer runtime is not supported on darwin">;
+def err_drv_unsupported_static_sanitizer_darwin : Error<
+  "static %0 runtime is not supported on darwin">;
 def err_drv_duplicate_config : Error<
   "no more than one option '--config' is allowed">;
 def err_drv_cannot_open_config_file : Error<
   "configuration file '%0' cannot be opened: %1">;
 def err_drv_config_file_not_found : Error<
   "configuration file '%0' cannot be found">;
 def note_drv_config_file_searched_in : Note<
   "was searched for in the directory: %0">;

clang/include/clang/Driver/Options.td

defm apple_pragma_pack : BoolFOption<"apple-pragma-pack",
   NegFlag<SetFalse>>;
 defm xl_pragma_pack : BoolFOption<"xl-pragma-pack",
   LangOpts<"XLPragmaPack">, DefaultFalse,
   PosFlag<SetTrue, [CC1Option], "Enable IBM XL #pragma pack handling">,
   NegFlag<SetFalse>>;
 def shared_libsan : Flag<["-"], "shared-libsan">,
   HelpText<"Dynamically link the sanitizer runtime">;
 def static_libsan : Flag<["-"], "static-libsan">,
-  HelpText<"Statically link the sanitizer runtime">;
+  HelpText<"Statically link the sanitizer runtime (Not supported for ASan, TSan or UBSan on darwin)">;
 def : Flag<["-"], "shared-libasan">, Alias<shared_libsan>;
 def fasm : Flag<["-"], "fasm">, Group<f_Group>;
 
 def fassume_sane_operator_new : Flag<["-"], "fassume-sane-operator-new">, Group<f_Group>;
 def fastcp : Flag<["-"], "fastcp">, Group<f_Group>;
 def fastf : Flag<["-"], "fastf">, Group<f_Group>;
 def fast : Flag<["-"], "fast">, Group<f_Group>;
 def fasynchronous_unwind_tables : Flag<["-"], "fasynchronous-unwind-tables">, Group<f_Group>;

clang/lib/Driver/ToolChains/Darwin.cpp

void DarwinClang::AddLinkRuntimeLibArgs(const ArgList &Args,
   // something like libstdc++, and needs its runtime support routines.
   if (const Arg *A = Args.getLastArg(options::OPT_static_libgcc)) {
     getDriver().Diag(diag::err_drv_unsupported_opt) << A->getAsString(Args);
     return;
   }
 
   const SanitizerArgs &Sanitize = getSanitizerArgs(Args);
 
-  if (!Sanitize.needsSharedRt() && Sanitize.needsUbsanRt()) {
-    getDriver().Diag(diag::err_drv_unsupported_static_ubsan_darwin);
+  if (!Sanitize.needsSharedRt()) {
+    const char *sanitizer = nullptr;
+    if (Sanitize.needsUbsanRt()) {
+      sanitizer = "UndefinedBehaviorSanitizer";
+    } else if (Sanitize.needsAsanRt()) {
+      sanitizer = "AddressSanitizer";
+    } else if (Sanitize.needsTsanRt()) {
+      sanitizer = "ThreadSanitizer";
+    }
+    if (sanitizer) {
+      getDriver().Diag(diag::err_drv_unsupported_static_sanitizer_darwin)
+          << sanitizer;
       return;
     }
+  }
 
   if (Sanitize.linkRuntimes()) {
-    if (Sanitize.needsAsanRt())
+    if (Sanitize.needsAsanRt()) {
+      assert(Sanitize.needsSharedRt() &&
+             "Static sanitizer runtimes not supported");
       AddLinkSanitizerLibArgs(Args, CmdArgs, "asan");
+    }
     if (Sanitize.needsLsanRt())
       AddLinkSanitizerLibArgs(Args, CmdArgs, "lsan");
     if (Sanitize.needsUbsanRt()) {
-      assert(Sanitize.needsSharedRt() && "Static sanitizer runtimes not supported");
-      AddLinkSanitizerLibArgs(Args, CmdArgs,
-                              Sanitize.requiresMinimalRuntime() ? "ubsan_minimal"
-                                                                : "ubsan");
-    }
-    if (Sanitize.needsTsanRt())
+      assert(Sanitize.needsSharedRt() &&
+             "Static sanitizer runtimes not supported");
+      AddLinkSanitizerLibArgs(
+          Args, CmdArgs,
+          Sanitize.requiresMinimalRuntime() ? "ubsan_minimal" : "ubsan");
+    }
+    if (Sanitize.needsTsanRt()) {
+      assert(Sanitize.needsSharedRt() &&
+             "Static sanitizer runtimes not supported");
       AddLinkSanitizerLibArgs(Args, CmdArgs, "tsan");
+    }
     if (Sanitize.needsFuzzer() && !Args.hasArg(options::OPT_dynamiclib)) {
       AddLinkSanitizerLibArgs(Args, CmdArgs, "fuzzer", /*shared=*/false);
 
         // Libfuzzer is written in C++ and requires libcxx.
         AddCXXStdlibLibArgs(Args, CmdArgs);
     }
     if (Sanitize.needsStatsRt()) {
       AddLinkRuntimeLib(Args, CmdArgs, "stats_client", RLO_AlwaysLink);

clang/test/Driver/sanitizer-ld.c

 // CHECK-UBSAN-MINIMAL-DARWIN: "{{.*}}libclang_rt.ubsan_minimal_osx_dynamic.dylib"
 
 // RUN: %clang -fsanitize=undefined -### %s 2>&1 \
 // RUN:     --target=x86_64-apple-darwin -fuse-ld=ld -static-libsan \
 // RUN:     --sysroot=%S/Inputs/basic_linux_tree \
 // RUN:   | FileCheck --check-prefix=CHECK-UBSAN-STATIC-DARWIN %s
 // CHECK-UBSAN-STATIC-DARWIN: {{.*}}error: static UndefinedBehaviorSanitizer runtime is not supported on darwin
 
+// RUN: %clang -fsanitize=address -### %s 2>&1 \
+// RUN:     --target=x86_64-apple-darwin -fuse-ld=ld -static-libsan \
+// RUN:     --sysroot=%S/Inputs/basic_linux_tree \
+// RUN:   | FileCheck --check-prefix=CHECK-ASAN-STATIC-DARWIN %s
+// CHECK-ASAN-STATIC-DARWIN: {{.*}}error: static AddressSanitizer runtime is not supported on darwin
+
+// RUN: %clang -fsanitize=thread -### %s 2>&1 \
+// RUN:     --target=x86_64-apple-darwin -fuse-ld=ld -static-libsan \
+// RUN:     --sysroot=%S/Inputs/basic_linux_tree \
+// RUN:   | FileCheck --check-prefix=CHECK-TSAN-STATIC-DARWIN %s
+// CHECK-TSAN-STATIC-DARWIN: {{.*}}error: static ThreadSanitizer runtime is not supported on darwin
+
 // RUN: %clang -fsanitize=address,undefined -### %s 2>&1 \
 // RUN:     --target=i386-unknown-linux -fuse-ld=ld \
 // RUN:     -resource-dir=%S/Inputs/resource_dir \
 // RUN:     --sysroot=%S/Inputs/basic_linux_tree \
 // RUN:   | FileCheck --check-prefix=CHECK-ASAN-UBSAN-LINUX %s
 // CHECK-ASAN-UBSAN-LINUX: "{{.*}}ld{{(.exe)?}}"
 // CHECK-ASAN-UBSAN-LINUX: "--whole-archive" "{{.*}}libclang_rt.asan-i386.a" "--no-whole-archive"
 // CHECK-ASAN-UBSAN-LINUX-NOT: libclang_rt.ubsan

compiler-rt/test/asan/TestCases/replaceable_new_delete_shared.cpp

-// Ensure that operator new/delete are still replaceable.
+// Ensure that operator new/delete are still replaceable using shared-libsan.
 
 // FIXME: Weak symbols aren't supported on Windows, although some code in
 // compiler-rt already exists to solve this problem. We should probably define
 // the new/delete interceptors as "weak" using those workarounds as well.
 // UNSUPPORTED: target={{.*windows.*}}
 
 // RUN: %clangxx %s -o %t -fsanitize=address -shared-libsan && not %run %t 2>&1 | FileCheck %s
-// RUN: %clangxx %s -o %t -fsanitize=address -static-libsan && not %run %t 2>&1 | FileCheck %s
 
 #include <cstdio>
 #include <cstdlib>
 #include <new>
 
 void *operator new[](size_t size) {
   fprintf(stderr, "replaced new\n");
   return malloc(size);

compiler-rt/test/asan/TestCases/replaceable_new_delete_static.cpp

-// Ensure that operator new/delete are still replaceable.
+// Ensure that operator new/delete are still replaceable using static-libsan.
 
 // FIXME: Weak symbols aren't supported on Windows, although some code in
 // compiler-rt already exists to solve this problem. We should probably define
 // the new/delete interceptors as "weak" using those workarounds as well.
 // UNSUPPORTED: target={{.*windows.*}}
 
-// RUN: %clangxx %s -o %t -fsanitize=address -shared-libsan && not %run %t 2>&1 | FileCheck %s
+// darwin only supports `shared-libsan`.
+// REQUIRES: asan-static-runtime
+
 // RUN: %clangxx %s -o %t -fsanitize=address -static-libsan && not %run %t 2>&1 | FileCheck %s
 
 #include <cstdio>
 #include <cstdlib>
 #include <new>
 
 void *operator new[](size_t size) {
   fprintf(stderr, "replaced new\n");
   return malloc(size);