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

[Michael137]

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

[llvmbot]

@llvm/pr-subscribers-lldb

Author: Michael Buch (Michael137)

Changes

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

---

Full diff: https://github.com/llvm/llvm-project/pull/102309.diff

3 Files Affected:

• (modified) lldb/include/lldb/Utility/XcodeSDK.h (+14)
• (modified) lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp (+49-3)
• (modified) lldb/source/Utility/XcodeSDK.cpp (+21)

diff --git a/lldb/include/lldb/Utility/XcodeSDK.h b/lldb/include/lldb/Utility/XcodeSDK.h
index 673ea578ffce85..69c7e23d3d0f63 100644
--- a/lldb/include/lldb/Utility/XcodeSDK.h
+++ b/lldb/include/lldb/Utility/XcodeSDK.h
@@ -85,6 +85,20 @@ class XcodeSDK {
   /// Whether LLDB feels confident importing Clang modules from this SDK.
   static bool SDKSupportsModules(Type type, llvm::VersionTuple version);
   static bool SDKSupportsModules(Type desired_type, const FileSpec &sdk_path);
+
+  /// Returns true if the SDK for the specified triple supports
+  /// builtin modules in system headers.
+  ///
+  /// NOTE: should be kept in sync with sdkSupportsBuiltinModules in
+  /// Toolchains/Darwin.cpp
+  ///
+  /// FIXME: this function will be removed once LLDB's ClangExpressionParser
+  /// constructs the compiler instance through the driver/toolchain. See \ref
+  /// SetupImportStdModuleLangOpts
+  ///
+  static bool SDKSupportsBuiltinModules(const llvm::Triple &triple,
+                                        llvm::VersionTuple sdk_version);
+
   /// Return the canonical SDK name, such as "macosx" for the macOS SDK.
   static std::string GetCanonicalName(Info info);
   /// Return the best-matching SDK type for a specific triple.
diff --git a/lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp b/lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp
index f41323d32ac863..3b67d28203c3de 100644
--- a/lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp
+++ b/lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp
@@ -39,6 +39,7 @@
 #include "llvm/ExecutionEngine/ExecutionEngine.h"
 #include "llvm/Support/CrashRecoveryContext.h"
 #include "llvm/Support/Debug.h"
+#include "llvm/Support/Error.h"
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/TargetSelect.h"
 
@@ -91,6 +92,8 @@
 #include "lldb/Utility/StringList.h"
 
 #include "Plugins/LanguageRuntime/ObjC/ObjCLanguageRuntime.h"
+#include "Plugins/Platform/MacOSX/PlatformDarwin.h"
+#include "lldb/Utility/XcodeSDK.h"
 
 #include <cctype>
 #include <memory>
@@ -279,6 +282,48 @@ class ClangDiagnosticManagerAdapter : public clang::DiagnosticConsumer {
   std::string m_output;
 };
 
+/// 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.
+///
+/// \param[in] target The target triple.
+///
+static llvm::Expected<bool>
+sdkSupportsBuiltinModules(lldb_private::Target &target) {
+  auto arch_spec = target.GetArchitecture();
+  auto const &triple = arch_spec.GetTriple();
+  auto module_sp = target.GetExecutableModule();
+  if (!module_sp)
+    return llvm::createStringError("Executable module not found.");
+
+  // Get SDK path that the target was compiled against.
+  auto sdk_or_err = PlatformDarwin::GetSDKPathFromDebugInfo(*module_sp);
+  if (!sdk_or_err)
+    return sdk_or_err.takeError();
+
+  // Use the SDK path from debug-info to find a local matching SDK directory.
+  auto sdk_path_or_err =
+      HostInfo::GetSDKRoot(HostInfo::SDKOptions{std::move(sdk_or_err->first)});
+  if (!sdk_path_or_err)
+    return sdk_path_or_err.takeError();
+
+  auto VFS = FileSystem::Instance().GetVirtualFileSystem();
+  if (!VFS)
+    return llvm::createStringError("No virtual filesystem available.");
+
+  // Extract SDK version from the /path/to/some.sdk/SDKSettings.json
+  auto parsed_or_err = clang::parseDarwinSDKInfo(*VFS, *sdk_path_or_err);
+  if (!parsed_or_err)
+    return parsed_or_err.takeError();
+
+  auto maybe_sdk = *parsed_or_err;
+  if (!maybe_sdk)
+    return llvm::createStringError("Couldn't find Darwin SDK info.");
+
+  return XcodeSDK::SDKSupportsBuiltinModules(triple, maybe_sdk->getVersion());
+}
+
 static void SetupModuleHeaderPaths(CompilerInstance *compiler,
                                    std::vector<std::string> include_directories,
                                    lldb::TargetSP target_sp) {
@@ -561,7 +606,8 @@ static void SetupLangOpts(CompilerInstance &compiler,
   lang_opts.NoBuiltin = true;
 }
 
-static void SetupImportStdModuleLangOpts(CompilerInstance &compiler) {
+static void SetupImportStdModuleLangOpts(CompilerInstance &compiler,
+                                         lldb_private::Target &target) {
   LangOptions &lang_opts = compiler.getLangOpts();
   lang_opts.Modules = true;
   // We want to implicitly build modules.
@@ -578,7 +624,7 @@ static void SetupImportStdModuleLangOpts(CompilerInstance &compiler) {
   lang_opts.GNUMode = true;
   lang_opts.GNUKeywords = true;
   lang_opts.CPlusPlus11 = true;
-  lang_opts.BuiltinHeadersInSystemModules = true;
+  lang_opts.BuiltinHeadersInSystemModules = !sdkSupportsBuiltinModules(target);
 
   // The Darwin libc expects this macro to be set.
   lang_opts.GNUCVersion = 40201;
@@ -659,7 +705,7 @@ 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);
     SetupModuleHeaderPaths(m_compiler.get(), m_include_directories, target_sp);
   }
 
diff --git a/lldb/source/Utility/XcodeSDK.cpp b/lldb/source/Utility/XcodeSDK.cpp
index 712d611db28f83..64e0672a963ca9 100644
--- a/lldb/source/Utility/XcodeSDK.cpp
+++ b/lldb/source/Utility/XcodeSDK.cpp
@@ -259,6 +259,27 @@ bool XcodeSDK::SupportsSwift() const {
   }
 }
 
+bool XcodeSDK::SDKSupportsBuiltinModules(const llvm::Triple &triple,
+                                         llvm::VersionTuple sdk_version) {
+  using namespace llvm;
+
+  switch (triple.getOS()) {
+  case Triple::OSType::MacOSX:
+    return sdk_version >= VersionTuple(15U);
+  case Triple::OSType::IOS:
+    return sdk_version >= VersionTuple(18U);
+  case Triple::OSType::TvOS:
+    return sdk_version >= VersionTuple(18U);
+  case Triple::OSType::WatchOS:
+    return sdk_version >= VersionTuple(11U);
+  case Triple::OSType::XROS:
+    return sdk_version >= VersionTuple(2U);
+  default:
+    // New SDKs support builtin modules from the start.
+    return true;
+  }
+}
+
 bool XcodeSDK::SDKSupportsModules(XcodeSDK::Type desired_type,
                                   const FileSpec &sdk_path) {
   ConstString last_path_component = sdk_path.GetFilename();

[adrian-prantl]

Thanks, this LGTM, next step should be to replace the use of include_directories with DW_AT_APPLE_sdk.

[ian-twilightcoder]

This really isn't the right approach. BuiltinHeadersInSystemModules isn't the only important default that the driver sets up. Can we change the invocation setup to use clang::createInvocation or clang::CompilerInvocation::CreateFromArgs or clang::BuildCompilation? Or is that planned for another change?

[ian-twilightcoder]

This function got changed in #102239

[Michael137]

I already accounted for the version check changes

[ian-twilightcoder]

But you didn't pick up the environment check

[Michael137]

Don't think we really want to support this in/care about non-native environments tbh

[Michael137]

This really isn't the right approach. BuiltinHeadersInSystemModules isn't the only important default that the driver sets up. Can we change the invocation setup to use clang::createInvocation or clang::CompilerInvocation::CreateFromArgs or clang::BuildCompilation? Or is that planned for another change?

Yes I 100% agree that we should use the driver. This has been a FIXME for many years now. But to unblock the current test failures I think this should be fine to land. And when we switch to using the driver properly we'll remove this.