From 6fcb857746c19b5ed46afdf732b839082326f9d4 Mon Sep 17 00:00:00 2001 From: Raphael Isemann Date: Mon, 4 Oct 2021 18:55:22 +0200 Subject: [PATCH] [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 --- .../Clang/ClangUserExpression.cpp | 15 +++-- .../module-build-errors/Makefile | 9 +++ .../TestStdModuleBuildErrors.py | 61 +++++++++++++++++++ .../module-build-errors/main.cpp | 8 +++ .../root/usr/include/c++/v1/algorithm | 18 ++++++ .../root/usr/include/c++/v1/module.modulemap | 3 + .../root/usr/include/c++/v1/vector | 0 .../root/usr/include/stdio.h | 1 + .../TestRetryWithStdModule.py | 10 --- 9 files changed, 111 insertions(+), 14 deletions(-) create mode 100644 lldb/test/API/commands/expression/import-std-module/module-build-errors/Makefile create mode 100644 lldb/test/API/commands/expression/import-std-module/module-build-errors/TestStdModuleBuildErrors.py create mode 100644 lldb/test/API/commands/expression/import-std-module/module-build-errors/main.cpp create mode 100644 lldb/test/API/commands/expression/import-std-module/module-build-errors/root/usr/include/c++/v1/algorithm create mode 100644 lldb/test/API/commands/expression/import-std-module/module-build-errors/root/usr/include/c++/v1/module.modulemap create mode 100644 lldb/test/API/commands/expression/import-std-module/module-build-errors/root/usr/include/c++/v1/vector create mode 100644 lldb/test/API/commands/expression/import-std-module/module-build-errors/root/usr/include/stdio.h diff --git a/lldb/source/Plugins/ExpressionParser/Clang/ClangUserExpression.cpp b/lldb/source/Plugins/ExpressionParser/Clang/ClangUserExpression.cpp index 41e86a1f897e..38166391006a 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 000000000000..617045d760cc --- /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 000000000000..55e6d420b64b --- /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 000000000000..b01fe1a78536 --- /dev/null +++ b/lldb/test/API/commands/expression/import-std-module/module-build-errors/main.cpp @@ -0,0 +1,8 @@ +#include + +int main(int argc, char **argv) { + // Makes sure we have the mock libc headers in the debug information. + libc_struct s; + std::vector 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 000000000000..af8738f075b3 --- /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 + 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 000000000000..0eb48492a65d --- /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 000000000000..e69de29bb2d1 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 000000000000..47525c9db346 --- /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 b84ab84e4b48..1ba80ca1d011 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 @@ class TestCase(TestBase): 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'"])