Add an internal bit to the XcodeSDK class

[aprantl]

Summary

For developing the OS itself there exists an "internal" variant of each SDK. This patch adds support for these SDK directories to the XcodeSDK class.

[JDevlieghere]

lldb/include/lldb/Utility/XcodeSDK.h

54–66

I think a struct would be less opaque.

[aprantl]

Address review feedback — added fallback mechanisms.

[aprantl]

lldb/include/lldb/Utility/XcodeSDK.h

54–66

Agreed. The tuple was meant to facilitate having the internal bit downstream, but we decided that there's no reason not have it upstream. I'll change it!

[aprantl]

Introduce struct SDKInfo as suggested by @JDevlieghere . Thanks!

[friss]

lldb/source/Host/macosx/objcxx/HostInfoMacOSX.mm

354–358

I don't think we need this last fallback. The internal and public SDKs are very different. You should be able to use an internal one to replace a public one, but not the other way around.

lldb/source/Utility/XcodeSDK.cpp

153–159

Why not leave this function as is and try to add both "internal" and ".internal" to the SDK name in GetXcodeSDK() without special-casing macOS? I'm not sure that macOS is actually special.

[JDevlieghere]

lldb/include/lldb/Utility/XcodeSDK.h

74

Can this now take an SDKInfo?

[aprantl]

Second round of feedback. Thanks everyone!

[aprantl]

Delete extra curly braces.

[JDevlieghere]

One more comment but otherwise this LGTM

lldb/source/Host/macosx/objcxx/HostInfoMacOSX.mm

332

I always find a while like this really suspicious, even though the logic looks correct. Can you achieve the same thing in a less clever way with a lambda and early return from that?

[aprantl]

lldb/source/Host/macosx/objcxx/HostInfoMacOSX.mm

332

Not sure. Maybe I'm doing something other than what you were thinking about, but the lambda I came up with looks even more pretentious ;-)

What complicates it is that I need to check the condition at the beginning and want to return out of the function from within the body. Do you have a specific suggestion in mind?

[JDevlieghere]

lldb/source/Host/macosx/objcxx/HostInfoMacOSX.mm

332

I'm probably missing something. What I had in mind was

[]() {
    // Try an alternate spelling of the name ("macosx10.9internal").
    if (info.type == XcodeSDK::Type::MacOSX && !info.version.empty() &&
        info.internal) {
      llvm::StringRef fixed(sdk_name);
      if (fixed.consume_back(".internal"))
        sdk_name = fixed.str()+"internal";
      path = find_sdk(sdk_name);
      if (!path.empty())
        return path;
    }

    Log *log = lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_HOST);
    LLDB_LOGF(log, "Couldn't find SDK %s on host", sdk_name.c_str());

    // Try without the version.
    if (!info.version.empty()) {
      info.version = {};
      sdk_name = XcodeSDK::GetCanonicalName(info);
      path = find_sdk(sdk_name);
      if (!path.empty())
        return path;
    }

    LLDB_LOGF(log, "Couldn't find any matching SDK on host");
    return {};
}

[aprantl]

lldb/source/Host/macosx/objcxx/HostInfoMacOSX.mm

332

But for it to work you'd need to wrap it inside

if (path.empty())
  path = [&]() { ... }();
if (path.empty()}
  return {};

And that is three lines longer than the original code, and the sequence [&]() { ... }(), which doesn't meet my threshold for being easier to read. This is all highly subjective of course, so let me know what you think. I'm not married to the loop either, and in the end it doesn't really matter :-)

[JDevlieghere]

lldb/source/Host/macosx/objcxx/HostInfoMacOSX.mm

332

You could move the first check into the lambda. But it seems like you thought this true so I trust your judgement, go with the while loop ;-)

