Modify lldb-test to print out ASTs from symbol file

[shafik]

Summary

Currently when invoking lldb-test symbols -dump-ast it parses all the debug symbols and calls print(...) on the TranslationUnitDecl.

While useful the TranslationUnitDecl::print(...) method gives us a higher level view then the dump from ASTDumper which is what we get when we invoke dump() on a specific AST node.

The main motivation for this change is allow us to verify that the AST nodes we create when we parse DWARF. For example in order to verify we are correctly using DIFlagExportSymbols added by D66667 we would want to dump a CXXRecordDecl:

CXXRecordDecl 0x7fdc578295f8 <<invalid sloc>> <invalid sloc> struct definition
|-DefinitionData is_anonymous pass_in_registers aggregate standard_layout trivially_copyable pod trivial literal
...

and check for the is_anonymous flag or the lack thereof.

My current approach is to enumerate the symbols and to obtain specific AST nodes if possible and dump them. It looks in some cases such as FunctionDecls we don't have direct access to it but I don't think that is an major problem at this point.

I did not implement it but we should probably have a way of request a specific type instead of indiscriminately dumping all of them.

This is a work in progress and I am not sure if the following is the best approach and I am open to alternative approaches or modification to the current one.

[shafik]

source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp

3060 ↗

(On Diff #221594)

I added this change because currently when end up trying to parse non-types which generates diagnostic like the following:

error: anon.o {0x0000001e}: unhandled type tag 0x0034 (DW_TAG_variable), please file a bug and attach the file at the start of this error message

[xiaobai]

source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp

3029 ↗

(On Diff #221594)

nit: stray whitespace

3032 ↗

(On Diff #221594)

Why is this now two lines?

tools/lldb-test/lldb-test.cpp

580 ↗

(On Diff #221594)

nit: clang-format this block, some of the lines are long and somewhat difficult to read on phabricator.

[JDevlieghere]

source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp

3029 ↗

(On Diff #221594)

Trailing whitespace.

3030 ↗

(On Diff #221594)

I assume this is unintentional?

3060 ↗

(On Diff #221594)

Assuming these are LLVM enum values, there's a method in Dwarf.h named isType which tells you if a given tag is a type. This list is missing at least some DWARF5 types (DW_TAG_dynamic_type, DW_TAG_atomic_type...).

tools/lldb-test/lldb-test.cpp

553 ↗

(On Diff #221594)

The formatting seems off. Did you run this through clang-format?

561 ↗

(On Diff #221594)

Personally I think this would be a lot more readable with braces.

[shafik]

• Formatting code
• Removing FunctionDecl case since it was not correct

[clayborg]

source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp

3032 ↗

(On Diff #221594)

indent is wrong

3061–3075 ↗

(On Diff #221594)

We should add a "IsType()" to DWARFDie here. This list can get out of date as new type tags are added. There are a few other places that could use the "bool DWARFDie::ItType() const" function too. Or we should just fix ParseType by adding an option to it like an extra bool like "bool warnIfNotType" so you don't get the "error:" message.

tools/lldb-test/lldb-test.cpp

552–579 ↗

(On Diff #221594)

I know that there is already clang AST stuff in this file, but it seems like anything in this file should be just passed to the type system for dumping? This code could be:

lldb_private::TypeList type_list;
size_t ntypes = symfile->GetTypes(nullptr, eTypeClassAny, type_list);
printf( "Type list size: %zu\n", ntypes);
for( size_t i = 0; i < ntypes; ++i)
  type_list.GetTypeAtIndex(i)->DumpTypeValue(...);

Better yet this entire function could just become:

Error opts::symbols::dumpAST(lldb_private::Module &Module) {
  Module.ParseAllDebugSymbols();

  auto symfile = Module.GetSymbolFile();
  if (!symfile)
    return make_string_error("Module has no symbol file.");

  auto type_system_or_err =
      symfile->GetTypeSystemForLanguage(eLanguageTypeC_plus_plus);
  if (type_system_or_err)
    type_system_or_err->DumpAST(...);
  else
    return make_string_error("Can't retrieve TypeSystem");
}

And all clang AST specific stuff can be removed from this binary? Tests would need to be updated.

[aprantl]

source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp

3077 ↗

(On Diff #221594)

You also probably don't want to actually print anything here?

tools/lldb-test/lldb-test.cpp

555 ↗

(On Diff #221594)

The rest of the file seems to use llvm::outs() instead of printf.

564 ↗

(On Diff #221594)

clang-format

570 ↗

(On Diff #221594)

switch over CompilerType->getAsClangType() ->getKind()?

[labath]

For dumping a specific type something like this could be right, but for "indiscriminately dumping" everything, this seems to be a bit fragile.

Would it be possible to make this use the SymbolFile::DumpClangAST method (this is what the "image dump ast" lldb command uses), and then possibly change that method to include any extra information you need? I already see the DefinitionData line when i do a "image dump clang ast", so it's possible you wouldn't need to change anything there...

FTR, this is what i get from image dump ast:

Dumping clang ast for 1 modules.
TranslationUnitDecl 0x561fa5fd9128 <<invalid sloc>> <invalid sloc> <undeserialized declarations>
`-NamespaceDecl 0x561fa5fd99e8 <<invalid sloc>> <invalid sloc> Q
  `-CXXRecordDecl 0x561fa5fd9a70 <<invalid sloc>> <invalid sloc> struct A definition
    |-DefinitionData pass_in_registers empty standard_layout trivially_copyable has_user_declared_ctor can_const_default_init
    | |-DefaultConstructor exists non_trivial user_provided defaulted_is_constexpr
    | |-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
    `-CXXConstructorDecl 0x561fa5fd9bf0 <<invalid sloc>> <invalid sloc> A 'void ()'

[shafik]

In D67994#1682051, @labath wrote:

For dumping a specific type something like this could be right, but for "indiscriminately dumping" everything, this seems to be a bit fragile.

Would it be possible to make this use the SymbolFile::DumpClangAST method (this is what the "image dump ast" lldb command uses), and then possibly change that method to include any extra information you need? I already see the DefinitionData line when i do a "image dump clang ast", so it's possible you wouldn't need to change anything there...

FTR, this is what i get from image dump ast:

Dumping clang ast for 1 modules.
TranslationUnitDecl 0x561fa5fd9128 <<invalid sloc>> <invalid sloc> <undeserialized declarations>
`-NamespaceDecl 0x561fa5fd99e8 <<invalid sloc>> <invalid sloc> Q
  `-CXXRecordDecl 0x561fa5fd9a70 <<invalid sloc>> <invalid sloc> struct A definition
    |-DefinitionData pass_in_registers empty standard_layout trivially_copyable has_user_declared_ctor can_const_default_init
    | |-DefaultConstructor exists non_trivial user_provided defaulted_is_constexpr
    | |-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
    `-CXXConstructorDecl 0x561fa5fd9bf0 <<invalid sloc>> <invalid sloc> A 'void ()'

Using this approach for this simple example:

using ULongArrayTypedef = unsigned long[10];

int main() {
    ULongArrayTypedef *p;
}

I only obtain:

TranslationUnitDecl 0x7fd5eb023608 <<invalid sloc>> <invalid sloc> <undeserialized declarations>

Vs this output for what I have now:

ULongArrayTypedef *
PointerType 0x7fea0a825080 'unsigned long (*)[10]'
`-ConstantArrayType 0x7fea0a824ed0 'unsigned long [10]' 10 
  `-BuiltinType 0x7fea0a8247c0 'unsigned long'
int
BuiltinType 0x7fea0a824700 'int'
ULongArrayTypedef
TypedefDecl 0x7fea0a825000 <<invalid sloc>> <invalid sloc> ULongArrayTypedef 'unsigned long [10]'
`-ConstantArrayType 0x7fea0a824ed0 'unsigned long [10]' 10 
  `-BuiltinType 0x7fea0a8247c0 'unsigned long'
long unsigned int
BuiltinType 0x7fea0a8247c0 'unsigned long'
unsigned long [10]
ConstantArrayType 0x7fea0a824ed0 'unsigned long [10]' 10 
`-BuiltinType 0x7fea0a8247c0 'unsigned long'
main
FunctionProtoType 0x7fea0a824f10 'int (void)' cdecl
`-BuiltinType 0x7fea0a824700 'int'

I believe this is due to us being lazy as to when we import.

[shafik]

source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp

3077 ↗

(On Diff #221594)

Apologies, I did not clean up all the these up.

[shafik]

tools/lldb-test/lldb-test.cpp

552–579 ↗

(On Diff #221594)

If we do stick with this approach pushing the DumpAST(...) into TypeSystem seems reasonable.

[labath]

In D67994#1683440, @shafik wrote:

I believe this is due to us being lazy as to when we import.

Yes, but doesn't calling Module::ParseAllDebugSymbols force us to parse everything? "image dump ast" does dump only the things that have been parsed, but that doesn't mean it lldb-test needs to do that too.

IOW, I was not saying you should use "image dump ast" to write the test you wanted to write. I was merely saying that we should try to make "lldb-test -dump-ast" use the same dumping code as "image dump ast" does (assuming the latter outputs the kind of data that you need, but it seems to me that it does...). "image dump ast" can remain lazy, and only show the things that have been parsed so far (which is also useful sometimes), while "lldb-test" can do whatever it takes to parse everything (I would hope that is merely calling Module::ParseAllDebugSymbols).

[shafik]

• Updated approach based on comment
• Pushed clang ast manipulation into ClangASTContext.cpp
• Created a new option in lldb-test to keep the new functionality separate from the old use called dumpClangAST
• Cleaned up checking DWARF type by using isType()

[shafik]

In D67994#1683934, @labath wrote:

In D67994#1683440, @shafik wrote:

I believe this is due to us being lazy as to when we import.

Yes, but doesn't calling Module::ParseAllDebugSymbols force us to parse everything? "image dump ast" does dump only the things that have been parsed, but that doesn't mean it lldb-test needs to do that too.

IOW, I was not saying you should use "image dump ast" to write the test you wanted to write. I was merely saying that we should try to make "lldb-test -dump-ast" use the same dumping code as "image dump ast" does (assuming the latter outputs the kind of data that you need, but it seems to me that it does...). "image dump ast" can remain lazy, and only show the things that have been parsed so far (which is also useful sometimes), while "lldb-test" can do whatever it takes to parse everything (I would hope that is merely calling Module::ParseAllDebugSymbols).

In order to reuse the functionality we would need to import the complete clang ASTs to the scratch AST context and that would end up being a lot more work then the current approach. So I stuck with the current approach.

[shafik]

I ended up splitting out the functionality into a new method dumpClangAST it looks PDB is not as lazy as DWARF and there are several PDB tests already using the current dumpAST as it is.

tools/lldb-test/lldb-test.cpp

552–579 ↗

(On Diff #221594)

It looked like pushing this into ClangASTContext made more sense. I am happy to bikeshed naming though.

[aprantl]

include/lldb/Symbol/ClangASTContext.h

895 ↗

(On Diff #222861)

nit: . at the end

896 ↗

(On Diff #222861)

What is "the symbol table" in this context? Does ClangASTContext have access to one?

source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp

3059 ↗

(On Diff #222861)

comment explaining why not subrange type?

source/Symbol/ClangASTContext.cpp

9166 ↗

(On Diff #222861)

Ah .. so should it be called DumpFromSymbolFile() or LookupAndDump()?

9173 ↗

(On Diff #222861)

This API with the return value doesn't exit any more.

9182 ↗

(On Diff #222861)

the AsCString() part seems to be bad for the performance.. is there a variant that returns a StringRef instead?

9184 ↗

(On Diff #222861)

A switch over the kind would be more efficient and potentially nicer to read?

9195 ↗

(On Diff #222861)

is an EnumDecl not a TagDecl?

[shafik]

Updates based on comments:

• Remove use of AsCString()
• Fixed use of old API
• other minor issues

[labath]

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

[labath]

In D67994#1691641, @shafik wrote:

In D67994#1683934, @labath wrote:

IOW, I was not saying you should use "image dump ast" to write the test you wanted to write. I was merely saying that we should try to make "lldb-test -dump-ast" use the same dumping code as "image dump ast" does (assuming the latter outputs the kind of data that you need, but it seems to me that it does...). "image dump ast" can remain lazy, and only show the things that have been parsed so far (which is also useful sometimes), while "lldb-test" can do whatever it takes to parse everything (I would hope that is merely calling Module::ParseAllDebugSymbols).

In order to reuse the functionality we would need to import the complete clang ASTs to the scratch AST context and that would end up being a lot more work then the current approach. So I stuck with the current approach.

I am sorry, all of these ast contexts are still fairly confusing to me, so I am not sure what this actually means. Can you elaborate?

[shafik]

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

include/lldb/Symbol/ClangASTContext.h

896 ↗

(On Diff #222861)

It comes from`GetSymbolFile() which is inherited from TypeSystem`

source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp

3059 ↗

(On Diff #222861)

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.

source/Symbol/ClangASTContext.cpp

9184 ↗

(On Diff #222861)

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.

9195 ↗

(On Diff #222861)

I don't understand the comment, TagDecl is handled above.

[teemperor]

source/Symbol/ClangASTContext.cpp

9195 ↗

(On Diff #222861)

I think Adrian's point is that EnumDecl is a subclass of TagDecl.

[labath]

In D67994#1695172, @shafik wrote:

In 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)?

[shafik]

In D67994#1697981, @labath wrote:

In D67994#1695172, @shafik wrote:

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

[labath]

My hope was that something like -find=variable -dump-clang-ast would make sense at least from a theoretical perspective. However, after reading to source code, it looks like that may not be true as (I think) we don't actually create ast nodes for variables. So, making this option a sort of a "top-level" thing may be the best thing to do. Given that now the "symbols" subcommand has a fair set of options, all of which are mutually incompatible (-verify, --dump-ast, -dump-clang-ast, the "default" mode), I think it might make sense to create new subcommands for each of these "modes" ("lldb-test symbols ast") so that they can each get their own options, independently of the others. However, I don't want to hold up other progress over that...

[shafik]

• Simplified DumpFromSymbolFile() based on comments
• Reused -name option in lldb-test and to replace -symbol-name

source/Symbol/ClangASTContext.cpp

9195 ↗

(On Diff #222861)

This is a good point, this also goes for CXXRecordDecl so removed those cases since they are both covered by TagDecl.

lldb/include/lldb/Symbol/ClangASTContext.h

#ifndef NDEBUG
   /// Convenience LLVM-style dump method for use in the debugger only.
   /// In contrast to the other \p Dump() methods this directly invokes
   /// \p clang::QualType::dump().
   LLVM_DUMP_METHOD void dump(lldb::opaque_compiler_type_t type) const override;
 #endif
 
   void Dump(Stream &s);
 
+  /// Dump clang AST types from the symbol file.
+  ///
+  /// \param[in] s
+  ///       A stream to send the dumped AST node(s) to
+  /// \param[in] symbol_name
+  ///       The name of the symbol to dump, if it is empty dump all the symbols
+  void DumpFromSymbolFile(Stream &s, llvm::StringRef symbol_name);
+
   void DumpValue(lldb::opaque_compiler_type_t type, ExecutionContext *exe_ctx,
                  Stream *s, lldb::Format format, const DataExtractor &data,
                  lldb::offset_t data_offset, size_t data_byte_size,
                  uint32_t bitfield_bit_size, uint32_t bitfield_bit_offset,
                  bool show_types, bool show_summary, bool verbose,
                  uint32_t depth) override;
 
   bool DumpTypeValue(lldb::opaque_compiler_type_t type, Stream *s,

lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp

TypeSP SymbolFileDWARF::ParseType(const SymbolContext &sc, const DWARFDIE &die,
   return type_sp;
 }
 
 size_t SymbolFileDWARF::ParseTypes(const SymbolContext &sc,
                                    const DWARFDIE &orig_die,
                                    bool parse_siblings, bool parse_children) {
   size_t types_added = 0;
   DWARFDIE die = orig_die;
+
   while (die) {
+    const dw_tag_t tag = die.Tag();
     bool type_is_new = false;
-    if (ParseType(sc, die, &type_is_new).get()) {
-      if (type_is_new)
-        ++types_added;
-    }
+
+    Tag dwarf_tag = static_cast<Tag>(tag);
+
+    // TODO: Currently ParseTypeFromDWARF(...) which is called by ParseType(...)
+    // does not handle DW_TAG_subrange_type. It is not clear if this is a bug or
+    // not.
+    if (isType(dwarf_tag) && tag != DW_TAG_subrange_type)
+      ParseType(sc, die, &type_is_new);
+
 
     if (parse_children && die.HasChildren()) {
       if (die.Tag() == DW_TAG_subprogram) {
         SymbolContext child_sc(sc);
         child_sc.function = sc.comp_unit->FindFunctionByUID(die.GetID()).get();
         types_added += ParseTypes(child_sc, die.GetFirstChild(), true, true);
       } else
         types_added += ParseTypes(sc, die.GetFirstChild(), true, true);

lldb/source/Symbol/ClangASTContext.cpp

 }
 #endif
 
 void ClangASTContext::Dump(Stream &s) {
   Decl *tu = Decl::castFromDeclContext(GetTranslationUnitDecl());
   tu->dump(s.AsRawOstream());
 }
 
+void ClangASTContext::DumpFromSymbolFile(Stream &s,
+                                         llvm::StringRef symbol_name) {
+  SymbolFile *symfile = GetSymbolFile();
+
+  if (!symfile)
+    return;
+
+  lldb_private::TypeList type_list;
+  symfile->GetTypes(nullptr, eTypeClassAny, type_list);
+  size_t ntypes = type_list.GetSize();
+
+  for (size_t i = 0; i < ntypes; ++i) {
+    TypeSP type = type_list.GetTypeAtIndex(i);
+
+    if (!symbol_name.empty())
+      if (symbol_name.compare(type->GetName().GetStringRef()) != 0)
+        continue;
+
+    s << type->GetName().AsCString() << "\n";
+
+    if (clang::TagDecl *tag_decl =
+                 GetAsTagDecl(type->GetFullCompilerType()))
+      tag_decl->dump(s.AsRawOstream());
+    else if (clang::TypedefNameDecl *typedef_decl =
+                 GetAsTypedefDecl(type->GetFullCompilerType()))
+      typedef_decl->dump(s.AsRawOstream());
+    else {
+      GetCanonicalQualType(type->GetFullCompilerType().GetOpaqueQualType())
+          .dump(s.AsRawOstream());
+    }
+  }
+}
+
 void ClangASTContext::DumpValue(
     lldb::opaque_compiler_type_t type, ExecutionContext *exe_ctx, Stream *s,
     lldb::Format format, const DataExtractor &data,
     lldb::offset_t data_byte_offset, size_t data_byte_size,
     uint32_t bitfield_bit_size, uint32_t bitfield_bit_offset, bool show_types,
     bool show_summary, bool verbose, uint32_t depth) {
   if (!type)
     return;

lldb/tools/lldb-test/lldb-test.cpp

 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/CommandLine.h"
 #include "llvm/Support/ManagedStatic.h"
 #include "llvm/Support/MathExtras.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/PrettyStackTrace.h"
 #include "llvm/Support/Signals.h"
 #include "llvm/Support/WithColor.h"
+
 #include <cstdio>
 #include <thread>
 
 using namespace lldb;
 using namespace lldb_private;
 using namespace llvm;
 
 namespace opts {
static FunctionNameType getFunctionNameFlags() {
   for (FunctionNameType Flag : FunctionNameFlags)
     Result = FunctionNameType(Result | Flag);
   return Result;
 }
 
 static cl::opt<bool> DumpAST("dump-ast",
                              cl::desc("Dump AST restored from symbols."),
                              cl::sub(SymbolsSubcommand));
+static cl::opt<bool>
+    DumpClangAST("dump-clang-ast",
+                 cl::desc("Dump clang AST restored from symbols."),
+                 cl::sub(SymbolsSubcommand));
 
 static cl::opt<bool> Verify("verify", cl::desc("Verify symbol information."),
                             cl::sub(SymbolsSubcommand));
 
 static cl::opt<std::string> File("file",
                                  cl::desc("File (compile unit) to search."),
                                  cl::sub(SymbolsSubcommand));
 static cl::opt<int> Line("line", cl::desc("Line to search."),
                          cl::sub(SymbolsSubcommand));
 
 static Expected<CompilerDeclContext> getDeclContext(SymbolFile &Symfile);
 
 static Error findFunctions(lldb_private::Module &Module);
 static Error findBlocks(lldb_private::Module &Module);
 static Error findNamespaces(lldb_private::Module &Module);
 static Error findTypes(lldb_private::Module &Module);
 static Error findVariables(lldb_private::Module &Module);
 static Error dumpModule(lldb_private::Module &Module);
 static Error dumpAST(lldb_private::Module &Module);
+static Error dumpClangAST(lldb_private::Module &Module);
 static Error verify(lldb_private::Module &Module);
 
 static Expected<Error (*)(lldb_private::Module &)> getAction();
 static int dumpSymbols(Debugger &Dbg);
 } // namespace symbols
 
 namespace irmemorymap {
 static cl::opt<std::string> Target(cl::Positional, cl::desc("<target>"),
Error opts::symbols::dumpModule(lldb_private::Module &Module) {
   Module.Dump(&Stream);
   outs() << Stream.GetData() << "\n";
   return Error::success();
 }
 
 Error opts::symbols::dumpAST(lldb_private::Module &Module) {
   Module.ParseAllDebugSymbols();
 
-  auto symfile = Module.GetSymbolFile();
+  SymbolFile *symfile = Module.GetSymbolFile();
   if (!symfile)
     return make_string_error("Module has no symbol file.");
 
-  auto type_system_or_err =
+  llvm::Expected<TypeSystem &> type_system_or_err =
       symfile->GetTypeSystemForLanguage(eLanguageTypeC_plus_plus);
   if (!type_system_or_err)
     return make_string_error("Can't retrieve ClangASTContext");
 
   auto *clang_ast_ctx =
       llvm::dyn_cast_or_null<ClangASTContext>(&type_system_or_err.get());
   if (!clang_ast_ctx)
     return make_string_error("Retrieved TypeSystem was not a ClangASTContext");
 
   auto ast_ctx = clang_ast_ctx->getASTContext();
   if (!ast_ctx)
     return make_string_error("Can't retrieve AST context.");
 
-  auto tu = ast_ctx->getTranslationUnitDecl();
+  clang::TranslationUnitDecl *tu = ast_ctx->getTranslationUnitDecl();
   if (!tu)
     return make_string_error("Can't retrieve translation unit declaration.");
 
   tu->print(outs());
 
   return Error::success();
 }
 
+Error opts::symbols::dumpClangAST(lldb_private::Module &Module) {
+  Module.ParseAllDebugSymbols();
+
+  SymbolFile *symfile = Module.GetSymbolFile();
+  if (!symfile)
+    return make_string_error("Module has no symbol file.");
+
+  llvm::Expected<TypeSystem &> type_system_or_err =
+      symfile->GetTypeSystemForLanguage(eLanguageTypeC_plus_plus);
+  if (!type_system_or_err)
+    return make_string_error("Can't retrieve ClangASTContext");
+
+  auto *clang_ast_ctx =
+      llvm::dyn_cast_or_null<ClangASTContext>(&type_system_or_err.get());
+  if (!clang_ast_ctx)
+    return make_string_error("Retrieved TypeSystem was not a ClangASTContext");
+
+  StreamString Stream;
+  clang_ast_ctx->DumpFromSymbolFile(Stream, Name);
+  outs() << Stream.GetData() << "\n";
+
+  return Error::success();
+}
+
 Error opts::symbols::verify(lldb_private::Module &Module) {
   SymbolFile *symfile = Module.GetSymbolFile();
   if (!symfile)
     return make_string_error("Module has no symbol file.");
 
   uint32_t comp_units_count = symfile->GetNumCompileUnits();
 
   outs() << "Found " << comp_units_count << " compile units.\n";
if (DumpAST) {
     if (Regex || !Context.empty() || !Name.empty() || !File.empty() ||
         Line != 0)
       return make_string_error(
           "-regex, -context, -name, -file and -line options are not "
           "applicable for dumping AST.");
     return dumpAST;
   }
 
+  if (DumpClangAST) {
+    if (Find != FindType::None)
+      return make_string_error("Cannot both search and dump clang AST.");
+    if (Regex || !Context.empty() || !File.empty() || Line != 0)
+      return make_string_error(
+          "-regex, -context, -name, -file and -line options are not "
+          "applicable for dumping clang AST.");
+    return dumpClangAST;
+  }
+
   if (Regex && !Context.empty())
     return make_string_error(
         "Cannot search using both regular expressions and context.");
 
   if (Regex && !RegularExpression(Name).IsValid())
     return make_string_error("`{0}` is not a valid regular expression.", Name);
 
   if (Regex + !Context.empty() + !File.empty() >= 2)