[Lldb-commits] [lldb] 6fcb857 - [lldb][import-std-module] Prefer the non-module diagnostics when in fallback mode
Raphael Isemann via lldb-commits
lldb-commits at lists.llvm.org
Mon Oct 4 10:16:23 PDT 2021
Author: Raphael Isemann
Date: 2021-10-04T19:16:03+02:00
New Revision: 6fcb857746c19b5ed46afdf732b839082326f9d4
URL: https://github.com/llvm/llvm-project/commit/6fcb857746c19b5ed46afdf732b839082326f9d4
DIFF: https://github.com/llvm/llvm-project/commit/6fcb857746c19b5ed46afdf732b839082326f9d4.diff
LOG: [lldb][import-std-module] Prefer the non-module diagnostics when in fallback mode
The `fallback` setting for import-std-module is supposed to allow running
expression that require an imported C++ module without causing any regressions
for users (neither in terms of functionality nor performance). This is done by
first trying to normally parse/evaluate an expression and when an error occurred
during this first attempt, we retry with the loaded 'std' module.
When we run into a system with a 'std' module that for some reason doesn't build
or otherwise causes parse errors, then this currently means that the second
parse attempt will overwrite the error diagnostics of the first parse attempt.
Given that the module build errors are outside of the scope of what the user can
influence, it makes more sense to show the errors from the first parse attempt
that are only concerned with the actual user input.
Reviewed By: aprantl
Differential Revision: https://reviews.llvm.org/D110696
Added:
lldb/test/API/commands/expression/import-std-module/module-build-errors/Makefile
lldb/test/API/commands/expression/import-std-module/module-build-errors/TestStdModuleBuildErrors.py
lldb/test/API/commands/expression/import-std-module/module-build-errors/main.cpp
lldb/test/API/commands/expression/import-std-module/module-build-errors/root/usr/include/c++/v1/algorithm
lldb/test/API/commands/expression/import-std-module/module-build-errors/root/usr/include/c++/v1/module.modulemap
lldb/test/API/commands/expression/import-std-module/module-build-errors/root/usr/include/c++/v1/vector
lldb/test/API/commands/expression/import-std-module/module-build-errors/root/usr/include/stdio.h
Modified:
lldb/source/Plugins/ExpressionParser/Clang/ClangUserExpression.cpp
lldb/test/API/commands/expression/import-std-module/retry-with-std-module/TestRetryWithStdModule.py
Removed:
################################################################################
diff --git a/lldb/source/Plugins/ExpressionParser/Clang/ClangUserExpression.cpp b/lldb/source/Plugins/ExpressionParser/Clang/ClangUserExpression.cpp
index 41e86a1f897e9..38166391006a0 100644
--- a/lldb/source/Plugins/ExpressionParser/Clang/ClangUserExpression.cpp
+++ b/lldb/source/Plugins/ExpressionParser/Clang/ClangUserExpression.cpp
@@ -685,15 +685,22 @@ bool ClangUserExpression::Parse(DiagnosticManager &diagnostic_manager,
SetupCppModuleImports(exe_ctx);
// If we did load any modules, then retry parsing.
if (!m_imported_cpp_modules.empty()) {
+ // Create a dedicated diagnostic manager for the second parse attempt.
+ // These diagnostics are only returned to the caller if using the fallback
+ // actually succeeded in getting the expression to parse. This prevents
+ // that module-specific issues regress diagnostic quality with the
+ // fallback mode.
+ DiagnosticManager retry_manager;
// The module imports are injected into the source code wrapper,
// so recreate those.
- CreateSourceCode(diagnostic_manager, exe_ctx, m_imported_cpp_modules,
+ CreateSourceCode(retry_manager, exe_ctx, m_imported_cpp_modules,
/*for_completion*/ false);
- // Clear the error diagnostics from the previous parse attempt.
- diagnostic_manager.Clear();
- parse_success = TryParse(diagnostic_manager, exe_scope, exe_ctx,
+ parse_success = TryParse(retry_manager, exe_scope, exe_ctx,
execution_policy, keep_result_in_memory,
generate_debug_info);
+ // Return the parse diagnostics if we were successful.
+ if (parse_success)
+ diagnostic_manager = std::move(retry_manager);
}
}
if (!parse_success)
diff --git a/lldb/test/API/commands/expression/import-std-module/module-build-errors/Makefile b/lldb/test/API/commands/expression/import-std-module/module-build-errors/Makefile
new file mode 100644
index 0000000000000..617045d760cc4
--- /dev/null
+++ b/lldb/test/API/commands/expression/import-std-module/module-build-errors/Makefile
@@ -0,0 +1,9 @@
+# We don't have any standard include directories, so we can't
+# parse the test_common.h header we usually inject as it includes
+# system headers.
+NO_TEST_COMMON_H := 1
+
+CXXFLAGS_EXTRAS = -I $(SRCDIR)/root/usr/include/c++/v1/ -I $(SRCDIR)/root/usr/include/ -nostdinc -nostdinc++ -DBUILDING_OUTSIDE_LLDB=1
+CXX_SOURCES := main.cpp
+
+include Makefile.rules
diff --git a/lldb/test/API/commands/expression/import-std-module/module-build-errors/TestStdModuleBuildErrors.py b/lldb/test/API/commands/expression/import-std-module/module-build-errors/TestStdModuleBuildErrors.py
new file mode 100644
index 0000000000000..55e6d420b64b7
--- /dev/null
+++ b/lldb/test/API/commands/expression/import-std-module/module-build-errors/TestStdModuleBuildErrors.py
@@ -0,0 +1,61 @@
+"""
+Tests that the import-std-module=fallback setting is only showing the error
+diagnostics from the first parse attempt which isn't using a module.
+This is supposed to prevent that a broken libc++ module renders failing
+expressions useless as the original failing errors are suppressed by the
+module build errors.
+"""
+
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+import os
+
+
+class TestCase(TestBase):
+
+ mydir = TestBase.compute_mydir(__file__)
+
+ # We only emulate a fake libc++ in this test and don't use the real libc++,
+ # but we still add the libc++ category so that this test is only run in
+ # test configurations where libc++ is actually supposed to be tested.
+ @add_test_categories(["libc++"])
+ @skipIfRemote
+ @skipIf(compiler=no_match("clang"))
+ def test(self):
+ self.build()
+
+ sysroot = os.path.join(os.getcwd(), "root")
+
+ # Set the sysroot this test is using to provide a custom libc++.
+ self.runCmd("platform select --sysroot '" + sysroot + "' host",
+ CURRENT_EXECUTABLE_SET)
+
+ lldbutil.run_to_source_breakpoint(self,
+ "// Set break point at this line.",
+ lldb.SBFileSpec("main.cpp"))
+
+ # The expected error message when the fake libc++ module in this test
+ # fails to build from within LLDB (as it contains invalid code).
+ module_build_error_msg = "unknown type name 'random_token_to_fail_the_build'"
+
+ # First force the std module to be imported. This should show the
+ # module build error to the user.
+ self.runCmd("settings set target.import-std-module true")
+ self.expect("expr (size_t)v.size()",
+ substrs=[module_build_error_msg],
+ error=True)
+
+ # In the fallback mode the module build error should not be shown.
+ self.runCmd("settings set target.import-std-module fallback")
+ fallback_expr = "expr v ; error_to_trigger_fallback_mode"
+ # First check for the actual expression error that should be displayed
+ # and is useful for the user.
+ self.expect(fallback_expr,
+ substrs=["use of undeclared identifier 'error_to_trigger_fallback_mode'"],
+ error=True)
+ # Test that the module build error is not displayed.
+ self.expect(fallback_expr,
+ substrs=[module_build_error_msg],
+ matching=False,
+ error=True)
diff --git a/lldb/test/API/commands/expression/import-std-module/module-build-errors/main.cpp b/lldb/test/API/commands/expression/import-std-module/module-build-errors/main.cpp
new file mode 100644
index 0000000000000..b01fe1a785361
--- /dev/null
+++ b/lldb/test/API/commands/expression/import-std-module/module-build-errors/main.cpp
@@ -0,0 +1,8 @@
+#include <algorithm>
+
+int main(int argc, char **argv) {
+ // Makes sure we have the mock libc headers in the debug information.
+ libc_struct s;
+ std::vector<int> v;
+ return 0; // Set break point at this line.
+}
diff --git a/lldb/test/API/commands/expression/import-std-module/module-build-errors/root/usr/include/c++/v1/algorithm b/lldb/test/API/commands/expression/import-std-module/module-build-errors/root/usr/include/c++/v1/algorithm
new file mode 100644
index 0000000000000..af8738f075b30
--- /dev/null
+++ b/lldb/test/API/commands/expression/import-std-module/module-build-errors/root/usr/include/c++/v1/algorithm
@@ -0,0 +1,18 @@
+// This is only defined when building, but LLDB is missing this flag when loading the standard
+// library module so the actual contents of the module are missing.
+#ifdef BUILDING_OUTSIDE_LLDB
+
+#include "stdio.h"
+
+namespace std {
+ inline namespace __1 {
+ // Pretend to be a std::vector template we need to instantiate
+ // in LLDB.
+ template<typename T>
+ struct vector { T i; int size() { return 2; } };
+ }
+}
+#else
+// Break the build when parsing from within LLDB.
+random_token_to_fail_the_build
+#endif // BUILDING_OUTSIDE_LLDB
diff --git a/lldb/test/API/commands/expression/import-std-module/module-build-errors/root/usr/include/c++/v1/module.modulemap b/lldb/test/API/commands/expression/import-std-module/module-build-errors/root/usr/include/c++/v1/module.modulemap
new file mode 100644
index 0000000000000..0eb48492a65d9
--- /dev/null
+++ b/lldb/test/API/commands/expression/import-std-module/module-build-errors/root/usr/include/c++/v1/module.modulemap
@@ -0,0 +1,3 @@
+module std {
+ module "algorithm" { header "algorithm" export * }
+}
diff --git a/lldb/test/API/commands/expression/import-std-module/module-build-errors/root/usr/include/c++/v1/vector b/lldb/test/API/commands/expression/import-std-module/module-build-errors/root/usr/include/c++/v1/vector
new file mode 100644
index 0000000000000..e69de29bb2d1d
diff --git a/lldb/test/API/commands/expression/import-std-module/module-build-errors/root/usr/include/stdio.h b/lldb/test/API/commands/expression/import-std-module/module-build-errors/root/usr/include/stdio.h
new file mode 100644
index 0000000000000..47525c9db3467
--- /dev/null
+++ b/lldb/test/API/commands/expression/import-std-module/module-build-errors/root/usr/include/stdio.h
@@ -0,0 +1 @@
+struct libc_struct {};
diff --git a/lldb/test/API/commands/expression/import-std-module/retry-with-std-module/TestRetryWithStdModule.py b/lldb/test/API/commands/expression/import-std-module/retry-with-std-module/TestRetryWithStdModule.py
index b84ab84e4b480..1ba80ca1d011b 100644
--- a/lldb/test/API/commands/expression/import-std-module/retry-with-std-module/TestRetryWithStdModule.py
+++ b/lldb/test/API/commands/expression/import-std-module/retry-with-std-module/TestRetryWithStdModule.py
@@ -48,16 +48,6 @@ def test(self):
self.expect("expr --top-level -- int i = std::max(1, 2);", error=True,
substrs=["no member named 'max' in namespace 'std'"])
- # Check that diagnostics from the first parse attempt don't show up
- # in the C++ module parse attempt. In the expression below, we first
- # fail to parse 'std::max'. Then we retry with a loaded C++ module
- # and succeed to parse the 'std::max' part. However, the
- # trailing 'unknown_identifier' will fail to parse even with the
- # loaded module. The 'std::max' diagnostic from the first attempt
- # however should not be shown to the user.
- self.expect("expr std::max(1, 2); unknown_identifier", error=True,
- matching=False,
- substrs=["no member named 'max'"])
# The proper diagnostic however should be shown on the retry.
self.expect("expr std::max(1, 2); unknown_identifier", error=True,
substrs=["use of undeclared identifier 'unknown_identifier'"])
More information about the lldb-commits
mailing list