lldb/include/lldb/Utility/XcodeSDK.h

 namespace lldb_private {
 
 /// An abstraction for Xcode-style SDKs that works like \ref ArchSpec.
 class XcodeSDK {
   std::string m_name;
 
 public:
   XcodeSDK() = default;
+  /// Initialize an XcodeSDK object with an SDK name. The SDK name is the last
+  /// directory component of a path one would pass to clang's -isysroot
+  /// parameter. For example, "MacOSX.10.14.sdk".
   XcodeSDK(std::string &&name) : m_name(std::move(name)) {}
   static XcodeSDK GetAnyMacOS() { return XcodeSDK("MacOSX.sdk"); }
 
   enum Type : int {
     MacOSX = 0,
     iPhoneSimulator,
     iPhoneOS,
     AppleTVSimulator,
     AppleTVOS,
     WatchSimulator,
     watchOS,
     bridgeOS,
     Linux,
     numSDKTypes,
     unknown = -1
   };
-  static llvm::StringRef GetNameForType(Type type);
 
   /// The merge function follows a strict order to maintain monotonicity:
   /// 1. SDK with the higher SDKType wins.
   /// 2. The newer SDK wins.
   void Merge(XcodeSDK other);
 
   XcodeSDK &operator=(XcodeSDK other);
   XcodeSDK(const XcodeSDK&) = default;
   bool operator==(XcodeSDK other);
 
-  /// Return parsed SDK number, and SDK version number.
-  std::tuple<Type, llvm::VersionTuple> Parse() const;
+  /// A parsed SDK directory name.
+  struct Info {
+    Type type = unknown;
+    llvm::VersionTuple version;
+    bool internal = false;
+
+    Info() = default;
+    bool operator<(const Info &other) const;
+  };
+
+  /// Return parsed SDK type and version number.
+  Info Parse() const;
+  bool IsAppleInternalSDK() const;
   llvm::VersionTuple GetVersion() const;
   Type GetType() const;
   llvm::StringRef GetString() const;
 
   static bool SDKSupportsModules(Type type, llvm::VersionTuple version);
   static bool SDKSupportsModules(Type desired_type, const FileSpec &sdk_path);
-  static llvm::StringRef GetSDKNameForType(Type type);
+  /// Return the canonical SDK name, such as "macosx" for the macOS SDK.
+  static std::string GetCanonicalName(Info info);
 };
 
 } // namespace lldb_private
 
 #endif

lldb/source/Host/macosx/objcxx/HostInfoMacOSX.mm

#else
       arch_32.GetTriple().setOS(llvm::Triple::IOS);
 #endif
       arch_64.Clear();
     }
   }
 }
 
 std::string HostInfoMacOSX::GetXcodeSDK(XcodeSDK sdk) {
-  std::string xcrun_cmd = "xcrun --show-sdk-path --sdk " +
-                          XcodeSDK::GetSDKNameForType(sdk.GetType()).str();
-  llvm::VersionTuple version = sdk.GetVersion();
-  if (!version.empty())
-    xcrun_cmd += version.getAsString();
-
+  XcodeSDK::Info info = sdk.Parse();
+  std::string sdk_name = XcodeSDK::GetCanonicalName(info);
+  auto find_sdk = [](std::string sdk_name) -> std::string {
+    std::string xcrun_cmd = "xcrun --show-sdk-path --sdk " + sdk_name;
     int status = 0;
     int signo = 0;
     std::string output_str;
     lldb_private::Status error =
         Host::RunShellCommand(xcrun_cmd.c_str(), FileSpec(), &status, &signo,
                               &output_str, std::chrono::seconds(15));
 
     // Check that xcrun return something useful.
     if (status != 0 || output_str.empty())
       return {};
 
     // Convert to a StringRef so we can manipulate the string without modifying
     // the underlying data.
     llvm::StringRef output(output_str);
 
     // Remove any trailing newline characters.
     output = output.rtrim();
 
     // Strip any leading newline characters and everything before them.
     const size_t last_newline = output.rfind('\n');
     if (last_newline != llvm::StringRef::npos)
       output = output.substr(last_newline + 1);
 
+    return output.str();
+  };
+
+  std::string path = find_sdk(sdk_name);
+  while (path.empty()) {
+    // Try an alternate spelling of the name ("macosx10.9internal").
+    if (info.type == XcodeSDK::Type::MacOSX && !info.version.empty() &&
+        info.internal) {
+      llvm::StringRef fixed(sdk_name);
+      if (fixed.consume_back(".internal"))
+        sdk_name = fixed.str() + "internal";
+      path = find_sdk(sdk_name);
+      if (!path.empty())
+        break;
+    }
+    Log *log = lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_HOST);
+    LLDB_LOGF(log, "Couldn't find SDK %s on host", sdk_name.c_str());
+
+    // Try without the version.
+    if (!info.version.empty()) {
+      info.version = {};
+      sdk_name = XcodeSDK::GetCanonicalName(info);
+      path = find_sdk(sdk_name);
+      if (!path.empty())
+        break;
+    }
+
+    LLDB_LOGF(log, "Couldn't find any matching SDK on host");
+    return {};
+  }
+
   // Whatever is left in output should be a valid path.
