[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

Author: Teemperor

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)

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

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)

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

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

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 * }
+}

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

@@ -0,0 +1 @@
+struct libc_struct {};

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

@@ -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'"])