[PATCH] D40301: [clangd] Ensure preamble outlives the AST

[Phabricator via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL318944: [clangd] Ensure preamble outlives the AST (authored 
by ibiryukov).

Repository:
  rL LLVM

https://reviews.llvm.org/D40301

Files:
  clang-tools-extra/trunk/clangd/ClangdUnit.cpp
  clang-tools-extra/trunk/clangd/ClangdUnit.h

Index: clang-tools-extra/trunk/clangd/ClangdUnit.h
===
--- clang-tools-extra/trunk/clangd/ClangdUnit.h
+++ clang-tools-extra/trunk/clangd/ClangdUnit.h
@@ -48,15 +48,25 @@
   llvm::SmallVector FixIts;
 };
 
+// Stores Preamble and associated data.
+struct PreambleData {
+  PreambleData(PrecompiledPreamble Preamble,
+   std::vector TopLevelDeclIDs,
+   std::vector Diags);
+
+  PrecompiledPreamble Preamble;
+  std::vector TopLevelDeclIDs;
+  std::vector Diags;
+};
+
 /// Stores and provides access to parsed AST.
 class ParsedAST {
 public:
   /// Attempts to run Clang and store parsed AST. If \p Preamble is non-null
   /// it is reused during parsing.
   static llvm::Optional
   Build(std::unique_ptr CI,
-const PrecompiledPreamble *Preamble,
-ArrayRef PreambleDeclIDs,
+std::shared_ptr Preamble,
 std::unique_ptr Buffer,
 std::shared_ptr PCHs,
 IntrusiveRefCntPtr VFS, clangd::Logger );
@@ -80,15 +90,18 @@
   const std::vector () const;
 
 private:
-  ParsedAST(std::unique_ptr Clang,
+  ParsedAST(std::shared_ptr Preamble,
+std::unique_ptr Clang,
 std::unique_ptr Action,
 std::vector TopLevelDecls,
-std::vector PendingTopLevelDecls,
 std::vector Diags);
 
 private:
   void ensurePreambleDeclsDeserialized();
 
+  // In-memory preambles must outlive the AST, it is important that this member
+  // goes before Clang and Action.
+  std::shared_ptr Preamble;
   // We store an "incomplete" FrontendAction (i.e. no EndSourceFile was called
   // on it) and CompilerInstance used to run it. That way we don't have to do
   // complex memory management of all Clang structures on our own. (They are
@@ -100,7 +113,7 @@
   // Data, stored after parsing.
   std::vector Diags;
   std::vector TopLevelDecls;
-  std::vector PendingTopLevelDecls;
+  bool PreambleDeclsDeserialized;
 };
 
 // Provides thread-safe access to ParsedAST.
@@ -124,17 +137,6 @@
   mutable llvm::Optional AST;
 };
 