-  if (!FileSystem::Instance().Exists(output))
+  if (!FileSystem::Instance().Exists(path))
     return {};
-  return output.str();
+  return path;
 }

lldb/source/Utility/XcodeSDK.cpp

if (i == name.size() || name[i++] != '.')
     return {};
 
   llvm::VersionTuple version;
   version.tryParse(name.slice(0, i - 1));
   name = name.drop_front(i);
   return version;
 }
 
+static bool ParseAppleInternalSDK(llvm::StringRef &name) {
+  return name.consume_front("Internal.");
+}
+
+XcodeSDK::Info XcodeSDK::Parse() const {
+  XcodeSDK::Info info;
+  llvm::StringRef input(m_name);
+  info.type = ParseSDKName(input);
+  info.version = ParseSDKVersion(input);
+  info.internal = ParseAppleInternalSDK(input);
+  return info;
+}
 
-std::tuple<XcodeSDK::Type, llvm::VersionTuple> XcodeSDK::Parse() const {
+bool XcodeSDK::IsAppleInternalSDK() const {
   llvm::StringRef input(m_name);
-  XcodeSDK::Type sdk = ParseSDKName(input);
-  llvm::VersionTuple version = ParseSDKVersion(input);
-  return std::make_tuple<XcodeSDK::Type, llvm::VersionTuple>(
-      std::move(sdk), std::move(version));
+  ParseSDKName(input);
+  ParseSDKVersion(input);
+  return ParseAppleInternalSDK(input);
 }
 
 llvm::VersionTuple XcodeSDK::GetVersion() const {
   llvm::StringRef input(m_name);
   ParseSDKName(input);
   return ParseSDKVersion(input);
 }
 
 XcodeSDK::Type XcodeSDK::GetType() const {
   llvm::StringRef input(m_name);
   return ParseSDKName(input);
 }
 
 llvm::StringRef XcodeSDK::GetString() const { return m_name; }
 
+bool XcodeSDK::Info::operator<(const Info &other) const {
+  return std::tie(type, version, internal) <
+         std::tie(other.type, other.version, other.internal);
+}
 void XcodeSDK::Merge(XcodeSDK other) {
   // The "bigger" SDK always wins.
-  if (Parse() < other.Parse())
+  auto l = Parse();
+  auto r = other.Parse();
+  if (l < r)
     *this = other;
+  else {
+    // The Internal flag always wins.
+    if (llvm::StringRef(m_name).endswith(".sdk"))
+      if (!l.internal && r.internal)
+        m_name =
+            m_name.substr(0, m_name.size() - 3) + std::string("Internal.sdk");
+  }
 }
 
-llvm::StringRef XcodeSDK::GetSDKNameForType(XcodeSDK::Type type) {
-  switch (type) {
+std::string XcodeSDK::GetCanonicalName(XcodeSDK::Info info) {
+  std::string name;
+  switch (info.type) {
   case MacOSX:
-    return "macosx";
+    name = "macosx";
+    break;
   case iPhoneSimulator:
-    return "iphonesimulator";
+    name = "iphonesimulator";
+    break;
   case iPhoneOS:
-    return "iphoneos";
+    name = "iphoneos";
+    break;
   case AppleTVSimulator:
-    return "appletvsimulator";
+    name = "appletvsimulator";
+    break;
   case AppleTVOS:
-    return "appletvos";
+    name = "appletvos";
+    break;
   case WatchSimulator:
-    return "watchsimulator";
+    name = "watchsimulator";
+    break;
   case watchOS:
-    return "watchos";
+    name = "watchos";
+    break;
   case bridgeOS:
-    return "bridgeos";
+    name = "bridgeos";
+    break;
   case Linux:
-    return "linux";
+    name = "linux";
+    break;
   case numSDKTypes:
   case unknown:
-    return "";
+    return {};
   }
-  llvm_unreachable("unhandled switch case");
+  if (!info.version.empty())
+    name += info.version.getAsString();
+  if (info.internal)
+    name += ".internal";
+  return name;
 }
 
 bool XcodeSDK::SDKSupportsModules(XcodeSDK::Type sdk_type,
                                   llvm::VersionTuple version) {
   switch (sdk_type) {
   case Type::MacOSX:
     return version >= llvm::VersionTuple(10, 10);
   case Type::iPhoneOS:
   case Type::iPhoneSimulator:
   case Type::AppleTVOS:

 bool XcodeSDK::SDKSupportsModules(XcodeSDK::Type desired_type,
                                   const FileSpec &sdk_path) {
   ConstString last_path_component = sdk_path.GetLastPathComponent();
 
   if (last_path_component) {
     const llvm::StringRef sdk_name = last_path_component.GetStringRef();
 
     const std::string sdk_name_lower = sdk_name.lower();
-    const llvm::StringRef sdk_string = GetSDKNameForType(desired_type);
+    Info info;
+    info.type = desired_type;
+    const llvm::StringRef sdk_string = GetCanonicalName(info);
     if (!llvm::StringRef(sdk_name_lower).startswith(sdk_string))
       return false;
 
     auto version_part = sdk_name.drop_front(sdk_string.size());
     version_part.consume_back(".sdk");
+    version_part.consume_back(".Internal");
 
     llvm::VersionTuple version;
     if (version.tryParse(version_part))
       return false;
     return SDKSupportsModules(desired_type, version);
   }
 
   return false;
 }

lldb/unittests/Host/HostInfoTest.cpp

EXPECT_EQ(HostInfo::GetAugmentedArchSpec(LLDB_ARCH_DEFAULT).GetTriple(),
             HostInfo::GetArchitecture(HostInfo::eArchKindDefault).GetTriple());
 }
 
 TEST_F(HostInfoTest, GetHostname) {
   // Check non-empty string input works correctly.
   std::string s("abc");
   EXPECT_TRUE(HostInfo::GetHostname(s));
 }
+
+#if defined(__APPLE__)
+TEST_F(HostInfoTest, GetXcodeSDK) {
+  EXPECT_FALSE(HostInfo::GetXcodeSDK(XcodeSDK("MacOSX.sdk")).empty());
+  // These are expected to fall back to an available version.
+  EXPECT_FALSE(HostInfo::GetXcodeSDK(XcodeSDK("MacOSX9999.sdk")).empty());
+  // This is expected to fail.
+  EXPECT_TRUE(HostInfo::GetXcodeSDK(XcodeSDK("CeciNestPasUnOS.sdk")).empty());
+}
+#endif

lldb/unittests/Utility/XcodeSDKTest.cpp

TEST(XcodeSDKTest, ParseTest) {
   EXPECT_EQ(XcodeSDK("AppleTVSimulator.sdk").GetType(), XcodeSDK::AppleTVSimulator);
   EXPECT_EQ(XcodeSDK("AppleTVOS.sdk").GetType(), XcodeSDK::AppleTVOS);
   EXPECT_EQ(XcodeSDK("WatchSimulator.sdk").GetType(), XcodeSDK::WatchSimulator);
   EXPECT_EQ(XcodeSDK("WatchOS.sdk").GetType(), XcodeSDK::watchOS);
   EXPECT_EQ(XcodeSDK("Linux.sdk").GetType(), XcodeSDK::Linux);
   EXPECT_EQ(XcodeSDK("MacOSX.sdk").GetVersion(), llvm::VersionTuple());
   EXPECT_EQ(XcodeSDK("MacOSX10.9.sdk").GetVersion(), llvm::VersionTuple(10, 9));
   EXPECT_EQ(XcodeSDK("MacOSX10.15.4.sdk").GetVersion(), llvm::VersionTuple(10, 15));
+  EXPECT_EQ(XcodeSDK("MacOSX.sdk").IsAppleInternalSDK(), false);
+  EXPECT_EQ(XcodeSDK("MacOSX10.15.Internal.sdk").GetType(), XcodeSDK::MacOSX);
+  EXPECT_EQ(XcodeSDK("MacOSX10.15.Internal.sdk").GetVersion(),
+            llvm::VersionTuple(10, 15));
+  EXPECT_EQ(XcodeSDK("MacOSX10.15.Internal.sdk").IsAppleInternalSDK(), true);
   EXPECT_EQ(XcodeSDK().GetType(), XcodeSDK::unknown);
   EXPECT_EQ(XcodeSDK().GetVersion(), llvm::VersionTuple());
 }
 
 TEST(XcodeSDKTest, MergeTest) {
   XcodeSDK sdk("MacOSX.sdk");
   sdk.Merge(XcodeSDK("WatchOS.sdk"));
   // This doesn't make any particular sense and shouldn't happen in practice, we
   // just want to guarantee a well-defined behavior when choosing one
   // SDK to fit all CUs in an lldb::Module.
   // -> The higher number wins.
   EXPECT_EQ(sdk.GetType(), XcodeSDK::watchOS);
   sdk.Merge(XcodeSDK("WatchOS1.1.sdk"));
   EXPECT_EQ(sdk.GetVersion(), llvm::VersionTuple(1, 1));
   sdk.Merge(XcodeSDK("WatchOS2.0.sdk"));
   EXPECT_EQ(sdk.GetVersion(), llvm::VersionTuple(2, 0));
+  sdk.Merge(XcodeSDK("WatchOS1.1.Internal.sdk"));
+  EXPECT_EQ(sdk.GetVersion(), llvm::VersionTuple(2, 0));
+  EXPECT_EQ(sdk.IsAppleInternalSDK(), true);
+  XcodeSDK empty;
+  empty.Merge(XcodeSDK("MacOSX10.14.Internal.sdk"));
+  EXPECT_EQ(empty.GetString(), llvm::StringRef("MacOSX10.14.Internal.sdk"));
 }
 
 TEST(XcodeSDKTest, SDKSupportsModules) {
   std::string base = "/Applications/Xcode.app/Contents/Developer/Platforms/";
   EXPECT_TRUE(XcodeSDK::SDKSupportsModules(
       XcodeSDK::Type::iPhoneSimulator,
       FileSpec(
           base +
           "iPhoneSimulator.platform/Developer/SDKs/iPhoneSimulator12.0.sdk")));
+  EXPECT_TRUE(XcodeSDK::SDKSupportsModules(
+      XcodeSDK::Type::iPhoneSimulator,
+      FileSpec(base + "iPhoneSimulator.platform/Developer/SDKs/"
+                      "iPhoneSimulator12.0.Internal.sdk")));
   EXPECT_FALSE(XcodeSDK::SDKSupportsModules(
       XcodeSDK::Type::iPhoneSimulator,
       FileSpec(
           base +
           "iPhoneSimulator.platform/Developer/SDKs/iPhoneSimulator7.2.sdk")));
   EXPECT_TRUE(XcodeSDK::SDKSupportsModules(
       XcodeSDK::Type::MacOSX,
       FileSpec(base + "MacOSX.platform/Developer/SDKs/MacOSX10.10.sdk")));
   EXPECT_FALSE(XcodeSDK::SDKSupportsModules(
       XcodeSDK::Type::MacOSX,
       FileSpec(base + "MacOSX.platform/Developer/SDKs/MacOSX10.9.sdk")));
 }
 
-TEST(XcodeSDKTest, GetSDKNameForType) {
-  EXPECT_EQ("macosx", XcodeSDK::GetSDKNameForType(XcodeSDK::Type::MacOSX));
-  EXPECT_EQ("iphonesimulator",
-            XcodeSDK::GetSDKNameForType(XcodeSDK::Type::iPhoneSimulator));
-  EXPECT_EQ("iphoneos", XcodeSDK::GetSDKNameForType(XcodeSDK::Type::iPhoneOS));
-  EXPECT_EQ("appletvsimulator",
-            XcodeSDK::GetSDKNameForType(XcodeSDK::Type::AppleTVSimulator));
-  EXPECT_EQ("appletvos",
-            XcodeSDK::GetSDKNameForType(XcodeSDK::Type::AppleTVOS));
-  EXPECT_EQ("watchsimulator",
-            XcodeSDK::GetSDKNameForType(XcodeSDK::Type::WatchSimulator));
-  EXPECT_EQ("watchos", XcodeSDK::GetSDKNameForType(XcodeSDK::Type::watchOS));
-  EXPECT_EQ("linux", XcodeSDK::GetSDKNameForType(XcodeSDK::Type::Linux));
-  EXPECT_EQ("", XcodeSDK::GetSDKNameForType(XcodeSDK::Type::numSDKTypes));
-  EXPECT_EQ("", XcodeSDK::GetSDKNameForType(XcodeSDK::Type::unknown));
+TEST(XcodeSDKTest, GetCanonicalName) {
+  XcodeSDK::Info info;
+  info.type = XcodeSDK::Type::MacOSX;
+  EXPECT_EQ("macosx", XcodeSDK::GetCanonicalName(info));
+
+  info.type = XcodeSDK::Type::iPhoneSimulator;
+  EXPECT_EQ("iphonesimulator", XcodeSDK::GetCanonicalName(info));
+
+  info.type = XcodeSDK::Type::iPhoneOS;
+  EXPECT_EQ("iphoneos", XcodeSDK::GetCanonicalName(info));
+
+  info.type = XcodeSDK::Type::AppleTVSimulator;
+  EXPECT_EQ("appletvsimulator", XcodeSDK::GetCanonicalName(info));
+
+  info.type = XcodeSDK::Type::AppleTVOS;
+  EXPECT_EQ("appletvos", XcodeSDK::GetCanonicalName(info));
+
+  info.type = XcodeSDK::Type::WatchSimulator;
+  EXPECT_EQ("watchsimulator", XcodeSDK::GetCanonicalName(info));
+
+  info.type = XcodeSDK::Type::watchOS;
+  EXPECT_EQ("watchos", XcodeSDK::GetCanonicalName(info));
+
+  info.type = XcodeSDK::Type::Linux;
+  EXPECT_EQ("linux", XcodeSDK::GetCanonicalName(info));
+
+  info.type = XcodeSDK::Type::numSDKTypes;
+  EXPECT_EQ("", XcodeSDK::GetCanonicalName(info));
+
+  info.type = XcodeSDK::Type::unknown;
+  EXPECT_EQ("", XcodeSDK::GetCanonicalName(info));
+
+  info.internal = true;
+  info.type = XcodeSDK::Type::MacOSX;
+  EXPECT_EQ("macosx.internal", XcodeSDK::GetCanonicalName(info));
+
+  info.type = XcodeSDK::Type::iPhoneSimulator;
+  EXPECT_EQ("iphonesimulator.internal", XcodeSDK::GetCanonicalName(info));
+
+  info.type = XcodeSDK::Type::iPhoneOS;
+  EXPECT_EQ("iphoneos.internal", XcodeSDK::GetCanonicalName(info));
+
+  info.type = XcodeSDK::Type::AppleTVSimulator;
+  EXPECT_EQ("appletvsimulator.internal", XcodeSDK::GetCanonicalName(info));
+
+  info.type = XcodeSDK::Type::AppleTVOS;
+  EXPECT_EQ("appletvos.internal", XcodeSDK::GetCanonicalName(info));
+
+  info.type = XcodeSDK::Type::WatchSimulator;
+  EXPECT_EQ("watchsimulator.internal", XcodeSDK::GetCanonicalName(info));
+
+  info.type = XcodeSDK::Type::watchOS;
+  EXPECT_EQ("watchos.internal", XcodeSDK::GetCanonicalName(info));
+
+  info.type = XcodeSDK::Type::MacOSX;
+  info.version = llvm::VersionTuple(10, 9);
+  EXPECT_EQ("macosx10.9.internal", XcodeSDK::GetCanonicalName(info));
+
+  info.type = XcodeSDK::Type::iPhoneOS;
+  info.version = llvm::VersionTuple(7, 0);
+  EXPECT_EQ("iphoneos7.0.internal", XcodeSDK::GetCanonicalName(info));
 }