Move the Xcode SDK path caching to HostInfo

[aprantl]

Summary

When debugging a remote platform, the platform you get from
GetPlatformForArchitecture doesn't inherit from PlatformDarwin.
HostInfoMacOSX seems like the right place to have a global store of
local paths.

[JDevlieghere]

LGTM modulo the inline comment.

lldb/include/lldb/Host/HostInfoBase.h

96

This should probably be named GetXcodeSDKDirectory for consistency with the other methods.

[labath]

Since the option of going to the "host platform" was discussed already and rejected, I think a new discussion is in order before going back to it. What are the exact circumstances where you did not get a PlatformDarwin object?

Note that I am not opposed (and I never was) having the list of xcode sdk be provided by the "host". However, I don't think it's a good idea to route all of this through the host *platform*, for a couple of reasons:

• it's completely redundant (if I "inline" GetHostPlatform()->GetSDKPath(sdk) I get exactly HostInfo::GetXcodeSDK(sdk))
• it makes it appear like it does more things than it actually does (asking PlatformLinux for an sdk does nothing, and it's not even clear if it should have that api)
• it removes the only usage of "platform" in module code (using platforms in modules is very questionable because it can break caching. In fact, the only reason this usage is not questionable is because the function always returns the exact same value it gets from the host).

[aprantl]

In D79364#2019818, @labath wrote:

Since the option of going to the "host platform" was discussed already and rejected, I think a new discussion is in order before going back to it. What are the exact circumstances where you did not get a PlatformDarwin object?

You're right. The gist is that the old scheme did not work for debugging iOS processes from a macOS host. The fundamental issue was that GetPlatformForArchitecture chooses the first out of a list of possible platforms, and I the first hit would be a PlatformRemoteGDBServer, which does not inherit from PlatformDarwin. Basically, inside of Module, we don't have enough information (we'd need a Target) to select a meaningful platform.

Note that I am not opposed (and I never was) having the list of xcode sdk be provided by the "host". However, I don't think it's a good idea to route all of this through the host *platform*, for a couple of reasons:

• it's completely redundant (if I "inline" GetHostPlatform()->GetSDKPath(sdk) I get exactly HostInfo::GetXcodeSDK(sdk))
• it makes it appear like it does more things than it actually does (asking PlatformLinux for an sdk does nothing, and it's not even clear if it should have that api)
• it removes the only usage of "platform" in module code (using platforms in modules is very questionable because it can break caching. In fact, the only reason this usage is not questionable is because the function always returns the exact same value it gets from the host).

Good point! The reason why I went with Platform::GetHostPlatform() over HostInfo::GetXcodeSDK(sdk)) was because I hadn't realized that I already put a default implementation of GetXcodeSDK into HostInfoBase, and I wanted to avoid guarding this in an ugly #if APPLE. But I think that that is completely unnecessary.

I'll change it to calling HostInfo directly!

[aprantl]

Removed the Platform detour.

[labath]

I think this is right, but it looks like you uploaded a diff based on the previous version of the patch instead of the master.

In D79364#2020728, @aprantl wrote:

Good point! The reason why I went with Platform::GetHostPlatform() over HostInfo::GetXcodeSDK(sdk)) was because I hadn't realized that I already put a default implementation of GetXcodeSDK into HostInfoBase, and I wanted to avoid guarding this in an ugly #if APPLE. But I think that that is completely unnecessary.

Yeah, having a noop implementation in HostInfoBase is consistent with what we do elsewhere, and with llvm support library for that matter (TBE, the support library usually has an additional bool isFeatureXAvailable() function and the other functions become llvm_unreachable if the feature is really unavailable, but I don't think that's particularly useful for an api with a single entry point).)

lldb/include/lldb/Host/HostInfoBase.h

