Merge pull request #3339 from Teemperor/cherry/6fcb857746c19b5ed46afdf732b839082326f9d4

[lldb][import-std-module] Prefer the non-module diagnostics when in f…

Author: Teemperor

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

@@ -689,15 +689,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'"])