Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[lldb]HostInfoMacOSX] Search CommandLineTools directory when looking up SDK paths #128712

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions lldb/include/lldb/Host/FileSystem.h
Original file line number Diff line number Diff line change
Expand Up @@ -183,8 +183,9 @@ class FileSystem {
eEnumerateDirectoryResultQuit
};

typedef EnumerateDirectoryResult (*EnumerateDirectoryCallbackType)(
void *baton, llvm::sys::fs::file_type file_type, llvm::StringRef);
typedef std::function<EnumerateDirectoryResult(
void *baton, llvm::sys::fs::file_type file_type, llvm::StringRef)>
EnumerateDirectoryCallbackType;

typedef std::function<EnumerateDirectoryResult(
llvm::sys::fs::file_type file_type, llvm::StringRef)>
Expand Down
60 changes: 60 additions & 0 deletions lldb/source/Host/macosx/objcxx/HostInfoMacOSX.mm
Original file line number Diff line number Diff line change
Expand Up @@ -15,11 +15,14 @@
#include "lldb/Utility/Log.h"
#include "lldb/Utility/Timer.h"

#include "clang/Basic/DarwinSDKInfo.h"
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this a dependency we can pull in? Probably not?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, this goes against all the work of localizing the places we depend on clang.

#include "llvm/ADT/ScopeExit.h"
#include "llvm/ADT/SmallString.h"
#include "llvm/ADT/StringMap.h"
#include "llvm/Support/Error.h"
#include "llvm/Support/FileSystem.h"
#include "llvm/Support/Path.h"
#include "llvm/Support/VersionTuple.h"
#include "llvm/Support/raw_ostream.h"

// C++ Includes
Expand Down Expand Up @@ -569,10 +572,52 @@ static bool ResolveAndVerifyCandidateSupportDir(FileSpec &path) {
cache.insert({key, {error, true}});
return llvm::createStringError(llvm::inconvertibleErrorCode(), error);
}

if (path_or_err->empty())
return llvm::createStringError("Empty path determined for '%s'",
key.data());

auto it_new = cache.insert({key, {*path_or_err, false}});
return it_new.first->second.str;
}

static llvm::Expected<std::string>
GetCommandLineToolsSDKRoot(llvm::VersionTuple version) {
std::string clt_root_dir;
FileSystem::Instance().EnumerateDirectory(
"/Library/Developer/CommandLineTools/SDKs/", /*find_directories=*/true,
/*find_files=*/false, /*find_other=*/false,
[&](void *baton, llvm::sys::fs::file_type file_type,
llvm::StringRef name) {
assert(file_type == llvm::sys::fs::file_type::directory_file);

if (!name.ends_with(".sdk"))
return FileSystem::eEnumerateDirectoryResultNext;

llvm::Expected<std::optional<clang::DarwinSDKInfo>> sdk_info =
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alternatively we can scan the .plist using LLDB's ApplePropertyList, and extract the SDK version/name from there. That would allow us not to depend on Clang here

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm but looks like it's only able to parse the XML version, not the binary one?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess worst case we could read the json ourselves and get the key we're looking for (Clang does a bit more than that, it looks at VersionMappings etc.)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it should support both. I'm sure it can write binary property lists. I would assume it can read them too.

clang::parseDarwinSDKInfo(
*FileSystem::Instance().GetVirtualFileSystem(), name);
if (!sdk_info) {
LLDB_LOG_ERROR(GetLog(LLDBLog::Expressions), sdk_info.takeError(),
"Error while parsing {1}: {0}", name);
return FileSystem::eEnumerateDirectoryResultNext;
}

if (!*sdk_info)
return FileSystem::eEnumerateDirectoryResultNext;

if (version == (*sdk_info)->getVersion()) {
clt_root_dir = name;
return FileSystem::eEnumerateDirectoryResultQuit;
}

return FileSystem::eEnumerateDirectoryResultNext;
},
/*baton=*/nullptr);

return clt_root_dir;
}

llvm::Expected<llvm::StringRef> HostInfoMacOSX::GetSDKRoot(SDKOptions options) {
static llvm::StringMap<ErrorOrPath> g_sdk_path;
static std::mutex g_sdk_path_mutex;
Expand All @@ -581,6 +626,21 @@ static bool ResolveAndVerifyCandidateSupportDir(FileSpec &path) {
"XcodeSDK not specified");
XcodeSDK sdk = *options.XcodeSDKSelection;
auto key = sdk.GetString();

// xcrun doesn't search SDKs in the CommandLineTools (CLT) directory. So if
// a program was compiled against a CLT SDK, but that SDK wasn't present in
// any of the Xcode installations, then xcrun would fail to find the SDK
// (which is expensive). To avoid this we first try to find the specified SDK
// in the CLT directory.
auto clt_root_dir = find_cached_path(g_sdk_path, g_sdk_path_mutex, key, [&] {
return GetCommandLineToolsSDKRoot(sdk.GetVersion());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this mean we're going to prefer the CommandLineTools SDK over the Xcode one when both exist? I know they should be the same, but I'd really prefer to avoid the CLT whenever we can.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's my only concern with this, but if we wanted to avoid this behavior, then we would need to reimplement all of xcrun's logic to find SDKs in Xcode. That's also not something I'm excited about.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, you don't know you need the CLT until you know it's not in Xcode, which is exactly the problem this is trying to avoid.

});

if (clt_root_dir)
return clt_root_dir;
else
llvm::consumeError(clt_root_dir.takeError());

return find_cached_path(g_sdk_path, g_sdk_path_mutex, key, [&](){
return GetXcodeSDK(sdk);
});
Expand Down