[Darwin] Add tests for OS version checking

[yln]

Summary

• Extract ParseVersion helper function for testing

[yln]

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

17

I include the implementation file here so that I can directly test the static ParseVersion helper function. Is this okay?

46

This essentially re-implements what we have already. Happy to follow any pointers on how to improve this.

[delcypher]

compiler-rt/lib/sanitizer_common/sanitizer_mac.cpp

632

Does your use of references here pass the sanitizer lint check? IIRC this is disallowed by the linter.

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

17

I don't think this is a good idea. Sanitizer functions aren't usually tested this way. IIRC the unit tests get linked against the sanitizer library object files so you should be able to call the function you need from the linked object file.

Can't you just remove the static on __sanitizer::ParseVersion(...) and call it directly? You might need to add a proto-type to lib/sanitizer_common/sanitizer_mac.h or in sanitizer_mac_test.cpp.

[yln]

Make helper non-static and forward declare it in test instead of including implementation file.

[yln]

compiler-rt/lib/sanitizer_common/sanitizer_mac.cpp

632

ninja SanitizerLintCheck comes back good for. Is there a pattern that it rejects so I can counter-check if it's running properly?

Just out of curiosity: the linter is executed when compiling sanitizer tests (e.g., check-sanitizer, check-asan), right?! I usually don't run it explicitly.

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

17

The reason this was that I tried to avoid making this helper (implementation detail) non-static just for the purpose of testing. I will remove static and forward declare in the test file.

[delcypher]

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

24

Test's aren't normally declared inside the __sanitizer namespace.

Maybe do something like this?

// Prototypes
namespace __sanitizer {
  void ParseVersion(const char *vers, u16 &major, u16 &minor);
}

using namespace __sanitizer;

TEST(SanitizerMac, GetMacosAlignedVersion) {
  ...
}

46

Nit: I think it's okay to use C++ in the unit tests so you could use std::ostringstream to avoid using snprintf for simpler code.

[delcypher]

compiler-rt/lib/sanitizer_common/sanitizer_mac.cpp

632

IIRC I hit this from the old google C++ style guide once

Parameters are either input to the function, output from the function, or both. Input parameters are usually values or const references, while output and input/output parameters will be pointers to non-const.

https://drake.mit.edu/styleguide/cppguide.html#Output_Parameters

which I think the linter script tries to enforce (at least partially).

If you're passing SanitizerLintCheck then you're probably fine.

If you want to check if it's working IIRC the linter doesn't like FIXME: something and instead wants something like FIXME(yln): something.

[yln]

Use std::ostringstream instead of snprintf in test code.

[yln]

compiler-rt/lib/sanitizer_common/sanitizer_mac.cpp

632

Hmm, I can't get it to trigger at all. Investigating...

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

24

The other tests in this directory (e.g., sanitizer_linux_test.cpp, sanitizer_symbolizer_test.cpp, etc.) follow the same pattern, so I would prefer to leave this as is.

46

Dreaming of std::format() ;)

[delcypher]

LGTM but please to make sure the linter is working correctly on your system before you land this.

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

24

Yeah some do, some don't. E.g. sanitizer_bvgraph_test.cpp and sanitizer_bitvector_test.cpp follow the pattern I suggest and others follow what you've done in this patch. Yay for inconsistency :(

I'll let you decide which you want to follow.

compiler-rt/lib/sanitizer_common/sanitizer_mac.cpp

 #endif
 #include <stdio.h>
 
 #include "sanitizer_common.h"
 #include "sanitizer_file.h"
 #include "sanitizer_flags.h"
 #include "sanitizer_internal_defs.h"
 #include "sanitizer_libc.h"
-#include "sanitizer_placement_new.h"
 #include "sanitizer_platform_limits_posix.h"
 #include "sanitizer_procmaps.h"
 #include "sanitizer_ptrauth.h"
 
 #if !SANITIZER_IOS
 #include <crt_externs.h>  // for _NSGetEnviron
 #else
 extern char **environ;
MacosVersion GetMacosAlignedVersion() {
   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);
   CHECK_EQ(res, 0);
 
-  // Format: <major>.<minor>.<patch>\0
-  CHECK_GE(len, 6);
-  const char *p = buf;
-  u16 major = internal_simple_strtoll(p, &p, /*base=*/10);
-  CHECK_EQ(*p, '.');
-  p += 1;
-  u16 minor = internal_simple_strtoll(p, &p, /*base=*/10);
+  u16 major, minor;
+  ParseVersion(buf, &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/CMakeLists.txt

set(SANITIZER_UNITTESTS
   sanitizer_common_test.cpp
   sanitizer_deadlock_detector_test.cpp
   sanitizer_flags_test.cpp
   sanitizer_format_interceptor_test.cpp
   sanitizer_ioctl_test.cpp
   sanitizer_libc_test.cpp
   sanitizer_linux_test.cpp
   sanitizer_list_test.cpp
+  sanitizer_mac_test.cpp
   sanitizer_mutex_test.cpp
   sanitizer_nolibc_test.cpp
   sanitizer_posix_test.cpp
   sanitizer_printf_test.cpp
   sanitizer_procmaps_test.cpp
   sanitizer_ring_buffer_test.cpp
   sanitizer_quarantine_test.cpp
   sanitizer_stackdepot_test.cpp

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 {
+
+TEST(SanitizerMac, GetMacosAlignedVersion) {
+  MacosVersion vers = GetMacosAlignedVersion();
+  EXPECT_EQ(vers.major, 10);
+  EXPECT_EQ(vers.minor, GetDarwinKernelVersion().major - 4);
+}
+
+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);
+}
+
+TEST(SanitizerMac, GetDarwinKernelVersion) {
+  DarwinKernelVersion vers = GetDarwinKernelVersion();
+  std::ostringstream oss;
+  oss << vers.major << '.' << vers.minor;
+  std::string actual = oss.str();
+
+  char buf[100];
+  size_t len = sizeof(buf);
+  int res = sysctlbyname("kern.osrelease", buf, &len, nullptr, 0);
+  ASSERT_EQ(res, KERN_SUCCESS);
+  std::string expected(buf);
+
+  // Prefix match
+  ASSERT_TRUE(expected.compare(0, actual.size(), actual) == 0);
+}
+
+}  // namespace __sanitizer
+
+#endif  // SANITIZER_MAC