Page MenuHomePhabricator

Add a command to dump a module's clang ast.
ClosedPublic

Authored by zturner on Nov 3 2018, 10:19 AM.

Details

Summary

This can be useful when diagnosing AST related problems. For example, I had a bug where I was accidentally creating a record type multiple times instead of re-using the first one. This is easy to see with a dump of the AST, because it will look like this:

TranslationUnitDecl 0x18a2bc53b98 <<invalid sloc>> <invalid sloc>
|-CXXRecordDecl 0x18a2bc54458 <<invalid sloc>> <invalid sloc> class ClassWithPadding
|-CXXRecordDecl 0x18a2bc54520 <<invalid sloc>> <invalid sloc> class ClassWithPadding
|-CXXRecordDecl 0x18a2bc545e0 <<invalid sloc>> <invalid sloc> class ClassWithPadding definition
| |-DefinitionData pass_in_registers standard_layout trivially_copyable 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
| |-MSInheritanceAttr 0x18a2bc546a0 <<invalid sloc>> Implicit __single_inheritance
| |-FieldDecl 0x18a2bc54748 <<invalid sloc>> <invalid sloc> a 'char'
| |-FieldDecl 0x18a2bc54798 <<invalid sloc>> <invalid sloc> b 'short'
| |-FieldDecl 0x18a2bc54828 <<invalid sloc>> <invalid sloc> c 'char [2]'
| |-FieldDecl 0x18a2bc54878 <<invalid sloc>> <invalid sloc> d 'int'
| |-FieldDecl 0x18a2bc548c8 <<invalid sloc>> <invalid sloc> e 'char'
| |-FieldDecl 0x18a2bc54918 <<invalid sloc>> <invalid sloc> f 'int'
| |-FieldDecl 0x18a2bc54968 <<invalid sloc>> <invalid sloc> g 'long long'
| |-FieldDecl 0x18a2bc549f8 <<invalid sloc>> <invalid sloc> h 'char [3]'
| |-FieldDecl 0x18a2bc54a48 <<invalid sloc>> <invalid sloc> i 'long long'
| |-FieldDecl 0x18a2bc54a98 <<invalid sloc>> <invalid sloc> j 'char [2]'
| |-FieldDecl 0x18a2bc54ae8 <<invalid sloc>> <invalid sloc> k 'long long'
| |-FieldDecl 0x18a2bc54b38 <<invalid sloc>> <invalid sloc> l 'char'
| `-FieldDecl 0x18a2bd96f00 <<invalid sloc>> <invalid sloc> m 'long long'
`-<undeserialized declarations>

Note there are 3 CXXRecordDecls with the same name, but only one definition. Given the complex interactions between debug info and AST reconstruction, a command like this makes problems within the AST very obvious. I found several other AST-related problems, so this was not even the only one, so I think this is a largely unexplored front when it comes to areas for potentially improved test coverage. And since it's in the REPL, it makes it very easy to test out commands in different orders, get a dump, do something else, get another dump, etc to see how the order of commands affects things.

Diff Detail

Repository
rL LLVM

Event Timeline

zturner created this revision.Nov 3 2018, 10:19 AM
zturner updated this revision to Diff 172510.Nov 3 2018, 4:54 PM

