PR23175 (unit test) - Infinite loop iterating Objective-C method declarations in categories when the AST was deserialized from an .ast file

[tahonermann]

Summary

This patch adds a unit test demonstrating an error in Objective-C method declaration enumeration that occurs when an AST is deserialized from an .ast file.

A few notes about the unit test:

1. The patch was produced based on r234313.
2. This adds a new unittests/Serialization directory and unit test to exercise AST matching on an AST that has been serialized and deserialized.
3. I'm not proficient with the AST matchers. There may be a better way to handle this. I added a declarationCountIs() AST matcher at one point, but I lacked convenient infrastructure like that available in unittests/ASTMatchers to validate the matches.
4. This adds an additional parameter with default argument to buildASTFromCode() and buildASTFromCodeWithArgs() that is used to indicate whether the code should be serialized and deserialized prior to returning the AST.

The test currently fails, but without going into an infinite loop (iteration is terminated if more than the expected number of declarations are enumerated). Passing 'true' for 'Reserialize' to buildASTFromCode() in the new test suffices to make the test pass, thus demonstrating that the issue is related to AST serialization and deserialization.

[tahonermann]

A fix for this issue has been proposed in D9127.

[rsmith]

unittests/Serialization/Reserialization.cpp

1

I'm not completely sure what "reserialization" means here; I'd expect this to be tests for serializing stuff we'd deserialized (which we generally avoid doing). Maybe something like "Serialization/Decls.cpp" would be a better name for tests of Decl deserialization?

[tahonermann]

unittests/Serialization/Reserialization.cpp

1

You are right, this could use a better name. I'm reaching for a term that describes "produce an AST, serialize it, deserialize it, then test it". I wasn't able to come up with anything better. Perhaps a term isn't necessary in which case 'Decls.cpp' would be just fine.

[klimek]

include/clang/Tooling/Tooling.h

173

