Page MenuHomePhabricator

Print discriminators when printing .debug_line in GNU style.
ClosedPublic

Authored by saugustine on Jan 23 2020, 5:56 PM.

Details

Summary

gnu addr2line prints DWARF line table discriminators like so:

<file>:<line> (discriminator <Number>)

This matches that behavior.

Add test for new GNU-style discriminator printing.

Event Timeline

saugustine created this revision.Jan 23 2020, 5:56 PM

Is there a particular reason to restrict this to GNU style? I think it's okay to expand on the LLVM style too, but might be worth getting more opinions on it.

llvm/test/tools/llvm-symbolizer/output-style-discriminator.test
1 ↗(On Diff #240076)

Could you add a leading '#' character here, (i.e. '# Check') to more clearly show that this is a comment. It helps improve readability.

I'd also rename this test to simply 'discriminator.test', and test both output styles' behaviour.

3 ↗(On Diff #240076)

This is very much a personal nit of mine, but I know others have agreed in the past: please use consistently double-dashes for long options, and single-dashes for single-letter options (in other words "-obj" -> "--obj").

saugustine marked 2 inline comments as done.
  • Rename and minor cleanup.

Is there a particular reason to restrict this to GNU style? I think it's okay to expand on the LLVM style too, but might be worth getting more opinions on it.

LLVM output style already prints the column number (file:line:column), which tells you the same thing as the discriminator, but at a higher level that is more friendly for users. I'm open to adding it, but it seems a bit redundant.

Otherwise updated.

rupprecht accepted this revision.Jan 24 2020, 1:38 PM

Is there a particular reason to restrict this to GNU style? I think it's okay to expand on the LLVM style too, but might be worth getting more opinions on it.

LLVM output style already prints the column number (file:line:column), which tells you the same thing as the discriminator, but at a higher level that is more friendly for users. I'm open to adding it, but it seems a bit redundant.

Otherwise updated.

So it would print:

foo
/tmp/discrim.c:5:17 (discriminator 2)

It doesn't seem *that* redundant so I'd also consider adding it for llvm style too. But I don't particularly need it.
FWIW, --verbose mode also includes discriminator for both LLVM and GNU style.

This revision is now accepted and ready to land.Jan 24 2020, 1:38 PM

Column information seems like enough to me for now. If we find a use-case for discriminator information as well as column info, then I'm open to adding it in the future though.

llvm/test/tools/llvm-symbolizer/discriminator.test
4

Let's add a test-case to show that LLVM style doesn't print the discriminator. Something like:

RUN: llvm-symbolizer --output-style=GNU -f --obj=%p/Inputs/discrim 0x400590 0x400575 | \
RUN:   FileCheck %s --check-prefix=LLVM --implicit-check-not="(discriminator"
CHECK: /tmp/discrim.c:5:12{{$}}
  • Check both output styles for proper discriminator or column printing.

Okay code and test looks good. Just needs a documentation change to go with it. I recommend updating the description about column numbers in llvm-symbolizer's command guide's --output-style description to mention the discriminators, ideally with an update to the corresponding example too. (The examples in the guide all use the source at the top of the file).

saugustine marked 2 inline comments as done.
  • Document how and when --output-style=GNU prints discriminators

Is there a particular reason to restrict this to GNU style? I think it's okay to expand on the LLVM style too, but might be worth getting more opinions on it.

LLVM output style already prints the column number (file:line:column), which tells you the same thing as the discriminator, but at a higher level that is more friendly for users. I'm open to adding it, but it seems a bit redundant.

Otherwise updated.

  • Document how and when --output-style=GNU prints discriminators

I debated whether or not to add the -fdebug-info-for-profiling option to the top compile commands, but it seemed to distract from the main point. Let me know if this is acceptable.

LGTM, with the suggested doc changes.

llvm/docs/CommandGuide/llvm-symbolizer.rst
230 ↗(On Diff #240953)

I might replace "dwarf" with "debug data" for more generality. I might also change the second sentence to be a little more general (the "only" feels like it could rot easily). How about "One way to produce discriminators is to compile with clang's -fdebug-info-for-profiling"?

252 ↗(On Diff #240953)

Change this path to something more generic like /tmp/test.cpp

saugustine marked 2 inline comments as done.
  • Tighten wording, use generic path.
This revision was automatically updated to reflect the committed changes.