public:
   /// "augment" these using information from the host and return the resulting
   /// ArchSpec object.
   static ArchSpec GetAugmentedArchSpec(llvm::StringRef triple);
 
   static bool ComputePathRelativeToLibrary(FileSpec &file_spec,
                                            llvm::StringRef dir);
 
   /// Return the directory containing a specific Xcode SDK.
-  static std::string GetXcodeSDK(XcodeSDK sdk) { return {}; }
+  static llvm::StringRef GetXcodeSDKPath(XcodeSDK sdk) { return {}; }
 
 protected:
   static bool ComputeSharedLibraryDirectory(FileSpec &file_spec);
   static bool ComputeSupportExeDirectory(FileSpec &file_spec);
   static bool ComputeProcessTempFileDirectory(FileSpec &file_spec);
   static bool ComputeGlobalTempFileDirectory(FileSpec &file_spec);
   static bool ComputeTempFileBaseDirectory(FileSpec &file_spec);
   static bool ComputeHeaderDirectory(FileSpec &file_spec);

lldb/include/lldb/Host/macosx/HostInfoMacOSX.h

public:
   static llvm::VersionTuple GetOSVersion();
   static llvm::VersionTuple GetMacCatalystVersion();
   static bool GetOSBuildString(std::string &s);
   static bool GetOSKernelDescription(std::string &s);
   static FileSpec GetProgramFileSpec();
   static std::string FindXcodeContentsDirectoryInPath(llvm::StringRef path);
 
   /// Query xcrun to find an Xcode SDK directory.
-  static std::string GetXcodeSDK(XcodeSDK sdk);
+  static llvm::StringRef GetXcodeSDKPath(XcodeSDK sdk);
 protected:
   static bool ComputeSupportExeDirectory(FileSpec &file_spec);
   static void ComputeHostArchitectureSupport(ArchSpec &arch_32,
                                              ArchSpec &arch_64);
   static bool ComputeHeaderDirectory(FileSpec &file_spec);
   static bool ComputeSystemPluginsDirectory(FileSpec &file_spec);
   static bool ComputeUserPluginsDirectory(FileSpec &file_spec);
 };
 }
 
 #endif

lldb/include/lldb/Target/Platform.h

 #include "lldb/Host/File.h"
 #include "lldb/Interpreter/Options.h"
 #include "lldb/Utility/ArchSpec.h"
 #include "lldb/Utility/ConstString.h"
 #include "lldb/Utility/FileSpec.h"
 #include "lldb/Utility/StructuredData.h"
 #include "lldb/Utility/Timeout.h"
 #include "lldb/Utility/UserIDResolver.h"
