[PATCH] D36810: Minimal runtime for UBSan.

[Vedant Kumar via Phabricator via llvm-commits]

vsk added inline comments.


================
Comment at: compiler-rt/cmake/config-ix.cmake:549
+if (COMPILER_RT_HAS_SANITIZER_COMMON AND UBSAN_SUPPORTED_ARCH AND
+    OS_NAME MATCHES "Linux|FreeBSD|NetBSD|Android")
+  set(COMPILER_RT_HAS_UBSAN_MINIMAL TRUE)
----------------
I think all of this works on Darwin without issue.


================
Comment at: compiler-rt/lib/ubsan_minimal/CMakeLists.txt:17
+
+if(APPLE)
+
----------------
This 'if Apple' branch doesn't seem necessary. We're not doing any add_weak_symbols() magic, because there's no ___ubsan_default_options.


================
Comment at: compiler-rt/lib/ubsan_minimal/ubsan_minimal_handlers.cc:45
+#else
+static void abort_with_message(const char *unused) { abort(); }
+#endif
----------------
Leave out the parameter name to avoid "Wunused"-style diagnostics?


================
Comment at: compiler-rt/lib/ubsan_minimal/ubsan_minimal_handlers.cc:53
+    if (!report_this_error(__builtin_return_address(0))) return; \
+    message("ubsan: " msg "\n");                                 \
+  }                                                              \
----------------
Printing out the return address here would aid debugging. It's extra complexity but would be worth it when triaging bugs, or when using the minimal runtime in low-level software.


================
Comment at: compiler-rt/test/ubsan_minimal/lit.common.cfg:32
+# Check that the host supports UndefinedBehaviorSanitizerMinimal tests
+if config.host_os not in ['Linux', 'FreeBSD', 'NetBSD']: # TODO: 'Darwin', 'Windows'
+  config.unsupported = True
----------------
We should be able to just include Darwin here too.


https://reviews.llvm.org/D36810