[Lldb-commits] [lldb] [WIP] [lldb][ClangExpressionParser] Set BuiltinHeadersInSystemModules depending on SDK version (PR #101778)

[via lldb-commits]

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