[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#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
  | |-DefaultConstructor exists trivial needs_implicit
  | |-CopyConstructor simple trivial has_const_param needs_implicit implicit_has_const_param
  | |-MoveConstructor exists simple trivial needs_implicit
  | |-CopyAssignment trivial has_const_param needs_implicit implicit_has_const_param
  | |-MoveAssignment exists simple trivial needs_implicit
  | `-Destructor simple irrelevant trivial needs_implicit
  |-CXXRecordDecl 0x7ff3698263f8 <<invalid sloc>> <invalid sloc> struct definition
  | |-DefinitionData is_anonymous pass_in_registers aggregate standard_layout trivially_copyable pod trivial literal
  | | |-DefaultConstructor exists trivial needs_implicit
  | | |-CopyConstructor simple trivial has_const_param needs_implicit implicit_has_const_param
  | | |-MoveConstructor exists simple trivial needs_implicit
  | | |-CopyAssignment trivial has_const_param needs_implicit implicit_has_const_param
  | | |-MoveAssignment exists simple trivial needs_implicit
  | | `-Destructor simple irrelevant trivial needs_implicit
  | `-FieldDecl 0x7ff369826528 <<invalid sloc>> <invalid sloc> x 'int'
  |-FieldDecl 0x7ff369826578 <<invalid sloc>> <invalid sloc> implicit 'A::(anonymous struct)'
  `-IndirectFieldDecl 0x7ff3698265f0 <<invalid sloc>> <invalid sloc> implicit x 'int'
    |-Field 0x7ff369826578 '' 'A::(anonymous struct)'
    `-Field 0x7ff369826528 'x' 'int'
  A::(anonymous struct)
  CXXRecordDecl 0x7ff3698263f8 <<invalid sloc>> <invalid sloc> struct definition
  |-DefinitionData is_anonymous pass_in_registers aggregate standard_layout trivially_copyable pod trivial literal
  | |-DefaultConstructor exists trivial needs_implicit
  | |-CopyConstructor simple trivial has_const_param needs_implicit implicit_has_const_param
  | |-MoveConstructor exists simple trivial needs_implicit
  | |-CopyAssignment trivial has_const_param needs_implicit implicit_has_const_param
  | |-MoveAssignment exists simple trivial needs_implicit
  | `-Destructor simple irrelevant trivial needs_implicit
  `-FieldDecl 0x7ff369826528 <<invalid sloc>> <invalid sloc> x 'int'
  int
  BuiltinType 0x7ff369825b00 'int'

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.



================
Comment at: include/lldb/Symbol/ClangASTContext.h:896
+  /// Dump clang AST types from the symbol table
+  ///
+  /// \param[in] s
----------------
aprantl wrote:
> What is "the symbol table" in this context? Does ClangASTContext have access to one?
It comes from`GetSymbolFile()` which is inherited from `TypeSystem`


================
Comment at: source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:3059
+
+    if (isType(dwarf_tag) && tag != DW_TAG_subrange_type)
+      ParseType(sc, die, &type_is_new);
----------------
aprantl wrote:
> comment explaining why not subrange type?
Good catch! Currently `ParseTypeFromDWARF(...)` which is called by `ParseType(...)` does not handle `DW_TAG_subrange_type` and it is not clear ATM if this is a bug or not. 


================
Comment at: source/Symbol/ClangASTContext.cpp:9184
+
+    if (clang::CXXRecordDecl *record_decl =
+            GetAsCXXRecordDecl(type->GetFullCompilerType().GetOpaqueQualType()))
----------------
aprantl wrote:
> A switch over the kind would be more efficient and potentially nicer to read?
That would be nice but I don't think there is a way to get to the type what will allow me to use a switch.


================
Comment at: source/Symbol/ClangASTContext.cpp:9195
+                 GetAsEnumDecl(type->GetFullCompilerType()))
+      enum_decl->dump(s.AsRawOstream());
+    else {
----------------
aprantl wrote:
> is an EnumDecl not a TagDecl?
I don't understand the comment, `TagDecl` is handled above.


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

https://reviews.llvm.org/D67994