llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-lldb Author: Michael Buch (Michael137) <details> <summary>Changes</summary> This patch addresses an issue that will arise with future SDKs. The ClangExpressionParser currently unconditionally sets `-fbuiltin-headers-in-system-modules` when evaluating expressions with the `target.import-std-module` setting. This flag should, however, be set depending on the SDK version (which is what the Clang Darwin toolchain does). Unfortunately, the compiler instance that we create with `ClangExpressionParser` never consults the Clang driver, and thus doesn't correctly infer `BuiltinHeadersInSystemModules`. Note, this isn't an issue with the `CompilerInstance` that the `ClangModulesDeclVendor` creates because it uses the `createInovcation` API, which calls into `Darwin::addClangTargetOptions`. This patch mimicks how `sdkSupportsBuiltinModules` is used in `Darwin::addClangTargetOptions`. This ensures that the `import-std-module` API tests run cleanly regardless of SDK version. The plan is to eventually make the `CompilerInstance` construction in `ClangExpressionParser` go through the driver, so we can avoid duplicating the logic in LLDB. But we aren't there yet. **Implementation** * We look for the `SDKSettings.json` in the sysroot directory that we found in DWARF (via `DW_AT_LLVM_sysroot`) * Then parse this file and extract the SDK version number out of it * Then mimick `sdkSupportsBuiltinModules` from `Toolchains/Darwin.cpp` and set `BuiltinHeadersInSystemModules` based on it rdar://116490281 --- Full diff: https://github.com/llvm/llvm-project/pull/101778.diff 1 Files Affected: - (modified) lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp (+89-3) ``````````diff diff --git a/lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp b/lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp index 2a8bdf29314e4..e05cd649bb044 100644 --- a/lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp +++ b/lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp @@ -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; + }); + 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. + 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); } `````````` </details> https://github.com/llvm/llvm-project/pull/101778 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
