[PATCH] D90221: Include more attribute details when dumping AST in JSON

[Lev Aronsky via Phabricator via cfe-commits]

aronsky added a comment.

> It seems surprising to me that you'd have to add those declarations; I think 
> that's a layering violation. There's something somewhat strange going on here 
> that I think is related. Given:
> [...]
> Notice how the annotate attribute has an `inner` array with the argument 
> inside of it while the visibility attribute does not? Ideally, we'd either 
> use `args` or `inner` but not a mixture of the two.

Good point. I'm actually not an expert on the inner workings of clang 
structures (this is my first foray into LLVM), so I am not sure what causes 
this discrepancy. For some context - I'm working on a static analysis engine, 
and using clang to parse C/C++. The changes I'm proposing here are due to the 
fact I was missing attribute details in JSON, that are present in standard 
dumping (which, of course, isn't very consumable, compared to JSON).

> I was imagining the design here to be that the JSON node dumper for 
> attributes would open a new JSON attribute named "args" whose value is an 
> array of arguments, then you'd loop over the arguments and `Visit` each one 
> in turn as the arguments are children of the attribute AST node in some 
> respects. For instance, consider dumping the arguments for the `enable_if` 
> attribute, which accepts arbitrary expressions -- dumping a type or a bare 
> decl ref is insufficient.

I concur that from a design standpoint, my solution is pretty lacking - it's 
more of a patch (no pun intended), rather than a fundamental fix to the issue 
at hand. That's also the reason for the extra whitespace you mentioned... I was 
trying to get the missing arguments with minimal changes, so I used existing 
code where available (in this case, code that dumped those arguments in 
TextNodeDumper). My hope was that the patch would be approved (since it does 
improve the output in a way, and doesn't break it), and someone more 
knowledgable in LLVM/clang internals would improve it further :). I would 
gladly take the responsibility, but it's not up to me - as far as my company is 
concerned, the changes are sufficient for the product, and further time spent 
on improvements is unwarranted. Maybe I can work on it in my free time, but I 
can't commit to it.


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

https://reviews.llvm.org/D90221

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

[PATCH] D90221: Include more attribute details when dumping AST in JSON

[Lev Aronsky via Phabricator via cfe-commits]

aronsky added a comment.

In D90221#2379793 , @aaron.ballman 
wrote:

> I think the issue is that ASTNodeTraverser is visiting all of the attribute 
> arguments (around ASTNodeTraverse.h:726) and your patch also visits them (as 
> part of what's emitted from ClangAttrEmitter.cpp).

Thanks! I will take a look at that code.

> Unfortunately, I don't think we can accept the patch as-is -- it's incomplete 
> (misses some kinds of attribute arguments), has correctness issues 
> (duplicates some kinds of attribute arguments), and would make it harder to 
> maintain the code moving forward (with the layering issue). I can definitely 
> understand not having a lot of time to investigate the right way to do this, 
> so I spent a bit of time this afternoon working on a patch that gets partway 
> to what I think needs to happen. I put up a pastebin for it here: 
> https://pastebin.com/6ybrPVu6. It does not solve the issue of duplicate 
> traversal, however, and I'm not certain I have more time to put into the 
> patch right now. Perhaps one of us will have the time to take this the last 
> mile -- I think the functionality is needed for JSON dumping.

Thank you! I appreciate your feedback and your work on the issue. I'll take a 
better look at your patch (from a quick look, it definitely looks more 
organized and correct than mine), and see if I can understand - and fix - the 
duplicate traversal issue.


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

https://reviews.llvm.org/D90221

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

[PATCH] D90221: Include attribute details when dumping AST in JSON

[Lev Aronsky via Phabricator via cfe-commits]

aronsky created this revision.
aronsky added reviewers: aaron.ballman, steveire.
aronsky created this object with visibility "All Users".
Herald added subscribers: cfe-commits, mgorny.
Herald added a project: clang.
aronsky requested review of this revision.

AST dumps in JSON format were missing attribute nodes, compared to the textual 
dumps. This commit fixes the issue.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D90221

Files:
  clang/include/clang/AST/CMakeLists.txt
  clang/include/clang/AST/JSONNodeDumper.h
  clang/lib/AST/JSONNodeDumper.cpp
  clang/utils/TableGen/ClangAttrEmitter.cpp
  clang/utils/TableGen/TableGen.cpp
  clang/utils/TableGen/TableGenBackends.h

Index: clang/utils/TableGen/TableGenBackends.h
===
--- clang/utils/TableGen/TableGenBackends.h
+++ clang/utils/TableGen/TableGenBackends.h
@@ -59,6 +59,8 @@
   llvm::raw_ostream &OS);
 void EmitClangAttrTextNodeDump(llvm::RecordKeeper &Records,
llvm::raw_ostream &OS);
+void EmitClangAttrJSONNodeDump(llvm::RecordKeeper &Records,
+   llvm::raw_ostream &OS);
 void EmitClangAttrNodeTraverse(llvm::RecordKeeper &Records,
llvm::raw_ostream &OS);
 
Index: clang/utils/TableGen/TableGen.cpp
===
--- clang/utils/TableGen/TableGen.cpp
+++ clang/utils/TableGen/TableGen.cpp
@@ -41,6 +41,7 @@
   GenClangAttrParsedAttrImpl,
   GenClangAttrParsedAttrKinds,
   GenClangAttrTextNodeDump,
+  GenClangAttrJSONNodeDump,
   GenClangAttrNodeTraverse,
   GenClangBasicReader,
   GenClangBasicWriter,
@@ -138,6 +139,8 @@
"Generate a clang parsed attribute kinds"),
 clEnumValN(GenClangAttrTextNodeDump, "gen-clang-attr-text-node-dump",
"Generate clang attribute text node dumper"),
+clEnumValN(GenClangAttrJSONNodeDump, "gen-clang-attr-json-node-dump",
+   "Generate clang attribute JSON node dumper"),
 clEnumValN(GenClangAttrNodeTraverse, "gen-clang-attr-node-traverse",
"Generate clang attribute traverser"),
 clEnumValN(GenClangDiagsDefs, "gen-clang-diags-defs",
@@ -295,6 +298,9 @@
   case GenClangAttrTextNodeDump:
 EmitClangAttrTextNodeDump(Records, OS);
 break;
+  case GenClangAttrJSONNodeDump:
+EmitClangAttrJSONNodeDump(Records, OS);
+break;
   case GenClangAttrNodeTraverse:
 EmitClangAttrNodeTraverse(Records, OS);
 break;
Index: clang/utils/TableGen/ClangAttrEmitter.cpp
===
--- clang/utils/TableGen/ClangAttrEmitter.cpp
+++ clang/utils/TableGen/ClangAttrEmitter.cpp
@@ -24,6 +24,7 @@
 #include "llvm/ADT/StringSwitch.h"
 #include "llvm/ADT/iterator_range.h"
 #include "llvm/Support/ErrorHandling.h"
+#include "llvm/Support/JSON.h"
 #include "llvm/Support/raw_ostream.h"
 #include "llvm/TableGen/Error.h"
 #include "llvm/TableGen/Record.h"
@@ -3895,7 +3896,7 @@
  << "}\n";
 }
 
