From 32c8da41dc0cb99651823a1a21130c2cbdf688e1 Mon Sep 17 00:00:00 2001 From: Raphael Isemann Date: Wed, 9 Sep 2020 09:54:47 +0200 Subject: [PATCH] [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 --- .../Plugins/ExpressionParser/Clang/ASTUtils.h | 15 +++---- .../forward_decl_from_module/Makefile | 9 +++++ .../TestForwardDeclFromStdModule.py | 39 +++++++++++++++++++ .../forward_decl_from_module/main.cpp | 8 ++++ .../root/usr/include/c++/v1/module.modulemap | 3 ++ .../root/usr/include/c++/v1/vector | 14 +++++++ .../root/usr/include/libc_header.h | 1 + 7 files changed, 80 insertions(+), 9 deletions(-) create mode 100644 lldb/test/API/commands/expression/import-std-module/forward_decl_from_module/Makefile create mode 100644 lldb/test/API/commands/expression/import-std-module/forward_decl_from_module/TestForwardDeclFromStdModule.py create mode 100644 lldb/test/API/commands/expression/import-std-module/forward_decl_from_module/main.cpp create mode 100644 lldb/test/API/commands/expression/import-std-module/forward_decl_from_module/root/usr/include/c++/v1/module.modulemap create mode 100644 lldb/test/API/commands/expression/import-std-module/forward_decl_from_module/root/usr/include/c++/v1/vector create mode 100644 lldb/test/API/commands/expression/import-std-module/forward_decl_from_module/root/usr/include/libc_header.h 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 @@ public: } 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) $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 + +int main(int argc, char **argv) { + // Makes sure we have the mock libc headers in the debug information. + libc_struct s; + std::vector 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 class vector; + // Pretend to be a std::vector template we need to instantiate in LLDB + // when import-std-module is enabled. + template + struct vector { class F; F *f; }; + // The definition of our forward declared nested class. + template class vector::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 {};