[PATCH] D141550: [CompilerRT] Remove ubsan static runtime on Apple

[Dan Liew via Phabricator via cfe-commits]

delcypher added a comment.

Overall approach LGTM. I just have some very minor nits.



================
Comment at: clang/include/clang/Basic/DiagnosticDriverKinds.td:219
+def err_drv_unsupported_static_ubsan_darwin : Error<
+  "Static UBSan runtime is not supported on darwin">;
 def err_drv_duplicate_config : Error<
----------------
Nit: Driver messages start with lowercase.

Also please check if "UBSan" is spelt differently in existing driver messages. It might actually be written more explicitly like "undefined behavior sanitizer".


================
Comment at: clang/lib/Driver/ToolChains/Darwin.cpp:1441
                             Sanitize.requiresMinimalRuntime() ? "ubsan_minimal"
-                                                              : "ubsan",
-                            Sanitize.needsSharedRt());
+                                                              : "ubsan");
   if (Sanitize.needsTsanRt())
----------------
Maybe add an assert? As the code is constructed right now it should never fail but in the future someone could change the code and break the assumption.

```lang=c++
  if (Sanitize.needsUbsanRt()) {
    assert(Sanitize.needsSharedRt() && "Static sanitizer runtimes not supported");
    AddLinkSanitizerLibArgs(Args, CmdArgs,
                            Sanitize.requiresMinimalRuntime() ? "ubsan_minimal"
                                                              : "ubsan");
}

```


================
Comment at: compiler-rt/lib/ubsan/CMakeLists.txt:118
+    if (NOT APPLE)
+      add_compiler_rt_runtime(clang_rt.ubsan
+        STATIC
----------------
I think you may have accidentally added tabs here when re-indenting.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D141550/new/

https://reviews.llvm.org/D141550