-// Emits the code to dump an attribute.
+// Emits the code to dump an attribute as text.
 void EmitClangAttrTextNodeDump(RecordKeeper &Records, raw_ostream &OS) {
   emitSourceFileHeader("Attribute text node dumper", OS);
 
@@ -3932,6 +3933,58 @@
   }
 }
 
+// Emits the code to dump an attribute as JSON.
+void EmitClangAttrJSONNodeDump(RecordKeeper &Records, raw_ostream &OS) {
+  emitSourceFileHeader("Attribute text node dumper", OS);
+
+  std::vector Attrs = Records.getAllDerivedDefinitions("Attr"), Args;
+  for (const auto *Attr : Attrs) {
+const Record &R = *Attr;
+if (!R.getValueAsBit("ASTNode"))
+  continue;
+
+OS << "  void Visit" << R.getName() << "Attr(const " << R.getName()
+   << "Attr *A) {\n";
+
+// If the attribute has a semantically-meaningful name (which is determined
+// by whether there is a Spelling enumeration for it), then write out the
+// spelling used for the attribute.
+
+std::string FunctionContent;
+llvm::raw_string_ostream SS(FunctionContent);
+
+Args = R.getValueAsListOfDefs("Args");
+
+if (!Args.empty())
+  OS << "const auto *SA = cast<" << R.getName()
+ << "Attr>(A); (void)SA;\n"
+ << "llvm::json::Array Args;\n";
+
+std::vector Spellings = GetFlattenedSpellings(R);
+if (Spellings.size() > 1 && !SpellingNamesAreCommon(Spellings))
+  OS << "JOS.attribute(\"spelling\", A->getSpelling());\n";
+
+for (const auto *Arg : Args) {
+  std::string ArgContent;
+  llvm::raw_string_ostream SS(ArgContent);
+
+  createArgument(*Arg, R.getName())->writeDump(SS);
+
+  if (SS.tell())
+OS << "{\n"
+   << "std::string Str;\n"
+   << "llvm::raw_string_ostream OS(Str);\n"
+   << SS.str() << "Args.push_back(OS.str());\n"
+   << "}\n";
+}
+
+if (!A

[PATCH] D90221: Include attribute details when dumping AST in JSON

[Lev Aronsky via Phabricator via cfe-commits]

aronsky added a comment.

In D90221#2356062 , @lebedev.ri wrote:

> Are there tests missing?

Quite possible. I followed the trail of the existing functions to figure out 
the difference between JSON and textual dumping, and tried replicating 
everything in a manner similar to the existing code. I haven't run into any 
tests, but that's probably because I wasn't looking for those. I'll add the 
appropriate tests ASAP.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90221

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

[PATCH] D90221: Include attribute details when dumping AST in JSON

[Lev Aronsky via Phabricator via cfe-commits]

aronsky added a comment.

In D90221#2357060 , @aaron.ballman 
wrote:

> In D90221#2356110 , @aronsky wrote:
>
>> In D90221#2356062 , @lebedev.ri 
>> wrote:
>>
>>> Are there tests missing?
>>
>> Quite possible. I followed the trail of the existing functions to figure out 
>> the difference between JSON and textual dumping, and tried replicating 
>> everything in a manner similar to the existing code. I haven't run into any 
>> tests, but that's probably because I wasn't looking for those. I'll add the 
>> appropriate tests ASAP.
>
> FWIW, the tests for dumping JSON live in `clang\test\AST` and typically have 
> a `-json` extension on them. There is a helper script named 
> `gen_ast_dump_json_test.py` that can be used to generate the expected output 
> from the test.

Thanks, I'll take a look at the Python script, that'll be helpful!

Those functions do look out of place, but they are actually called via 
polymorphism (I wish I could point to the exact location - it wasn't easy 
figuring that out in the first place, and the actual work was done about a 
month ago, I just got to publishing the PR yesterday). The code that calls 
these functions is emitted at `writeDump` (in 
`clang/utils/TableGen/ClangAttrEmitter.cpp`) - which, in turn, is called by 
`EmitClangAttrJSONNodeDump` and `EmitClangAttrTextNodeDump` in the same file.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90221

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

[PATCH] D90221: Include more attribute details when dumping AST in JSON

[Lev Aronsky via Phabricator via cfe-commits]

aronsky updated this revision to Diff 302558.
aronsky retitled this revision from "Include attribute details when dumping AST 
in JSON" to "Include more attribute details when dumping AST in JSON".
aronsky edited the summary of this revision.
aronsky added a comment.

Updated the relevant unit-test: it now passes, and also verifies the changes.


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

https://reviews.llvm.org/D90221

Files:
  clang/include/clang/AST/CMakeLists.txt
  clang/include/clang/AST/JSONNodeDumper.h
  clang/lib/AST/JSONNodeDumper.cpp
  clang/test/AST/ast-dump-stmt-json.cpp
  clang/utils/TableGen/ClangAttrEmitter.cpp
  clang/utils/TableGen/TableGen.cpp
  clang/utils/TableGen/TableGenBackends.h

Index: clang/utils/TableGen/TableGenBackends.h
===
--- clang/utils/TableGen/TableGenBackends.h
+++ clang/utils/TableGen/TableGenBackends.h
@@ -59,6 +59,8 @@
   llvm::raw_ostream &OS);
 void EmitClangAttrTextNodeDump(llvm::RecordKeeper &Records,
llvm::raw_ostream &OS);
+void EmitClangAttrJSONNodeDump(llvm::RecordKeeper &Records,
+   llvm::raw_ostream &OS);
 void EmitClangAttrNodeTraverse(llvm::RecordKeeper &Records,
llvm::raw_ostream &OS);
 
Index: clang/utils/TableGen/TableGen.cpp
===
--- clang/utils/TableGen/TableGen.cpp
+++ clang/utils/TableGen/TableGen.cpp
@@ -41,6 +41,7 @@
   GenClangAttrParsedAttrImpl,
   GenClangAttrParsedAttrKinds,
   GenClangAttrTextNodeDump,
+  GenClangAttrJSONNodeDump,
   GenClangAttrNodeTraverse,
   GenClangBasicReader,
   GenClangBasicWriter,
@@ -138,6 +139,8 @@
"Generate a clang parsed attribute kinds"),
 clEnumValN(GenClangAttrTextNodeDump, "gen-clang-attr-text-node-dump",
"Generate clang attribute text node dumper"),
+clEnumValN(GenClangAttrJSONNodeDump, "gen-clang-attr-json-node-dump",
+   "Generate clang attribute JSON node dumper"),
 clEnumValN(GenClangAttrNodeTraverse, "gen-clang-attr-node-traverse",
"Generate clang attribute traverser"),
 clEnumValN(GenClangDiagsDefs, "gen-clang-diags-defs",
@@ -295,6 +298,9 @@
   case GenClangAttrTextNodeDump:
 EmitClangAttrTextNodeDump(Records, OS);
 break;
+  case GenClangAttrJSONNodeDump:
+EmitClangAttrJSONNodeDump(Records, OS);
+break;
   case GenClangAttrNodeTraverse:
 EmitClangAttrNodeTraverse(Records, OS);
 break;
Index: clang/utils/TableGen/ClangAttrEmitter.cpp
===
--- clang/utils/TableGen/ClangAttrEmitter.cpp
+++ clang/utils/TableGen/ClangAttrEmitter.cpp
@@ -24,6 +24,7 @@
 #include "llvm/ADT/StringSwitch.h"
 #include "llvm/ADT/iterator_range.h"
 #include "llvm/Support/ErrorHandling.h"
+#include "llvm/Support/JSON.h"
 #include "llvm/Support/raw_ostream.h"
 #include "llvm/TableGen/Error.h"
 #include "llvm/TableGen/Record.h"
@@ -3895,7 +3896,7 @@
  << "}\n";
 }
 
-// Emits the code to dump an attribute.
+// Emits the code to dump an attribute as text.
 void EmitClangAttrTextNodeDump(RecordKeeper &Records, raw_ostream &OS) {
   emitSourceFileHeader("Attribute text node dumper", OS);
 
@@ -3932,6 +3933,58 @@
   }
 }
 
