llvm-symbolizer: Add support for -i and -inlines as aliases for -inlining

[dyung]

Summary

This change adds to options, -i and -inlines as aliases for the -inlining option to llvm-symbolizer to improve compatibility with the GNU addr2line utility which accepts these options.

It also modifies existing tests that use -inlining to exercise these new aliases as well.

This fixes PR40073.

[jhenderson]

test/tools/llvm-symbolizer/coff-dwarf.test

8

I don't think we want to be testing --i. GNU addr2line's help text only lists -i.

test/tools/llvm-symbolizer/coff-exports.test

8

Ditto.

tools/llvm-symbolizer/llvm-symbolizer.cpp

58

Please add cl::Grouping here and update the test in D57046 when that lands.

[dyung]

Update review incorporating provided feedback.

1. Changed --i options to -i in tests
2. Added cl::Grouping option to -i.

[jhenderson]

Thanks. D57046 has landed as rL351936, and added the test "test/tools/llvm-symbolizer/flag-grouping.test" that can be updated as part of this change.

[dyung]

Added -i/-inlines tests to newly added test flag-grouping.test.

[jhenderson]

test/tools/llvm-symbolizer/flag-grouping.test

4 ↗

(On Diff #183072)

You don't need the other test cases, only this one. You can delete the rest, including the original one.

[jhenderson]

@Quolyk or @ruiu, would one of you mind taking a quick look at this too? I don't see there being any issues, but I don't want to approve it with only two Sony employees looking at it (i.e. myself and @dyung), just in case that breaks some sort of unwritten community rule!

[dyung]

Update test case as suggested.

[jhenderson]

LGTM.

[Quolyk]

I think we need to update docs as well.

[dyung]

Update documentation for llvm-symbolizers with new switches.

[Quolyk]

test/tools/llvm-symbolizer/sym.test

26

One more nit. This file contains redundant tests. coff-exports.test tests are already covered in coff-dwarf.test. On the one hand the more tests the better, on the other hand they test the same thing. @jhenderson is more confident, I suggest him to clarify this. Probably you have opinion on that.

[jhenderson]

test/tools/llvm-symbolizer/sym.test

26

I'm not too bothered by this either way. In an ideal world, we would actually have a test dedicated to --inlining and its aliases, rather than adding them here, but I don't know if there's a huge benefit to doing that. @dyung, if you're happy enough writing such a test, it might be worthwhile.

[Quolyk]

LGTM.

[ruiu]

I don't think that there's an unwritten rule that a person from the same company cannot approve coworkers' patches, but anyways, I wonder where --inlining option came from. This is not a new option, but GNU addr2line doesn't seem to take the option. At least, in their manual, there's no description about --inlining; the tool only accepts -i and --inlines. Maybe we should remove --inlining from llvm-symbolizer if it is doable?

[pcc]

llvm-symbolizer has a totally different interface to addr2line. Does it really make sense to make llvm-symbolizer similar in this way or should there be a separate tool that tries to be compatible with addr2line?

[jhenderson]

In D57083#1368695, @pcc wrote:

llvm-symbolizer has a totally different interface to addr2line. Does it really make sense to make llvm-symbolizer similar in this way or should there be a separate tool that tries to be compatible with addr2line?

I'm not sure it's that dissimilar is it? Following the recent changes, I think it is pretty close to a drop-in replacement. What differences are you concerned about in particular?

[pcc]

In D57083#1369028, @jhenderson wrote:

In D57083#1368695, @pcc wrote:

llvm-symbolizer has a totally different interface to addr2line. Does it really make sense to make llvm-symbolizer similar in this way or should there be a separate tool that tries to be compatible with addr2line?

I'm not sure it's that dissimilar is it? Following the recent changes, I think it is pretty close to a drop-in replacement. What differences are you concerned about in particular?

Okay, I didn't see the most recent changes.

I think my main concern is the difference in output format. For llvm-symbolizer we have:

$ bin/llvm-symbolizer -e ../d/bin/llvm-tblgen 0x7dd050 
main
/usr/local/google/home/pcc/l/d/../llvm/utils/TableGen/TableGen.cpp:246:0

While for addr2line we have:

$ addr2line -e ../d/bin/llvm-tblgen 0x7dd050 
/usr/local/google/home/pcc/l/d/../llvm/utils/TableGen/TableGen.cpp:246

The sanitizers (and various other tools, e.g. https://chromium-review.googlesource.com/c/android_ndk/+/952475 ) depend on the current default llvm-symbolizer output format, and presumably users of addr2line depend on the default output format of that tool as well. So it makes much more sense to me for the addr2line compatibility features to be either part of a separate tool or be busyboxed into the current llvm-symbolizer binary, similar to llvm-ar/llvm-lib.

[jhenderson]

In D57083#1369833, @pcc wrote:

I think my main concern is the difference in output format. For llvm-symbolizer we have:

$ bin/llvm-symbolizer -e ../d/bin/llvm-tblgen 0x7dd050 
main
/usr/local/google/home/pcc/l/d/../llvm/utils/TableGen/TableGen.cpp:246:0

While for addr2line we have:

$ addr2line -e ../d/bin/llvm-tblgen 0x7dd050 
/usr/local/google/home/pcc/l/d/../llvm/utils/TableGen/TableGen.cpp:246

The sanitizers (and various other tools, e.g. https://chromium-review.googlesource.com/c/android_ndk/+/952475 ) depend on the current default llvm-symbolizer output format, and presumably users of addr2line depend on the default output format of that tool as well. So it makes much more sense to me for the addr2line compatibility features to be either part of a separate tool or be busyboxed into the current llvm-symbolizer binary, similar to llvm-ar/llvm-lib.

FWIW, ignoring the column index, you can get the same output between the two by adding --functions to addr2line or --functions=none to llvm-symbolizer. This appears to be a historical difference that has existed ever since llvm-symbolizer was created. I did some initial investigation of this in https://bugs.llvm.org/show_bug.cgi?id=40072#c2, with my conclusion being that we need a wider discussion on this topic at a later date. I've actually submitted a BoF proposal for Euro LLVM for discussing GNU compatibility of LLVM binary tools like llvm-symbolizer, though I don't yet know if it's been accepted.

test/tools/llvm-symbolizer/coff-dwarf.test

 RUN: grep '^ADDR:' %s | sed -s 's/ADDR: //' \
 RUN:     | llvm-symbolizer --inlining --relative-address -obj="%p/Inputs/coff-dwarf.exe" \
 RUN:     | FileCheck %s
+RUN: grep '^ADDR:' %s | sed -s 's/ADDR: //' \
+RUN:     | llvm-symbolizer --inlines --relative-address -obj="%p/Inputs/coff-dwarf.exe" \
+RUN:     | FileCheck %s
+RUN: grep '^ADDR:' %s | sed -s 's/ADDR: //' \
+RUN:     | llvm-symbolizer -i --relative-address -obj="%p/Inputs/coff-dwarf.exe" \
+RUN:     | FileCheck %s
 
 This test relies on UnDecorateSymbolName, which is Windows-only.
 REQUIRES: target-windows, system-windows
 
 ADDR: 0x5009
 ADDR: 0x5038
 
 CHECK: foo(void)
 CHECK: coff-dwarf.cpp:7
 CHECK: bar(void)
 CHECK: coff-dwarf.cpp:13
 CHECK: main
 CHECK: coff-dwarf.cpp:17

test/tools/llvm-symbolizer/coff-exports.test

 RUN: grep '^ADDR:' %s | sed -s 's/ADDR: //' \
 RUN:   | llvm-symbolizer --inlining --relative-address -obj="%p/Inputs/coff-exports.exe" \
 RUN:   | FileCheck %s
+RUN: grep '^ADDR:' %s | sed -s 's/ADDR: //' \
+RUN:   | llvm-symbolizer --inlines --relative-address -obj="%p/Inputs/coff-exports.exe" \
+RUN:   | FileCheck %s
+RUN: grep '^ADDR:' %s | sed -s 's/ADDR: //' \
+RUN:   | llvm-symbolizer -i --relative-address -obj="%p/Inputs/coff-exports.exe" \
+RUN:   | FileCheck %s
 
 This test relies on UnDecorateSymbolName, which is Win32-only.
 REQUIRES: system-windows
 REQUIRES: target-windows
 FIXME: This test depends on host, not target.
 
 ADDR: 0x500A
 ADDR: 0x5038

test/tools/llvm-symbolizer/sym.test

 #
 #Build as : clang -g -O2 addr.c
 
 RUN: llvm-symbolizer -print-address -obj=%p/Inputs/addr.exe < %p/Inputs/addr.inp | FileCheck %s
 RUN: llvm-symbolizer -addresses -obj=%p/Inputs/addr.exe < %p/Inputs/addr.inp | FileCheck %s
 RUN: llvm-symbolizer -a -obj=%p/Inputs/addr.exe < %p/Inputs/addr.inp | FileCheck %s
 RUN: llvm-symbolizer -inlining -print-address -pretty-print -obj=%p/Inputs/addr.exe < %p/Inputs/addr.inp | FileCheck -check-prefix="PRETTY" %s 
 RUN: llvm-symbolizer -inlining -print-address -p -obj=%p/Inputs/addr.exe < %p/Inputs/addr.inp | FileCheck -check-prefix="PRETTY" %s
+RUN: llvm-symbolizer -inlines -print-address -pretty-print -obj=%p/Inputs/addr.exe < %p/Inputs/addr.inp | FileCheck -check-prefix="PRETTY" %s 
+RUN: llvm-symbolizer -inlines -print-address -p -obj=%p/Inputs/addr.exe < %p/Inputs/addr.inp | FileCheck -check-prefix="PRETTY" %s
+RUN: llvm-symbolizer -i -print-address -pretty-print -obj=%p/Inputs/addr.exe < %p/Inputs/addr.inp | FileCheck -check-prefix="PRETTY" %s 
+RUN: llvm-symbolizer -i -print-address -p -obj=%p/Inputs/addr.exe < %p/Inputs/addr.inp | FileCheck -check-prefix="PRETTY" %s
 RUN: echo "0x1" > %t.input
 RUN: llvm-symbolizer -obj=%p/Inputs/zero < %t.input | FileCheck -check-prefix="ZERO" %s
 
 #CHECK: some text
 #CHECK: 0x40054d
 #CHECK: main
 #CHECK: {{[/\]+}}tmp{{[/\]+}}x.c:14:0
 #CHECK: some text2

tools/llvm-symbolizer/llvm-symbolizer.cpp

 static cl::opt<bool>
     ClUseRelativeAddress("relative-address", cl::init(false),
                          cl::desc("Interpret addresses as relative addresses"),
                          cl::ReallyHidden);
 
 static cl::opt<bool>
     ClPrintInlining("inlining", cl::init(true),
                     cl::desc("Print all inlined frames for a given address"));
+static cl::alias
+    ClPrintInliningAliasI("i", cl::desc("Alias for -inlining"),
+                          cl::NotHidden, cl::aliasopt(ClPrintInlining),
+                          cl::Grouping);
+static cl::alias
+    ClPrintInliningAliasInlines("inlines", cl::desc("Alias for -inlining"),
+                                cl::NotHidden, cl::aliasopt(ClPrintInlining));
 
 // -basenames, -s
 static cl::opt<bool> ClBasenames("basenames", cl::init(false),
                                  cl::desc("Strip directory names from paths"));
 static cl::alias ClBasenamesShort("s", cl::desc("Alias for -basenames"),
                                   cl::NotHidden, cl::aliasopt(ClBasenames));
 
 // -demangle, -C, -no-demangle