[Darwin] Fix OS version checks inside simulators

[yln]

Summary

compiler-rt checks OS versions by querying the Darwin kernel version.
This is not necessarily correct inside the simulators if the simulator
runtime is not aligned with the host macOS. Let's instead check the
SIMULATOR_RUNTIME_VERSION env var.
rdar://63031937

[yln]

@delcypher
I addressed your remaining comments here: https://reviews.llvm.org/D79979#2156659

[delcypher]

compiler-rt/lib/sanitizer_common/sanitizer_mac.cpp

655

Wait. Why do we go from Simulator OS version -> darwin kernel version -> macOS version? Can't we just compute the macOS version from the simulator OS version using a known offset?

[yln]

compiler-rt/lib/sanitizer_common/sanitizer_mac.cpp

655

Yes, this is correct. The naming scheme change for macOS 11 complicates the mapping (there are two cases now) and this way we can avoid duplicating this logic.

[delcypher]

compiler-rt/lib/sanitizer_common/sanitizer_mac.cpp

655

I see. I guess there's a trade-off here:

• Simulator OS version -> macOS version

Con: We need to duplicate handling of the versioning for the macOS 11 transition.
Pro: The conversion of version numbers is easier understand due to only one conversion rather than two,
Pro: It gives us the ability to preserve the patch (pre macOS 11) or minor (macOS 11 and newer) number.

• Simulator OS version -> macOS Darwin kernel version -> macOS version

Pro: Allows re-using the logic for the handling of the versioning for the macOS 11 transition.
Con: The conversion of version numbers is harder to understand due to two conversions happening
Con: Can't use the minor/patch version of macOS.. Although maybe we don't need this right now?

I'm not convinced that avoiding duplication of the macOS 11 versioning code is worth the complication it introduces.

However, taking a step back. I think the problem here is that we've decided to use the macOS version numbers as the canonical representation for version checks in the runtime. Given that macOS now has this change in versioning at macOS 11 I don't really think it's the right choice anymore. Some ideas for alternatives:

1. If we used iOS's versioning scheme as the canonical representation we could potentially make things simpler for ourselves.
2. We could use a scheme where we instead always work with the native platform version at runtime and instead our branches encode all the versions: e.g.

if (OS_VERSION(10.15, 13.0, 6.0, 13.0) > GetOSVersion()) {
  //.. os specific action.
}

Here OS_VERSION is a macro (or some kind of template magic) that expands to the Version for the platform we are compiling for at compile time. The syntax probably isn't quite right here but the above is meant to just illustrate the idea.

This makes GetOSVersion() really simple because we never need to get kernel versions involved. The implementation would be something like:

if (SANITIZER_IOSSIM) {
  if (auto vers = GetEnv("SIMULATOR_RUNTIME_VERSION")) {
    u16 major, minor;
    ParseVersion(vers, &major, &minor);
    return OSVersion(major, minor, 0);
  }
  // Emit warning here...
}
// Get OS from systl kern.osproductversion
char vers[100];
size_t len = sizeof(vers);
int res = internal_sysctlbyname("kern.osproductversion", vers, &len, nullptr, 0);
CHECK_EQ(res, 0);
return OSVersion(major, minor, 0);

@yln @kubamracek What do you think?

[yln]

compiler-rt/lib/sanitizer_common/sanitizer_mac.cpp

655

I agree that what you outlined above, i.e., something along the lines:

if (OS_VERSION(10.15, 13.0, 6.0, 13.0) > GetOSVersion()) {
  //.. os specific action.
}

would be much better. This is also pretty much what availability checking code supported by language constructs (Swift, objc) looks like.