I don't think this is the right choice.
It seems like we could provide two functions serialize and deserialize so that Reserialize=true becomes
deserialize(serialize(buildASTFromCode(...))
?

[tahonermann]

include/clang/Tooling/Tooling.h

173

Ah, yes. I agree that better encapsulates separation of concerns.

[klimek]

What's the state of this?

[tahonermann]

What's the state of this?

I'm not sure who that question is directed to. You haven't accepted the review, so I presume you are awaiting updates, probably from me. Unfortunately, my time is very limited.

include/clang/ASTMatchers/ASTMatchers.h

 /// Given
 /// \code
 ///   class X { friend void foo(); };
 /// \endcode
 /// friendDecl()
 ///   matches 'friend void foo()'.
 const internal::VariadicDynCastAllOfMatcher<Decl, FriendDecl> friendDecl;
 
+/// \brief Matches Objective-C method declarations.
+///
+/// Given
+/// \code
+///   @interface AClass
+///   - (void)AMethod;
+///   @end
+/// \endcode
+/// objCMethodDecl()
+///   matches '- (void)AMethod'.
+const internal::VariadicDynCastAllOfMatcher<
+  Decl,
+  ObjCMethodDecl> objCMethodDecl;
+
 /// \brief Matches statements.
 ///
 /// Given
 /// \code
 ///   { ++a; }
 /// \endcode
 /// stmt()
 ///   matches both the compound statement '{ ++a; }' and '++a'.

include/clang/Tooling/Tooling.h

 
 /// \brief Builds an AST for 'Code'.
 ///
 /// \param Code C++ code.
 /// \param FileName The file name which 'Code' will be mapped as.
 ///
 /// \return The resulting AST or null if an error occurred.
 std::unique_ptr<ASTUnit> buildASTFromCode(const Twine &Code,
-                                          const Twine &FileName = "input.cc");
+                                          const Twine &FileName = "input.cc",
+                                          bool Reserialize = false);
 
 /// \brief Builds an AST for 'Code' with additional flags.
 ///
 /// \param Code C++ code.
 /// \param Args Additional flags to pass on.
 /// \param FileName The file name which 'Code' will be mapped as.
 ///
 /// \return The resulting AST or null if an error occurred.
 std::unique_ptr<ASTUnit>
 buildASTFromCodeWithArgs(const Twine &Code,
                          const std::vector<std::string> &Args,
-                         const Twine &FileName = "input.cc");
+                         const Twine &FileName = "input.cc",
+                         bool Reserialize = false);
 
 /// \brief Utility to run a FrontendAction in a single clang invocation.
 class ToolInvocation {
  public:
   /// \brief Create a tool invocation.
   ///
   /// \param CommandLine The command line arguments to clang. Note that clang
   /// uses its binary name (CommandLine[0]) to locate its builtin headers.

lib/Tooling/Tooling.cpp

 #include "clang/Frontend/TextDiagnosticPrinter.h"
 #include "clang/Tooling/ArgumentsAdjusters.h"
 #include "clang/Tooling/CompilationDatabase.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/Config/llvm-config.h"
 #include "llvm/Option/Option.h"
 #include "llvm/Support/Debug.h"
 #include "llvm/Support/FileSystem.h"
+#include "llvm/Support/FileUtilities.h"
 #include "llvm/Support/Host.h"
 #include "llvm/Support/raw_ostream.h"
 
 // For chdir, see the comment in ClangTool::run for more information.
 #ifdef LLVM_ON_WIN32
 #  include <direct.h>
 #else
 #  include <unistd.h>

 }
 
 int ClangTool::buildASTs(std::vector<std::unique_ptr<ASTUnit>> &ASTs) {
   ASTBuilderAction Action(ASTs);
   return run(&Action);
 }
 
 std::unique_ptr<ASTUnit> buildASTFromCode(const Twine &Code,
-                                          const Twine &FileName) {
-  return buildASTFromCodeWithArgs(Code, std::vector<std::string>(), FileName);
+                                          const Twine &FileName,
+                                          bool Reserialize) {
+  return buildASTFromCodeWithArgs(Code, std::vector<std::string>(), FileName,
+                                  Reserialize);
 }
 
 std::unique_ptr<ASTUnit>
 buildASTFromCodeWithArgs(const Twine &Code,
                          const std::vector<std::string> &Args,
-                         const Twine &FileName) {
+                         const Twine &FileName,
+                         bool Reserialize) {
   SmallString<16> FileNameStorage;
   StringRef FileNameRef = FileName.toNullTerminatedStringRef(FileNameStorage);
 
   std::vector<std::unique_ptr<ASTUnit>> ASTs;
   ASTBuilderAction Action(ASTs);
   ToolInvocation Invocation(getSyntaxOnlyToolArgs(Args, FileNameRef), &Action,
                             nullptr);
 
   SmallString<1024> CodeStorage;
   Invocation.mapVirtualFile(FileNameRef,
                             Code.toNullTerminatedStringRef(CodeStorage));
   if (!Invocation.run())
     return nullptr;
 
   assert(ASTs.size() == 1);
+
+  if (Reserialize) {
+    SmallString<64> ASTFileName;
+    std::error_code EC = llvm::sys::fs::createTemporaryFile("input", "ast",
+                                                            ASTFileName);
+    if (EC) {
+      llvm::errs() << "createTemporaryFile failed: " << EC.message() << "\n";
+      return nullptr;
+    }
+    llvm::FileRemover ASTFileRemover(ASTFileName);
+
+    if (ASTs[0]->Save(ASTFileName)) {
+      llvm::errs() << "Save as AST file " << ASTFileName.str() << " failed\n";
+      return nullptr;
+    }
+
+    IntrusiveRefCntPtr<DiagnosticsEngine> Diags =
+        CompilerInstance::createDiagnostics(new DiagnosticOptions());
+    ASTs[0] = ASTUnit::LoadFromASTFile(ASTFileName.str(), Diags,
+                                       FileSystemOptions());
+    if (!ASTs[0]) {
+      llvm::errs() << "Load from AST file " << ASTFileName.str() << " failed\n";
+      return nullptr;
+    }
+  }
+
   return std::move(ASTs[0]);
 }
 
 } // end namespace tooling
 } // end namespace clang

unittests/CMakeLists.txt

 add_subdirectory(Format)
 add_subdirectory(Rewrite)
 add_subdirectory(Sema)
 add_subdirectory(CodeGen)
 # FIXME: Why are the libclang unit tests disabled on Windows?
 if(NOT WIN32) 
   add_subdirectory(libclang)
 endif()
+add_subdirectory(Serialization)

unittests/Makefile

 # If CLANG_LEVEL is not set, then we are the top-level Makefile. Otherwise, we
 # are being included from a subdirectory makefile.
 
 ifndef CLANG_LEVEL
 
 IS_UNITTEST_LEVEL := 1
 CLANG_LEVEL := ..
 PARALLEL_DIRS = CodeGen Basic Lex Driver Format ASTMatchers AST Tooling \
