Print discriminators when printing .debug_line in GNU style.

[saugustine]

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.

[jhenderson]

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]

• Rename and minor cleanup.

[saugustine]

In D73318#1838181, @jhenderson wrote:

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]

In D73318#1839196, @saugustine wrote:

In D73318#1838181, @jhenderson wrote:

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.

[jhenderson]

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

3

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{{$}}

[saugustine]

• Check both output styles for proper discriminator or column printing.

[jhenderson]

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]

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

[saugustine]

In D73318#1838181, @jhenderson wrote:

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.

In D73318#1845267, @saugustine wrote:

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

[jhenderson]

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]

• Tighten wording, use generic path.

llvm/lib/DebugInfo/Symbolize/DIPrinter.cpp

void DIPrinter::print(const DILineInfo &Info, bool Inlined) {
   if (Filename == DILineInfo::BadString)
     Filename = DILineInfo::Addr2LineBadString;
   else if (Basenames)
     Filename = llvm::sys::path::filename(Filename);
   if (!Verbose) {
     OS << Filename << ":" << Info.Line;
     if (Style == OutputStyle::LLVM)
       OS << ":" << Info.Column;
+    else if (Style == OutputStyle::GNU && Info.Discriminator != 0)
+      OS << " (discriminator " << Info.Discriminator << ")";
     OS << "\n";
     printContext(Filename, Info.Line);
     return;
   }
   OS << "  Filename: " << Filename << "\n";
   if (Info.StartLine)
     OS << "Function start line: " << Info.StartLine << "\n";
   OS << "  Line: " << Info.Line << "\n";

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

+# Check that llvm-symbolizer prints line-table discriminators properly.
+
+RUN: llvm-symbolizer --output-style=GNU -f --obj=%p/Inputs/discrim 0x400590 0x400575 | FileCheck %s
+
+CHECK: foo
+CHECK: /tmp/discrim.c:5
+CHECK: main
+CHECK: /tmp/discrim.c:10
+CHECK: foo
+CHECK: /tmp/discrim.c:5 (discriminator 2)