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

Michael137 · 2024-08-03T00:06:02Z

This patch changes the way we initialize BuiltinHeadersInSystemModules which is one of the flags controlling Clang's behaviour when the Darwin module is split into more fine-grained modules. 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

…ding on SDK version 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 make the `CompilerInstance` construction in `ClangExpressionParser` go through the driver, so we can avoid duplicating the logic in LLDB.

llvmbot · 2024-08-05T08:02:22Z

@llvm/pr-subscribers-lldb

Author: Michael Buch (Michael137)

Changes

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 --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);
   }

adrian-prantl · 2024-08-05T17:09:42Z

lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp

+  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 is also this, which might be shorter. https://llvm.org/doxygen/namespacellvm_1_1sys_1_1path.html#ad7c38f73fbf2f1a4ed9578621c6bfe8b

Nice! good to know

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?

adrian-prantl · 2024-08-05T17:10:52Z

lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp

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


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.

adrian-prantl · 2024-08-05T17:11:11Z

lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp

+// NOTE: should be kept in sync with sdkSupportsBuiltinModules in
+// Toolchains/Darwin.cpp
+static bool
+sdkSupportsBuiltinModulesImpl(const llvm::Triple &triple,


Can you move this into XcodeSDK.h?

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)

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.

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.

Michael137 · 2024-08-06T16:56:11Z

Talked to Adrian and we decided it's best if we landed this on the apple/llvm-project fork 6.0 branch. There's a larger refactor we'll do on llvm.org main instead (mainly re-using the PlatformDarwin and HostInfo utilities to get the correct SDK directory, instead of the DWARF support files).

Michael137 requested review from adrian-prantl and ian-twilightcoder August 3, 2024 00:06

Michael137 added 5 commits August 3, 2024 01:12

fixup! fix comment

664059c

Merge branch 'main' into lldb/builtin-headers-sdk-parsing-fix

a1a23f7

fixup!

eb2f213

fixup! add comments

3070e78

fixup! move assert

b4b79b9

Michael137 marked this pull request as ready for review August 5, 2024 08:01

Michael137 requested a review from JDevlieghere as a code owner August 5, 2024 08:01

llvmbot added the lldb label Aug 5, 2024

Michael137 changed the title ~~[WIP] [lldb][ClangExpressionParser] Set BuiltinHeadersInSystemModules depending on SDK version~~ [lldb][ClangExpressionParser] Set BuiltinHeadersInSystemModules depending on SDK version Aug 5, 2024

adrian-prantl reviewed Aug 5, 2024

View reviewed changes

fixup! add another FIXME

ff7333f

Michael137 closed this Aug 6, 2024

Michael137 mentioned this pull request Aug 6, 2024

[lldb][ClangExpressionParser] Set BuiltinHeadersInSystemModules depending on SDK version swiftlang/llvm-project#9059

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

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

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

Uh oh!

Michael137 commented Aug 3, 2024 •

edited

Loading

Uh oh!

llvmbot commented Aug 5, 2024

Uh oh!

adrian-prantl Aug 5, 2024

Uh oh!

Michael137 Aug 5, 2024

Uh oh!

Michael137 Aug 5, 2024 •

edited

Loading

Uh oh!

adrian-prantl Aug 5, 2024

Uh oh!

adrian-prantl Aug 5, 2024

Uh oh!

Michael137 Aug 5, 2024 •

edited

Loading

Uh oh!

JDevlieghere Aug 6, 2024

Uh oh!

Michael137 Aug 6, 2024 •

edited

Loading

Uh oh!

Michael137 commented Aug 6, 2024

Uh oh!

Uh oh!

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

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

Uh oh!

Conversation

Michael137 commented Aug 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Aug 5, 2024

Uh oh!

adrian-prantl Aug 5, 2024

Choose a reason for hiding this comment

Uh oh!

Michael137 Aug 5, 2024

Choose a reason for hiding this comment

Uh oh!

Michael137 Aug 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adrian-prantl Aug 5, 2024

Choose a reason for hiding this comment

Uh oh!

adrian-prantl Aug 5, 2024

Choose a reason for hiding this comment

Uh oh!

Michael137 Aug 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JDevlieghere Aug 6, 2024

Choose a reason for hiding this comment

Uh oh!

Michael137 Aug 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Michael137 commented Aug 6, 2024

Uh oh!

Uh oh!

Michael137 commented Aug 3, 2024 •

edited

Loading

Michael137 Aug 5, 2024 •

edited

Loading

Michael137 Aug 5, 2024 •

edited

Loading

Michael137 Aug 6, 2024 •

edited

Loading