[PATCH] D71491: [ubsan] Check implicit casts in ObjC for-in statements

[Vedant Kumar via Phabricator via cfe-commits]

vsk added inline comments.


================
Comment at: clang/lib/Driver/ToolChain.cpp:953
+      SanitizerKind::ImplicitConversion | SanitizerKind::Nullability |
+      SanitizerKind::LocalBounds | SanitizerKind::ObjCCast;
   if (getTriple().getArch() == llvm::Triple::x86 ||
----------------
delcypher wrote:
> `SanitizerKind::ObjCCast` doesn't seem to fit the comment above. It is platform dependent (only really works for Apple platforms) and it **does** require runtime support (i.e. the Objective-C runtime).
The runtime dependency is optional, and there's nothing inherently Apple-specific about this check. However, perhaps it's best not to inadvertently advertise support for the GNU objc runtime before it's in tree.


================
Comment at: compiler-rt/lib/ubsan/ubsan_value.cpp:29
+const char *__ubsan::getObjCClassName(ValueHandle Pointer) {
+#if defined(__APPLE__)
+  // We need to query the ObjC runtime for some information, but do not want
----------------
delcypher wrote:
> The compiler-rt codebase tends to use `SANITIZER_MAC` macro (defined to be 1 if Apple otherwise it's 0) rather than `__APPLE__`.
I see. That seems problematic, as it makes it tricky to add macOS-specific (or iOS-specific) functionality down the road. I don't think SANITIZER_MAC should be defined unless TARGET_OS_MACOS is true.


================
Comment at: compiler-rt/lib/ubsan/ubsan_value.cpp:37
+
+  if (!AttemptedDlopen) {
+    ObjCHandle = dlopen(
----------------
delcypher wrote:
> You might want some sort of lock here (or atomic variable) to ensure we don't race and try to `dlopen()` multiple times.
Yes, thanks.


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

https://reviews.llvm.org/D71491