Page MenuHomePhabricator

Add -pretty-print option to llvm-symbolizer

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
105 ↗(On Diff #37154)

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

49 ↗(On Diff #37154)

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

164 ↗(On Diff #37154)

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 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
41 ↗(On Diff #39257)

You don't need this now, do you?

27 ↗(On Diff #39257)

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

36 ↗(On Diff #39257)

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

48 ↗(On Diff #39257)

extra semicolon?

55 ↗(On Diff #39257)

Do you need to use printName here?

58 ↗(On Diff #39257)

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
31 ↗(On Diff #39380)

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

36 ↗(On Diff #39380)

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

58 ↗(On Diff #39380)

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

82 ↗(On Diff #39380)

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

113 ↗(On Diff #39713)

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

114 ↗(On Diff #39713)

Grammar: "refer to"

33 ↗(On Diff #39713)

Delimiter (CamelCase)

35 ↗(On Diff #39713)

OS << Prefix << FunctionName << Delimiter;

41 ↗(On Diff #39713)

run this with clang-format.

56–57 ↗(On Diff #39713)

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
35 ↗(On Diff #39843)

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 ↗(On Diff #39843)

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.