[PATCH] D80878: [clang] Prevent that Decl::dump on a CXXRecordDecl deserialises further declarations.

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai added a comment.

As long as JSON dumper still compiles, it looks good to me. Just have a few 
small nits.




Comment at: clang/include/clang/AST/TextNodeDumper.h:47
+  /// Indicates if we can deserialize declarations from the ExternalASTSource.
+  bool Deserialize = true;
+

Would it be better to make the default value the same as in `ASTNodeTraverser`? 
I.e.,

```lang=c++
  /// Indicates whether we should trigger deserialization of nodes that had
  /// not already been loaded.
  bool Deserialize = false;
```



Comment at: clang/lib/AST/TextNodeDumper.cpp:2031-2034
+  if (!D->hasExternalVisibleStorage() || getDeserialize()) {
+FLAG(hasConstexprDestructor, constexpr);
+  } else
+OS << " maybe_constexpr";

The inconsistency that visually single-statement "if" branch has curly braces 
but "else" branch doesn't is grating. I understand a macro can expand into 
multiple statements but visually it looks like a single statement. Probably I 
would add braces to "else" as well.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D80878/new/

https://reviews.llvm.org/D80878

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

[PATCH] D80878: [clang] Prevent that Decl::dump on a CXXRecordDecl deserialises further declarations.

[Raphael Isemann via Phabricator via cfe-commits]

teemperor updated this revision to Diff 364385.
teemperor added a comment.

- Make setter propagate 'deserialize' to the node dumper (thanks Volodymyr!)


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D80878/new/

https://reviews.llvm.org/D80878

Files:
  clang/include/clang/AST/ASTNodeTraverser.h
  clang/include/clang/AST/TextNodeDumper.h
  clang/lib/AST/TextNodeDumper.cpp
  clang/test/AST/ast-dump-lambda.cpp
  clang/test/AST/ast-dump-records.cpp
  clang/unittests/AST/ASTDumpTest.cpp
  clang/unittests/AST/CMakeLists.txt