-// Stores Preamble and associated data.
-struct PreambleData {
-  PreambleData(PrecompiledPreamble Preamble,
-   std::vector TopLevelDeclIDs,
-   std::vector Diags);
-
-  PrecompiledPreamble Preamble;
-  std::vector TopLevelDeclIDs;
-  std::vector Diags;
-};
-
 /// Manages resources, required by clangd. Allows to rebuild file with new
 /// contents, and provides AST and Preamble for it.
 class CppFile : public std::enable_shared_from_this {
Index: clang-tools-extra/trunk/clangd/ClangdUnit.cpp
===
--- clang-tools-extra/trunk/clangd/ClangdUnit.cpp
+++ clang-tools-extra/trunk/clangd/ClangdUnit.cpp
@@ -879,18 +879,19 @@
 
 llvm::Optional
 ParsedAST::Build(std::unique_ptr CI,
- const PrecompiledPreamble *Preamble,
- ArrayRef PreambleDeclIDs,
+ std::shared_ptr Preamble,
  std::unique_ptr Buffer,
  std::shared_ptr PCHs,
  IntrusiveRefCntPtr VFS,
  clangd::Logger ) {
 
   std::vector ASTDiags;
   StoreDiagsConsumer UnitDiagsConsumer(/*ref*/ ASTDiags);
 
+  const PrecompiledPreamble *PreamblePCH =
+  Preamble ? >Preamble : nullptr;
   auto Clang = prepareCompilerInstance(
-  std::move(CI), Preamble, std::move(Buffer), std::move(PCHs),
+  std::move(CI), PreamblePCH, std::move(Buffer), std::move(PCHs),
   std::move(VFS), /*ref*/ UnitDiagsConsumer);
 
   // Recover resources if we crash before exiting this method.
@@ -912,15 +913,8 @@
   Clang->getDiagnostics().setClient(new EmptyDiagsConsumer);
 
   std::vector ParsedDecls = Action->takeTopLevelDecls();
-  std::vector PendingDecls;
-  if (Preamble) {
-PendingDecls.reserve(PreambleDeclIDs.size());
-PendingDecls.insert(PendingDecls.begin(), PreambleDeclIDs.begin(),
-PreambleDeclIDs.end());
-  }
-
-  return ParsedAST(std::move(Clang), std::move(Action), std::move(ParsedDecls),
-   std::move(PendingDecls), std::move(ASTDiags));
+  return ParsedAST(std::move(Preamble), std::move(Clang), std::move(Action),
+   std::move(ParsedDecls), std::move(ASTDiags));
 }
 
 namespace {
@@ -1061,24 +1055,25 @@
 }
 
 void ParsedAST::ensurePreambleDeclsDeserialized() {
-  if (PendingTopLevelDecls.empty())
+  if (PreambleDeclsDeserialized || !Preamble)
 return;
 
   std::vector Resolved;
-  Resolved.reserve(PendingTopLevelDecls.size());
+  Resolved.reserve(Preamble->TopLevelDeclIDs.size());
 
   ExternalASTSource  

[PATCH] D40301: [clangd] Ensure preamble outlives the AST

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov updated this revision to Diff 124167.
ilya-biryukov added a comment.

- Moved PreambleData declaration up to avoid fwd-decl.


https://reviews.llvm.org/D40301

Files:
  clangd/ClangdUnit.cpp
  clangd/ClangdUnit.h

Index: clangd/ClangdUnit.h
===
--- clangd/ClangdUnit.h
+++ clangd/ClangdUnit.h
@@ -48,15 +48,25 @@
   llvm::SmallVector FixIts;
 };
 
+// Stores Preamble and associated data.
+struct PreambleData {
+  PreambleData(PrecompiledPreamble Preamble,
+   std::vector TopLevelDeclIDs,
+   std::vector Diags);
+
+  PrecompiledPreamble Preamble;
+  std::vector TopLevelDeclIDs;
+  std::vector Diags;
+};
+
 /// Stores and provides access to parsed AST.
 class ParsedAST {
 public:
   /// Attempts to run Clang and store parsed AST. If \p Preamble is non-null
   /// it is reused during parsing.
   static llvm::Optional
   Build(std::unique_ptr CI,
-const PrecompiledPreamble *Preamble,
-ArrayRef PreambleDeclIDs,
+std::shared_ptr Preamble,
 std::unique_ptr Buffer,
 std::shared_ptr PCHs,
 IntrusiveRefCntPtr VFS, clangd::Logger );
@@ -80,15 +90,18 @@
   const std::vector () const;
 
 private:
-  ParsedAST(std::unique_ptr Clang,
+  ParsedAST(std::shared_ptr Preamble,
+std::unique_ptr Clang,
 std::unique_ptr Action,
 std::vector TopLevelDecls,
-std::vector PendingTopLevelDecls,
 std::vector Diags);
 
 private:
   void ensurePreambleDeclsDeserialized();
 
+  // In-memory preambles must outlive the AST, it is important that this member
+  // goes before Clang and Action.
+  std::shared_ptr Preamble;
   // We store an "incomplete" FrontendAction (i.e. no EndSourceFile was called
   // on it) and CompilerInstance used to run it. That way we don't have to do
   // complex memory management of all Clang structures on our own. (They are
@@ -100,7 +113,7 @@
   // Data, stored after parsing.
   std::vector Diags;
   std::vector TopLevelDecls;
-  std::vector PendingTopLevelDecls;
+  bool PreambleDeclsDeserialized;
 };
 
 // Provides thread-safe access to ParsedAST.
@@ -124,17 +137,6 @@
   mutable llvm::Optional AST;
 };
 
-// Stores Preamble and associated data.
-struct PreambleData {
-  PreambleData(PrecompiledPreamble Preamble,
-   std::vector TopLevelDeclIDs,
-   std::vector Diags);
-
-  PrecompiledPreamble Preamble;
-  std::vector TopLevelDeclIDs;
-  std::vector Diags;
-};
-
 /// Manages resources, required by clangd. Allows to rebuild file with new
 /// contents, and provides AST and Preamble for it.
 class CppFile : public std::enable_shared_from_this {
Index: clangd/ClangdUnit.cpp
===
--- clangd/ClangdUnit.cpp
+++ clangd/ClangdUnit.cpp
@@ -879,18 +879,19 @@
 
 llvm::Optional
 ParsedAST::Build(std::unique_ptr CI,
- const PrecompiledPreamble *Preamble,
- ArrayRef PreambleDeclIDs,
+ std::shared_ptr Preamble,
  std::unique_ptr Buffer,
  std::shared_ptr PCHs,
  IntrusiveRefCntPtr VFS,
  clangd::Logger ) {
 
   std::vector ASTDiags;
   StoreDiagsConsumer UnitDiagsConsumer(/*ref*/ ASTDiags);
 
+  const PrecompiledPreamble *PreamblePCH =
+  Preamble ? >Preamble : nullptr;
   auto Clang = prepareCompilerInstance(
-  std::move(CI), Preamble, std::move(Buffer), std::move(PCHs),
+  std::move(CI), PreamblePCH, std::move(Buffer), std::move(PCHs),
   std::move(VFS), /*ref*/ UnitDiagsConsumer);
 
   // Recover resources if we crash before exiting this method.
@@ -912,15 +913,8 @@
   Clang->getDiagnostics().setClient(new EmptyDiagsConsumer);
 
   std::vector ParsedDecls = Action->takeTopLevelDecls();
-  std::vector PendingDecls;
-  if (Preamble) {
-PendingDecls.reserve(PreambleDeclIDs.size());
-PendingDecls.insert(PendingDecls.begin(), PreambleDeclIDs.begin(),
-PreambleDeclIDs.end());
-  }
-
-  return ParsedAST(std::move(Clang), std::move(Action), std::move(ParsedDecls),
-   std::move(PendingDecls), std::move(ASTDiags));
+  return ParsedAST(std::move(Preamble), std::move(Clang), std::move(Action),
+   std::move(ParsedDecls), std::move(ASTDiags));
 }
 
 namespace {
@@ -1061,24 +1055,25 @@
 }
 
 void ParsedAST::ensurePreambleDeclsDeserialized() {
-  if (PendingTopLevelDecls.empty())
+  if (PreambleDeclsDeserialized || !Preamble)
 return;
 
   std::vector Resolved;
-  Resolved.reserve(PendingTopLevelDecls.size());
+  Resolved.reserve(Preamble->TopLevelDeclIDs.size());
 
   ExternalASTSource  = *getASTContext().getExternalSource();
-  for (serialization::DeclID TopLevelDecl : PendingTopLevelDecls) {
+  for (serialization::DeclID TopLevelDecl : Preamble->TopLevelDeclIDs) {
 // Resolve the declaration ID to an actual declaration, 

[PATCH] D40301: [clangd] Ensure preamble outlives the AST

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added inline comments.
This revision is now accepted and ready to land.



Comment at: clangd/ClangdUnit.h:51
 
+struct PreambleData;
+

can you move the definition here to avoid the extra decl?
(I tend to find this more readable and the struct is small, but if you prefer 
"top-down" that's fine too)


https://reviews.llvm.org/D40301



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits