This is an archive of the discontinued LLVM Phabricator instance.

Colored syntax highlighting for llvm-dwarfdump.
ClosedPublic

Authored by aprantl on Jan 5 2015, 3:38 PM.

Details

Summary

While waiting for clang to compile (http://xkcd.com/303/) I started hacking on the text output of llvm-dwarfdump, specifically I added colored syntax highlighting. The color scheme is the one that Greg's colorize script for dwarfdump on Darwin is using.

This patch starts by colorizing the .debug_info output. Adding more sections and syntax elements should be trivial.

Do we want this?

Diff Detail

Event Timeline

aprantl updated this revision to Diff 17817.Jan 5 2015, 3:38 PM
aprantl retitled this revision from to Colored syntax highlighting for llvm-dwarfdump..
aprantl updated this object.
aprantl edited the test plan for this revision. (Show Details)
aprantl added reviewers: friss, echristo, dblaikie.
aprantl set the repository for this revision to rL LLVM.
aprantl added a subscriber: Unknown Object (MLST).
echristo edited edge metadata.Jan 5 2015, 3:41 PM

Definitely want it.

Two things:

a) Might want to make it an option?
b) Formatting for the SyntaxHighlighting.h file.

Is there any way we can piggy back more off the existing color support? (Not that this is complicated and I've not looked at it at all)

-eric

Definitely want it.

Two things:

a) Might want to make it an option?

With this implementation colorizing is depending on the terminal type: Invoking llvm-dwarfdump results in color output, whereas llvm-dwarfdump | less will be monochrome.
The frontend provides an -fdiagnostics-color option that is on by default. We could do something similar here; If we do this; it should override the terminal detection, such that llvm-dwarfdump --color | less produces color output no matter what.

b) Formatting for the SyntaxHighlighting.h file.

Is there any way we can piggy back more off the existing color support? (Not that this is complicated and I've not looked at it at all)

Is there more/better color support somewhere that we could re-use? I only looked at the AST dumper in clang so far, which just hardcodes the colors.

friss added inline comments.Jan 5 2015, 4:23 PM
lib/DebugInfo/DWARFDebugInfoEntry.cpp
44–47

I'm not too fond of all the extra scopes, dunno what others think.

If this could be implemented too look like:

WithColor(OS, syntax::Address) << format("\n0x%8.8x: ", Offset);

I think I'd prefer that (not sure if just adding a conversion operator from WithColor to raw_ostream would do the trick).

aprantl updated this revision to Diff 17819.Jan 5 2015, 4:45 PM
aprantl edited edge metadata.
aprantl removed rL LLVM as the repository for this revision.
  • Ran everything through clang-format (with the exception of the switch statement, which becomes unreadable).
  • A new --color option can be used to forcibly turn color on or off. Default is autodetect.
  • adrian
echristo accepted this revision.Jan 5 2015, 4:48 PM
echristo edited edge metadata.

I'm fine with this or with what Frederic is suggesting :)

This revision is now accepted and ready to land.Jan 5 2015, 4:48 PM
aprantl updated this revision to Diff 17820.Jan 5 2015, 5:13 PM
aprantl edited edge metadata.

thanks, Eric!

@Fred: this is the closest I could get it so far. I think this is still better than the previous version.
Bonus points for the person who gets rid of the get() function!

aprantl updated this revision to Diff 17821.Jan 5 2015, 5:18 PM

got rid of a (now) unnecessary lexical scope.

friss accepted this revision.Jan 5 2015, 5:28 PM
friss edited edge metadata.

Yes, I like it much better. You already have Eric's sign-off anyway, but here are a couple of additional suggestions if you like:

lib/DebugInfo/DWARFDebugInfoEntry.cpp
153–156

Could you write this:

WithColor(OS, Color).get << Name;

and have Color be a variable that you initialize to Enumerator and reset to String if we get Name from the linetable? This way you don't duplicate the {decl,call}_file condtiion with the if above.

lib/DebugInfo/DWARFFormValue.cpp
487–492

You don't need the extra scope here.

aprantl closed this revision.Aug 18 2015, 10:42 AM