Skip to content

[lldb][ClangExpressionParser] Set BuiltinHeadersInSystemModules depending on SDK version #101778

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

Closed
Closed
Changes from 6 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
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
#include "clang/AST/ExternalASTSource.h"
#include "clang/AST/PrettyPrinter.h"
#include "clang/Basic/Builtins.h"
#include "clang/Basic/DarwinSDKInfo.h"
#include "clang/Basic/DiagnosticIDs.h"
#include "clang/Basic/SourceLocation.h"
#include "clang/Basic/TargetInfo.h"
Expand Down Expand Up @@ -561,7 +562,85 @@ static void SetupLangOpts(CompilerInstance &compiler,
lang_opts.NoBuiltin = true;
}

static void SetupImportStdModuleLangOpts(CompilerInstance &compiler) {
// NOTE: should be kept in sync with sdkSupportsBuiltinModules in
// Toolchains/Darwin.cpp
static bool
sdkSupportsBuiltinModulesImpl(const llvm::Triple &triple,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you move this into XcodeSDK.h?

Copy link
Member Author

@Michael137 Michael137 Aug 5, 2024

Choose a reason for hiding this comment

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

Like make it a static function on XcodeSDK? We're not creating any XCodeSDK instances here (since we're not actually parsing the DW_AT_LLVM_sysroot here; instead we're just given directory paths)

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, even if it's a static method it'd be good to centralize that logic. If someone needs something similar in the future, it's the first place they'd look.

Copy link
Member Author

@Michael137 Michael137 Aug 6, 2024

Choose a reason for hiding this comment

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

Tbh I'd prefer keeping this as an implementation detail. We really don't want to duplicate this logic (currently this is copied from Toolchains/Darwin.cpp. And can hopefully remove it eventually. If it lives as an API in a header that has a slight chance of making it harder to remove.
But lmk if either of you feel strongly about it.

const std::optional<DarwinSDKInfo> &SDKInfo) {
if (!SDKInfo)
return false;

VersionTuple SDKVersion = SDKInfo->getVersion();
switch (triple.getOS()) {
case Triple::OSType::MacOSX:
return SDKVersion >= VersionTuple(15U);
case Triple::OSType::IOS:
return SDKVersion >= VersionTuple(18U);
case Triple::OSType::TvOS:
return SDKVersion >= VersionTuple(18U);
case Triple::OSType::WatchOS:
return SDKVersion >= VersionTuple(11U);
case Triple::OSType::XROS:
return SDKVersion >= VersionTuple(2U);
default:
// New SDKs support builtin modules from the start.
return true;
}
}

/// Returns true if the SDK for the specified triple supports
/// builtin modules in system headers. This is used to decide
/// whether to pass -fbuiltin-headers-in-system-modules to
/// the compiler instance when compiling the `std` module.
///
/// This function assumes one of the directories in \ref include_dirs
/// is an SDK path that exists on the host. The SDK version is
/// read from the SDKSettings.json in that directory.
///
/// \param[in] triple The target triple.
/// \param[in] include_dirs The include directories the compiler will use
/// during header search.
///
static bool
sdkSupportsBuiltinModules(llvm::Triple const &triple,
std::vector<std::string> const &include_dirs) {
// Find an SDK directory.
static constexpr std::string_view s_sdk_suffix = ".sdk";
auto it = llvm::find_if(include_dirs, [](std::string const &path) {
return path.find(s_sdk_suffix) != std::string::npos;
});
Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Nice! good to know

Copy link
Member Author

@Michael137 Michael137 Aug 5, 2024

Choose a reason for hiding this comment

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

Or actually, would this buy us anything? Unless I'm missing something, we'd still have to do something like

auto it = llvm::find_if(include_dirs, [](std::string const &dir) {
    return sys::llvm::path::extension(dir) == s_sdk_suffix;
  });

Which I guess is slightly more readable than the current version?

if (it == include_dirs.end())
return false;

auto VFS = FileSystem::Instance().GetVirtualFileSystem();
if (!VFS)
return false;

// Extract: /path/to/some.sdk
size_t suffix = it->find(s_sdk_suffix);
assert(suffix != std::string::npos);
std::string sdk_path = it->substr(0, suffix + s_sdk_suffix.size());

// Extract SDK version from the /path/to/some.sdk/SDKSettings.json
auto parsed = clang::parseDarwinSDKInfo(*VFS, sdk_path);
if (!parsed)
return false;

return sdkSupportsBuiltinModulesImpl(triple, *parsed);
}

/// Sets the LangOptions to prepare for running with `import-std-module`
/// enabled.
///
/// \param[out] compiler CompilerInstance on which to set the LangOptions.
/// \param[in] triple The target triple.
/// \param[in] include_dirs The include directories the compiler will use
/// during header search.
///
static void
SetupImportStdModuleLangOpts(CompilerInstance &compiler,
llvm::Triple const &triple,
std::vector<std::string> const &include_dirs) {
LangOptions &lang_opts = compiler.getLangOpts();
lang_opts.Modules = true;
// We want to implicitly build modules.
Expand All @@ -578,7 +657,12 @@ static void SetupImportStdModuleLangOpts(CompilerInstance &compiler) {
lang_opts.GNUMode = true;
lang_opts.GNUKeywords = true;
lang_opts.CPlusPlus11 = true;
lang_opts.BuiltinHeadersInSystemModules = true;

// FIXME: We should use the driver to derive this for us.
// ClangModulesDeclVendor already parses the SDKSettings for the purposes of
// this check.
Copy link
Collaborator

Choose a reason for hiding this comment

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

we should put another FIXME here (we can fix that later) that we should use DW_AT_LLVM_sdk and then look up a matching local SDK using HostInfo, instead of the include dirs.

lang_opts.BuiltinHeadersInSystemModules =
!sdkSupportsBuiltinModules(triple, include_dirs);

// The Darwin libc expects this macro to be set.
lang_opts.GNUCVersion = 40201;
Expand Down Expand Up @@ -663,7 +747,9 @@ ClangExpressionParser::ClangExpressionParser(
if (auto *clang_expr = dyn_cast<ClangUserExpression>(&m_expr);
clang_expr && clang_expr->DidImportCxxModules()) {
LLDB_LOG(log, "Adding lang options for importing C++ modules");
SetupImportStdModuleLangOpts(*m_compiler);
SetupImportStdModuleLangOpts(*m_compiler,
target_sp->GetArchitecture().GetTriple(),
m_include_directories);
SetupModuleHeaderPaths(m_compiler.get(), m_include_directories, target_sp);
}

Expand Down
Loading