[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

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

90

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

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

26

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 ↗

(On Diff #250821)

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 ↗

(On Diff #250821)

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

1433 ↗

(On Diff #250821)

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 ↗

(On Diff #250821)

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

1433 ↗

(On Diff #250821)

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 ↗

(On Diff #250847)

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

1942 ↗

(On Diff #250847)

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

1949 ↗

(On Diff #250847)

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

[JDevlieghere]

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

1362 ↗

(On Diff #250847)

tvOS = iOS, but yes watchOS should be 6.

[aprantl]

Thanks!

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

1998 ↗

(On Diff #250890)

early-exitify?

[JDevlieghere]

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

1998 ↗

(On Diff #250890)

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

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

public:
 
   void
   AddClangModuleCompilationOptions(lldb_private::Target *target,
                                    std::vector<std::string> &options) override {
     return PlatformDarwin::AddClangModuleCompilationOptionsForSDKType(
         target, options, PlatformDarwin::SDKType::MacOSX);
   }
 
+  struct XcodeSDK {
+    std::string default_sdk;
+    std::string contents_path;
+    XcodeSDK(std::string sdk = {})
+        : default_sdk(std::move(sdk)), contents_path() {}
+    explicit operator bool() {
+      return !default_sdk.empty() && !contents_path.empty();
+    }
+  };
+
+  /// Get the SDK path from xcrun.
+  static XcodeSDK GetXcodeSDK(bool use_xcrun = true);
+
+protected:
+  static bool ComputeContentsPath(XcodeSDK &sdk);
+
 private:
   DISALLOW_COPY_AND_ASSIGN(PlatformMacOSX);
 };
 
 #endif // LLDB_SOURCE_PLUGINS_PLATFORM_MACOSX_PLATFORMMACOSX_H

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

 PlatformMacOSX::PlatformMacOSX(bool is_host) : PlatformDarwin(is_host) {}
 
 /// Destructor.
 ///
 /// The destructor is virtual since this class is designed to be
 /// inherited from by the plug-in instance.
 PlatformMacOSX::~PlatformMacOSX() {}
 
+bool PlatformMacOSX::ComputeContentsPath(XcodeSDK &sdk) {
+  auto begin = llvm::sys::path::begin(sdk.default_sdk);
+  auto end = llvm::sys::path::end(sdk.default_sdk);
+
+  // Iterate over the path components until we find something that ends with
+  // .app. If the next component is Contents then we've found the Xcode content
+  // 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);
+        sdk.contents_path = buffer.str().str();
+        return true;
+      }
+    }
+  }
+
+  return false;
+}
+
+PlatformMacOSX::XcodeSDK PlatformMacOSX::GetXcodeSDK(bool use_xcrun) {
+  if (FileSpec fspec = HostInfo::GetShlibDir()) {
+    if (FileSystem::Instance().Exists(fspec)) {
+      XcodeSDK sdk(fspec.GetPath());
+      if (ComputeContentsPath(sdk))
+        return sdk;
+    }
+  }
+
+  // If we cannot use xcrun then we're done.
+  if (!use_xcrun)
+    return {};
+
+  int status = 0;
+  int signo = 0;
+  std::string output_str;
+  lldb_private::Status error = Host::RunShellCommand(
+      "xcrun -sdk macosx --show-sdk-path", FileSpec(), &status, &signo,
+      &output_str, std::chrono::seconds(3));
+
+  // Check that xcrun return something.
+  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 it's not we're done.
+  if (!FileSystem::Instance().Exists(output))
+    return {};
+
+  XcodeSDK sdk(output.str());
+  if (ComputeContentsPath(sdk))
+    return sdk;
+
+  return {};
+}
+
 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;
+      XcodeSDK 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/"));
-            }
-          }
-        }
+        sdk = GetXcodeSDK();
 
-        if (!xcode_contents_path.empty()) {
+        if (!sdk.contents_path.empty()) {
           StreamString sdk_path;
-          sdk_path.Printf("%sDeveloper/Platforms/MacOSX.platform/Developer/"
+          sdk_path.Printf("%s/Developer/Platforms/MacOSX.platform/Developer/"
                           "SDKs/MacOSX%u.%u.sdk",
-                          xcode_contents_path.c_str(), version.getMajor(),
+                          sdk.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());
         }
 
-        if (!default_xcode_sdk.empty()) {
-          fspec.SetFile(default_xcode_sdk, FileSpec::Style::native);
+        if (!sdk.default_sdk.empty()) {
+          fspec.SetFile(sdk.default_sdk, FileSpec::Style::native);
           if (FileSystem::Instance().Exists(fspec))
-            return ConstString(default_xcode_sdk);
+            return ConstString(sdk.default_sdk);
         }
       }
     }
   }
   return ConstString();
 }
 
 Status PlatformMacOSX::GetSymbolFile(const FileSpec &platform_file,

lldb/unittests/Platform/CMakeLists.txt

 add_lldb_unittest(LLDBPlatformTests
   PlatformDarwinTest.cpp
+  PlatformMacOSXTest.cpp
 
   LINK_LIBS
     lldbPluginPlatformMacOSX
   LINK_COMPONENTS
     Support
   )

lldb/unittests/Platform/PlatformMacOSXTest.cpp

+//===-- PlatformMacOSXTest.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 "Plugins/Platform/MacOSX/PlatformMacOSX.h"
+#include "llvm/ADT/StringRef.h"
+#include "gtest/gtest.h"
+#include <tuple>
+
+using namespace lldb;
+using namespace lldb_private;
+
+struct PlatformMacOSXTester : public PlatformMacOSX {
+public:
+  using PlatformMacOSX::ComputeContentsPath;
+};
+
+TEST(PlatformMacOSXTest, ComputeContentsPath) {
+  PlatformMacOSX::XcodeSDK standard(
+      "/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/"
+      "Developer/SDKs/MacOSX10.15.sdk");
+  EXPECT_FALSE(static_cast<bool>(standard));
+  EXPECT_TRUE(PlatformMacOSXTester::ComputeContentsPath(standard));
+  EXPECT_TRUE(static_cast<bool>(standard));
+  EXPECT_EQ(standard.default_sdk,
+            "/Applications/Xcode.app/Contents/Developer/Platforms/"
+            "MacOSX.platform/Developer/SDKs/MacOSX10.15.sdk");
+  EXPECT_EQ(standard.contents_path, "/Applications/Xcode.app/Contents");
+
+  PlatformMacOSX::XcodeSDK beta(
+      "/Applications/Xcode-beta.app/Contents/Developer/Platforms/"
+      "MacOSX.platform/Developer/SDKs/MacOSX10.15.sdk");
+  EXPECT_FALSE(static_cast<bool>(beta));
+  EXPECT_TRUE(PlatformMacOSXTester::ComputeContentsPath(beta));
+  EXPECT_TRUE(static_cast<bool>(beta));
+  EXPECT_EQ(beta.default_sdk,
+            "/Applications/Xcode-beta.app/Contents/Developer/Platforms/"
+            "MacOSX.platform/Developer/SDKs/MacOSX10.15.sdk");
+  EXPECT_EQ(beta.contents_path, "/Applications/Xcode-beta.app/Contents");
+
+  PlatformMacOSX::XcodeSDK no_app(
+      "/Applications/Xcode/Contents/Developer/Platforms/MacOSX.platform/"
+      "Developer/SDKs/MacOSX10.15.sdk");
+  EXPECT_FALSE(static_cast<bool>(no_app));
+  EXPECT_FALSE(PlatformMacOSXTester::ComputeContentsPath(no_app));
+  EXPECT_FALSE(static_cast<bool>(no_app));
+  EXPECT_EQ(no_app.default_sdk,
+            "/Applications/Xcode/Contents/Developer/Platforms/MacOSX.platform/"
+            "Developer/SDKs/MacOSX10.15.sdk");
+  EXPECT_EQ(no_app.contents_path, "");
+
+  PlatformMacOSX::XcodeSDK no_contents(
+      "/Applications/Xcode.app/Developer/Platforms/MacOSX.platform/Developer/"
+      "SDKs/MacOSX10.15.sdk");
+  EXPECT_FALSE(static_cast<bool>(no_contents));
+  EXPECT_FALSE(PlatformMacOSXTester::ComputeContentsPath(no_contents));
+  EXPECT_FALSE(static_cast<bool>(no_contents));
+  EXPECT_EQ(no_contents.default_sdk,
+            "/Applications/Xcode.app/Developer/Platforms/MacOSX.platform/"
+            "Developer/SDKs/MacOSX10.15.sdk");
+  EXPECT_EQ(no_contents.contents_path, "");
+}