fixup! store sysroot inside XcodeSDK

Author: Michael137

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

@@ -32,6 +32,9 @@ class HostInfoMacOSX : public HostInfoPosix {
   static FileSpec GetXcodeDeveloperDirectory();
 
   /// Query xcrun to find an Xcode SDK directory.
+  ///
+  /// Note, this is an expensive operation if the SDK we're querying
+  /// does not exist in an Xcode installation path on the host.
   static llvm::Expected<llvm::StringRef> GetSDKRoot(SDKOptions options);
   static llvm::Expected<llvm::StringRef> FindSDKTool(XcodeSDK sdk,
                                                      llvm::StringRef tool);

lldb/include/lldb/Utility/XcodeSDK.h

@@ -23,6 +23,8 @@ namespace lldb_private {
 /// An abstraction for Xcode-style SDKs that works like \ref ArchSpec.
 class XcodeSDK {
   std::string m_name;
+  std::optional<std::string>
+      m_sysroot_path; // TODO: should this be std::optional<FileSpec>?
 
 public:
   /// Different types of Xcode SDKs.
@@ -62,6 +64,8 @@ class XcodeSDK {
   /// directory component of a path one would pass to clang's -isysroot
   /// parameter. For example, "MacOSX.10.14.sdk".
   XcodeSDK(std::string &&name) : m_name(std::move(name)) {}
+  XcodeSDK(std::string name, std::string sysroot)
+      : m_name(std::move(name)), m_sysroot_path(std::move(sysroot)) {}
   static XcodeSDK GetAnyMacOS() { return XcodeSDK("MacOSX.sdk"); }
 
   /// The merge function follows a strict order to maintain monotonicity:
@@ -79,6 +83,7 @@ class XcodeSDK {
   llvm::VersionTuple GetVersion() const;
   Type GetType() const;
   llvm::StringRef GetString() const;
+  std::optional<llvm::StringRef> GetSysroot() const;
   /// Whether this Xcode SDK supports Swift.
   bool SupportsSwift() const;
 

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

@@ -79,6 +79,14 @@ static Status ExceptionMaskValidator(const char *string, void *unused) {
   return {};
 }
 
+static bool XcodeSysrootExists(XcodeSDK const &sdk) {
+  auto maybe_sysroot = sdk.GetSysroot();
+  if (!maybe_sysroot)
+    return false;
+
+  return FileSystem::Instance().Exists(*maybe_sysroot);
+}
+
 /// Destructor.
 ///
 /// The destructor is virtual since this class is designed to be
@@ -1395,6 +1403,7 @@ PlatformDarwin::GetSDKPathFromDebugInfo(Module &module) {
         llvm::formatv("No symbol file available for module '{0}'",
                       module.GetFileSpec().GetFilename().AsCString("")));
 
+  std::string sysroot;
   bool found_public_sdk = false;
   bool found_internal_sdk = false;
   XcodeSDK merged_sdk;
@@ -1425,6 +1434,9 @@ PlatformDarwin::ResolveSDKPathFromDebugInfo(Module &module) {
 
   auto [sdk, _] = std::move(*sdk_or_err);
 
+  if (XcodeSysrootExists(sdk))
+    return sdk.GetSysroot()->str();
+
   auto path_or_err = HostInfo::GetSDKRoot(HostInfo::SDKOptions{sdk});
   if (!path_or_err)
     return llvm::createStringError(

lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp

@@ -993,21 +993,26 @@ XcodeSDK SymbolFileDWARF::ParseXcodeSDK(CompileUnit &comp_unit) {
   const char *sdk = cu_die.GetAttributeValueAsString(DW_AT_APPLE_sdk, nullptr);
   if (!sdk)
     return {};
-  const char *sysroot =
+  std::string sysroot =
       cu_die.GetAttributeValueAsString(DW_AT_LLVM_sysroot, "");
-  // Register the sysroot path remapping with the module belonging to
-  // the CU as well as the one belonging to the symbol file. The two
-  // would be different if this is an OSO object and module is the
-  // corresponding debug map, in which case both should be updated.
-  ModuleSP module_sp = comp_unit.GetModule();
-  if (module_sp)
-    module_sp->RegisterXcodeSDK(sdk, sysroot);
 
-  ModuleSP local_module_sp = m_objfile_sp->GetModule();
-  if (local_module_sp && local_module_sp != module_sp)
-    local_module_sp->RegisterXcodeSDK(sdk, sysroot);
+  // RegisterXcodeSDK calls into xcrun which is not aware of CLT, which is
+  // expensive.
+  if (sysroot.find("/Library/Developer/CommandLineTools/SDKs") != 0) {
+    // Register the sysroot path remapping with the module belonging to
+    // the CU as well as the one belonging to the symbol file. The two
+    // would be different if this is an OSO object and module is the
+    // corresponding debug map, in which case both should be updated.
+    ModuleSP module_sp = comp_unit.GetModule();
+    if (module_sp)
+      module_sp->RegisterXcodeSDK(sdk, sysroot);
+
+    ModuleSP local_module_sp = m_objfile_sp->GetModule();
+    if (local_module_sp && local_module_sp != module_sp)
+      local_module_sp->RegisterXcodeSDK(sdk, sysroot);
+  }
 
-  return {sdk};
+  return {sdk, std::move(sysroot)};
 }
 
 size_t SymbolFileDWARF::ParseFunctions(CompileUnit &comp_unit) {

lldb/source/Utility/XcodeSDK.cpp

@@ -142,6 +142,10 @@ XcodeSDK::Type XcodeSDK::GetType() const {
 
 llvm::StringRef XcodeSDK::GetString() const { return m_name; }
 
+std::optional<llvm::StringRef> XcodeSDK::GetSysroot() const {
+  return m_sysroot_path;
+}
+
 bool XcodeSDK::Info::operator<(const Info &other) const {
   return std::tie(type, version, internal) <
          std::tie(other.type, other.version, other.internal);
@@ -153,17 +157,24 @@ bool XcodeSDK::Info::operator==(const Info &other) const {
 }
 
 void XcodeSDK::Merge(const XcodeSDK &other) {
+  auto add_internal_sdk_suffix = [](std::string const &sdk) {
+    return sdk.substr(0, sdk.size() - 3) + "Internal.sdk";
+  };
+
   // The "bigger" SDK always wins.
   auto l = Parse();
   auto r = other.Parse();
   if (l < r)
     *this = other;
   else {
     // The Internal flag always wins.
-    if (llvm::StringRef(m_name).ends_with(".sdk"))
-      if (!l.internal && r.internal)
-        m_name =
-            m_name.substr(0, m_name.size() - 3) + std::string("Internal.sdk");
+    if (!l.internal && r.internal) {
+      if (llvm::StringRef(m_name).ends_with(".sdk"))
+        m_name = add_internal_sdk_suffix(m_name);
+
+      if (m_sysroot_path && llvm::StringRef(*m_sysroot_path).ends_with(".sdk"))
+        m_sysroot_path.emplace(add_internal_sdk_suffix(*m_sysroot_path));
+    }
   }
 }
 

lldb/unittests/SymbolFile/DWARF/XcodeSDKModuleTests.cpp

@@ -310,13 +310,28 @@ SDKPathParsingTestData sdkPathParsingTestCases[] = {
      .expect_internal_sdk = true,
      .expect_sdk_path_pattern = "Internal.sdk"},
 
-    /// Two CUs with an internal SDK each
+    /// Two CUs with a public (non-CommandLineTools) SDK each
+    {.input_sdk_paths = {"/Path/To/SDKs/iPhoneOS14.1.sdk",
+                         "/Path/To/SDKs/MacOSX11.3.sdk"},
+     .expect_mismatch = false,
+     .expect_internal_sdk = false,
+     .expect_sdk_path_pattern = "iPhoneOS14.1.sdk"},
+
+    /// One CU with CommandLineTools and the other a public SDK
     {.input_sdk_paths =
          {"/Library/Developer/CommandLineTools/SDKs/iPhoneOS14.1.sdk",
-          "/Library/Developer/CommandLineTools/SDKs/MacOSX11.3.sdk"},
+          "/Path/To/SDKs/MacOSX11.3.sdk"},
      .expect_mismatch = false,
      .expect_internal_sdk = false,
      .expect_sdk_path_pattern = "iPhoneOS14.1.sdk"},
+
+    /// One CU with CommandLineTools and the other an internal SDK
+    {.input_sdk_paths =
+         {"/Library/Developer/CommandLineTools/SDKs/iPhoneOS14.1.sdk",
+          "/Path/To/SDKs/MacOSX11.3.Internal.sdk"},
+     .expect_mismatch = false,
+     .expect_internal_sdk = false,
+     .expect_sdk_path_pattern = "iPhoneOS14.1.Internal.sdk"},
 };
 
 INSTANTIATE_TEST_SUITE_P(SDKPathParsingTests, SDKPathParsingMultiparamTests,

lldb/unittests/Utility/XcodeSDKTest.cpp

@@ -34,12 +34,16 @@ TEST(XcodeSDKTest, ParseTest) {
   EXPECT_EQ(XcodeSDK("MacOSX10.9.sdk").GetVersion(), llvm::VersionTuple(10, 9));
   EXPECT_EQ(XcodeSDK("MacOSX10.15.4.sdk").GetVersion(), llvm::VersionTuple(10, 15));
   EXPECT_EQ(XcodeSDK("MacOSX.sdk").IsAppleInternalSDK(), false);
+  EXPECT_EQ(XcodeSDK("MacOSX.sdk", "/Path/To/MacOSX.sdk").GetSysroot(),
+            "/Path/To/MacOSX.sdk");
   EXPECT_EQ(XcodeSDK("MacOSX10.15.Internal.sdk").GetType(), XcodeSDK::MacOSX);
   EXPECT_EQ(XcodeSDK("MacOSX10.15.Internal.sdk").GetVersion(),
             llvm::VersionTuple(10, 15));
   EXPECT_EQ(XcodeSDK("MacOSX10.15.Internal.sdk").IsAppleInternalSDK(), true);
+  EXPECT_EQ(XcodeSDK("MacOSX10.15.Internal.sdk").GetSysroot(), std::nullopt);
   EXPECT_EQ(XcodeSDK().GetType(), XcodeSDK::unknown);
   EXPECT_EQ(XcodeSDK().GetVersion(), llvm::VersionTuple());
+  EXPECT_EQ(XcodeSDK().GetSysroot(), std::nullopt);
 }
 
 TEST(XcodeSDKTest, MergeTest) {
@@ -60,6 +64,14 @@ TEST(XcodeSDKTest, MergeTest) {
   XcodeSDK empty;
   empty.Merge(XcodeSDK("MacOSX10.14.Internal.sdk"));
   EXPECT_EQ(empty.GetString(), llvm::StringRef("MacOSX10.14.Internal.sdk"));
+  EXPECT_EQ(empty.GetSysroot(), std::nullopt);
+  empty.Merge(XcodeSDK("MacOSX9.5.Internal.sdk", "/Path/To/9.5.sdk"));
+  EXPECT_EQ(empty.GetSysroot(), std::nullopt);
+  empty.Merge(XcodeSDK("MacOSX12.5.sdk", "/Path/To/12.5.sdk"));
+  EXPECT_EQ(empty.GetSysroot(), "/Path/To/12.5.sdk");
+  empty.Merge(
+      XcodeSDK("MacOSX11.5.Internal.sdk", "/Path/To/12.5.Internal.sdk"));
+  EXPECT_EQ(empty.GetSysroot(), "/Path/To/12.5.Internal.sdk");
 }
 
 #ifndef _WIN32