Revert "[lldb][HostInfoMacOSX] Try to use DW_AT_LLVM_sysroot instead of xcrun when looking up SDK" #129621
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…of xcrun…" This reverts commit da293b8.
|
@llvm/pr-subscribers-lldb Author: Augusto Noronha (augusto2112) ChangesReverts llvm/llvm-project#128712 @Michael137 I think this patch is causing green dragon to fail, so I'm reverting it for now. Full diff: https://github.com/llvm/llvm-project/pull/129621.diff 7 Files Affected:
diff --git a/lldb/include/lldb/Host/macosx/HostInfoMacOSX.h b/lldb/include/lldb/Host/macosx/HostInfoMacOSX.h
index 9034c80fdefa4..8eb2ede382c22 100644
--- a/lldb/include/lldb/Host/macosx/HostInfoMacOSX.h
+++ b/lldb/include/lldb/Host/macosx/HostInfoMacOSX.h
@@ -32,9 +32,6 @@ 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);
diff --git a/lldb/include/lldb/Utility/XcodeSDK.h b/lldb/include/lldb/Utility/XcodeSDK.h
index 3a6cbb9c98f6b..2720d0d8a44a1 100644
--- a/lldb/include/lldb/Utility/XcodeSDK.h
+++ b/lldb/include/lldb/Utility/XcodeSDK.h
@@ -9,7 +9,6 @@
#ifndef LLDB_UTILITY_SDK_H
#define LLDB_UTILITY_SDK_H
-#include "lldb/Utility/FileSpec.h"
#include "lldb/lldb-forward.h"
#include "llvm/ADT/StringRef.h"
#include "llvm/Support/VersionTuple.h"
@@ -24,7 +23,6 @@ namespace lldb_private {
/// An abstraction for Xcode-style SDKs that works like \ref ArchSpec.
class XcodeSDK {
std::string m_name;
- FileSpec m_sysroot;
public:
/// Different types of Xcode SDKs.
@@ -64,8 +62,6 @@ 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, FileSpec sysroot)
- : m_name(std::move(name)), m_sysroot(std::move(sysroot)) {}
static XcodeSDK GetAnyMacOS() { return XcodeSDK("MacOSX.sdk"); }
/// The merge function follows a strict order to maintain monotonicity:
@@ -83,7 +79,6 @@ class XcodeSDK {
llvm::VersionTuple GetVersion() const;
Type GetType() const;
llvm::StringRef GetString() const;
- const FileSpec &GetSysroot() const;
/// Whether this Xcode SDK supports Swift.
bool SupportsSwift() const;
diff --git a/lldb/source/Plugins/Platform/MacOSX/PlatformDarwin.cpp b/lldb/source/Plugins/Platform/MacOSX/PlatformDarwin.cpp
index ee8e256748cea..665500f23e95d 100644
--- a/lldb/source/Plugins/Platform/MacOSX/PlatformDarwin.cpp
+++ b/lldb/source/Plugins/Platform/MacOSX/PlatformDarwin.cpp
@@ -1425,9 +1425,6 @@ PlatformDarwin::ResolveSDKPathFromDebugInfo(Module &module) {
auto [sdk, _] = std::move(*sdk_or_err);
- if (FileSystem::Instance().Exists(sdk.GetSysroot()))
- return sdk.GetSysroot().GetPath();
-
auto path_or_err = HostInfo::GetSDKRoot(HostInfo::SDKOptions{sdk});
if (!path_or_err)
return llvm::createStringError(
diff --git a/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp b/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
index d562de84db94f..58b544a9a137b 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
+++ b/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
@@ -993,26 +993,21 @@ XcodeSDK SymbolFileDWARF::ParseXcodeSDK(CompileUnit &comp_unit) {
const char *sdk = cu_die.GetAttributeValueAsString(DW_AT_APPLE_sdk, nullptr);
if (!sdk)
return {};
- std::string sysroot =
+ const char *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);
- // 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);
- }
+ 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, FileSpec{std::move(sysroot)}};
+ return {sdk};
}
size_t SymbolFileDWARF::ParseFunctions(CompileUnit &comp_unit) {
diff --git a/lldb/source/Utility/XcodeSDK.cpp b/lldb/source/Utility/XcodeSDK.cpp
index 02cf7866e22fb..b7d51f7e0827a 100644
--- a/lldb/source/Utility/XcodeSDK.cpp
+++ b/lldb/source/Utility/XcodeSDK.cpp
@@ -142,8 +142,6 @@ XcodeSDK::Type XcodeSDK::GetType() const {
llvm::StringRef XcodeSDK::GetString() const { return m_name; }
-const FileSpec &XcodeSDK::GetSysroot() const { return m_sysroot; }
-
bool XcodeSDK::Info::operator<(const Info &other) const {
return std::tie(type, version, internal) <
std::tie(other.type, other.version, other.internal);
@@ -155,10 +153,6 @@ bool XcodeSDK::Info::operator==(const Info &other) const {
}
void XcodeSDK::Merge(const XcodeSDK &other) {
- auto add_internal_sdk_suffix = [](llvm::StringRef sdk) {
- return (sdk.substr(0, sdk.size() - 3) + "Internal.sdk").str();
- };
-
// The "bigger" SDK always wins.
auto l = Parse();
auto r = other.Parse();
@@ -166,13 +160,10 @@ void XcodeSDK::Merge(const XcodeSDK &other) {
*this = other;
else {
// The Internal flag always wins.
- if (!l.internal && r.internal) {
- if (llvm::StringRef(m_name).ends_with(".sdk"))
- m_name = add_internal_sdk_suffix(m_name);
-
- if (m_sysroot.GetFileNameExtension() == ".sdk")
- m_sysroot.SetFilename(add_internal_sdk_suffix(m_sysroot.GetFilename()));
- }
+ 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");
}
}
diff --git a/lldb/unittests/SymbolFile/DWARF/XcodeSDKModuleTests.cpp b/lldb/unittests/SymbolFile/DWARF/XcodeSDKModuleTests.cpp
index 21b87eb1a75ba..b3bf4d9219d3e 100644
--- a/lldb/unittests/SymbolFile/DWARF/XcodeSDKModuleTests.cpp
+++ b/lldb/unittests/SymbolFile/DWARF/XcodeSDKModuleTests.cpp
@@ -307,28 +307,13 @@ SDKPathParsingTestData sdkPathParsingTestCases[] = {
.expect_internal_sdk = true,
.expect_sdk_path_pattern = "Internal.sdk"},
- /// 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
+ /// Two CUs with an internal SDK each
{.input_sdk_paths =
{"/Library/Developer/CommandLineTools/SDKs/iPhoneOS14.1.sdk",
- "/Path/To/SDKs/MacOSX11.3.sdk"},
+ "/Library/Developer/CommandLineTools/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 = true,
- .expect_sdk_path_pattern = "iPhoneOS14.1.Internal.sdk"},
};
INSTANTIATE_TEST_SUITE_P(SDKPathParsingTests, SDKPathParsingMultiparamTests,
diff --git a/lldb/unittests/Utility/XcodeSDKTest.cpp b/lldb/unittests/Utility/XcodeSDKTest.cpp
index bca7f1291c425..8bf7ee1be1dba 100644
--- a/lldb/unittests/Utility/XcodeSDKTest.cpp
+++ b/lldb/unittests/Utility/XcodeSDKTest.cpp
@@ -34,17 +34,12 @@ 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", FileSpec{"/Path/To/MacOSX.sdk"}).GetSysroot(),
- FileSpec("/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_FALSE(XcodeSDK("MacOSX10.15.Internal.sdk").GetSysroot());
EXPECT_EQ(XcodeSDK().GetType(), XcodeSDK::unknown);
EXPECT_EQ(XcodeSDK().GetVersion(), llvm::VersionTuple());
- EXPECT_FALSE(XcodeSDK().GetSysroot());
}
TEST(XcodeSDKTest, MergeTest) {
@@ -65,14 +60,6 @@ TEST(XcodeSDKTest, MergeTest) {
XcodeSDK empty;
empty.Merge(XcodeSDK("MacOSX10.14.Internal.sdk"));
EXPECT_EQ(empty.GetString(), llvm::StringRef("MacOSX10.14.Internal.sdk"));
- EXPECT_FALSE(empty.GetSysroot());
- empty.Merge(XcodeSDK("MacOSX9.5.Internal.sdk", FileSpec{"/Path/To/9.5.sdk"}));
- EXPECT_FALSE(empty.GetSysroot());
- empty.Merge(XcodeSDK("MacOSX12.5.sdk", FileSpec{"/Path/To/12.5.sdk"}));
- EXPECT_EQ(empty.GetSysroot(), FileSpec{"/Path/To/12.5.sdk"});
- empty.Merge(XcodeSDK("MacOSX11.5.Internal.sdk",
- FileSpec{"/Path/To/12.5.Internal.sdk"}));
- EXPECT_EQ(empty.GetSysroot(), FileSpec{"/Path/To/12.5.Internal.sdk"});
}
#ifndef _WIN32
|
Michael137
added a commit
that referenced
this pull request
Mar 4, 2025
…of xcrun when looking up SDK" (#129621)" This reverts commit 6041c74. Relands the original patch with the test-case data fixed. Weirldy the PR CI didn't seem to run the unit-tests? In any case, the problem was an incorrect expectation in the test-case data. Since we have both public and internal SDK in that test-case, we should `expect_mismatch` to be `true`.
Michael137
added a commit
to swiftlang/llvm-project
that referenced
this pull request
Mar 4, 2025
…of xcrun when looking up SDK" (llvm#129621)" This reverts commit 6041c74. Relands the original patch with the test-case data fixed. Weirldy the PR CI didn't seem to run the unit-tests? In any case, the problem was an incorrect expectation in the test-case data. Since we have both public and internal SDK in that test-case, we should `expect_mismatch` to be `true`. (cherry picked from commit 77a8770)
|
Oh weird, that means the unit-tests weren't run in PR CI? Relanded in 77a8770 |
|
Oh it's because these unit-tests are only run on Apple platforms, nvm |
jph-13
pushed a commit
to jph-13/llvm-project
that referenced
this pull request
Mar 21, 2025
…of xcrun when looking up SDK" (llvm#129621) Reverts llvm#128712 ``` ******************** TEST 'lldb-unit :: SymbolFile/DWARF/./SymbolFileDWARFTests/10/14' FAILED ******************** Script(shard): -- GTEST_OUTPUT=json:/Users/ec2-user/jenkins/workspace/llvm.org/as-lldb-cmake/lldb-build/tools/lldb/unittests/SymbolFile/DWARF/./SymbolFileDWARFTests-lldb-unit-1021-10-14.json GTEST_SHUFFLE=1 GTEST_TOTAL_SHARDS=14 GTEST_SHARD_INDEX=10 GTEST_RANDOM_SEED=62233 /Users/ec2-user/jenkins/workspace/llvm.org/as-lldb-cmake/lldb-build/tools/lldb/unittests/SymbolFile/DWARF/./SymbolFileDWARFTests -- Script: -- /Users/ec2-user/jenkins/workspace/llvm.org/as-lldb-cmake/lldb-build/tools/lldb/unittests/SymbolFile/DWARF/./SymbolFileDWARFTests --gtest_filter=SDKPathParsingTests/SDKPathParsingMultiparamTests.TestSDKPathFromDebugInfo/6 -- /Users/ec2-user/jenkins/workspace/llvm.org/as-lldb-cmake/llvm-project/lldb/unittests/SymbolFile/DWARF/XcodeSDKModuleTests.cpp:265: Failure Expected equality of these values: found_mismatch Which is: true expect_mismatch Which is: false /Users/ec2-user/jenkins/workspace/llvm.org/as-lldb-cmake/llvm-project/lldb/unittests/SymbolFile/DWARF/XcodeSDKModuleTests.cpp:265 Expected equality of these values: found_mismatch Which is: true expect_mismatch Which is: false ```
jph-13
pushed a commit
to jph-13/llvm-project
that referenced
this pull request
Mar 21, 2025
…of xcrun when looking up SDK" (llvm#129621)" This reverts commit 6041c74. Relands the original patch with the test-case data fixed. Weirldy the PR CI didn't seem to run the unit-tests? In any case, the problem was an incorrect expectation in the test-case data. Since we have both public and internal SDK in that test-case, we should `expect_mismatch` to be `true`.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Reverts #128712
@Michael137 I think this patch is causing green dragon to fail, so I'm reverting it for now.