clang dumps to stderr. Only use color if stderr supports this (e.g. if it's not redirected to a file).

labath added inline comments.Nov 4 2018, 2:23 AM
lldb/source/Commands/CommandObjectTarget.cpp
2275 ↗(On Diff #172510)

The dump function should take the output stream from the result object, so the output ends up where it's expected to go. (clang::DeclBase::dump has an overload which takes a raw_ostream, and lldb streams have a raw_ostream accessor now).

zturner added a subscriber: vsk.Nov 4 2018, 5:18 AM

Unfortunately then color output is impossible. Where else would the output
be expected to go?

zturner added a subscriber: davide.Nov 4 2018, 5:33 AM

Ok so probably this commands output will not go to a log file when logging
is enabled? I can’t think of any other side effects.

Given that the immediate use case for this is interactive investigation and
testing, the first of which is helped by color output and the second of
which doesn’t need logging, what do you think of allowing it as is?

labath added a comment.Nov 4 2018, 5:37 AM

Unfortunately then color output is impossible. Where else would the output
be expected to go?

If you execute the command over the SB API (SBCommandInterpreter::HandleCommand) then it will go into the provided result object. Also, some clients (typically IDEs) use SetInput/Output/ErrorFileHandle to set what is considered to be the default input/output streams for a given debugger instance (typically, to redirect these to some console window).

I don't think this directly precludes colored output, although it may require a bit more plumbing to pass the information whether the final consumer is willing to accept color escape codes. (We can already get that via StreamFile->GetFile().GetIsTerminalWithColors(), so you would just need to somehow pass this information to the proxy raw_ostream you give to the clang dump function.)

The issue is that clang writes directly to stderr

I think Pavel's point is to call the dump overload which allows specifying our own custom raw_ostream: https://clang.llvm.org/doxygen/classclang_1_1Decl.html#a278b3b87b6f9d3b20ed566a8684341a6 And our raw_ostream is configured by LLDB to correctly choose if we want color or not.

I tried that one and it didn’t show color, bit maybe I need to add an
overload of dumpColor that allows to pass a raw_ostream

Yeah it uses the color settings of the diagnostic engine which are probably set to false in LLDB. I think activating colors there should fix the issue.

So I tried to go down this route, but alas, I still can't come up with a good way to make color output work, because the Stream is not a StreamFile object, it is a StreamString object, even when the output is going to a terminal. We could have an argument such as --color but perhaps it should be the default and the option should be --no-color.

So I tried to go down this route, but alas, I still can't come up with a good way to make color output work, because the Stream is not a StreamFile object, it is a StreamString object, even when the output is going to a terminal. We could have an argument such as --color but perhaps it should be the default and the option should be --no-color.

I tried some more, and yea this is really not easy. The only raw_ostream in LLVM that supports color output is the raw_fd_ostream, so we can't even easily set up the forwarder to support any of this. Even if we configure the diagnostics engine to use color output and pass it the forwarder raw_ostream it still won't work, because it will just call raw_ostream::changeColor which does nothing, since that method is only implemented on raw_fd_ostream.

I'm open to suggestions, but it would be a real shame to not be able to take advantage of the built-in colorization here. Perhaps we can make this a hidden or unsupported command.

labath added a comment.Nov 5 2018, 6:21 AM

If it comes down to choosing between colored output going to stderr and plain output going where it should, i'd go with the second option.

The way I'd implement this to support both things is approximately this:

  • add color (has_colors, changeColor and friends) support to Stream class, same as llvm::raw_ostream. Apart from solving this problem, this will make the two classes more similar, making eventual llvm transition smoother.
  • the raw_ostream shim in the Stream class can just forward the relevant operations
  • make the colored-ness of StringStream configurable at construction time. When constructing the StringStream holding the command output, we would look at whether the final output supports colors (Raphael, would that be Debugger.getUseColor()?) or not, and initialize it appropriately.
  • as an alternative to previous step, it might be good to check how can look a

It may be more work than you're ready to put into this right now, but it would be very cool nonetheless, as it would unlock colored output for all other commands (and I can think of a couple who could use that).

The downside here is that this would require duplicating the ansi escape sequence code in raw_fd_ostream. However, I believe we already have a code for handling those, so that can hopefully be reused.

I updated the patch to not dump color. We can address the color issue in a followup. So for now it just goes to the result object's output stream. Perhaps we can add a -c option that ignores the result object's outptu stream (and is documented as doing so), but I'll save that for a followup

This revision was not accepted when it landed; it landed in state Needs Review.Nov 5 2018, 9:43 AM
This revision was automatically updated to reflect the committed changes.