-
Notifications
You must be signed in to change notification settings - Fork 14.3k
[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
Changes from 6 commits
3d7c2b8
664059c
a1a23f7
eb2f213
3070e78
b4b79b9
ff7333f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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" | ||
|
@@ -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, | ||
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; | ||
}); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There is also this, which might be shorter. https://llvm.org/doxygen/namespacellvm_1_1sys_1_1path.html#ad7c38f73fbf2f1a4ed9578621c6bfe8b There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nice! good to know There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
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. | ||
|
@@ -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. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; | ||
|
@@ -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); | ||
} | ||
|
||
|
There was a problem hiding this comment.
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
?Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
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 onXcodeSDK
? We're not creating anyXCodeSDK
instances here (since we're not actually parsing theDW_AT_LLVM_sysroot
here; instead we're just given directory paths)There was a problem hiding this comment.
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.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
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.