[lldb][Modules] Fix error handling of parseAndLoadModuleMapFile #141220
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
`parseAndLoadModuleMapFile` returns `true` on error. This seems to have always been an issue? This is now preventing me from fixing a different modules related issue. So this patch checks the return value correctly.
|
@llvm/pr-subscribers-lldb Author: Michael Buch (Michael137) Changes
Full diff: https://github.com/llvm/llvm-project/pull/141220.diff 1 Files Affected:
diff --git a/lldb/source/Plugins/ExpressionParser/Clang/ClangModulesDeclVendor.cpp b/lldb/source/Plugins/ExpressionParser/Clang/ClangModulesDeclVendor.cpp
index eb62cb618f254..c99ed9dd0a68d 100644
--- a/lldb/source/Plugins/ExpressionParser/Clang/ClangModulesDeclVendor.cpp
+++ b/lldb/source/Plugins/ExpressionParser/Clang/ClangModulesDeclVendor.cpp
@@ -330,7 +330,7 @@ bool ClangModulesDeclVendorImpl::AddModule(const SourceModule &module,
auto file = HS.lookupModuleMapFile(*dir, is_framework);
if (!file)
return error();
- if (!HS.parseAndLoadModuleMapFile(*file, is_system))
+ if (HS.parseAndLoadModuleMapFile(*file, is_system))
return error();
}
}
|
JDevlieghere
approved these changes
May 23, 2025
Michael137
added a commit
that referenced
this pull request
May 29, 2025
…le (#141220)" This reverts commit 57f3151. LLDB was hitting an assert when compiling the `std` module. The `std` module was being pulled in because we use `#import <cstdio>` in the test to set a breakpoint on `puts`. That's redundant and to work around the crash we just remove that include. The underlying issue of compiling the `std` module still exists and I'll investigate that separately. The reason it started failing after the `ClangModulesDeclVendor` patch is that we would previously just fail to load the modulemap (and thus not load any of the modules). Now we do load the modulemap (and modules) when we prepare for parsing the expression.
svkeerthy
pushed a commit
that referenced
this pull request
May 29, 2025
…le (#141220)" This reverts commit 57f3151. LLDB was hitting an assert when compiling the `std` module. The `std` module was being pulled in because we use `#import <cstdio>` in the test to set a breakpoint on `puts`. That's redundant and to work around the crash we just remove that include. The underlying issue of compiling the `std` module still exists and I'll investigate that separately. The reason it started failing after the `ClangModulesDeclVendor` patch is that we would previously just fail to load the modulemap (and thus not load any of the modules). Now we do load the modulemap (and modules) when we prepare for parsing the expression.
Michael137
added a commit
to Michael137/llvm-project
that referenced
this pull request
May 29, 2025
…path for submodules Inferred submodule declarations are emitted in DWARF as `DW_TAG_module`s without `DW_AT_LLVM_include_path`s. Instead the parent DIE will have the include path. This patch adds support for such setups. Without this, the `ClangModulesDeclVendor` would fail to `AddModule` the submodules (i.e., compile and load the submodules). This would cause errors such as: ``` note: error: Header search couldn't locate module 'TopLevel' ``` The test added here also tests llvm#141220. Unfortunately the embedded clang instance doesn't allow us to use the decls we find in the modules. But we'll try to fix that in a separate patch.
google-yfyang
pushed a commit
to google-yfyang/llvm-project
that referenced
this pull request
May 29, 2025
…le (llvm#141220)" The TestTemplateWithSameArg.py API test is crashing on macOS with this change. Michael is going to look into it; reverting for now to unblock the CI bots. This reverts commit 052c704.
google-yfyang
pushed a commit
to google-yfyang/llvm-project
that referenced
this pull request
May 29, 2025
…le (llvm#141220)" This reverts commit 57f3151. LLDB was hitting an assert when compiling the `std` module. The `std` module was being pulled in because we use `#import <cstdio>` in the test to set a breakpoint on `puts`. That's redundant and to work around the crash we just remove that include. The underlying issue of compiling the `std` module still exists and I'll investigate that separately. The reason it started failing after the `ClangModulesDeclVendor` patch is that we would previously just fail to load the modulemap (and thus not load any of the modules). Now we do load the modulemap (and modules) when we prepare for parsing the expression.
Michael137
added a commit
that referenced
this pull request
May 30, 2025
…path for submodules (#142044) Inferred submodule declarations are emitted in DWARF as `DW_TAG_module`s without `DW_AT_LLVM_include_path`s. Instead the parent DIE will have the include path. This patch adds support for such setups. Without this, the `ClangModulesDeclVendor` would fail to `AddModule` the submodules (i.e., compile and load the submodules). This would cause errors such as: ``` note: error: Header search couldn't locate module 'TopLevel' ``` The test added here also tests #141220. Without that patch we'd fail with: ``` note: error: No module map file in /Users/jonas/Git/llvm-worktrees/llvm-project/lldb/test/API/lang/cpp/decl-from-submodule ``` Unfortunately the embedded clang instance doesn't allow us to use the decls we find in the modules. But we'll try to fix that in a separate patch. rdar://151022173
llvm-sync bot
pushed a commit
to arm/arm-toolchain
that referenced
this pull request
May 30, 2025
…VM_include_path for submodules (#142044) Inferred submodule declarations are emitted in DWARF as `DW_TAG_module`s without `DW_AT_LLVM_include_path`s. Instead the parent DIE will have the include path. This patch adds support for such setups. Without this, the `ClangModulesDeclVendor` would fail to `AddModule` the submodules (i.e., compile and load the submodules). This would cause errors such as: ``` note: error: Header search couldn't locate module 'TopLevel' ``` The test added here also tests llvm/llvm-project#141220. Without that patch we'd fail with: ``` note: error: No module map file in /Users/jonas/Git/llvm-worktrees/llvm-project/lldb/test/API/lang/cpp/decl-from-submodule ``` Unfortunately the embedded clang instance doesn't allow us to use the decls we find in the modules. But we'll try to fix that in a separate patch. rdar://151022173
Michael137
added a commit
to swiftlang/llvm-project
that referenced
this pull request
May 30, 2025
…path for submodules (llvm#142044) Inferred submodule declarations are emitted in DWARF as `DW_TAG_module`s without `DW_AT_LLVM_include_path`s. Instead the parent DIE will have the include path. This patch adds support for such setups. Without this, the `ClangModulesDeclVendor` would fail to `AddModule` the submodules (i.e., compile and load the submodules). This would cause errors such as: ``` note: error: Header search couldn't locate module 'TopLevel' ``` The test added here also tests llvm#141220. Without that patch we'd fail with: ``` note: error: No module map file in /Users/jonas/Git/llvm-worktrees/llvm-project/lldb/test/API/lang/cpp/decl-from-submodule ``` Unfortunately the embedded clang instance doesn't allow us to use the decls we find in the modules. But we'll try to fix that in a separate patch. rdar://151022173 (cherry picked from commit 7b96272)
adrian-prantl
pushed a commit
to swiftlang/llvm-project
that referenced
this pull request
May 30, 2025
…_path for submodules (#10764) * [lldb][SymbolFileDWARF] Fall back to using parent DW_AT_LLVM_include_path for submodules (llvm#142044) Inferred submodule declarations are emitted in DWARF as `DW_TAG_module`s without `DW_AT_LLVM_include_path`s. Instead the parent DIE will have the include path. This patch adds support for such setups. Without this, the `ClangModulesDeclVendor` would fail to `AddModule` the submodules (i.e., compile and load the submodules). This would cause errors such as: ``` note: error: Header search couldn't locate module 'TopLevel' ``` The test added here also tests llvm#141220. Without that patch we'd fail with: ``` note: error: No module map file in /Users/jonas/Git/llvm-worktrees/llvm-project/lldb/test/API/lang/cpp/decl-from-submodule ``` Unfortunately the embedded clang instance doesn't allow us to use the decls we find in the modules. But we'll try to fix that in a separate patch. rdar://151022173 (cherry picked from commit 7b96272) * [lldb][test] Disable DeclFromSubmodule test on Windows Or more precisely, when linking with link.exe, but Windows is our closest proxy for that right now. This test requires the DWARF info to work, on the Windows on Arm buildbot it fails: ``` AssertionError: Ran command: "expr func(1, 2)" Got output: error: <user expression 0>:1:1: use of undeclared identifier 'func' 1 | func(1, 2) | ^~~~ Expecting start string: "error: <user expression 0>:1:1: 'func' has unknown return type" (was not found) ``` (cherry picked from commit 7a66b28) --------- Co-authored-by: David Spickett <david.spickett@linaro.org>
sivan-shani
pushed a commit
to sivan-shani/llvm-project
that referenced
this pull request
Jun 3, 2025
…#141220) `parseAndLoadModuleMapFile` returns `true` on error. This seems to have always been an issue? This is now preventing me from fixing a different modules related issue. So this patch checks the return value correctly.
sivan-shani
pushed a commit
to sivan-shani/llvm-project
that referenced
this pull request
Jun 3, 2025
…le (llvm#141220)" The TestTemplateWithSameArg.py API test is crashing on macOS with this change. Michael is going to look into it; reverting for now to unblock the CI bots. This reverts commit 052c704.
sivan-shani
pushed a commit
to sivan-shani/llvm-project
that referenced
this pull request
Jun 3, 2025
…le (llvm#141220)" This reverts commit 57f3151. LLDB was hitting an assert when compiling the `std` module. The `std` module was being pulled in because we use `#import <cstdio>` in the test to set a breakpoint on `puts`. That's redundant and to work around the crash we just remove that include. The underlying issue of compiling the `std` module still exists and I'll investigate that separately. The reason it started failing after the `ClangModulesDeclVendor` patch is that we would previously just fail to load the modulemap (and thus not load any of the modules). Now we do load the modulemap (and modules) when we prepare for parsing the expression.
sivan-shani
pushed a commit
to sivan-shani/llvm-project
that referenced
this pull request
Jun 3, 2025
…path for submodules (llvm#142044) Inferred submodule declarations are emitted in DWARF as `DW_TAG_module`s without `DW_AT_LLVM_include_path`s. Instead the parent DIE will have the include path. This patch adds support for such setups. Without this, the `ClangModulesDeclVendor` would fail to `AddModule` the submodules (i.e., compile and load the submodules). This would cause errors such as: ``` note: error: Header search couldn't locate module 'TopLevel' ``` The test added here also tests llvm#141220. Without that patch we'd fail with: ``` note: error: No module map file in /Users/jonas/Git/llvm-worktrees/llvm-project/lldb/test/API/lang/cpp/decl-from-submodule ``` Unfortunately the embedded clang instance doesn't allow us to use the decls we find in the modules. But we'll try to fix that in a separate patch. rdar://151022173
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
parseAndLoadModuleMapFilereturnstrueon error. This seems to have always been an issue? This is now preventing me from fixing a different modules related issue. So this patch checks the return value correctly.