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

Fri Nov 6 12:44:11 PST 2020

aaron.ballman added a comment.

In D90221#2374344 <https://reviews.llvm.org/D90221#2374344>, @aronsky wrote:

>> 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.

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).

> 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).

Yup, this is a great use for `-ast-dump=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.

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.

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

https://reviews.llvm.org/D90221