I would like to keep the scope of this patch small: fix OS version checking in the simulators.
This patch accomplishes that and improves the current state. I am happy to see further improvements as follow-ups (I don't think this patch makes those harder), and therefore would like to continue as is.

I'm not convinced that avoiding duplication of the macOS 11 versioning code is worth the complication it introduces.

I will try to change the patch to go from OS version -> macOS version directly.

[yln]

Map from OS version -> macOS version directly.

[delcypher]

Looks really good. I just have a few nits.

compiler-rt/lib/sanitizer_common/sanitizer_mac.cpp

642

Do we use _ in the name of types?

645

I think we usually use uptr rather than size_t.

646

Suggestion: You could add a sanity check here.

CHECK_GE(*major, 10);
if (*major == 10) {
  CHECK_GE(*minor, /*whatever our minimum version is */);
}

653

Suggestion. You could do

uptr env_var_len  = internal_strlen(vers_env);
CHECK_LT(env_var_len, sizeof(vers_str));

as a sanity check.

655

Cool. You combined my sketch with the patch. This looks great.

[delcypher]

compiler-rt/lib/sanitizer_common/sanitizer_mac.cpp

666

Oh you have an early return (which is great) but if you do the sanity check you have to do it here too.

[yln]

compiler-rt/lib/sanitizer_common/sanitizer_mac.cpp

653

The check CHECK_LT(len, sizeof(vers_str)); below (which applies to both sides of the if-else) should accomplish the same.

strlcpy() returns the total length of the string it tried to create (essentially strlen(src)), which should be smaller than the dst buffer.

682–684

Switching this over to VersStr as well and adding assert for the length.

[yln]

vers_str -> VersStr, size_t -> uptr, add additional CHECK assert.

[delcypher]

compiler-rt/lib/sanitizer_common/sanitizer_mac.cpp

646

@yln Did you miss this one? It's only a suggestion so it's fine not to do it but if you're not going to do it then it would be good to say why.

[yln]

compiler-rt/lib/sanitizer_common/sanitizer_mac.cpp

646

This specific code behaves predictably for any version numbers (even if it's producing invalid numbers for invalid inputs). It's more a constraint of application in compiler-rt that we don't support anything pre 10.9, but the code is okay with anything.

In the previous iteration of this I had a similar-looking CHECK, because the mapping was defined in a way where we had an "invalid case":

darwin_kernel_vers >= 20 -> macOS 11+  //  -20 + 11
darwin_kernel_vers >= 4 -> macos 10.x  // -4
Invalid, smaller versions: -4 would wrap around // guard with CHECK

Hope this makes sense.

[delcypher]

LGTM except for the lint checks failing. Please fix those before landing.

compiler-rt/lib/sanitizer_common/tests/sanitizer_mac_test.cpp

34

Could you fix these lint checks before you land this?

compiler-rt/lib/sanitizer_common/sanitizer_mac.cpp

 //===-- sanitizer_mac.cpp -------------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 //
HandleSignalMode GetHandleSignalMode(int signum) {
   if ((SANITIZER_WATCHOS || SANITIZER_TVOS) && !(SANITIZER_IOSSIM))
     return kHandleSignalNo;
   HandleSignalMode result = GetHandleSignalModeImpl(signum);
   if (result == kHandleSignalYes && !common_flags()->allow_user_segv_handler)
     return kHandleSignalExclusive;
   return result;
 }
 
+void ParseVersion(const char *vers, u16 *major, u16 *minor) {
+  // Format: <major>.<minor>[.<patch>]\0
+  CHECK_GE(internal_strlen(vers), 3);
+  const char *p = vers;
+  *major = internal_simple_strtoll(p, &p, /*base=*/10);
+  CHECK_EQ(*p, '.');
+  p += 1;
+  *minor = internal_simple_strtoll(p, &p, /*base=*/10);
+}
+
+// Aligned versions example:
+// Darwin 19 -- macOS 10.15 -- iOS 13 -- tvOS 13 -- watchOS 6
+static u16 GetDarwinKernelMajorFromOSMajor(u16 os_major) {
+  u16 offset;
+  if (SANITIZER_IOS || SANITIZER_TVOS)
+    offset = 6;
+  else if (SANITIZER_WATCHOS)
+    offset = 13;
+  else  // macOS
+    UNREACHABLE("GetDarwinKernelMajorFromOSMajor() does not support macOS");
+
+  return os_major + offset;
+}
+
 // This corresponds to Triple::getMacOSXVersion() in the Clang driver.
-static MacosVersion GetMacosAlignedVersionInternal() {
-  u16 kernel_major = GetDarwinKernelVersion().major;
+static MacosVersion GetMacosVersionFromDarwinMajor(u16 kernel_major) {
   // Darwin 0-3  -> unsupported
   // Darwin 4-19 -> macOS 10.x
   // Darwin 20+  -> macOS 11+
   CHECK_GE(kernel_major, 4);
   u16 major, minor;
   if (kernel_major < 20) {
     major = 10;
     minor = kernel_major - 4;
   } else {
     major = 11 + kernel_major - 20;
     minor = 0;
   }
   return MacosVersion(major, minor);
 }
 
+static MacosVersion GetMacosAlignedVersionInternal() {
+  if (SANITIZER_IOSSIM) {
+    if (auto vers = GetEnv("SIMULATOR_RUNTIME_VERSION")) {
+      u16 major, minor;
+      ParseVersion(vers, &major, &minor);
+      u16 kernel_major = GetDarwinKernelMajorFromOSMajor(major);
+      return GetMacosVersionFromDarwinMajor(kernel_major);
+    }
+    Report("WARNING: Running in simulator but SIMULATOR_RUNTIME_VERSION env "
+           "var is not set.\n");
+  }
+
+  u16 kernel_major = GetDarwinKernelVersion().major;
+  return GetMacosVersionFromDarwinMajor(kernel_major);
+}
+
 static_assert(sizeof(MacosVersion) == sizeof(atomic_uint32_t::Type),
               "MacosVersion cache size");
 static atomic_uint32_t cached_macos_version;
 
 MacosVersion GetMacosAlignedVersion() {
   atomic_uint32_t::Type result =
       atomic_load(&cached_macos_version, memory_order_acquire);
   if (!result) {
     MacosVersion version = GetMacosAlignedVersionInternal();
     result = *reinterpret_cast<atomic_uint32_t::Type *>(&version);
     atomic_store(&cached_macos_version, result, memory_order_release);
   }
   return *reinterpret_cast<MacosVersion *>(&result);
 }
 
-void ParseVersion(const char *vers, u16 *major, u16 *minor) {
-  // Format: <major>.<minor>.<patch>\0
-  CHECK_GE(internal_strlen(vers), 5);
-  const char *p = vers;
-  *major = internal_simple_strtoll(p, &p, /*base=*/10);
-  CHECK_EQ(*p, '.');
-  p += 1;
-  *minor = internal_simple_strtoll(p, &p, /*base=*/10);
-}
-
 DarwinKernelVersion GetDarwinKernelVersion() {
-  char buf[100];
-  size_t len = sizeof(buf);
-  int res = internal_sysctlbyname("kern.osrelease", buf, &len, nullptr, 0);
+  char vers[100];
+  size_t len = sizeof(vers);
+  int res = internal_sysctlbyname("kern.osrelease", vers, &len, nullptr, 0);
   CHECK_EQ(res, 0);
 
   u16 major, minor;
-  ParseVersion(buf, &major, &minor);
+  ParseVersion(vers, &major, &minor);
 
   return DarwinKernelVersion(major, minor);
 }
 
 uptr GetRSS() {
   struct task_basic_info info;
   unsigned count = TASK_BASIC_INFO_COUNT;
   kern_return_t result =

compiler-rt/lib/sanitizer_common/tests/sanitizer_mac_test.cpp

 //===-- sanitizer_mac_test.cpp --------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 //
 // Tests for sanitizer_mac.{h,cpp}
 //
 //===----------------------------------------------------------------------===//
 
 #include "sanitizer_common/sanitizer_platform.h"
 #if SANITIZER_MAC
 
 #include "sanitizer_common/sanitizer_mac.h"
 
 #include "gtest/gtest.h"
 
 #include <sys/sysctl.h>  // sysctlbyname
 #include <mach/kern_return.h>  // KERN_SUCCESS
 
 namespace __sanitizer {
 
+void ParseVersion(const char *vers, u16 *major, u16 *minor);
+
+TEST(SanitizerMac, ParseVersion) {
+  u16 major, minor;
+
+  ParseVersion("11.22.33", &major, &minor);
+  EXPECT_EQ(major, 11); EXPECT_EQ(minor, 22);
+
+  ParseVersion("1.2", &major, &minor);
+  EXPECT_EQ(major, 1); EXPECT_EQ(minor, 2);
+}
+
+// TODO(yln): Run sanitizer unit tests for the simulators (rdar://65680742)
+#if SANITIZER_IOSSIM
+TEST(SanitizerMac, GetMacosAlignedVersion) {
+  const char *vers_str;
+  if (SANITIZER_IOS || SANITIZER_TVOS) {
+    vers_str = "13.0";
+  } else if (SANITIZER_WATCHOS) {
+    vers_str = "6.5";
+  } else {
+    FAIL() << "unsupported simulator runtime";
+  }
+  setenv("SIMULATOR_RUNTIME_VERSION", vers_str, /*overwrite=*/1);
+
+  MacosVersion vers = GetMacosAlignedVersion();
+  EXPECT_EQ(vers.major, 10);
+  EXPECT_EQ(vers.minor, 15);
+}
+#else
 TEST(SanitizerMac, GetMacosAlignedVersion) {
   MacosVersion vers = GetMacosAlignedVersion();
   u16 kernel_major = GetDarwinKernelVersion().major;
   bool macos_11 = (kernel_major >= 20);
   u16 expected_major = macos_11 ? (kernel_major - 9) : 10;
   u16 expected_minor = macos_11 ? 0 : (kernel_major - 4);
   EXPECT_EQ(vers.major, expected_major);
   EXPECT_EQ(vers.minor, expected_minor);
 }
-
-void ParseVersion(const char *vers, u16 *major, u16 *minor);
-
-TEST(SanitizerMac, ParseVersion) {
-  u16 major, minor;
-  ParseVersion("11.22.33", &major, &minor);
-  EXPECT_EQ(major, 11);
-  EXPECT_EQ(minor, 22);
-}
+#endif
 
 TEST(SanitizerMac, GetDarwinKernelVersion) {
   DarwinKernelVersion vers = GetDarwinKernelVersion();
   std::ostringstream oss;
   oss << vers.major << '.' << vers.minor;
   std::string actual = oss.str();
 
   char buf[100];