[lldb][HostInfoMacOSX] Try to use DW_AT_LLVM_sysroot instead of xcrun when looking up SDK

[Michael137]

GetSDKRoot uses xcrun to find an SDK root path for a given SDK version string. But if the SDK doesn't exist in the Xcode installations, but instead lives in the CommandLineTools, xcrun will fail to find it. Negative searches for an SDK path cost a lot (a few seconds) each time xcrun is invoked. We do cache negative results in find_cached_path inside LLDB, but we would still pay the price on every new debug session the first time we evaluate an expression. This doesn't only cause a noticable delay in running the expression, but also generates following error:

error: Error while searching for Xcode SDK: timed out waiting for shell command to complete
(int) $0 = 42

In this patch we avoid these possibly expensive calls to xcrun by checking the DW_AT_LLVM_sysroot, and if it exists, using that as the SDK path. We need an explicit check for the CommandLineTools path before we call RegisterXcodeSDK, because that will try to call xcrun. This won't prevent other uses of GetSDKRoot popping up that cause us to make expensive xcrun calls, but for now this addresses the regression in the expression evaluator. We also had to adjust the XcodeSDK::Merge logic to update the sysroot. There is one case for which this wouldn't make sense: if a CU was compiled with CommandLineTools and a different one with an older internal SDK, in that case we would update the CommandLineTools sysroot with a .Internal.sdk prefix, which won't possibly exist for CommandLineTools. I added a unit-test for this. Not sure if we want to explicitly detect and disallow this, given it's quite a niche scenario.

rdar://113619904
rdar://113619723

[llvmbot]

@llvm/pr-subscribers-lldb

Author: Michael Buch (Michael137)

Changes

GetSDKRoot uses xcrun to find an SDK root path for a given SDK version string. But if the SDK doesn't exist in the Xcode installations, but instead lives in the CommandLineTools, xcrun will fail to find it. Negative searches for an SDK path cost a lot (a few seconds) each time xcrun is invoked. We do cache negative results in find_cached_path inside LLDB, but we would still pay the price on every new debug session the first time we evaluate an expression. This doesn't only cause a noticable delay in running the expression, but also generates following error:

error: Error while searching for Xcode SDK: timed out waiting for shell command to complete
(int) $0 = 42

To avoid this xcrun penalty, we search CommandLineTools for a matching SDK ourselves, and only if we don't find it, do we fall back to calling xcrun.

rdar://113619904
rdar://113619723

---

Full diff: https://github.com/llvm/llvm-project/pull/128712.diff

2 Files Affected:

• (modified) lldb/include/lldb/Host/FileSystem.h (+3-2)
• (modified) lldb/source/Host/macosx/objcxx/HostInfoMacOSX.mm (+60)

diff --git a/lldb/include/lldb/Host/FileSystem.h b/lldb/include/lldb/Host/FileSystem.h
index 640f3846e448c..4128d7b012041 100644
--- a/lldb/include/lldb/Host/FileSystem.h
+++ b/lldb/include/lldb/Host/FileSystem.h
@@ -183,8 +183,9 @@ class FileSystem {
     eEnumerateDirectoryResultQuit
   };
 
-  typedef EnumerateDirectoryResult (*EnumerateDirectoryCallbackType)(
-      void *baton, llvm::sys::fs::file_type file_type, llvm::StringRef);
+  typedef std::function<EnumerateDirectoryResult(
+      void *baton, llvm::sys::fs::file_type file_type, llvm::StringRef)>
+      EnumerateDirectoryCallbackType;
 
   typedef std::function<EnumerateDirectoryResult(
       llvm::sys::fs::file_type file_type, llvm::StringRef)>
diff --git a/lldb/source/Host/macosx/objcxx/HostInfoMacOSX.mm b/lldb/source/Host/macosx/objcxx/HostInfoMacOSX.mm
index 6e924fdc684cf..a94fd3b57f9d6 100644
--- a/lldb/source/Host/macosx/objcxx/HostInfoMacOSX.mm
+++ b/lldb/source/Host/macosx/objcxx/HostInfoMacOSX.mm
@@ -15,11 +15,14 @@
 #include "lldb/Utility/Log.h"
 #include "lldb/Utility/Timer.h"
 
+#include "clang/Basic/DarwinSDKInfo.h"
 #include "llvm/ADT/ScopeExit.h"
 #include "llvm/ADT/SmallString.h"
 #include "llvm/ADT/StringMap.h"
+#include "llvm/Support/Error.h"
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/Path.h"
+#include "llvm/Support/VersionTuple.h"
 #include "llvm/Support/raw_ostream.h"
 
 // C++ Includes
@@ -569,10 +572,52 @@ static bool ResolveAndVerifyCandidateSupportDir(FileSpec &path) {
     cache.insert({key, {error, true}});
     return llvm::createStringError(llvm::inconvertibleErrorCode(), error);
   }
+
+  if (path_or_err->empty())
+    return llvm::createStringError("Empty path determined for '%s'",
+                                   key.data());
+
   auto it_new = cache.insert({key, {*path_or_err, false}});
   return it_new.first->second.str;
 }
 