-#include "lldb/Utility/XcodeSDK.h"
 #include "lldb/lldb-private-forward.h"
 #include "lldb/lldb-public.h"
 #include "llvm/Support/VersionTuple.h"
 
 namespace lldb_private {
 
 class ProcessInstanceInfo;
 class ProcessInstanceInfoMatch;
public:
 
   // Given a target, find the local SDK directory if one exists on the current
   // host.
   virtual lldb_private::ConstString
   GetSDKDirectory(lldb_private::Target &target) {
     return lldb_private::ConstString();
   }
 
-  virtual llvm::StringRef GetSDKPath(lldb_private::XcodeSDK sdk) { return {}; }
-
   const std::string &GetRemoteURL() const { return m_remote_url; }
 
   bool IsHost() const {
     return m_is_host; // Is this the default host platform?
   }
 
   bool IsRemote() const { return !m_is_host; }
 

lldb/source/Core/Module.cpp

 #include "lldb/Core/Debugger.h"
 #include "lldb/Core/FileSpecList.h"
 #include "lldb/Core/Mangled.h"
 #include "lldb/Core/ModuleSpec.h"
 #include "lldb/Core/SearchFilter.h"
 #include "lldb/Core/Section.h"
 #include "lldb/Host/FileSystem.h"
 #include "lldb/Host/Host.h"
+#include "lldb/Host/HostInfo.h"
 #include "lldb/Interpreter/CommandInterpreter.h"
 #include "lldb/Interpreter/ScriptInterpreter.h"
 #include "lldb/Symbol/CompileUnit.h"
 #include "lldb/Symbol/Function.h"
 #include "lldb/Symbol/ObjectFile.h"
 #include "lldb/Symbol/Symbol.h"
 #include "lldb/Symbol/SymbolContext.h"
 #include "lldb/Symbol/SymbolFile.h"
 #include "lldb/Symbol/SymbolVendor.h"
 #include "lldb/Symbol/Symtab.h"
 #include "lldb/Symbol/Type.h"
 #include "lldb/Symbol/TypeList.h"
 #include "lldb/Symbol/TypeMap.h"
 #include "lldb/Symbol/TypeSystem.h"
 #include "lldb/Target/Language.h"
-#include "lldb/Target/Platform.h"
 #include "lldb/Target/Process.h"
 #include "lldb/Target/Target.h"
 #include "lldb/Utility/DataBufferHeap.h"
 #include "lldb/Utility/LLDBAssert.h"
 #include "lldb/Utility/Log.h"
 #include "lldb/Utility/Logging.h"
 #include "lldb/Utility/RegularExpression.h"
 #include "lldb/Utility/Status.h"

 bool Module::RemapSourceFile(llvm::StringRef path,
                              std::string &new_path) const {
   std::lock_guard<std::recursive_mutex> guard(m_mutex);
   return m_source_mappings.RemapPath(path, new_path);
 }
 
 void Module::RegisterXcodeSDK(llvm::StringRef sdk_name, llvm::StringRef sysroot) {
   XcodeSDK sdk(sdk_name.str());
-  PlatformSP module_platform =
-      Platform::GetPlatformForArchitecture(GetArchitecture(), nullptr);
-  ConstString sdk_path(module_platform->GetSDKPath(sdk));
+  ConstString sdk_path(HostInfo::GetXcodeSDKPath(sdk));
   if (!sdk_path)
     return;
-  // If merged SDK changed for a previously registered source path, update it.
+  // If the SDK changed for a previously registered source path, update it.
   // This could happend with -fdebug-prefix-map, otherwise it's unlikely.
   ConstString sysroot_cs(sysroot);
   if (!m_source_mappings.Replace(sysroot_cs, sdk_path, true))
     // In the general case, however, append it to the list.
     m_source_mappings.Append(sysroot_cs, sdk_path, false);
 }
 
 bool Module::MergeArchitecture(const ArchSpec &arch_spec) {

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) {
+static std::string GetXcodeSDK(XcodeSDK sdk) {
   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 =
while (path.empty()) {
     return {};
   }
 
   // Whatever is left in output should be a valid path.
   if (!FileSystem::Instance().Exists(path))
     return {};
   return path;
 }
+
+llvm::StringRef HostInfoMacOSX::GetXcodeSDKPath(XcodeSDK sdk) {
+  static llvm::StringMap<std::string> g_sdk_path;
+  static std::mutex g_sdk_path_mutex;
+
+  std::lock_guard<std::mutex> guard(g_sdk_path_mutex);
+  std::string &path = g_sdk_path[sdk.GetString()];
+  if (path.empty())
+    path = GetXcodeSDK(sdk);
+  return path;
+}

lldb/source/Plugins/Platform/MacOSX/PlatformDarwin.h

public:
   LaunchProcess(lldb_private::ProcessLaunchInfo &launch_info) override;
 
   static std::tuple<llvm::VersionTuple, llvm::StringRef>
   ParseVersionBuildDir(llvm::StringRef str);
 
   llvm::Expected<lldb_private::StructuredData::DictionarySP>
   FetchExtendedCrashInformation(lldb_private::Process &process) override;
 
-  llvm::StringRef GetSDKPath(lldb_private::XcodeSDK sdk) override;
-
   static lldb_private::FileSpec GetXcodeContentsDirectory();
   static lldb_private::FileSpec GetXcodeDeveloperDirectory();
 
   /// Return the toolchain directory the current LLDB instance is located in.
   static lldb_private::FileSpec GetCurrentToolchainDirectory();
 
   /// Return the command line tools directory the current LLDB instance is
   /// located in.

lldb/source/Plugins/Platform/MacOSX/PlatformDarwin.cpp

if (it->endswith(".app")) {
         return buffer.str().str();
       }
     }
   }
 
   return {};
 }
 
