aprantl updated this revision to Diff 478762.
aprantl added a comment.
Herald added a subscriber: fedor.sergeev.

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


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D138060/new/

https://reviews.llvm.org/D138060

Files:
  lldb/include/lldb/Host/HostInfoBase.h
  lldb/include/lldb/Host/macosx/HostInfoMacOSX.h
  lldb/source/Core/Module.cpp
  lldb/source/Host/macosx/objcxx/HostInfoMacOSX.mm
  lldb/source/Plugins/Platform/MacOSX/PlatformAppleSimulator.cpp
  lldb/source/Plugins/Platform/MacOSX/PlatformMacOSX.cpp
  lldb/unittests/Host/CMakeLists.txt
  lldb/unittests/Host/HostInfoTest.cpp

Index: lldb/unittests/Host/HostInfoTest.cpp
===================================================================
--- lldb/unittests/Host/HostInfoTest.cpp
+++ lldb/unittests/Host/HostInfoTest.cpp
@@ -56,11 +56,21 @@
 
 #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
 
Index: lldb/unittests/Host/CMakeLists.txt
===================================================================
--- lldb/unittests/Host/CMakeLists.txt
+++ lldb/unittests/Host/CMakeLists.txt
@@ -32,6 +32,7 @@
   ${FILES}
   LINK_LIBS
     lldbHost
+    lldbCore
     lldbUtilityHelpers
     lldbHostHelpers
     LLVMTestingSupport
Index: lldb/source/Plugins/Platform/MacOSX/PlatformMacOSX.cpp
===================================================================
--- lldb/source/Plugins/Platform/MacOSX/PlatformMacOSX.cpp
+++ lldb/source/Plugins/Platform/MacOSX/PlatformMacOSX.cpp
@@ -17,6 +17,7 @@
 #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"
@@ -123,8 +124,14 @@
   }
 
   // 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());
Index: lldb/source/Plugins/Platform/MacOSX/PlatformAppleSimulator.cpp
===================================================================
--- lldb/source/Plugins/Platform/MacOSX/PlatformAppleSimulator.cpp
+++ lldb/source/Plugins/Platform/MacOSX/PlatformAppleSimulator.cpp
@@ -12,6 +12,7 @@
 #include <dlfcn.h>
 #endif
 
+#include "lldb/Core/Debugger.h"
 #include "lldb/Core/Module.h"
 #include "lldb/Core/PluginManager.h"
 #include "lldb/Host/HostInfo.h"
@@ -278,9 +279,19 @@
 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;
 }
 
Index: lldb/source/Host/macosx/objcxx/HostInfoMacOSX.mm
===================================================================
--- lldb/source/Host/macosx/objcxx/HostInfoMacOSX.mm
+++ lldb/source/Host/macosx/objcxx/HostInfoMacOSX.mm
@@ -6,15 +6,15 @@
 //
 //===----------------------------------------------------------------------===//
 
+#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"
@@ -337,7 +337,14 @@
       }
     }
 
-    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 =
@@ -365,12 +372,18 @@
   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");
@@ -391,13 +404,29 @@
     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.
@@ -414,7 +443,8 @@
     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()) {
@@ -430,8 +460,10 @@
         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;
         }
       }
@@ -441,7 +473,10 @@
     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() &&
@@ -449,44 +484,68 @@
       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 {
Index: lldb/source/Core/Module.cpp
===================================================================
--- lldb/source/Core/Module.cpp
+++ lldb/source/Core/Module.cpp
@@ -1647,7 +1647,15 @@
 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.
Index: lldb/include/lldb/Host/macosx/HostInfoMacOSX.h
===================================================================
--- lldb/include/lldb/Host/macosx/HostInfoMacOSX.h
+++ lldb/include/lldb/Host/macosx/HostInfoMacOSX.h
@@ -30,7 +30,7 @@
   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
Index: lldb/include/lldb/Host/HostInfoBase.h
===================================================================
--- lldb/include/lldb/Host/HostInfoBase.h
+++ lldb/include/lldb/Host/HostInfoBase.h
@@ -108,7 +108,9 @@
   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.
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Reply via email to