[lldb][ClangExpressionParser] Set BuiltinHeadersInSystemModules depending on SDK version #9059
Conversation
…ding on SDK version 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
|
@swift-ci test |
| // NOTE: should be kept in sync with sdkSupportsBuiltinModules in | ||
| // Toolchains/Darwin.cpp | ||
| static bool | ||
| sdkSupportsBuiltinModulesImpl(const llvm::Triple &triple, |
There was a problem hiding this comment.
I'm about to change the implementation of this function in the Darwin driver
There was a problem hiding this comment.
Implementation in what sense? Changes to the SDK version checks?
There was a problem hiding this comment.
Ah yes I accounted for that in this patch already
|
@cachemeifyoucan @Bigcheese @cyndyishida is it really that difficult to change this usage to use |
I have something that works to some extent. But it's definitely a risky enough change to land for 6.0 |
I have been advocating lldb to change to use them to avoid a lot of the module configuration issues. |
Whats the risk involved? |
| @@ -576,7 +644,8 @@ ClangExpressionParser::ClangExpressionParser( | |||
| lang_opts.GNUMode = true; | |||
| lang_opts.GNUKeywords = true; | |||
| lang_opts.CPlusPlus11 = true; | |||
| lang_opts.BuiltinHeadersInSystemModules = true; | |||
| lang_opts.BuiltinHeadersInSystemModules = !sdkSupportsBuiltinModules( | |||
There was a problem hiding this comment.
This ! is the intended direction, right?
There was a problem hiding this comment.
Yes. The builtin headers have to go in the system modules if the sdk doesn't support the builtin modules.
I wouldn't want to change the way we setup up the CompilerInvocation this late in the release branch's life. |
Abandoned llvm#101778 in favour of this
This patch changes the way we initialize
BuiltinHeadersInSystemModuleswhich 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-moduleswhen evaluating expressions with thetarget.import-std-modulesetting. 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
ClangExpressionParsernever consults the Clang driver, and thus doesn't correctly inferBuiltinHeadersInSystemModules. Note, this isn't an issue with theCompilerInstancethat theClangModulesDeclVendorcreates because it uses thecreateInovcationAPI, which calls intoDarwin::addClangTargetOptions.This patch mimicks how
sdkSupportsBuiltinModulesis used inDarwin::addClangTargetOptions.This ensures that the
import-std-moduleAPI tests run cleanly regardless of SDK version.The plan is to eventually make the
CompilerInstanceconstruction inClangExpressionParsergo through the driver, so we can avoid duplicating the logic in LLDB. But we aren't there yet.Implementation
SDKSettings.jsonin the sysroot directory that we found in DWARF (viaDW_AT_LLVM_sysroot)sdkSupportsBuiltinModulesfromToolchains/Darwin.cppand setBuiltinHeadersInSystemModulesbased on itrdar://116490281