+// Emits the code to dump an attribute as JSON.
+void EmitClangAttrJSONNodeDump(RecordKeeper &Records, raw_ostream &OS) {
+  emitSourceFileHeader("Attribute text node dumper", OS);
+
+  std::vector Attrs = Records.getAllDerivedDefinitions("Attr"), Args;
+  for (const auto *Attr : Attrs) {
+const Record &R = *Attr;
+if (!R.getValueAsBit("ASTNode"))
+  continue;
+
+OS << "  void Visit" << R.getName() << "Attr(const " << R.getName()
+   << "Attr *A) {\n";
+
+// If the attribute has a semantically-meaningful name (which is determined
+// by whether there is a Spelling enumeration for it), then write out the
+// spelling used for the attribute.
+
+std::string FunctionContent;
+llvm::raw_string_ostream SS(FunctionContent);
+
+Args = R.getValueAsListOfDefs("Args");
+
+if (!Args.empty())
+  OS << "const auto *SA = cast<" << R.getName()
+ << "Attr>(A); (void)SA;\n"
+ << "llvm::json::Array Args;\n";
+
+std::vector Spellings = GetFlattenedSpellings(R);
+if (Spellings.size() > 1 && !SpellingNamesAreCommon(Spellings))
+  OS << "JOS.attribute(\"spelling\", A->getSpelling());\n";
+
+for (const auto *Arg : Args) {
+  std::string ArgContent;
+  llvm::raw_string_ostream SS(ArgContent);
+
+  createArgument(*Arg, R.getName())->writeDump(SS);
+
+  if (SS.tell())
+OS << "{\n"
+   << "std::string Str;\n"
+   << "llvm::raw_string_ostream OS(Str);\n"
+   << SS.str() << "Args.push_back(OS.str());\n"
+