Improve error logging when xcrun fails to execute successfully

[aprantl]

Summary

Because Host::RunShellCommand runs commands through $SHELL there is an opportunity for this to fail spectacularly on systems that use custom shells with odd behaviors. This patch makes these situations easier to debug by at least logging the result of the failed xcrun invocation.

rdar://102389438

[kastiglione]

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

397

Maybe a comment explaining how this is useful in the Types category.

406–407

Why is the timeout increased?

407–414

Should this produce a diagnostic? Seems like failure here should be surfaced to the user.

[aprantl]

Implemented error handling of sorts. Because the computation result is cached and most uses are nested deep in places that have no proper error handling themselves I'm logging to the global error stream.

[JDevlieghere]

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

420

Why not return an Error?

424

Same question.

509

Here too, why not return a StringError with the log message from the live above?

[labath]

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

14

We shouldn't have Host->Core dependencies. Could you report the error in the caller?

411

why not just pass run_in_shell=false (the optional argument coming after the timeout)?

542

ReportError(toString(path_or_err.takeError())) ?

lldb/unittests/Host/HostTest.cpp

40 ↗

(On Diff #475630)

On one (linux) machine I have access to, this is present in /bin/true. A different one has the same file in both paths.

[aprantl]

Address feedback

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

420

For the caller of this lambda "there is no such SDK" is not an error that should be surfaced to the user. There's a loop that will try multiple alternate spellings until it finds a matching SDK.

[aprantl]

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

14

How strong to you fell about this?
It would be an odd interface. GetXcodeSDKPath also caches negative results because negative lookups are really slow. This means we would only raise an error on the first call and not on the subsequent ones. I can do that, but it also feels weird. I guess we could cache the error string as well?

[JDevlieghere]

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

14

I would say pretty strongly. While we still have cycles in LLDB, we haven't introduced new one, at least not that I'm aware off. The diagnostics can be fixed by percolating the error up further and dealing with the errors at the call site, which presumably can depend on Host. Caching the error sounds reasonable to me.

That approach doesn't work for the progress updates though. I can imagine that there's more places in LLDB that don't rely on Core from where we'd like to report progress. One potential solution to that is moving Progress to Utility and replacing the dependency on Core with a callback. Similar to the diagnostics we could make it something that follows the Initialize/Terminate patter and then keeps a static list of callbacks for reporting progress. Definitely not the most elegant solution, but it could work for both Progress and Diagnostics if we wanted to.

[aprantl]

Don't introduce a Host->Core dependency. Cache the errors and remove Progress reports (since they also depend on Core).

[JDevlieghere]

LGTM

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

10

342–344

lldb/include/lldb/Host/HostInfoBase.h

public:
 
   static bool ComputePathRelativeToLibrary(FileSpec &file_spec,
                                            llvm::StringRef dir);
 
   static FileSpec GetXcodeContentsDirectory() { return {}; }
   static FileSpec GetXcodeDeveloperDirectory() { return {}; }
   
   /// Return the directory containing a specific Xcode SDK.
-  static llvm::StringRef GetXcodeSDKPath(XcodeSDK sdk) { return {}; }
+  static llvm::Expected<llvm::StringRef> GetXcodeSDKPath(XcodeSDK sdk) {
+    return "";
+  }
 
   /// Return information about module \p image_name if it is loaded in
   /// the current process's address space.
   static SharedCacheImageInfo
   GetSharedCacheImageInfo(llvm::StringRef image_name) {
     return {};
   }
 

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

public:
   static llvm::VersionTuple GetOSVersion();
   static llvm::VersionTuple GetMacCatalystVersion();
   static llvm::Optional<std::string> GetOSBuildString();
   static FileSpec GetProgramFileSpec();
   static FileSpec GetXcodeContentsDirectory();
   static FileSpec GetXcodeDeveloperDirectory();
 
   /// Query xcrun to find an Xcode SDK directory.
-  static llvm::StringRef GetXcodeSDKPath(XcodeSDK sdk);
+  static llvm::Expected<llvm::StringRef> GetXcodeSDKPath(XcodeSDK sdk);
 
   /// Shared cache utilities
   static SharedCacheImageInfo
   GetSharedCacheImageInfo(llvm::StringRef image_name);
 
 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/source/Core/Module.cpp

Module::RemapSourceFile(llvm::StringRef path) const {
   if (auto remapped = m_source_mappings.RemapPath(path))
     return remapped->GetPath();
   return {};
 }
 
 void Module::RegisterXcodeSDK(llvm::StringRef sdk_name,
                               llvm::StringRef sysroot) {
   XcodeSDK sdk(sdk_name.str());
-  llvm::StringRef sdk_path(HostInfo::GetXcodeSDKPath(sdk));
+  auto sdk_path_or_err = HostInfo::GetXcodeSDKPath(sdk);
+
+  if (!sdk_path_or_err) {
+    Debugger::ReportError("Error while searching for Xcode SDK: " +
+                          toString(sdk_path_or_err.takeError()));
+    return;
+  }
+
+  auto sdk_path = *sdk_path_or_err;
   if (sdk_path.empty())
     return;
   // If the SDK changed for a previously registered source path, update it.
   // This could happend with -fdebug-prefix-map, otherwise it's unlikely.
   if (!m_source_mappings.Replace(sysroot, sdk_path, true))
     // In the general case, however, append it to the list.
     m_source_mappings.Append(sysroot, sdk_path, false);
 }

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

 //===-- HostInfoMacOSX.mm ---------------------------------------*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
+#include "lldb/Host/macosx/HostInfoMacOSX.h"
+#include "Utility/UuidCompatibility.h"
 #include "lldb/Host/FileSystem.h"
 #include "lldb/Host/Host.h"
 #include "lldb/Host/HostInfo.h"
-#include "lldb/Host/macosx/HostInfoMacOSX.h"
 #include "lldb/Utility/Args.h"
 #include "lldb/Utility/LLDBLog.h"
 #include "lldb/Utility/Log.h"
 #include "lldb/Utility/Timer.h"
-#include "Utility/UuidCompatibility.h"
 
 #include "llvm/ADT/ScopeExit.h"
 #include "llvm/ADT/SmallString.h"
 #include "llvm/ADT/StringMap.h"
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/raw_ostream.h"
 
if (!env_developer_dir.empty()) {
       std::string xcode_contents_dir =
           XcodeSDK::FindXcodeContentsDirectoryInPath(env_developer_dir);
       if (!xcode_contents_dir.empty()) {
         g_xcode_contents_path = FileSpec(xcode_contents_dir);
         return;
       }
     }
 
-    FileSpec fspec(HostInfo::GetXcodeSDKPath(XcodeSDK::GetAnyMacOS()));
+    auto sdk_path_or_err = HostInfo::GetXcodeSDKPath(XcodeSDK::GetAnyMacOS());
+    if (!sdk_path_or_err) {
+      Log *log = GetLog(LLDBLog::Host);
+      LLDB_LOGF(log, "Error while searching for Xcode SDK: %s",
+                toString(sdk_path_or_err.takeError()).c_str());
+      return;
+    }
+    FileSpec fspec(*sdk_path_or_err);
     if (fspec) {
       if (FileSystem::Instance().Exists(fspec)) {
         std::string xcode_contents_dir =
             XcodeSDK::FindXcodeContentsDirectoryInPath(fspec.GetPath());
         if (!xcode_contents_dir.empty()) {
           g_xcode_contents_path = FileSpec(xcode_contents_dir);
           return;
         }
if (FileSpec fspec = GetXcodeContentsDirectory()) {
       fspec.AppendPathComponent("Developer");
       if (FileSystem::Instance().Exists(fspec))
         g_developer_directory = fspec;
     }
   });
   return g_developer_directory;
 }
 