+static llvm::Expected<std::string>
+GetCommandLineToolsSDKRoot(llvm::VersionTuple version) {
+  std::string clt_root_dir;
+  FileSystem::Instance().EnumerateDirectory(
+      "/Library/Developer/CommandLineTools/SDKs/", /*find_directories=*/true,
+      /*find_files=*/false, /*find_other=*/false,
+      [&](void *baton, llvm::sys::fs::file_type file_type,
+          llvm::StringRef name) {
+        assert(file_type == llvm::sys::fs::file_type::directory_file);
+
+        if (!name.ends_with(".sdk"))
+          return FileSystem::eEnumerateDirectoryResultNext;
+
+        llvm::Expected<std::optional<clang::DarwinSDKInfo>> sdk_info =
+            clang::parseDarwinSDKInfo(
+                *FileSystem::Instance().GetVirtualFileSystem(), name);
+        if (!sdk_info) {
+          LLDB_LOG_ERROR(GetLog(LLDBLog::Expressions), sdk_info.takeError(),
+                         "Error while parsing {1}: {0}", name);
+          return FileSystem::eEnumerateDirectoryResultNext;
+        }
+
+        if (!*sdk_info)
+          return FileSystem::eEnumerateDirectoryResultNext;
+
+        if (version == (*sdk_info)->getVersion()) {
+          clt_root_dir = name;
+          return FileSystem::eEnumerateDirectoryResultQuit;
+        }
+
+        return FileSystem::eEnumerateDirectoryResultNext;
+      },
+      /*baton=*/nullptr);
+
+  return clt_root_dir;
+}
+
 llvm::Expected<llvm::StringRef> HostInfoMacOSX::GetSDKRoot(SDKOptions options) {
   static llvm::StringMap<ErrorOrPath> g_sdk_path;
   static std::mutex g_sdk_path_mutex;
@@ -581,6 +626,21 @@ static bool ResolveAndVerifyCandidateSupportDir(FileSpec &path) {
                                    "XcodeSDK not specified");
   XcodeSDK sdk = *options.XcodeSDKSelection;
   auto key = sdk.GetString();
+
+  // xcrun doesn't search SDKs in the CommandLineTools (CLT) directory. So if
+  // a program was compiled against a CLT SDK, but that SDK wasn't present in
+  // any of the Xcode installations, then xcrun would fail to find the SDK
+  // (which is expensive). To avoid this we first try to find the specified SDK
+  // in the CLT directory.
+  auto clt_root_dir = find_cached_path(g_sdk_path, g_sdk_path_mutex, key, [&] {
+    return GetCommandLineToolsSDKRoot(sdk.GetVersion());
+  });
+
+  if (clt_root_dir)
+    return clt_root_dir;
+  else
+    llvm::consumeError(clt_root_dir.takeError());
+
   return find_cached_path(g_sdk_path, g_sdk_path_mutex, key, [&](){
     return GetXcodeSDK(sdk);
   });

[Michael137]

Is this a dependency we can pull in? Probably not?

[JDevlieghere]

Yeah, this goes against all the work of localizing the places we depend on clang.

[Michael137]

Alternatively we can scan the .plist using LLDB's ApplePropertyList, and extract the SDK version/name from there. That would allow us not to depend on Clang here

[Michael137]

Hmm but looks like it's only able to parse the XML version, not the binary one?

[Michael137]

I guess worst case we could read the json ourselves and get the key we're looking for (Clang does a bit more than that, it looks at VersionMappings etc.)

[JDevlieghere]

I think it should support both. I'm sure it can write binary property lists. I would assume it can read them too.

[JDevlieghere]

This feels like it's working around a shortcoming of xcrun. Since that's somewhat within our control, have we asked the team to look into the CLT for an SDK? They already do for binaries so I'm not sure why the SDK would need to behave differently.

[JDevlieghere]

Yeah, this goes against all the work of localizing the places we depend on clang.

[JDevlieghere]

Does this mean we're going to prefer the CommandLineTools SDK over the Xcode one when both exist? I know they should be the same, but I'd really prefer to avoid the CLT whenever we can.

[adrian-prantl]

That's my only concern with this, but if we wanted to avoid this behavior, then we would need to reimplement all of xcrun's logic to find SDKs in Xcode. That's also not something I'm excited about.

[JDevlieghere]

Yeah, you don't know you need the CLT until you know it's not in Xcode, which is exactly the problem this is trying to avoid.

[Michael137]

Ok so latest approach is to check the DW_AT_LLVM_sysroot, and if that path exists, we don't run xcrun. I removed the GetSDKPathFromDebugInfo overload that iterated over all CUs and merged the XcodeSDK objects (@adrian-prantl said this was flawed anyway and we shouldn't require this overload). So now I'm just looking at the first CUs DW_AT_LLVM_sysroot.

[JDevlieghere]

I definitely like this approach much better. Two suggestions:

• Should this use a FileSpec instead of a std::string for the sysroot?
• I like the simplicity of a std::pair but on the other hand it's hard to tell what the string represents. I think it would help to either store the sysroot in the XcodeSDK (potentially as an std::optional) or having this return a struct with named fiels (e.g. xcode_sdk, sysroot). Putting the sysroot in the XcodeSDK means you don't have to update the Doxygen comments which are now all outdated.

[Michael137]

I definitely like this approach much better. Two suggestions:

• Should this use a FileSpec instead of a std::string for the sysroot?

Yea I don't mind doing that. There's no concrete need for it but it's easier to reason about than a raw string I suppose

• I like the simplicity of a std::pair but on the other hand it's hard to tell what the string represents. I think it would help to either store the sysroot in the XcodeSDK (potentially as an std::optional) or having this return a struct with named fiels (e.g. xcode_sdk, sysroot). Putting the sysroot in the XcodeSDK means you don't have to update the Doxygen comments which are now all outdated.

Agreed. I was toying around with storing it inside XcodeSDK. Though it's a bit awkward to have a concrete sysroot path since the XcodeSDK class currently represents a merged view of all SDKs (which may have different sysroots). Or at least that's how I interpreted this class. Happy to change it to a named structure. Don't have a strong opinion on this

[Michael137]

Regarding source mapping tests, see #129526

[Michael137]

Updated the PR to store the DW_AT_LLVM_sysroot as a FileSpec inside XcodeSDK. Also updated the PR description explaining this a bit more

[JDevlieghere]

LGTM. Thanks!