[Support] sys::getProcessTriple should return a macOS triple using the system's version of macOS

[arphaman]

Summary

Right now, sys::getProcessTriple returns the LLVM_HOST_TRIPLE, whose system version might not be the actual version of the system on which the compiler running. This patch ensures that for macOS, sys::getProcessTriple returns a triple with the system's macOS version.

The function sys::getDefaultTargetTriple does this version adjustment as well. However, it seems wrong to me (the default target OS might not be the host OS). I left it alone in this patch to avoid breaking things, but I think that the version adjustment in sys::getDefaultTargetTriple should check that the host OS is the same as the target OS before making the adjustment.

I'm not sure if it's possible to test this change (Please let me know if I'm wrong though). I do have a commit in Clang r305678 (that I reverted for now) that depends on this change.

[dexonsmith]

Right now, sys::getProcessTriple returns the LLVM_HOST_TRIPLE, whose system version might not be the actual version of the system on which the compiler running.

Are we sure no one depends on that behaviour?

I'm not sure if it's possible to test this change (Please let me know if I'm wrong though).

I think we could add a lit test with REQUIRES: shell that only runs on macOS and uses sw_vers to pull out the system OS version. If any builder has a non-matching LLVM_HOST_TRIPLE, we have some test coverage.

[arphaman]

Add a test

[arphaman]

In D34446#787263, @dexonsmith wrote:

Right now, sys::getProcessTriple returns the LLVM_HOST_TRIPLE, whose system version might not be the actual version of the system on which the compiler running.

Are we sure no one depends on that behaviour?

The primary users of this function are the JIT libraries, but so far I haven't found any checks for the OS version in the JIT code. And it looks like most of the other users of this function don't check the OS version.

I'm not sure if it's possible to test this change (Please let me know if I'm wrong though).

I think we could add a lit test with REQUIRES: shell that only runs on macOS and uses sw_vers to pull out the system OS version. If any builder has a non-matching LLVM_HOST_TRIPLE, we have some test coverage.

I've added a unittest that uses sw_vers.

[dexonsmith]

In D34446#787934, @arphaman wrote:

In D34446#787263, @dexonsmith wrote:

Right now, sys::getProcessTriple returns the LLVM_HOST_TRIPLE, whose system version might not be the actual version of the system on which the compiler running.

Are we sure no one depends on that behaviour?

The primary users of this function are the JIT libraries, but so far I haven't found any checks for the OS version in the JIT code. And it looks like most of the other users of this function don't check the OS version.

Lang, do you know if it's reasonable to change the behaviour of sys::getProcessTriple?

[arphaman]

Ping.

[arphaman]

Ping.

[lhames]

Sorry for the delayed reply.

LGTM (though see inline comment).

The getProcessTriple function is pretty fundamentally broken as it stands: it doesn't do the right thing when you're cross compiling (e.g. an ARM compiler built on x86 will claim that the process triple is x86_64-...). One day we should tackle that. This seems like a strict improvement in the mean time though.

• Lang.

lib/Support/Unix/Host.inc

37–53 ↗

(On Diff #103567)

Minor nitpick: This seems like it would be nicer as a function from a string to a string, rather than a mutating operation.

[thakis]

llvm/trunk/lib/Support/Unix/Host.inc

58

This is the function that clang calls to determine its default triple (here: http://llvm-cs.pcc.me.uk/tools/clang/lib/Frontend/CompilerInvocation.cpp#3410)

LLVM_DEFAULT_TARGET_TRIPLE is what we tell people to set when they build cross compilers. It's what we use to make sure our compiler isn't dependent on the host OS version.

Changing this here to rewrite LLVM_DEFAULT_TARGET_TRIPLE to use the host OS version seems like the wrong thing to do, from the perspective of clang's default triple. Do you agree?

[lhames]

llvm/trunk/lib/Support/Unix/Host.inc

58

I missed this at the time.

It looks like this patch only partially changed the behavior here: We were already modifying the OS version for -darwin triples but now we're doing it for -macos too.

The JIT doesn't use getDefaultTargetTriple, so I don't mind if we switch to a behavior that makes more sense to clang. The getProcessTriple function should still update the version though.

llvm/trunk/lib/Support/Host.cpp

#endif
 
   return true;
 }
 #else
 bool sys::getHostCPUFeatures(StringMap<bool> &Features) { return false; }
 #endif
 
 std::string sys::getProcessTriple() {
-  Triple PT(Triple::normalize(LLVM_HOST_TRIPLE));
+  std::string TargetTripleString = updateTripleOSVersion(LLVM_HOST_TRIPLE);
+  Triple PT(Triple::normalize(TargetTripleString));
 
   if (sizeof(void *) == 8 && PT.isArch32Bit())
     PT = PT.get64BitArchVariant();
   if (sizeof(void *) == 4 && PT.isArch64Bit())
     PT = PT.get32BitArchVariant();
 
   return PT.str();
 }

llvm/trunk/lib/Support/Unix/Host.inc

static std::string getOSVersion() {
   struct utsname info;
 
   if (uname(&info))
     return "";
 
   return info.release;
 }
 
-std::string sys::getDefaultTargetTriple() {
-  std::string TargetTripleString(LLVM_DEFAULT_TARGET_TRIPLE);
-
-  // On darwin, we want to update the version to match that of the
-  // target.
+static std::string updateTripleOSVersion(std::string TargetTripleString) {
+  // On darwin, we want to update the version to match that of the target.
   std::string::size_type DarwinDashIdx = TargetTripleString.find("-darwin");
   if (DarwinDashIdx != std::string::npos) {
     TargetTripleString.resize(DarwinDashIdx + strlen("-darwin"));
     TargetTripleString += getOSVersion();
+    return TargetTripleString;
+  }
+  std::string::size_type MacOSDashIdx = TargetTripleString.find("-macos");
+  if (MacOSDashIdx != std::string::npos) {
+    TargetTripleString.resize(MacOSDashIdx);
+    // Reset the OS to darwin as the OS version from `uname` doesn't use the
+    // macOS version scheme.
+    TargetTripleString += "-darwin";
+    TargetTripleString += getOSVersion();
   }
+  return TargetTripleString;
+}
+
+std::string sys::getDefaultTargetTriple() {
+  std::string TargetTripleString =
+      updateTripleOSVersion(LLVM_DEFAULT_TARGET_TRIPLE);
 
-  // Override the default target with an environment variable named by LLVM_TARGET_TRIPLE_ENV.
+  // Override the default target with an environment variable named by
+  // LLVM_TARGET_TRIPLE_ENV.
 #if defined(LLVM_TARGET_TRIPLE_ENV)
   if (const char *EnvTriple = std::getenv(LLVM_TARGET_TRIPLE_ENV))
     TargetTripleString = EnvTriple;
 #endif
 
   return Triple::normalize(TargetTripleString);
 }

llvm/trunk/lib/Support/Windows/Host.inc

 //===----------------------------------------------------------------------===//
 
 #include "WindowsSupport.h"
 #include <cstdio>
 #include <string>
 
 using namespace llvm;
 
+static void updateTripleOSVersion(std::string &) {
+  // Do nothing.
+}
+
 std::string sys::getDefaultTargetTriple() {
   const char *Triple = LLVM_DEFAULT_TARGET_TRIPLE;
 
   // Override the default target with an environment variable named by LLVM_TARGET_TRIPLE_ENV.
 #if defined(LLVM_TARGET_TRIPLE_ENV)
   if (const char *EnvTriple = std::getenv(LLVM_TARGET_TRIPLE_ENV))
     Triple = EnvTriple;
 #endif
 
   return Triple::normalize(Triple);
 }

llvm/trunk/unittests/Support/Host.cpp

 //========- unittests/Support/Host.cpp - Host.cpp tests --------------========//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #include "llvm/Support/Host.h"
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/Triple.h"
+#include "llvm/Support/FileSystem.h"
+#include "llvm/Support/Path.h"
+#include "llvm/Support/Program.h"
 
 #include "gtest/gtest.h"
 
+#define ASSERT_NO_ERROR(x)                                                     \
+  if (std::error_code ASSERT_NO_ERROR_ec = x) {                                \
+    SmallString<128> MessageStorage;                                           \
+    raw_svector_ostream Message(MessageStorage);                               \
+    Message << #x ": did not return errc::success.\n"                          \
+            << "error number: " << ASSERT_NO_ERROR_ec.value() << "\n"          \
+            << "error message: " << ASSERT_NO_ERROR_ec.message() << "\n";      \
+    GTEST_FATAL_FAILURE_(MessageStorage.c_str());                              \
+  } else {                                                                     \
+  }
+
 using namespace llvm;
 
 class HostTest : public testing::Test {
   Triple Host;
 
 protected:
   bool isSupportedArchAndOS() {
     // Initially this is only testing detection of the number of

 CPU revision    : 3
 
 Hardware        : Qualcomm Technologies, Inc MSM8992
 )";
 
   EXPECT_EQ(sys::detail::getHostCPUNameForARM(MSM8992ProcCpuInfo),
             "cortex-a53");
 }
+
+#if defined(__APPLE__)
+TEST_F(HostTest, getMacOSHostVersion) {
+  using namespace llvm::sys;
+  llvm::Triple HostTriple(getProcessTriple());
+  if (!HostTriple.isMacOSX())
+    return;
+
+  SmallString<128> TestDirectory;
+  ASSERT_NO_ERROR(fs::createUniqueDirectory("host_test", TestDirectory));
+  SmallString<128> OutputFile(TestDirectory);
+  path::append(OutputFile, "out");
+
+  const char *SwVersPath = "/usr/bin/sw_vers";
+  const char *argv[] = {SwVersPath, "-productVersion", nullptr};
+  StringRef OutputPath = OutputFile.str();
+  const StringRef *Redirects[] = {/*STDIN=*/nullptr, /*STDOUT=*/&OutputPath,
+                                  /*STDERR=*/nullptr};
+  int RetCode = ExecuteAndWait(SwVersPath, argv, /*env=*/nullptr, Redirects);
+  ASSERT_EQ(0, RetCode);
+
+  int FD = 0;
+  ASSERT_NO_ERROR(fs::openFileForRead(OutputPath, FD));
+  off_t Size = ::lseek(FD, 0, SEEK_END);
+  ASSERT_NE(-1, Size);
+  ::lseek(FD, 0, SEEK_SET);
+  std::unique_ptr<char[]> Buffer = llvm::make_unique<char[]>(Size);
+  ASSERT_EQ(::read(FD, Buffer.get(), Size), Size);
+  ::close(FD);
+
+  // Ensure that the two versions match.
+  StringRef SystemVersion(Buffer.get(), Size);
+  unsigned SystemMajor, SystemMinor, SystemMicro;
+  ASSERT_EQ(llvm::Triple((Twine("x86_64-apple-macos") + SystemVersion))
+                .getMacOSXVersion(SystemMajor, SystemMinor, SystemMicro),
+            true);
+  unsigned HostMajor, HostMinor, HostMicro;
+  ASSERT_EQ(HostTriple.getMacOSXVersion(HostMajor, HostMinor, HostMicro), true);
+
+  // Don't compare the 'Micro' version, as it's always '0' for the 'Darwin'
+  // triples.
+  ASSERT_EQ(std::tie(SystemMajor, SystemMinor), std::tie(HostMajor, HostMinor));
+
+  ASSERT_NO_ERROR(fs::remove(OutputPath));
+  ASSERT_NO_ERROR(fs::remove(TestDirectory.str()));
+}
+#endif