[lldb] Add another fallback to GetXcodeSDK

[JDevlieghere]

Summary

If we tried getting the SDK from xcrun with the developer dir set and it failed, try without the developer dir.

[aprantl]

So this is adding a fallback that ignores DEVELOPER_DIR if it is set, but doesn't exist? Is that really desirable, or am I misunderstanding the patch? I would expect LLDB to fail hard if the DEVELOPER_DIR in the environment is wrong and not silently ignore it...

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

395

Can we delete this comment? I find it more confusing than helpful...

[JDevlieghere]

In D88866#2313371, @aprantl wrote:

So this is adding a fallback that ignores DEVELOPER_DIR if it is set, but doesn't exist? Is that really desirable, or am I misunderstanding the patch? I would expect LLDB to fail hard if the DEVELOPER_DIR in the environment is wrong and not silently ignore it...

It's slightly more subtle than that, it falls back to running xcrun without a developer dir if we couldn't find the desired SDK with the developer dir set. It doesn't mean the developer dir is invalid or doesn't exist, it just means we couldn't find the SDK there. Imagine for the sake of argument that you're running an Xcode that has no SDKs and that this Xcode is not xcode-selected. Based on lldb's location in LLDB.framework it will do an xcrun with the developer set to the Xcode without any SDKs and that will fail. With this patch, when that happens, we'll fall back to trying the xcode-selected by running xcrun without a developer dir specified. I know it sounds like a contrived example but that's exactly what I was running into...

[aprantl]

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

411

perhaps add

bool have_developer_dir = !developer_dir.empty()`

433

and here if (!have_developer_dir)

to avoid silently ignoring an explicitly specified DEVELOPER_DIR

[aprantl]

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

424

Can you use a fresh std::string with a different name here?

[aprantl]

Apart from a cosmetic detail (inline) this LGTM!

[aprantl]

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

378

In theory we could make this a StringRef and then have one less copy. Not important though.

423

std::string shlib_developer_dir(...); ?

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

llvm::call_once(g_once_flag, []() {
     }
   });
   return g_developer_directory;
 }
 
 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;
-    std::string developer_dir = GetEnvDeveloperDir();
-    if (developer_dir.empty())
-      if (FileSpec fspec = HostInfo::GetShlibDir())
-        if (FileSystem::Instance().Exists(fspec)) {
-          FileSpec path(
-              XcodeSDK::FindXcodeContentsDirectoryInPath(fspec.GetPath()));
-          if (path.RemoveLastPathComponent())
-            developer_dir = path.GetPath();
-        }
+
+  auto xcrun = [](const std::string &sdk,
+                  llvm::StringRef developer_dir = "") -> std::string {
+    std::string xcrun_cmd = "xcrun --show-sdk-path --sdk " + sdk;
     if (!developer_dir.empty())
-      xcrun_cmd = "/usr/bin/env DEVELOPER_DIR=\"" + developer_dir + "\" ";
-    xcrun_cmd += "xcrun --show-sdk-path --sdk " + sdk_name;
+      xcrun_cmd = "/usr/bin/env DEVELOPER_DIR=\"" + developer_dir.str() +
+                  "\" " + xcrun_cmd;
 
     int status = 0;
     int signo = 0;
     std::string output_str;
     lldb_private::Status error =
-        Host::RunShellCommand(xcrun_cmd.c_str(), FileSpec(), &status, &signo,
+        Host::RunShellCommand(xcrun_cmd, 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();
   };
 
+  auto find_sdk = [&xcrun](const std::string &sdk_name) -> std::string {
+    // Invoke xcrun with the developer dir specified in the environment.
+    std::string developer_dir = GetEnvDeveloperDir();
+    if (!developer_dir.empty()) {
+      // Don't fallback if DEVELOPER_DIR was set.
+      return xcrun(sdk_name, developer_dir);
+    }
+
+    // Invoke xcrun with the shlib dir.
+    if (FileSpec fspec = HostInfo::GetShlibDir()) {
+      if (FileSystem::Instance().Exists(fspec)) {
+        std::string contents_dir =
+            XcodeSDK::FindXcodeContentsDirectoryInPath(fspec.GetPath());
+        llvm::StringRef shlib_developer_dir =
+            llvm::sys::path::parent_path(contents_dir);
+        if (!shlib_developer_dir.empty()) {
+          std::string sdk = xcrun(sdk_name, std::move(shlib_developer_dir));
+          if (!sdk.empty())
+            return sdk;
+        }
+      }
+    }
+
+    // Invoke xcrun without a developer dir as a last resort.
+    return xcrun(sdk_name);
+  };
+
   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";