-static std::string GetXcodeSDK(XcodeSDK sdk) {
+llvm::Expected<std::string> GetXcodeSDK(XcodeSDK sdk) {
   XcodeSDK::Info info = sdk.Parse();
   std::string sdk_name = XcodeSDK::GetCanonicalName(info);
+  if (sdk_name.empty())
+    return llvm::createStringError(llvm::inconvertibleErrorCode(),
+                                   "Unrecognized SDK type: " + sdk.GetString());
+
+  Log *log = GetLog(LLDBLog::Host);
 
   auto xcrun = [](const std::string &sdk,
-                  llvm::StringRef developer_dir = "") -> std::string {
+                  llvm::StringRef developer_dir =
+                      "") -> llvm::Expected<std::string> {
     Args args;
     if (!developer_dir.empty()) {
       args.AppendArgument("/usr/bin/env");
       args.AppendArgument("DEVELOPER_DIR=" + developer_dir.str());
     }
     args.AppendArgument("/usr/bin/xcrun");
     args.AppendArgument("--show-sdk-path");
     args.AppendArgument("--sdk");
     args.AppendArgument(sdk);
 
     Log *log = GetLog(LLDBLog::Host);
     if (log) {
       std::string cmdstr;
       args.GetCommandString(cmdstr);
       log->Printf("GetXcodeSDK() running shell cmd '%s'", cmdstr.c_str());
     }
 
     int status = 0;
     int signo = 0;
     std::string output_str;
-    lldb_private::Status error =
-        Host::RunShellCommand(args, FileSpec(), &status, &signo, &output_str,
-                              std::chrono::seconds(15));
-
-    // Check that xcrun return something useful.
-    if (status != 0 || output_str.empty())
-      return {};
+    // The first time after Xcode was updated or freshly installed,
+    // xcrun can take surprisingly long to build up its database.
+    auto timeout = std::chrono::seconds(60);
+    bool run_in_shell = false;
+    lldb_private::Status error = Host::RunShellCommand(
+        args, FileSpec(), &status, &signo, &output_str, timeout, run_in_shell);
+
+    // Check that xcrun returned something useful.
+    if (error.Fail()) {
+      // Catastrophic error.
+      LLDB_LOG(log, "xcrun failed to execute: %s", error.AsCString());
+      return error.ToError();
+    }
+    if (status != 0) {
+      // xcrun didn't find a matching SDK. Not an error, we'll try
+      // different spellings.
+      LLDB_LOG(log, "xcrun returned exit code %d", status);
+      return "";
+    }
+    if (output_str.empty()) {
+      LLDB_LOG(log, "xcrun returned no results");
+      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 {
+  auto find_sdk =
+      [&xcrun](const std::string &sdk_name) -> llvm::Expected<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())
+          auto sdk = xcrun(sdk_name, std::move(shlib_developer_dir));
+          if (!sdk)
+            return sdk.takeError();
+          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);
+  auto path_or_err = find_sdk(sdk_name);
+  if (!path_or_err)
+    return path_or_err.takeError();
+  std::string path = *path_or_err;
   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);
+      path_or_err = find_sdk(sdk_name);
+      if (!path_or_err)
+        return path_or_err.takeError();
+      path = *path_or_err;
       if (!path.empty())
         break;
     }
-    Log *log = GetLog(LLDBLog::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);
+      path_or_err = find_sdk(sdk_name);
+      if (!path_or_err)
+        return path_or_err.takeError();
+      path = *path_or_err;
       if (!path.empty())
         break;
     }
 
     LLDB_LOGF(log, "Couldn't find any matching SDK on host");