Index: clang/unittests/AST/CMakeLists.txt
===
--- clang/unittests/AST/CMakeLists.txt
+++ clang/unittests/AST/CMakeLists.txt
@@ -6,6 +6,7 @@
 
 add_clang_unittest(ASTTests
   ASTContextParentMapTest.cpp
+  ASTDumpTest.cpp
   ASTImporterFixtures.cpp
   ASTImporterTest.cpp
   ASTImporterObjCTest.cpp
Index: clang/unittests/AST/ASTDumpTest.cpp
===
--- /dev/null
+++ clang/unittests/AST/ASTDumpTest.cpp
@@ -0,0 +1,182 @@
+//===- unittests/AST/ASTDumpTest.cpp --- Declaration tests ===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+//
+// Tests Decl::dump().
+//
+//===--===//
+
+#include "clang/AST/ASTContext.h"
+#include "clang/AST/DeclCXX.h"
+#include "clang/AST/DeclObjC.h"
+#include "clang/Basic/Builtins.h"
+#include "clang/Basic/FileManager.h"
+#include "clang/Basic/SourceManager.h"
+#include "gtest/gtest.h"
+
+using namespace clang;
+
+namespace clang {
+namespace ast {
+
+namespace {
+/// An ExternalASTSource that asserts if it is queried for information about
+/// any declaration.
+class TrappingExternalASTSource : public ExternalASTSource {
+  ~TrappingExternalASTSource() override = default;
+  bool FindExternalVisibleDeclsByName(const DeclContext *,
+  DeclarationName) override {
+assert(false && "Unexpected call to FindExternalVisibleDeclsByName");
+return true;
+  }
+
+  void FindExternalLexicalDecls(const DeclContext *,
+llvm::function_ref,
+SmallVectorImpl &) override {
+assert(false && "Unexpected call to FindExternalLexicalDecls");
+  }
+
+  void completeVisibleDeclsMap(const DeclContext *) override {
+assert(false && "Unexpected call to completeVisibleDeclsMap");
+  }
+
+  void CompleteRedeclChain(const Decl *) override {
+assert(false && "Unexpected call to CompleteRedeclChain");
+  }
+
+  void CompleteType(TagDecl *) override {
+assert(false && "Unexpected call to CompleteType(Tag Decl*)");
+  }
+
+  void CompleteType(ObjCInterfaceDecl *) override {
+assert(false && "Unexpected call to CompleteType(ObjCInterfaceDecl *)");
+  }
+};
+
+/// An ExternalASTSource that records which DeclContexts were queried so far.
+class RecordingExternalASTSource : public ExternalASTSource {
+public:
+  llvm::DenseSet QueriedDCs;
+  ~RecordingExternalASTSource() override = default;
+
+  void FindExternalLexicalDecls(const DeclContext *DC,
+llvm::function_ref,
+SmallVectorImpl &) override {
+QueriedDCs.insert(DC);
+  }
+};
+
+class ASTDumpTestBase : public ::testing::Test {
+protected:
+  ASTDumpTestBase(clang::ExternalASTSource *Source)
+  : FileMgr(FileMgrOpts), DiagID(new DiagnosticIDs()),
+Diags(DiagID, new DiagnosticOptions, new IgnoringDiagConsumer()),
+SourceMgr(Diags, FileMgr), Idents(LangOpts, nullptr),
+Ctxt(LangOpts, SourceMgr, Idents, Sels, Builtins, TU_Complete) {
+Ctxt.setExternalSource(Source);
+  }
+
+  FileSystemOptions FileMgrOpts;
+  FileManager FileMgr;
+  IntrusiveRefCntPtr DiagID;
+  DiagnosticsEngine Diags;
+  SourceManager SourceMgr;
+  LangOptions LangOpts;
+  IdentifierTable Idents;
+  SelectorTable Sels;
+  Builtin::Context Builtins;
+  ASTContext Ctxt;
+};
+
+/// Tests that Decl::dump doesn't load additional declarations from the
+/// ExternalASTSource.
+class NoDeserializeTest : public ASTDumpTestBase {
+protected:
+  NoDeserializeTest() : ASTDumpTestBase(new TrappingExternalASTSource()) {}
+};
+
+/// Tests which declarations Decl::dump deserializes;
+class DeserializeTest : public ASTDumpTestBase {
+protected:
+  DeserializeTest()
+  : ASTDumpTestBase(Recorder = new RecordingExternalASTSource()) {}
+  RecordingExternalASTSource *Recorder;
+};
+} // unnamed namespace
+
+/// Set all flags that activate queries to the ExternalASTSource.
+static void setExternalStorageFlags(DeclContext *DC) {
+  DC->setHasExternalLexicalStorage();
+  DC->setHasExternalVisibleStorage();
+  

[PATCH] D80878: [clang] Prevent that Decl::dump on a CXXRecordDecl deserialises further declarations.

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai added inline comments.



Comment at: clang/lib/AST/ASTDumper.cpp:199
 P.setDeserialize(Deserialize);
+P.doGetNodeDelegate().setDeserialize(Deserialize);
 P.Visit(this);

It is possible it will complicate the implementation too much but what if 
`P.setDeserialize(Deserialize);` is responsible for propagating `Deserialize` 
to its NodeDelegate?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D80878/new/

https://reviews.llvm.org/D80878

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

[PATCH] D80878: [clang] Prevent that Decl::dump on a CXXRecordDecl deserialises further declarations.

[Raphael Isemann via Phabricator via cfe-commits]

teemperor added a comment.

This is technically in the "Waiting on Review" queue, that's why I also added 
another reviewer :). The original revision got accepted and then I reverted and 
updated the patch/test to honor the `Deserialize` value (which was broken in 
the first version). I guess the fact that this got reviewed, landed, reverted 
and back into review caused some confusion

Also obligatory "friendly ping" on this :)


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D80878/new/

https://reviews.llvm.org/D80878

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

[PATCH] D80878: [clang] Prevent that Decl::dump on a CXXRecordDecl deserialises further declarations.

[Adrian Prantl via Phabricator via cfe-commits]

aprantl added a comment.

@teemperor is there a reason why the fixed patch never landed? This looks a 
good improvement to have.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D80878/new/

https://reviews.llvm.org/D80878

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

[PATCH] D80878: [clang] Prevent that Decl::dump on a CXXRecordDecl deserialises further declarations.

[Raphael Isemann via Phabricator via cfe-commits]

teemperor updated this revision to Diff 301577.
teemperor added a comment.

- Respect forced deserialisation.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D80878/new/

https://reviews.llvm.org/D80878

Files:
  clang/include/clang/AST/TextNodeDumper.h
  clang/lib/AST/ASTDumper.cpp
  clang/lib/AST/TextNodeDumper.cpp
  clang/test/AST/ast-dump-lambda.cpp
  clang/test/AST/ast-dump-records.cpp
  clang/unittests/AST/ASTDumpTest.cpp
  clang/unittests/AST/CMakeLists.txt

Index: clang/unittests/AST/CMakeLists.txt
===
--- clang/unittests/AST/CMakeLists.txt
+++ clang/unittests/AST/CMakeLists.txt
@@ -6,6 +6,7 @@
 
 add_clang_unittest(ASTTests
   ASTContextParentMapTest.cpp
+  ASTDumpTest.cpp
   ASTImporterFixtures.cpp
   ASTImporterTest.cpp
   ASTImporterGenericRedeclTest.cpp
Index: clang/unittests/AST/ASTDumpTest.cpp
===
--- /dev/null
+++ clang/unittests/AST/ASTDumpTest.cpp
@@ -0,0 +1,182 @@
+//===- unittests/AST/ASTDumpTest.cpp --- Declaration tests ===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+//
+// Tests Decl::dump().
+//
+//===--===//
+
+#include "clang/AST/ASTContext.h"
+#include "clang/AST/DeclCXX.h"
+#include "clang/AST/DeclObjC.h"
+#include "clang/Basic/Builtins.h"
+#include "clang/Basic/FileManager.h"
+#include "clang/Basic/SourceManager.h"
+#include "gtest/gtest.h"
+
+using namespace clang;
+
+namespace clang {
+namespace ast {
+
+namespace {
+/// An ExternalASTSource that asserts if it is queried for information about
+/// any declaration.
+class TrappingExternalASTSource : public ExternalASTSource {
+  ~TrappingExternalASTSource() override = default;
+  bool FindExternalVisibleDeclsByName(const DeclContext *,
+  DeclarationName) override {
+assert(false && "Unexpected call to FindExternalVisibleDeclsByName");
+return true;
+  }
+
+  void FindExternalLexicalDecls(const DeclContext *,
+llvm::function_ref,
+SmallVectorImpl &) override {
+assert(false && "Unexpected call to FindExternalLexicalDecls");
+  }
+
+  void completeVisibleDeclsMap(const DeclContext *) override {
+assert(false && "Unexpected call to completeVisibleDeclsMap");
+  }
+
+  void CompleteRedeclChain(const Decl *) override {
+assert(false && "Unexpected call to CompleteRedeclChain");
+  }
+
+  void CompleteType(TagDecl *) override {
+assert(false && "Unexpected call to CompleteType(Tag Decl*)");
+  }
+
+  void CompleteType(ObjCInterfaceDecl *) override {
+assert(false && "Unexpected call to CompleteType(ObjCInterfaceDecl *)");
+  }
+};
+
+/// An ExternalASTSource that records which DeclContexts were queried so far.
+class RecordingExternalASTSource : public ExternalASTSource {
+public:
+  llvm::DenseSet QueriedDCs;
+  ~RecordingExternalASTSource() override = default;
+
+  void FindExternalLexicalDecls(const DeclContext *DC,
+llvm::function_ref,
+SmallVectorImpl &) override {
+QueriedDCs.insert(DC);
+  }
+};
+
+
+class ASTDumpTestBase : public ::testing::Test {
+protected:
+  ASTDumpTestBase(clang::ExternalASTSource *Source)
+  : FileMgr(FileMgrOpts), DiagID(new DiagnosticIDs()),
+Diags(DiagID, new DiagnosticOptions, new IgnoringDiagConsumer()),
+SourceMgr(Diags, FileMgr), Idents(LangOpts, nullptr),
+Ctxt(LangOpts, SourceMgr, Idents, Sels, Builtins) {
+Ctxt.setExternalSource(Source);
+  }
+
+  FileSystemOptions FileMgrOpts;
+  FileManager FileMgr;
+  IntrusiveRefCntPtr DiagID;
+  DiagnosticsEngine Diags;
+  SourceManager SourceMgr;
+  LangOptions LangOpts;
+  IdentifierTable Idents;
+  SelectorTable Sels;
+  Builtin::Context Builtins;
+  ASTContext Ctxt;
+};
+
+/// Tests that Decl::dump doesn't load additional declarations from the
+/// ExternalASTSource.
+class NoDeserializeTest : public ASTDumpTestBase {
+protected:
+  NoDeserializeTest() : ASTDumpTestBase(new TrappingExternalASTSource()) {}
+};
+
+/// Tests which declarations Decl::dump deserializes;
+class DeserializeTest : public ASTDumpTestBase {
+protected:
+  DeserializeTest() : ASTDumpTestBase(Recorder = new RecordingExternalASTSource()) {}
+  RecordingExternalASTSource *Recorder;
+};
+} // unnamed namespace
+
+/// Set all flags that activate queries to the ExternalASTSource.
+static void setExternalStorageFlags(DeclContext *DC) {
+  DC->setHasExternalLexicalStorage();
+  DC->setHasExternalVisibleStorage();
+  DC->setMustBuildLookupTable();
+}
+
+/// Dumps the given Decl.
+static void 

[PATCH] D80878: [clang] Prevent that Decl::dump on a CXXRecordDecl deserialises further declarations.

[Raphael Isemann via Phabricator via cfe-commits]

teemperor reopened this revision.
teemperor added a comment.
This revision is now accepted and ready to land.

In D80878#2258942 , @riccibruno wrote:

> And what if deserialization is forced?

That's a good point. That should indeed continue to work with the ASTDumper and 
now (potentially) doesn't. I'll revert.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D80878/new/

https://reviews.llvm.org/D80878

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

[PATCH] D80878: [clang] Prevent that Decl::dump on a CXXRecordDecl deserialises further declarations.

[Bruno Ricci via Phabricator via cfe-commits]

riccibruno added a comment.

And what if deserialization is forced?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D80878/new/

https://reviews.llvm.org/D80878

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

[PATCH] D80878: [clang] Prevent that Decl::dump on a CXXRecordDecl deserialises further declarations.

[Raphael Isemann via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG0478720157f6: [clang] Prevent that Decl::dump on a 
CXXRecordDecl deserialises further… (authored by teemperor).
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D80878/new/

https://reviews.llvm.org/D80878

Files:
  clang/lib/AST/TextNodeDumper.cpp
  clang/test/AST/ast-dump-lambda.cpp
  clang/test/AST/ast-dump-records.cpp
  clang/unittests/AST/ASTDumpTest.cpp
  clang/unittests/AST/CMakeLists.txt

Index: clang/unittests/AST/CMakeLists.txt
===
--- clang/unittests/AST/CMakeLists.txt
+++ clang/unittests/AST/CMakeLists.txt
@@ -6,6 +6,7 @@
 
 add_clang_unittest(ASTTests
   ASTContextParentMapTest.cpp
+  ASTDumpTest.cpp
   ASTImporterFixtures.cpp
   ASTImporterTest.cpp
   ASTImporterGenericRedeclTest.cpp
Index: clang/unittests/AST/ASTDumpTest.cpp
===
--- /dev/null
+++ clang/unittests/AST/ASTDumpTest.cpp
@@ -0,0 +1,140 @@
+//===- unittests/AST/ASTDumpTest.cpp --- Declaration tests ===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+//
+// Tests Decl::dump().
+//
+//===--===//
+
+#include "clang/AST/ASTContext.h"
+#include "clang/AST/DeclCXX.h"
+#include "clang/AST/DeclObjC.h"
+#include "clang/Basic/Builtins.h"
+#include "clang/Basic/FileManager.h"
+#include "clang/Basic/SourceManager.h"
+#include "gtest/gtest.h"
+
+using namespace clang;
+
+namespace clang {
+namespace ast {
+
+namespace {
+/// An ExternalASTSource that asserts if it is queried for information about
+/// any declaration.
+class TrappingExternalASTSource : public ExternalASTSource {
+  ~TrappingExternalASTSource() override = default;
+  bool FindExternalVisibleDeclsByName(const DeclContext *,
+  DeclarationName) override {
+assert(false && "Unexpected call to FindExternalVisibleDeclsByName");
+return true;
+  }
+
+  void FindExternalLexicalDecls(const DeclContext *,
+llvm::function_ref,
+SmallVectorImpl &) override {
+assert(false && "Unexpected call to FindExternalLexicalDecls");
+  }
+
+  void completeVisibleDeclsMap(const DeclContext *) override {
+assert(false && "Unexpected call to completeVisibleDeclsMap");
+  }
+
+  void CompleteRedeclChain(const Decl *) override {
+assert(false && "Unexpected call to CompleteRedeclChain");
+  }
+
+  void CompleteType(TagDecl *) override {
+assert(false && "Unexpected call to CompleteType(Tag Decl*)");
+  }
+
+  void CompleteType(ObjCInterfaceDecl *) override {
+assert(false && "Unexpected call to CompleteType(ObjCInterfaceDecl *)");
+  }
+};
+
+/// Tests that Decl::dump doesn't load additional declarations from the
+/// ExternalASTSource.
+class ExternalASTSourceDumpTest : public ::testing::Test {
+protected:
+  ExternalASTSourceDumpTest()
+  : FileMgr(FileMgrOpts), DiagID(new DiagnosticIDs()),
+Diags(DiagID, new DiagnosticOptions, new IgnoringDiagConsumer()),
+SourceMgr(Diags, FileMgr), Idents(LangOpts, nullptr),
+Ctxt(LangOpts, SourceMgr, Idents, Sels, Builtins) {
+Ctxt.setExternalSource(new TrappingExternalASTSource());
+  }
+
+  FileSystemOptions FileMgrOpts;
+  FileManager FileMgr;
+  IntrusiveRefCntPtr DiagID;
+  DiagnosticsEngine Diags;
+  SourceManager SourceMgr;
+  LangOptions LangOpts;
+  IdentifierTable Idents;
+  SelectorTable Sels;
+  Builtin::Context Builtins;
+  ASTContext Ctxt;
+};
+} // unnamed namespace
+
+/// Set all flags that activate queries to the ExternalASTSource.
+static void setExternalStorageFlags(DeclContext *DC) {
+  DC->setHasExternalLexicalStorage();
+  DC->setHasExternalVisibleStorage();
+  DC->setMustBuildLookupTable();
+}
+
+/// Dumps the given Decl.
+static void dumpDecl(Decl *D) {
+  // Try dumping the decl which shouldn't trigger any calls to the
+  // ExternalASTSource.
+
+  std::string Out;
+  llvm::raw_string_ostream OS(Out);
+  D->dump(OS);
+}
+
+TEST_F(ExternalASTSourceDumpTest, DumpObjCInterfaceDecl) {
+  // Define an Objective-C interface.
+  ObjCInterfaceDecl *I = ObjCInterfaceDecl::Create(
+  Ctxt, Ctxt.getTranslationUnitDecl(), SourceLocation(),
+  ("c"), nullptr, nullptr);
+  Ctxt.getTranslationUnitDecl()->addDecl(I);
+
+  setExternalStorageFlags(I);
+  dumpDecl(I);
+}
+
+TEST_F(ExternalASTSourceDumpTest, DumpRecordDecl) {
+  // Define a struct.
+  RecordDecl *R = RecordDecl::Create(
+  Ctxt, TagDecl::TagKind::TTK_Class,