This is an archive of the discontinued LLVM Phabricator instance.

Add -pretty-print option to llvm-symbolizer
ClosedPublic

Authored by khemant on Oct 12 2015, 1:31 PM.

Diff Detail

Event Timeline

khemant updated this revision to Diff 37154.Oct 12 2015, 1:31 PM
khemant retitled this revision from to Add -pretty-print option to llvm-symbolizer.
khemant updated this object.
khemant added reviewers: llvm-commits, samsonov, emaste.
khemant set the repository for this revision to rL LLVM.
samsonov added inline comments.Oct 20 2015, 12:14 PM
docs/CommandGuide/llvm-symbolizer.rst
113

Consider providing more details about the output difference (closer to how -pretty-print is described in "man addr2line"). You can also expand EXAMPLE section.

tools/llvm-symbolizer/LLVMSymbolize.h
49 ↗(On Diff #37154)

Let's keep the same order of members and ctor args.

tools/llvm-symbolizer/llvm-symbolizer.cpp
170–171

You can just use ternary operator here (and above).

khemant updated this revision to Diff 38142.Oct 22 2015, 10:01 AM
samsonov edited edge metadata.Oct 29 2015, 10:25 AM

You will need to update it after r251316. Also see http://reviews.llvm.org/D14099 with a brief plan of future changes - in case you want to hold this off until we factor out DILineInfo / DIInliningInfo rendering into separate class (PrettyPrint should be used there). It's fine if you want to proceed with this earlier, though,

khemant marked 2 inline comments as done.Oct 29 2015, 10:55 AM

I can wait till you re-factor the code. When are you planning to merge the patch that has SymbolizableObjectFile ?

I've changed a bunch of related code recently. Probably now you will need to add this functionality to DIPrinter implementation.

I was looking at it at this very moment.

khemant updated this revision to Diff 39257.Nov 4 2015, 2:30 PM
khemant edited edge metadata.

Changed for new design.

samsonov added inline comments.Nov 4 2015, 3:55 PM
include/llvm/DebugInfo/Symbolize/Symbolize.h
41 ↗(On Diff #39257)

You don't need this now, do you?

lib/DebugInfo/Symbolize/DIPrinter.cpp
27

argument/variable names start with capital letters (here and below)

35

Just make this function print all the data to OS instead of creating a string.

47

extra semicolon?

51–54

Do you need to use printName here?

57

i is always less than FrameNum
This whole block can be just

OS << printName(Info.getFrame(i), i > 0);
khemant updated this revision to Diff 39380.Nov 5 2015, 10:23 AM

Did some clean up.

samsonov added inline comments.Nov 6 2015, 5:12 PM
include/llvm/DebugInfo/Symbolize/DIPrinter.h
31

"InlinedFrame"? (also, CamelCase for variables).

lib/DebugInfo/Symbolize/DIPrinter.cpp
35

I still don't see why you need std::string here - just pass all the stuff you print directly to OS.

56–57

CamelCase. Also, probably you don't need extra var for this.

tools/llvm-symbolizer/llvm-symbolizer.cpp
82

"readable". Also, I would prefer smth. along the lines of Make the output more human friendly.

khemant updated this revision to Diff 39711.Nov 9 2015, 9:43 AM
khemant removed rL LLVM as the repository for this revision.
khemant marked 3 inline comments as done.

Done with the changes.

khemant updated this revision to Diff 39713.Nov 9 2015, 9:49 AM

Looks almost fine, minor remaining comments below.

docs/CommandGuide/llvm-symbolizer.rst
113

You probably need to enclose "-inlining" in double ticks as we do with other flags (check generated file).

114

Grammar: "refer to"

lib/DebugInfo/Symbolize/DIPrinter.cpp
33

Delimiter (CamelCase)

35

OS << Prefix << FunctionName << Delimiter;

41

run this with clang-format.

56–58

Please address

khemant updated this revision to Diff 39843.Nov 10 2015, 12:24 PM
khemant set the repository for this revision to rL LLVM.

Camelcased all variables.

samsonov added inline comments.Nov 10 2015, 2:18 PM
lib/DebugInfo/Symbolize/DIPrinter.cpp
35

Please address. I don't think you need .str calls, or even extra vars:

if (PrintPretty && Inlined)
  OS << " (inlined by) ";
OS << FunctionName;
OS << (PrintPretty ? " at " : "\n");
40–45

Do you still need std::to_string calls?

khemant updated this revision to Diff 39855.Nov 10 2015, 2:52 PM
samsonov accepted this revision.Nov 10 2015, 3:23 PM
samsonov edited edge metadata.

LGTM. Thanks for implementing this, and for patience!

This revision is now accepted and ready to land.Nov 10 2015, 3:23 PM
This revision was automatically updated to reflect the committed changes.