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

384

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

396

Why is the timeout increased?

399–406

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

418

Why not return an Error?

422

Same question.

479

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

[labath]

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

13

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

409

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

509

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

lldb/unittests/Host/HostTest.cpp

38

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

418

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

13

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

13

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/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/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"
if (!env_developer_dir.empty()) {
       if (!xcode_contents_dir.empty()) {
         g_xcode_contents_path = FileSpec(xcode_contents_dir);
         return;
       }
     }
 
     FileSpec fspec(HostInfo::GetXcodeSDKPath(XcodeSDK::GetAnyMacOS()));
     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;
         }
       }
     }
   });
   return g_xcode_contents_path;
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);
+    Log *log = GetLog(LLDBLog::Host | LLDBLog::Types);
     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));
+                              std::chrono::seconds(30));
 
-    // Check that xcrun return something useful.
-    if (status != 0 || output_str.empty())
+    // Check that xcrun returned something useful.
+    if (error.Fail()) {
+      LLDB_LOG(log, "xcrun failed to execute: %s", error.AsCString());
+      return {};
+    }
+    if (status != 0) {
+      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 {
     // Invoke xcrun with the developer dir specified in the environment.
     std::string developer_dir = GetEnvDeveloperDir();
     if (!developer_dir.empty()) {
if (!info.version.empty()) {
       info.version = {};
       sdk_name = XcodeSDK::GetCanonicalName(info);
       path = find_sdk(sdk_name);
       if (!path.empty())
         break;
     }
 
     LLDB_LOGF(log, "Couldn't find any matching SDK on host");
     return {};
   }
 
   // Whatever is left in output should be a valid path.
   if (!FileSystem::Instance().Exists(path))
     return {};
   return path;
 }
 

 
 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;
   uuid_t dylibUuid;
   const char *path; // pointer invalid at end of iterations
   // following fields all exist in version 2
   uint64_t textSegmentOffset; // offset from start of cache
 };
 typedef struct dyld_shared_cache_dylib_text_info
     dyld_shared_cache_dylib_text_info;
 }
 
 extern "C" int dyld_shared_cache_iterate_text(

lldb/unittests/Host/HostTest.cpp

EXPECT_EQ("Exited with status 4",
             formatv("{0}", WaitStatus{WaitStatus::Exit, 4}).str());
 }
 
 TEST(Host, GetEnvironment) {
   putenv(const_cast<char *>("LLDB_TEST_ENVIRONMENT_VAR=Host::GetEnvironment"));
   ASSERT_EQ("Host::GetEnvironment",
             Host::GetEnvironment().lookup("LLDB_TEST_ENVIRONMENT_VAR"));
 }
+
+#if defined(LLVM_ON_UNIX)
+TEST(Host, RunShellCommand) {
+  std::string shell = "SHELL=" + getenv("SHELL");
+  putenv(const_cast<char *>("SHELL=/bin/LLDB_TEST_this-file-does-not-exist"));
+  int status;
+  std::string out;
+  Status error =
+      Host::RunShellCommand("/usr/bin/true", FileSpec(), &status, &signo, &out);
+  ASSERT_TRUE(error.Fail());
+  putenv(shell);
+}
+#endif