[lldb][import-std-module] Prefer the non-module diagnostics when in fallback mode

[teemperor]

Summary

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.

[aprantl]

Nifty testcase!

lldb/source/Plugins/ExpressionParser/Clang/ClangUserExpression.cpp

bool parse_success = TryParse(diagnostic_manager, exe_scope, exe_ctx,
                                 generate_debug_info);
   // If the expression failed to parse, check if retrying parsing with a loaded
   // C++ module is possible.
   if (!parse_success && shouldRetryWithCppModule(*target, execution_policy)) {
     // Load the loaded C++ modules.
     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)
     return false;
 
   if (exe_ctx.GetProcessPtr() && execution_policy == eExecutionPolicyTopLevel) {
     Status static_init_error =
         m_parser->RunStaticInitializers(m_execution_unit_sp, exe_ctx);

lldb/test/API/commands/expression/import-std-module/module-build-errors/Makefile

+# 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

lldb/test/API/commands/expression/import-std-module/module-build-errors/TestStdModuleBuildErrors.py

+"""
+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)

lldb/test/API/commands/expression/import-std-module/module-build-errors/main.cpp

+#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.
+}

lldb/test/API/commands/expression/import-std-module/module-build-errors/root/usr/include/c++/v1/algorithm

+// 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

lldb/test/API/commands/expression/import-std-module/module-build-errors/root/usr/include/c++/v1/module.modulemap

+module std {
+  module "algorithm" { header "algorithm" export * }
+}

lldb/test/API/commands/expression/import-std-module/module-build-errors/root/usr/include/stdio.h

+struct libc_struct {};

lldb/test/API/commands/expression/import-std-module/retry-with-std-module/TestRetryWithStdModule.py

def test(self):
 
 
         # Try to declare top-level declarations that require a C++ module to parse.
         # Top-level expressions don't support importing the C++ module (yet), so
         # this should still fail as before.
         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'"])
 
         # Turn on the 'import-std-module' setting and make sure we import the
         # C++ module.
         self.runCmd("settings set target.import-std-module true")
         # This is still expected to work.
         self.expect_expr("std::max<std::size_t>(0U, a.size())", result_value="3")
 
         # Turn of the 'import-std-module' setting and make sure we don't load
         # the module (which should prevent parsing the expression involving
         # 'std::max').
         self.runCmd("settings set target.import-std-module false")
         self.expect("expr std::max(1, 2);", error=True,
                     substrs=["no member named 'max' in namespace 'std'"])