Page MenuHomePhabricator

Modify lldb-test to print out ASTs from symbol file
ClosedPublic

Authored by shafik on Sep 24 2019, 3:53 PM.

Details

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.

Diff Detail

Event Timeline

shafik created this revision.Sep 24 2019, 3:53 PM
shafik marked an inline comment as done.Sep 24 2019, 3:55 PM
shafik added inline comments.
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 added inline comments.Sep 24 2019, 4:04 PM
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 added inline comments.Sep 24 2019, 4:12 PM
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 updated this revision to Diff 221622.Sep 24 2019, 4:15 PM
  • Formatting code
  • Removing FunctionDecl case since it was not correct
clayborg added inline comments.
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.

shafik marked 7 inline comments as done.Sep 24 2019, 4:16 PM
aprantl added inline comments.Sep 24 2019, 4:16 PM
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()?

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 ()'

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 marked an inline comment as done.Sep 25 2019, 4:55 PM
shafik added inline comments.
source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
3077 ↗(On Diff #221594)

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

shafik marked an inline comment as done.Sep 25 2019, 4:58 PM
shafik added inline comments.
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.

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 this revision to Diff 222861.Oct 2 2019, 10:22 AM
shafik retitled this revision from [WIP] Modify lldb-test to print out ASTs from symbol file to Modify lldb-test to print out ASTs from symbol file.
  • 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()

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 marked 7 inline comments as done.EditedOct 2 2019, 10:30 AM

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 added inline comments.Oct 2 2019, 10:44 AM
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 updated this revision to Diff 222935.Oct 2 2019, 4:50 PM
shafik marked 14 inline comments as done.

Updates based on comments:

  • Remove use of AsCString()
  • Fixed use of old API
  • other minor issues
labath added a comment.Oct 3 2019, 4:17 AM

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 added a comment.Oct 3 2019, 4:20 AM

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 added a comment.EditedOct 4 2019, 1:06 PM

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 added inline comments.Oct 7 2019, 11:30 AM
source/Symbol/ClangASTContext.cpp
9195 ↗(On Diff #222861)

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

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 added a comment.Oct 8 2019, 4:06 PM

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 accepted this revision.Oct 9 2019, 12:04 PM

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

This revision is now accepted and ready to land.Oct 9 2019, 12:04 PM
shafik updated this revision to Diff 224479.Oct 10 2019, 2:28 PM
shafik marked 2 inline comments as done.
  • 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.

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptOct 11 2019, 9:38 AM