[lldb] Don't infinite loop in SemaSourceWithPriorities::CompleteType when trying to complete a forward decl

[teemperor]

Summary

SemaSourceWithPriorities is a special SemaSource that wraps our normal LLDB ExternalASTSource and
the ASTReader (which is used for the C++ module loading). It's only active when the import-std-module
setting is turned on.

The CompleteType function there in SemaSourceWithPriorities is looping over all ExternalASTSources and
asks each to complete the type. However, that loop is in another loop that keeps doing that until the type is
complete. If that function is ever called on a type that is a forward decl then that causes LLDB to go into an
infinite loop.

I remember I added that second loop and the comment because I thought I saw a similar pattern in some other Clang
code, but after some grepping I can't find that code anywhere and it seems the rest of the code base only calls
CompleteType once (It would also be kinda silly to have calling it multiple times). So it seems that's just a silly mistake.

The is implicitly tested by importing std::pair, but I also added a simpler dedicated test that creates a dummy
libc++ module with some forward declarations and then imports them into the scratch AST context. At some point
the ASTImporter will check if one of the forward decls could be completed by the ExternalASTSource, which will cause the
SemaSourceWithPriorities to go into an infinite loop once it receives the CompleteType call.

[teemperor]

• Upload correct diff.

[shafik]

LGTM

lldb/source/Plugins/ExpressionParser/Clang/ASTUtils.h

public:
   void
   FindFileRegionDecls(clang::FileID File, unsigned Offset, unsigned Length,
                       llvm::SmallVectorImpl<clang::Decl *> &Decls) override {
     for (size_t i = 0; i < Sources.size(); ++i)
       Sources[i]->FindFileRegionDecls(File, Offset, Length, Decls);
   }
 
   void CompleteType(clang::TagDecl *Tag) override {
-    while (!Tag->isCompleteDefinition())
-      for (size_t i = 0; i < Sources.size(); ++i) {
-        // FIXME: We are technically supposed to loop here too until
-        // Tag->isCompleteDefinition() is true, but if our low quality source
-        // is failing to complete the tag this code will deadlock.
-        Sources[i]->CompleteType(Tag);
-        if (Tag->isCompleteDefinition())
-          break;
-      }
+    for (clang::ExternalSemaSource *S : Sources) {
+      S->CompleteType(Tag);
+      // Stop after the first source completed the type.
   }
 
   void CompleteType(clang::ObjCInterfaceDecl *Class) override {
     for (size_t i = 0; i < Sources.size(); ++i)
       Sources[i]->CompleteType(Class);
   }
 
   void ReadComments() override {

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

std::initializer_list<const char *> supported_names = {
       "stack",
       "vector",
       // pointers
       "shared_ptr",
       "unique_ptr",
       "weak_ptr",
       // utility
       "allocator",
+      "pair",
   };
   m_supported_templates.insert(supported_names.begin(), supported_names.end());
 }
 
 /// Builds a list of scopes that point into the given context.
 ///
 /// \param sema The sema that will be using the scopes.
 /// \param ctxt The context that the scope should look into.

lldb/test/API/commands/expression/import-std-module/forward_decl_from_module/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++
+CXX_SOURCES := main.cpp
+
+include Makefile.rules

lldb/test/API/commands/expression/import-std-module/forward_decl_from_module/TestForwardDeclFromStdModule.py

+"""
+Tests forward declarations coming from the `std` module.
+"""
+
+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 where our dummy libc++ exists.
+        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"))
+
+        self.runCmd("settings set target.import-std-module true")
+
+        # Print the dummy `std::vector`. It only has the dummy member in it
+        # so the standard `std::vector` formatter can't format it. Instead use
+        # the raw output so LLDB has to show the member variable.
+        # Both `std::vector` and the type of the member have forward
+        # declarations before their definitions.
+        self.expect("expr --raw -- v",
+                    substrs=['(std::__1::vector<int>) $0 = {', 'f = 0x', '}'])

lldb/test/API/commands/expression/import-std-module/forward_decl_from_module/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/forward_decl_from_module/root/usr/include/c++/v1/algorithm

+#include "libc_header.h"
+
+namespace std {
+  inline namespace __1 {
+    // A forward decl of `vector`.
+    template<typename T> class vector;
+    // Pretend to be a std::vector template we need to instantiate in LLDB
+    // when import-std-module is enabled.
+    template<typename T>
+    struct vector { class F; F *f; };
+    // The definition of our forward declared nested class.
+    template<typename T> class vector<T>::F { int x; };
+  }
+}

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

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

lldb/test/API/commands/expression/import-std-module/forward_decl_from_module/root/usr/include/libc_header.h

+struct libc_struct {};

lldb/test/API/commands/expression/import-std-module/pair/Makefile

+USE_LIBCPP := 1
+CXX_SOURCES := main.cpp
+include Makefile.rules

lldb/test/API/commands/expression/import-std-module/pair/TestPairFromStdModule.py

+"""
+Test basic std::pair functionality.
+"""
+
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+class TestCase(TestBase):
+
+    mydir = TestBase.compute_mydir(__file__)
+
+    @add_test_categories(["libc++"])
+    @skipIf(compiler=no_match("clang"))
+    def test(self):
+        self.build()
+
+        lldbutil.run_to_source_breakpoint(self,
+            "// Set break point at this line.", lldb.SBFileSpec("main.cpp"))
+
+        self.runCmd("settings set target.import-std-module true")
+
+        self.expect_expr("pair_int.first", result_type="int", result_value="1234")
+        self.expect_expr("pair_int.second", result_type="int", result_value="5678")
+        self.expect("expr pair_int", substrs=['first = 1234, second = 5678'])
 No newline at end of file

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

+#include <utility>
+
+int main(int argc, char **argv) {
+  std::pair<int, int> pair_int(1234, 5678);
+  return 0; // Set break point at this line.
+}