[Lldb-commits] [lldb] 32c8da4 - [lldb] Don't infinite loop in SemaSourceWithPriorities::CompleteType when trying to complete a forward decl

[Raphael Isemann via lldb-commits]

Author: Raphael Isemann
Date: 2020-09-09T10:05:57+02:00
New Revision: 32c8da41dc0cb99651823a1a21130c2cbdf688e1

URL: https://github.com/llvm/llvm-project/commit/32c8da41dc0cb99651823a1a21130c2cbdf688e1
DIFF: https://github.com/llvm/llvm-project/commit/32c8da41dc0cb99651823a1a21130c2cbdf688e1.diff

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

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.

Reviewed By: shafik

Differential Revision: https://reviews.llvm.org/D87289

Added: 
    lldb/test/API/commands/expression/import-std-module/forward_decl_from_module/Makefile
    lldb/test/API/commands/expression/import-std-module/forward_decl_from_module/TestForwardDeclFromStdModule.py
    lldb/test/API/commands/expression/import-std-module/forward_decl_from_module/main.cpp
    lldb/test/API/commands/expression/import-std-module/forward_decl_from_module/root/usr/include/c++/v1/module.modulemap
    lldb/test/API/commands/expression/import-std-module/forward_decl_from_module/root/usr/include/c++/v1/vector
    lldb/test/API/commands/expression/import-std-module/forward_decl_from_module/root/usr/include/libc_header.h

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

Removed: 
    


################################################################################
diff  --git a/lldb/source/Plugins/ExpressionParser/Clang/ASTUtils.h b/lldb/source/Plugins/ExpressionParser/Clang/ASTUtils.h
index 769b18d54ced..b70ec223df4d 100644
--- a/lldb/source/Plugins/ExpressionParser/Clang/ASTUtils.h
+++ b/lldb/source/Plugins/ExpressionParser/Clang/ASTUtils.h
@@ -359,15 +359,12 @@ class SemaSourceWithPriorities : public clang::ExternalSemaSource {
   }
 
   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.
+      if (Tag->isCompleteDefinition())
+        break;
+    }
   }
 
   void CompleteType(clang::ObjCInterfaceDecl *Class) override {

diff  --git a/lldb/test/API/commands/expression/import-std-module/forward_decl_from_module/Makefile b/lldb/test/API/commands/expression/import-std-module/forward_decl_from_module/Makefile
new file mode 100644
index 000000000000..4915cdae8764
--- /dev/null
+++ b/lldb/test/API/commands/expression/import-std-module/forward_decl_from_module/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++
+CXX_SOURCES := main.cpp
+
+include Makefile.rules

diff  --git a/lldb/test/API/commands/expression/import-std-module/forward_decl_from_module/TestForwardDeclFromStdModule.py b/lldb/test/API/commands/expression/import-std-module/forward_decl_from_module/TestForwardDeclFromStdModule.py
new file mode 100644
index 000000000000..48459abb9266
--- /dev/null
+++ b/lldb/test/API/commands/expression/import-std-module/forward_decl_from_module/TestForwardDeclFromStdModule.py
@@ -0,0 +1,39 @@
+"""
+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', '}'])

diff  --git a/lldb/test/API/commands/expression/import-std-module/forward_decl_from_module/main.cpp b/lldb/test/API/commands/expression/import-std-module/forward_decl_from_module/main.cpp
new file mode 100644
index 000000000000..a0b02d5c6814
--- /dev/null
+++ b/lldb/test/API/commands/expression/import-std-module/forward_decl_from_module/main.cpp
@@ -0,0 +1,8 @@
+#include <vector>
+
+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.
+}

diff  --git a/lldb/test/API/commands/expression/import-std-module/forward_decl_from_module/root/usr/include/c++/v1/module.modulemap b/lldb/test/API/commands/expression/import-std-module/forward_decl_from_module/root/usr/include/c++/v1/module.modulemap
new file mode 100644
index 000000000000..f149be7b7d21
--- /dev/null
+++ b/lldb/test/API/commands/expression/import-std-module/forward_decl_from_module/root/usr/include/c++/v1/module.modulemap
@@ -0,0 +1,3 @@
+module std {
+  module "vector" { header "vector" export * }
+}

diff  --git a/lldb/test/API/commands/expression/import-std-module/forward_decl_from_module/root/usr/include/c++/v1/vector b/lldb/test/API/commands/expression/import-std-module/forward_decl_from_module/root/usr/include/c++/v1/vector
new file mode 100644
index 000000000000..c2d77aab0711
--- /dev/null
+++ b/lldb/test/API/commands/expression/import-std-module/forward_decl_from_module/root/usr/include/c++/v1/vector
@@ -0,0 +1,14 @@
+#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; };
+  }
+}

diff  --git a/lldb/test/API/commands/expression/import-std-module/forward_decl_from_module/root/usr/include/libc_header.h b/lldb/test/API/commands/expression/import-std-module/forward_decl_from_module/root/usr/include/libc_header.h
new file mode 100644
index 000000000000..47525c9db346
--- /dev/null
+++ b/lldb/test/API/commands/expression/import-std-module/forward_decl_from_module/root/usr/include/libc_header.h
@@ -0,0 +1 @@
+struct libc_struct {};