[Lldb-commits] [PATCH] D67994: Modify lldb-test to print out ASTs from symbol file

[Shafik Yaghmour via Phabricator via lldb-commits]

shafik added a comment.

In D67994#1697981 <https://reviews.llvm.org/D67994#1697981>, @labath wrote:

> In D67994#1695172 <https://reviews.llvm.org/D67994#1695172>, @shafik wrote:
>
> > In D67994#1692645 <https://reviews.llvm.org/D67994#1692645>, @labath wrote:
> >
> > > Maybe this is my fault since I'm the one who introduced the first bunch of arguments here IIRC, but anyway, I have a feeling all of these dumping options are getting out of hand. Looking at the argument list, you'd expect that -dump-ast, and -dump-clang-ast do something completely different, but they actually print the exact same AST, only one allows you to print a subset of it, while the other one doesn't.
> > >
> > > Given that the ast dumping is currently incompatible with all/most of the other dumping options anyway, maybe we should turn over a clean slate, and implement a new subcommand specifically dedicated to dumping clang ast (as opposed to the internal lldb representations of types/functions/..., which is what the "symbols" subcommand started out as, and which is what most of its options are dedicated to).
> > >
> > > OR, and this would-be super cool, if it was actually possible, we could try to make ast-dumping compatible with the existing searching options.  Currently, if you type `lldb-test symbols -find=type -name=foo` it will search for types named "foo", and print some summary of the lldb object representing that type. What if we made it so that adding `-dump-ast` to that command caused the tool to dump the *AST* of the types it has found (e.g., in addition to the previous summary)? I think that would make the behavior of the tool most natural to a new user, but I am not sure whether that would actually fit in with your goals here...
> >
> >
> > The output is actually quite different for example given:
> >
> >   struct A {
> >     struct {
> >         int x;
> >     };
> >   } a;
> >
> >
> > the output of `lldb-test symbols -dump-ast  anon.o` is:
> >
> >   Module: anon.o
> >   struct A;
> >
> >
> > while the output of `lldb-test symbols -dump-clang-ast  anon.o` is:
> >
> >   Module: anon.o
> >   A
> >   CXXRecordDecl 0x7ff3698262c8 <<invalid sloc>> <invalid sloc> struct A definition
> >   |-DefinitionData pass_in_registers aggregate standard_layout trivially_copyable pod trivial literal
> >   ...
> >
> >
> > Given they are both working with the Symbol File I believe they both belong under  the `symbol` command. I feel like `-dump-ast` is a little misleading but `-dump-clang-ast` feels good.
>
>
> You're right about. I'm sorry, I should have checked what -dump-ast does. It definitely is confusing that it does something completely different than the "dump ast" lldb command.
>
> But what about the second part of my comment? I think it would still be nice if the `lldb-test symbols` options could be somehow factored into two groups:
>
> - a group which selects *what* to dump: this would be `-find`, `-name`, etc.
> - a group which selects *how* to dump it: -dump-clang-ast, -dump-ast :/, the default mode
>
>   would it be somehow possible to hook in this logic into the `findTypes` function in lldb-test, so that instead of calling TypeMap::Dump, it does something else if `-dump-clang-ast` flag is present (this "else" resuling in the printing of clang ast)?


I sympathize with the sentiment that it would be nice to collapse some of the arguments. I spent some time trying to see how to combine these features nicely and I don't think it will end up being any cleaner.

Folding in the `-dump-ast` and `-dump-clang-ast` into `-find` only seems to make sense with `-find=type` or `-find=none` so this feels like shoe horning by lumping it with the `-find` option. Since I now need to add error handling for the blocks, variables and namespaces

If you look at the switch for `Find` it already has a mess of conflicting options :-( for example `-find=block` is the only one that use `-line`.

We won’t be saving much in the way of code just shifting where the if/else will be. Each functionality is really providing different information and I can see how each one is useful, so I don’t want to remove any of them.

I do think the `-dump-ast`  could use a better name maybe `-print-decl` because it end up using `DeclPrinter` while `-dump-clang-ast` end up using `ASTDumper`.

This utility can be probably be refactored to be cleaner but that is a bigger change and outside the scope of being able to have the ability to verify fixes for how we generate clang ASTs from DWARF which we have no way of doing and I need now to be able to verify a fix I have now.


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

https://reviews.llvm.org/D67994