[lldb/PlatformMacOSX] Be more robust in computing the SDK path with xcrun

[JDevlieghere]

Summary

The current implementation isn't very resilient when it comes to the output of xcrun. Currently it cannot deal with:

• Trailing newlines.
• Leading newlines and errors/warnings before the Xcode path.
• Xcode not being named Xcode.app.

This extract the logic into a helper in PlatformMacOSX and fixes those issues.

[aprantl]

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

225

Ah. We always ask for the macos SDK?

226

There's a global variable for various timeouts that we should use instead. Grep for commits with timeout in the name by me.

250

This looks a lot nicer than the original function.

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

81 ↗

(On Diff #250654)

Can you add a comment like:
/// path/to/Xcode.app/Developer/... so it's easier to visualize what these are?

90 ↗

(On Diff #250654)

The comment and the parameter seem to contradict each other about whether xcrun is being used? :-)

Should the function be called, GetDefaultXcodeSDK?
I'm also wondering why a function with this name doesn't take a triple.

lldb/unittests/Platform/PlatformMacOSXTest.cpp

22 ↗

(On Diff #250654)

Awesome!

[JDevlieghere]

Use the same timeout as UtilityExpressionTimeout.

[JDevlieghere]

We should be able to avoid most of the post processing by just asking xcrun directly for the right SKD. I'm going to try to implement that and hopefully that removes some of the complexity here.

[aprantl]

lldb/unittests/Platform/PlatformMacOSXTest.cpp

25 ↗

(On Diff #250659)

This also needs a testcase where the SDK is called MacOSX.sdk, which is actually the more common scenario.

[JDevlieghere]

New approach. Test is coming but upload the patch so Adrian can take a look.

[JDevlieghere]

Add unit test

[aprantl]

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

1273

How about hard-coding this in switch statement instead? The we can't have off-by-one errors quite as easily...

case watchOS: return "watchos"; etc.

1364

Not your fault, but watchOS and tvOS should also be in this list.

1431–1432

Not your fault again, but I find the name of this function super confusing. What is the :"ForModules" part supposed to mean?

lldb/source/Plugins/Platform/MacOSX/PlatformDarwin.h

89

Technically, I suppose BridgeOS belongs in here, too.

lldb/unittests/Platform/PlatformDarwinTest.cpp

80 ↗

(On Diff #250821)

Thanks!

[JDevlieghere]

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

1431–1432

It filters out SDK versions that don't support modules. We have something similar in SwiftASTContext, it only returns SDKs that support Swift.

[aprantl]

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

1362–1364

The corresponding watchOS and tvOS numbers for iOS 8 should be lower, right? If you are in doubt, just pick the current ones.

1940

empty stringref? That we can at least check for at the call site.

1947

--sdk for symmetry? Or is that the correct option?

[JDevlieghere]

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

1362–1364

tvOS = iOS, but yes watchOS should be 6.

[aprantl]

Thanks!

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

1995

early-exitify?

[JDevlieghere]

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

1995

Doesn't work because you want to fallback rather than return.

lldb/source/Plugins/Platform/MacOSX/PlatformDarwin.h

public:
   lldb_private::FileSpec LocateExecutable(const char *basename) override;
 
   lldb_private::Status
   LaunchProcess(lldb_private::ProcessLaunchInfo &launch_info) override;
 
   static std::tuple<llvm::VersionTuple, llvm::StringRef>
   ParseVersionBuildDir(llvm::StringRef str);
 
-  enum SDKType : unsigned {
+  enum SDKType : int {
     MacOSX = 0,
     iPhoneSimulator,
     iPhoneOS,
+    AppleTVSimulator,
+    AppleTVOS,
+    WatchSimulator,
+    watchOS,
+    Linux,
+    numSDKTypes,
+    unknown = -1
   };
 
   llvm::Expected<lldb_private::StructuredData::DictionarySP>
   FetchExtendedCrashInformation(lldb_private::Process &process) override;
 
 protected:
   struct CrashInfoAnnotations {
     uint64_t version;          // unsigned long
protected:
   const char *GetDeveloperDirectory();
 
   lldb_private::Status FindBundleBinaryInExecSearchPaths(
       const lldb_private::ModuleSpec &module_spec,
       lldb_private::Process *process, lldb::ModuleSP &module_sp,
       const lldb_private::FileSpecList *module_search_paths_ptr,
       lldb::ModuleSP *old_module_sp_ptr, bool *did_create_ptr);
 
+  static llvm::StringRef GetSDKNameForType(SDKType type);
+  static lldb_private::FileSpec GetXcodeSDK(SDKType type);
+  static lldb_private::FileSpec GetXcodeContentsDirectory();
+
 protected:
+  static std::string FindXcodeContentsDirectoryInPath(llvm::StringRef path);
+
   std::string m_developer_directory;
 
 private:
   DISALLOW_COPY_AND_ASSIGN(PlatformDarwin);
 };
 
 #endif // LLDB_SOURCE_PLUGINS_PLATFORM_MACOSX_PLATFORMDARWIN_H

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

PlatformDarwin::GetResumeCountForLaunchInfo(ProcessLaunchInfo &launch_info) {
   } else
     return 1;
 }
 
 void PlatformDarwin::CalculateTrapHandlerSymbolNames() {
   m_trap_handlers.push_back(ConstString("_sigtramp"));
 }
 
-static const char *const sdk_strings[] = {
-    "MacOSX", "iPhoneSimulator", "iPhoneOS",
-};
+const char *const sdk_strings[] = {
+    "macosx",    "iphonesimulator", "iphoneos", "appletvsimulator",
+    "appletvos", "watchsimulator",  "watchos",  "linux"};
 
 static FileSpec CheckPathForXcode(const FileSpec &fspec) {
   if (FileSystem::Instance().Exists(fspec)) {
     const char substr[] = ".app/Contents";
 
     std::string path_to_shlib = fspec.GetPath();
     size_t pos = path_to_shlib.rfind(substr);
     if (pos != std::string::npos) {

 
 bool PlatformDarwin::SDKSupportsModules(SDKType sdk_type,
                                         llvm::VersionTuple version) {
   switch (sdk_type) {
   case SDKType::MacOSX:
     return version >= llvm::VersionTuple(10, 10);
   case SDKType::iPhoneOS:
   case SDKType::iPhoneSimulator:
     return version >= llvm::VersionTuple(8);
+  default:
+    return false;
   }
 
   return false;
 }
 
 bool PlatformDarwin::SDKSupportsModules(SDKType desired_type,
                                         const FileSpec &sdk_path) {
   ConstString last_path_component = sdk_path.GetLastPathComponent();
 
   if (last_path_component) {
-    const llvm::StringRef sdk_name = last_path_component.GetStringRef();
+    const llvm::StringRef sdk_name = last_path_component.GetStringRef().lower();
 
     if (!sdk_name.startswith(sdk_strings[desired_type]))
       return false;
     auto version_part =
         sdk_name.drop_front(strlen(sdk_strings[desired_type]));
     version_part.consume_back(".sdk");
 
     llvm::VersionTuple version;
FileSystem::Instance().EnumerateDirectory(
       DirectoryEnumerator, &enumerator_info);
 
   if (FileSystem::Instance().IsDirectory(enumerator_info.found_path))
     return enumerator_info.found_path;
   else
     return FileSpec();
 }
 
 FileSpec PlatformDarwin::GetSDKDirectoryForModules(SDKType sdk_type) {
-  switch (sdk_type) {
-  case SDKType::MacOSX:
-  case SDKType::iPhoneSimulator:
-  case SDKType::iPhoneOS:
-    break;
-  }
-
   FileSpec sdks_spec = GetXcodeContentsPath();
   sdks_spec.AppendPathComponent("Developer");
   sdks_spec.AppendPathComponent("Platforms");
 
   switch (sdk_type) {
   case SDKType::MacOSX:
     sdks_spec.AppendPathComponent("MacOSX.platform");
     break;
   case SDKType::iPhoneSimulator:
     sdks_spec.AppendPathComponent("iPhoneSimulator.platform");
     break;
   case SDKType::iPhoneOS:
     sdks_spec.AppendPathComponent("iPhoneOS.platform");
     break;
+  default:
+    llvm_unreachable("unsupported sdk");
   }
 
   sdks_spec.AppendPathComponent("Developer");
   sdks_spec.AppendPathComponent("SDKs");
 
   if (sdk_type == SDKType::MacOSX) {
     llvm::VersionTuple version = HostInfo::GetOSVersion();
 
#endif
 
   case SDKType::MacOSX:
 #if defined(__i386__) || defined(__x86_64__)
     use_current_os_version = true;
 #else
     use_current_os_version = false;
 #endif
     break;
+  default:
+    break;
   }
 
   llvm::VersionTuple version;
   if (use_current_os_version)
     version = GetOSVersion();
   else if (target) {
     // Our OS doesn't match our executable so we need to get the min OS version
     // from the object file
case SDKType::iPhoneOS:
       break;
     case SDKType::iPhoneSimulator:
       minimum_version_option.PutCString("-mios-simulator-version-min=");
       minimum_version_option.PutCString(version.getAsString());
       break;
     case SDKType::MacOSX:
       minimum_version_option.PutCString("-mmacosx-version-min=");
       minimum_version_option.PutCString(version.getAsString());
+      break;
+    default:
+      llvm_unreachable("unsupported sdk");
     }
     options.push_back(std::string(minimum_version_option.GetString()));
   }
 
   FileSpec sysroot_spec;
   // Scope for mutex locker below
   {
     std::lock_guard<std::mutex> guard(m_mutex);
if (!env_vars.count(disable_env_var)) {
     // os_log and NSLog messages mirrored to the target process stderr.
     env_vars.try_emplace("OS_ACTIVITY_DT_MODE", "enable");
   }
 
   // Let our parent class do the real launching.
   return PlatformPOSIX::LaunchProcess(launch_info);
 }
 
-lldb_private::Status
-PlatformDarwin::FindBundleBinaryInExecSearchPaths (const ModuleSpec &module_spec, Process *process, 
-                                                   ModuleSP &module_sp, 
-                                                   const FileSpecList *module_search_paths_ptr, 
-                                                   ModuleSP *old_module_sp_ptr, bool *did_create_ptr)
-{
+lldb_private::Status PlatformDarwin::FindBundleBinaryInExecSearchPaths(
+    const ModuleSpec &module_spec, Process *process, ModuleSP &module_sp,
+    const FileSpecList *module_search_paths_ptr, ModuleSP *old_module_sp_ptr,
+    bool *did_create_ptr) {
   const FileSpec &platform_file = module_spec.GetFileSpec();
   // See if the file is present in any of the module_search_paths_ptr
   // directories.
   if (!module_sp && module_search_paths_ptr && platform_file) {
     // create a vector of all the file / directory names in platform_file e.g.
     // this might be
     // /System/Library/PrivateFrameworks/UIFoundation.framework/UIFoundation
     //
for (size_t i = 0; i < num_module_search_paths; ++i) {
             return new_error;
           }
         }
       }
     }
   }
   return Status();
 }
+
+std::string
+PlatformDarwin::FindXcodeContentsDirectoryInPath(llvm::StringRef path) {
+  auto begin = llvm::sys::path::begin(path);
+  auto end = llvm::sys::path::end(path);
+
+  // Iterate over the path components until we find something that ends with
+  // .app. If the next component is Contents then we've found the Contents
+  // directory.
+  for (auto it = begin; it != end; ++it) {
+    if (it->endswith(".app")) {
+      auto next = it;
+      if (++next != end && *next == "Contents") {
+        llvm::SmallString<128> buffer;
+        llvm::sys::path::append(buffer, begin, ++next);
+        return buffer.str().str();
+      }
+    }
+  }
+
+  return {};
+}
+
+llvm::StringRef PlatformDarwin::GetSDKNameForType(SDKType type) {
+  if (type != SDKType::unknown)
+    return sdk_strings[type];
+  return "unknown";
+}
+
+FileSpec PlatformDarwin::GetXcodeSDK(SDKType type) {
+  std::string xcrun_cmd =
+      "xcrun --show-sdk-path -sdk " + GetSDKNameForType(type).str();
+
+  int status = 0;
+  int signo = 0;
+  std::string output_str;
+  lldb_private::Status error =
+      Host::RunShellCommand(xcrun_cmd.c_str(), 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);
+
+  // Whatever is left in output should be a valid path.
+  if (!FileSystem::Instance().Exists(output))
+    return {};
+
+  // Find the contents dir in the xcrun provided path.
+  std::string xcode_contents_dir = FindXcodeContentsDirectoryInPath(output);
+  if (xcode_contents_dir.empty())
+    return {};
+
+  return FileSpec(xcode_contents_dir);
+}
+
+FileSpec PlatformDarwin::GetXcodeContentsDirectory() {
+  static FileSpec g_xcode_contents_path;
+  static std::once_flag g_once_flag;
+  std::call_once(g_once_flag, [&]() {
+    // Try the shlib dir first.
+    if (FileSpec fspec = HostInfo::GetShlibDir()) {
+      if (FileSystem::Instance().Exists(fspec)) {
+        std::string xcode_contents_dir =
+            FindXcodeContentsDirectoryInPath(fspec.GetPath());
+        if (!xcode_contents_dir.empty()) {
+          g_xcode_contents_path = FileSpec(xcode_contents_dir);
+          return;
+        }
+      }
+    }
+
+    if (FileSpec fspec = GetXcodeSDK(SDKType::MacOSX)) {
+      if (FileSystem::Instance().Exists(fspec)) {
+        std::string xcode_contents_dir =
+            FindXcodeContentsDirectoryInPath(fspec.GetPath());
+        if (!xcode_contents_dir.empty()) {
+          g_xcode_contents_path = FileSpec(xcode_contents_dir);
+          return;
+        }
+      }
+    }
+  });
+  return g_xcode_contents_path;
+}

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

 /// Destructor.
 ///
 /// The destructor is virtual since this class is designed to be
 /// inherited from by the plug-in instance.
 PlatformMacOSX::~PlatformMacOSX() {}
 
 ConstString PlatformMacOSX::GetSDKDirectory(lldb_private::Target &target) {
   ModuleSP exe_module_sp(target.GetExecutableModule());
-  if (exe_module_sp) {
-    ObjectFile *objfile = exe_module_sp->GetObjectFile();
-    if (objfile) {
-      std::string xcode_contents_path;
-      std::string default_xcode_sdk;
-      FileSpec fspec;
-      llvm::VersionTuple version = objfile->GetSDKVersion();
-      if (!version.empty()) {
-        fspec = HostInfo::GetShlibDir();
-        if (fspec) {
-          std::string path;
-          xcode_contents_path = fspec.GetPath();
-          size_t pos = xcode_contents_path.find("/Xcode.app/Contents/");
-          if (pos != std::string::npos) {
-            // LLDB.framework is inside an Xcode app bundle, we can locate the
-            // SDK from here
-            xcode_contents_path.erase(pos + strlen("/Xcode.app/Contents/"));
-          } else {
-            xcode_contents_path.clear();
-            // Use the selected Xcode
-            int status = 0;
-            int signo = 0;
-            std::string output;
-            const char *command = "xcrun -sdk macosx --show-sdk-path";
-            lldb_private::Status error = RunShellCommand(
-                command,    // shell command to run
-                FileSpec(), // current working directory
-                &status,    // Put the exit status of the process in here
-                &signo,     // Put the signal that caused the process to exit in
-                            // here
-                &output, // Get the output from the command and place it in this
-                         // string
-                std::chrono::seconds(3));
-            if (status == 0 && !output.empty()) {
-              size_t first_non_newline = output.find_last_not_of("\r\n");
-              if (first_non_newline != std::string::npos)
-                output.erase(first_non_newline + 1);
-              default_xcode_sdk = output;
-
-              pos = default_xcode_sdk.find("/Xcode.app/Contents/");
-              if (pos != std::string::npos)
-                xcode_contents_path = default_xcode_sdk.substr(
-                    0, pos + strlen("/Xcode.app/Contents/"));
-            }
-          }
-        }
+  if (!exe_module_sp)
+    return {};
+
+  if (!objfile)
+    return {};
+
+  if (version.empty())
+    return {};
 
-        if (!xcode_contents_path.empty()) {
-          StreamString sdk_path;
-          sdk_path.Printf("%sDeveloper/Platforms/MacOSX.platform/Developer/"
-                          "SDKs/MacOSX%u.%u.sdk",
-                          xcode_contents_path.c_str(), version.getMajor(),
-                          version.getMinor().getValue());
-          fspec.SetFile(sdk_path.GetString(), FileSpec::Style::native);
-          if (FileSystem::Instance().Exists(fspec))
-            return ConstString(sdk_path.GetString());
-        }
+  // First try to find an SDK that matches the given SDK version.
+  if (FileSpec fspec = GetXcodeContentsDirectory()) {
+    sdk_path.Printf("%s/Developer/Platforms/MacOSX.platform/Developer/"
+                    fspec.GetPath().c_str(), version.getMajor(),
 
-        if (!default_xcode_sdk.empty()) {
-          fspec.SetFile(default_xcode_sdk, FileSpec::Style::native);
-          if (FileSystem::Instance().Exists(fspec))
-            return ConstString(default_xcode_sdk);
-        }
-      }
-    }
-  }
-  return ConstString();
+  // Use the default SDK as a fallback.
+  if (FileSpec fspec = GetXcodeSDK(SDKType::MacOSX)) {
+      return ConstString(fspec.GetPath());
+
+  return {};
 }
 
 Status PlatformMacOSX::GetSymbolFile(const FileSpec &platform_file,
                                      const UUID *uuid_ptr,
                                      FileSpec &local_file) {
   if (IsRemote()) {
     if (m_remote_platform_sp)
       return m_remote_platform_sp->GetFileWithUUID(platform_file, uuid_ptr,
                                                    local_file);
   }
 
   // Default to the local case
   local_file = platform_file;
   return Status();
 }
 
 lldb_private::Status
 PlatformMacOSX::GetFileWithUUID(const lldb_private::FileSpec &platform_file,
                                 const lldb_private::UUID *uuid_ptr,
                                 lldb_private::FileSpec &local_file) {
   if (IsRemote() && m_remote_platform_sp) {
     std::string local_os_build;
 #if !defined(__linux__)
     HostInfo::GetOSBuildString(local_os_build);
 #endif
     std::string remote_os_build;
     m_remote_platform_sp->GetOSBuildString(remote_os_build);
     if (local_os_build == remote_os_build) {
       // same OS version: the local file is good enough
       local_file = platform_file;
       return Status();
     } else {
       // try to find the file in the cache
       std::string cache_path(GetLocalCacheDirectory());
       std::string module_path(platform_file.GetPath());
       cache_path.append(module_path);
       FileSpec module_cache_spec(cache_path);
       if (FileSystem::Instance().Exists(module_cache_spec)) {
         local_file = module_cache_spec;
         return Status();
       }
       // bring in the remote module file