-          Rewrite Sema
+          Rewrite Sema Serialization
 
 include $(CLANG_LEVEL)/../..//Makefile.config
 
 ifeq ($(ENABLE_CLANG_ARCMT),1)
 PARALLEL_DIRS += Frontend libclang StaticAnalyzer
 endif
 
 endif  # CLANG_LEVEL

unittests/Serialization/CMakeLists.txt

+set(LLVM_LINK_COMPONENTS
+  Support
+  )
+
+add_clang_unittest(SerializationTests
+  Reserialization.cpp
+  )
+
+target_link_libraries(SerializationTests
+  clangAST
+  clangASTMatchers
+  clangBasic
+  clangFrontend
+  clangTooling
+  )

unittests/Serialization/Makefile

+##===- unittests/Serialization/Makefile --------------------*- Makefile -*-===##
+#
+#                     The LLVM Compiler Infrastructure
+#
+# This file is distributed under the University of Illinois Open Source
+# License. See LICENSE.TXT for details.
+#
+##===----------------------------------------------------------------------===##
+
+CLANG_LEVEL = ../..
+TESTNAME = Serialization
+include $(CLANG_LEVEL)/../../Makefile.config
+LINK_COMPONENTS := $(TARGETS_TO_BUILD) asmparser bitreader support mc option
+USEDLIBS = clangTooling.a clangFrontend.a clangSerialization.a clangDriver.a \
+           clangRewrite.a clangRewriteFrontend.a \
+           clangParse.a clangSema.a clangAnalysis.a \
+           clangEdit.a clangAST.a clangASTMatchers.a clangLex.a clangBasic.a
+
+include $(CLANG_LEVEL)/unittests/Makefile

unittests/Serialization/Reserialization.cpp

+//===- unittests/Serialization/Reserialization.cpp - Reserialization tests ===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+//
+// Unit tests for serializing and deserializing the AST.
+//
+//===----------------------------------------------------------------------===//
+
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/Tooling/Tooling.h"
+#include "gtest/gtest.h"
+
+using namespace clang;
+using namespace clang::ast_matchers;
+using namespace clang::tooling;
+
+// This is a regression test for PR23175.  After deserializing an AST containing
+// ObjCCategoryImplDecl nodes, attempts to iterate ObjCMethodDecl nodes declared
+// in the category interface or implementation resulted in an infinite loop.
+TEST(Reserialization, ObjCMethodDeclIteration) {
+  std::unique_ptr<ASTUnit> AST(tooling::buildASTFromCode(
+    "__attribute__((objc_root_class))\n"
+    "@interface AClass\n"
+    "-(void)AMethod;\n"                    // #0 (Redeclared by #4)
+    "@end\n"
+    "@interface AClass (ACategory)\n"
+    "-(void)ACategoryMethod;\n"            // #1 (Redeclared by #2)
+    "@end\n"
+    "@implementation AClass (ACategory)\n"
+    "-(void)ACategoryMethod {}\n"          // #2 (Redeclares #1)
+    "@end\n"
+    "@interface AClass ()\n"
+    "-(void)AClassExtensionMethod;\n"      // #3 (Not redeclared)
+    "@end\n"
+    "@implementation AClass\n"
+    "-(void)AMethod {}\n"                  // #4 (Redeclares #0)
+    "-(void)AClassExtensionMethod {}\n"    // #5 (Not a redeclaration)
+    "@end\n"
+    "",
+    "input.m",
+    /*Reserialize*/ true));
+  ASSERT_TRUE(AST.get());
+  ASTContext &Context = AST->getASTContext();
+
+  MatchFinder Finder;
+  internal::CollectMatchesCallback Callback;
+  Finder.addMatcher(
+      objCMethodDecl().bind("id"),
+      &Callback);
+  Finder.matchAST(Context);
+  ASSERT_TRUE(Callback.Nodes.size() == 6);
+
+  const int ExpectedDeclCounts[6] = { 2, 2, 2, 1, 2, 1 };
+  for (std::size_t i = 0; i < Callback.Nodes.size(); ++i) {
+    const ObjCMethodDecl *MD =
+        Callback.Nodes[i].getNodeAs<ObjCMethodDecl>("id");
+    ASSERT_TRUE(MD);
+
+    int DeclCounts = 0;
+    for (const auto *D : MD->redecls()) {
+      (void)D;
+      ++DeclCounts;
+      if (DeclCounts > ExpectedDeclCounts[i]) {
+        // Avoid infinite loops.  Going one too far is far enough.
+        break;
+      }
+    }
+    ASSERT_TRUE(DeclCounts == ExpectedDeclCounts[i]);
+  }
+}