-llvm::StringRef PlatformDarwin::GetSDKPath(XcodeSDK sdk) {
-  std::lock_guard<std::mutex> guard(m_sdk_path_mutex);
-  std::string &path = m_sdk_path[sdk.GetString()];
-  if (path.empty())
-    path = HostInfo::GetXcodeSDK(sdk);
-  return path;
-}
-
 FileSpec PlatformDarwin::GetXcodeContentsDirectory() {
   static FileSpec g_xcode_contents_path;
   static std::once_flag g_once_flag;
   std::call_once(g_once_flag, [&]() {
     // Try the shlib dir first.
     if (FileSpec fspec = HostInfo::GetShlibDir()) {
       if (FileSystem::Instance().Exists(fspec)) {
         std::string xcode_contents_dir =
if (const char *developer_dir_env_var = getenv("DEVELOPER_DIR")) {
             FindXcodeContentsDirectoryInPath(fspec.GetPath());
         if (!xcode_contents_dir.empty()) {
           g_xcode_contents_path = FileSpec(xcode_contents_dir);
           return;
         }
       }
     }
 
-    FileSpec fspec(HostInfo::GetXcodeSDK(XcodeSDK::GetAnyMacOS()));
+    FileSpec fspec(HostInfo::GetXcodeSDKPath(XcodeSDK::GetAnyMacOS()));
     if (fspec) {
       if (FileSystem::Instance().Exists(fspec)) {
         std::string xcode_contents_dir =
             FindXcodeContentsDirectoryInPath(fspec.GetPath());
         if (!xcode_contents_dir.empty()) {
           g_xcode_contents_path = FileSpec(xcode_contents_dir);
           return;
         }

lldb/source/Plugins/Platform/MacOSX/PlatformMacOSX.cpp

sdk_path.Printf("%s/Developer/Platforms/MacOSX.platform/Developer/"
                     "SDKs/MacOSX%u.%u.sdk",
                     fspec.GetPath().c_str(), version.getMajor(),
                     version.getMinor().getValue());
     if (FileSystem::Instance().Exists(fspec))
       return ConstString(sdk_path.GetString());
   }
 
   // Use the default SDK as a fallback.
-  FileSpec fspec(HostInfo::GetXcodeSDK(lldb_private::XcodeSDK::GetAnyMacOS()));
+  FileSpec fspec(
+      HostInfo::GetXcodeSDKPath(lldb_private::XcodeSDK::GetAnyMacOS()));
   if (fspec) {
     if (FileSystem::Instance().Exists(fspec))
       return ConstString(fspec.GetPath());
   }
 
   return {};
 }
 

lldb/unittests/Host/HostInfoTest.cpp

 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());
+  EXPECT_FALSE(HostInfo::GetXcodeSDKPath(XcodeSDK("MacOSX.sdk")).empty());
   // These are expected to fall back to an available version.
-  EXPECT_FALSE(HostInfo::GetXcodeSDK(XcodeSDK("MacOSX9999.sdk")).empty());
+  EXPECT_FALSE(HostInfo::GetXcodeSDKPath(XcodeSDK("MacOSX9999.sdk")).empty());
   // This is expected to fail.
-  EXPECT_TRUE(HostInfo::GetXcodeSDK(XcodeSDK("CeciNestPasUnOS.sdk")).empty());
+  EXPECT_TRUE(HostInfo::GetXcodeSDKPath(XcodeSDK("CeciNestPasUnOS.sdk")).empty());
 }
 #endif