-    return {};
+    return "";
   }
 
   // Whatever is left in output should be a valid path.
-  if (!FileSystem::Instance().Exists(path))
-    return {};
+  if (!FileSystem::Instance().Exists(path)) {
+    LLDB_LOGF(log, "SDK returned by xcrun doesn't exist");
+    return llvm::createStringError(llvm::inconvertibleErrorCode(),
+                                   "SDK returned by xcrun doesn't exist");
+  }
   return path;
 }
 
-llvm::StringRef HostInfoMacOSX::GetXcodeSDKPath(XcodeSDK sdk) {
-  static llvm::StringMap<std::string> g_sdk_path;
+llvm::Expected<llvm::StringRef> HostInfoMacOSX::GetXcodeSDKPath(XcodeSDK sdk) {
+  struct ErrorOrPath {
+    std::string str;
+    bool is_error;
+  };
+  static llvm::StringMap<ErrorOrPath> g_sdk_path;
   static std::mutex g_sdk_path_mutex;
 
   std::lock_guard<std::mutex> guard(g_sdk_path_mutex);
   LLDB_SCOPED_TIMER();
 
-  auto it = g_sdk_path.find(sdk.GetString());
-  if (it != g_sdk_path.end())
-    return it->second;
-  auto it_new = g_sdk_path.insert({sdk.GetString(), GetXcodeSDK(sdk)});
-  return it_new.first->second;
+  auto key = sdk.GetString();
+  auto it = g_sdk_path.find(key);
+  if (it != g_sdk_path.end()) {
+    if (it->second.is_error)
+      return llvm::createStringError(llvm::inconvertibleErrorCode(),
+                                     it->second.str);
+    else
+      return it->second.str;
+  }
+  auto path_or_err = GetXcodeSDK(sdk);
+  if (!path_or_err) {
+    std::string error = toString(path_or_err.takeError());
+    g_sdk_path.insert({key, {error, true}});
+    return llvm::createStringError(llvm::inconvertibleErrorCode(), error);
+  }
+  auto it_new = g_sdk_path.insert({key, {*path_or_err, false}});
+  return it_new.first->second.str;
 }
 
 namespace {
 struct dyld_shared_cache_dylib_text_info {
   uint64_t version; // current version 1
   // following fields all exist in version 1
   uint64_t loadAddressUnslid;
   uint64_t textSegmentSize;

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

 //===-- PlatformAppleSimulator.cpp ----------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "PlatformAppleSimulator.h"
 
 #if defined(__APPLE__)
 #include <dlfcn.h>
 #endif
 
+#include "lldb/Core/Debugger.h"
 #include "lldb/Core/Module.h"
 #include "lldb/Core/PluginManager.h"
 #include "lldb/Host/HostInfo.h"
 #include "lldb/Host/PseudoTerminal.h"
 #include "lldb/Target/Process.h"
 #include "lldb/Utility/LLDBAssert.h"
 #include "lldb/Utility/LLDBLog.h"
 #include "lldb/Utility/Log.h"
std::vector<ArchSpec> PlatformAppleSimulator::GetSupportedArchitectures(
   llvm::transform(m_supported_triples, result.begin(),
                   [](llvm::StringRef triple) { return ArchSpec(triple); });
   return result;
 }
 
 static llvm::StringRef GetXcodeSDKDir(std::string preferred,
                                       std::string secondary) {
   llvm::StringRef sdk;
-  sdk = HostInfo::GetXcodeSDKPath(XcodeSDK(std::move(preferred)));
+  auto get_sdk = [&](std::string sdk) -> llvm::StringRef {
+    auto sdk_path_or_err = HostInfo::GetXcodeSDKPath(XcodeSDK(std::move(sdk)));
+    if (!sdk_path_or_err) {
+      Debugger::ReportError("Error while searching for Xcode SDK: " +
+                            toString(sdk_path_or_err.takeError()));
+      return {};
+    }
+    return *sdk_path_or_err;
+  };
+
+  sdk = get_sdk(preferred);
   if (sdk.empty())
-    sdk = HostInfo::GetXcodeSDKPath(XcodeSDK(std::move(secondary)));
+    sdk = get_sdk(secondary);
   return sdk;
 }
 
 PlatformSP PlatformAppleSimulator::CreateInstance(
     const char *class_name, const char *description, ConstString plugin_name,
     llvm::SmallVector<llvm::Triple::ArchType, 4> supported_arch,
     llvm::Triple::OSType preferred_os,
     llvm::SmallVector<llvm::Triple::OSType, 4> supported_os,

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

 #if defined(__APPLE__)
 #include "PlatformAppleSimulator.h"
 #include "PlatformDarwinKernel.h"
 #include "PlatformRemoteAppleBridge.h"
 #include "PlatformRemoteAppleTV.h"
 #include "PlatformRemoteAppleWatch.h"
 #endif
 #include "lldb/Breakpoint/BreakpointLocation.h"
+#include "lldb/Core/Debugger.h"
 #include "lldb/Core/Module.h"
 #include "lldb/Core/ModuleList.h"
 #include "lldb/Core/ModuleSpec.h"
 #include "lldb/Core/PluginManager.h"
 #include "lldb/Host/Config.h"
 #include "lldb/Host/Host.h"
 #include "lldb/Host/HostInfo.h"
 #include "lldb/Symbol/ObjectFile.h"
sdk_path.Printf("%s/Developer/Platforms/MacOSX.platform/Developer/"
                     "SDKs/MacOSX%u.%u.sdk",
                     fspec.GetPath().c_str(), version.getMajor(),
                     *version.getMinor());
     if (FileSystem::Instance().Exists(fspec))
       return ConstString(sdk_path.GetString());
   }
 
   // Use the default SDK as a fallback.
-  FileSpec fspec(
-      HostInfo::GetXcodeSDKPath(lldb_private::XcodeSDK::GetAnyMacOS()));
+  auto sdk_path_or_err = HostInfo::GetXcodeSDKPath(XcodeSDK::GetAnyMacOS());
+  if (!sdk_path_or_err) {
+    Debugger::ReportError("Error while searching for Xcode SDK: " +
+                          toString(sdk_path_or_err.takeError()));
+    return {};
+  }
+
+  FileSpec fspec(*sdk_path_or_err);
   if (fspec) {
     if (FileSystem::Instance().Exists(fspec))
       return ConstString(fspec.GetPath());
   }
 
   return {};
 }
 

lldb/unittests/Host/CMakeLists.txt

list(APPEND FILES
     posix/TerminalTest.cpp
   )
 endif()
 
 add_lldb_unittest(HostTests
   ${FILES}
   LINK_LIBS
     lldbHost
+    lldbCore
     lldbUtilityHelpers
     lldbHostHelpers
     LLVMTestingSupport
   )

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::GetXcodeSDKPath(XcodeSDK("MacOSX.sdk")).empty());
+  auto get_sdk = [](std::string sdk, bool error = false) -> llvm::StringRef {
+    auto sdk_path_or_err = HostInfo::GetXcodeSDKPath(XcodeSDK(std::move(sdk)));
+    if (!error) {
+      EXPECT_TRUE((bool)sdk_path_or_err);
+      return *sdk_path_or_err;
+    }
+    EXPECT_FALSE((bool)sdk_path_or_err);
+    llvm::consumeError(sdk_path_or_err.takeError());
+    return {};
+  };
+  EXPECT_FALSE(get_sdk("MacOSX.sdk").empty());
   // These are expected to fall back to an available version.
-  EXPECT_FALSE(HostInfo::GetXcodeSDKPath(XcodeSDK("MacOSX9999.sdk")).empty());
+  EXPECT_FALSE(get_sdk("MacOSX9999.sdk").empty());
   // This is expected to fail.
-  EXPECT_TRUE(HostInfo::GetXcodeSDKPath(XcodeSDK("CeciNestPasUnOS.sdk")).empty());
+  EXPECT_TRUE(get_sdk("CeciNestPasUnOS.sdk", true).empty());
 }
 #endif
 
 TEST(HostInfoTestInitialization, InitTwice) {
   llvm::VersionTuple Version;
   {
     SubsystemRAII<FileSystem, HostInfo> subsystems;
     Version = HostInfo::GetOSVersion();
   }
 
   {
     SubsystemRAII<FileSystem, HostInfo> subsystems;
     EXPECT_EQ(Version, HostInfo::GetOSVersion());
   }
 }