[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

676

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

676

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

676

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

676

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

609–620

Do we use _ in the name of types?

612

I think we usually use uptr rather than size_t.

620

Suggestion. You could do

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

as a sanity check.

657–667

Suggestion: You could add a sanity check here.

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

676

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

[delcypher]

compiler-rt/lib/sanitizer_common/sanitizer_mac.cpp

643

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

620

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.

687–689

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

657–667

@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

657–667

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

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;
 }
 
-// This corresponds to Triple::getMacOSXVersion() in the Clang driver.
-static MacosVersion GetMacosAlignedVersionInternal() {
-  u16 kernel_major = GetDarwinKernelVersion().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;
+using VersStr = char[64];
+
+static void GetOSVersion(VersStr vers) {
+  uptr len = sizeof(VersStr);
+  if (SANITIZER_IOSSIM) {
+    const char *vers_env = GetEnv("SIMULATOR_RUNTIME_VERSION");
+    if (!vers_env) {
+      Report("ERROR: Running in simulator but SIMULATOR_RUNTIME_VERSION env "
+          "var is not set.\n");
+      Die();
+    }
+    len = internal_strlcpy(vers, vers_env, len);
   } else {
-    major = 11 + kernel_major - 20;
-    minor = 0;
-  }
+    int res =
+        internal_sysctlbyname("kern.osproductversion", vers, &len, nullptr, 0);
+    CHECK_EQ(res, 0);
+  }
+  CHECK_LT(len, sizeof(VersStr));
+}
+
+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:
+// macOS 10.15 -- iOS 13 -- tvOS 13 -- watchOS 6
+static void MapToMacos(u16 *major, u16 *minor) {
+  if (TARGET_OS_OSX)
+    return;
+
+  if (SANITIZER_IOS || SANITIZER_TVOS)
+    *major += 2;
+  else if (SANITIZER_WATCHOS)
+    *major += 9;
+  else
+    UNREACHABLE("unsupported platform");
+
+  if (*major >= 16) {  // macOS 11+
+    *major -= 5;
+  } else {  // macOS 10.15 and below
+    *minor = *major;
+    *major = 10;
+  }
+}
+
+static MacosVersion GetMacosAlignedVersionInternal() {
+  VersStr vers;
+  GetOSVersion(vers);
+
+  u16 major, minor;
+  ParseVersion(vers, &major, &minor);
+  MapToMacos(&major, &minor);
+
   return MacosVersion(major, minor);
 }
 
 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);
+  VersStr vers;
+  uptr len = sizeof(VersStr);
+  int res = internal_sysctlbyname("kern.osrelease", vers, &len, nullptr, 0);
   CHECK_EQ(res, 0);
+  CHECK_LT(len, sizeof(VersStr));
 
   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

 
 #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];