This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by dyung on Jan 22 2019, 8:12 PM.

Details

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.

Diff Detail

Repository
rL LLVM

Event Timeline

dyung created this revision.Jan 22 2019, 8:12 PM
jhenderson added inline comments.Jan 23 2019, 1:31 AM
test/tools/llvm-symbolizer/coff-dwarf.test
8 ↗(On Diff #183034)

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

Ditto.

tools/llvm-symbolizer/llvm-symbolizer.cpp
58 ↗(On Diff #183034)

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

dyung updated this revision to Diff 183069.Jan 23 2019, 2:42 AM

Update review incorporating provided feedback.

  1. Changed --i options to -i in tests
  2. Added cl::Grouping option to -i.
dyung marked 3 inline comments as done.Jan 23 2019, 2:43 AM

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 updated this revision to Diff 183072.Jan 23 2019, 3:19 AM

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

jhenderson added inline comments.Jan 23 2019, 3:25 AM
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.

@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 updated this revision to Diff 183074.Jan 23 2019, 3:30 AM

Update test case as suggested.

dyung marked an inline comment as done.Jan 23 2019, 3:31 AM
This revision is now accepted and ready to land.Jan 23 2019, 3:33 AM

I think we need to update docs as well.

dyung updated this revision to Diff 183082.Jan 23 2019, 3:55 AM

Update documentation for llvm-symbolizers with new switches.

Quolyk added inline comments.Jan 23 2019, 4:07 AM
test/tools/llvm-symbolizer/sym.test
26 ↗(On Diff #183082)

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 added inline comments.Jan 23 2019, 4:34 AM
test/tools/llvm-symbolizer/sym.test
26 ↗(On Diff #183082)

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 accepted this revision.Jan 23 2019, 4:40 AM

LGTM.

ruiu accepted this revision.Jan 23 2019, 9:23 AM

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?

This revision was automatically updated to reflect the committed changes.
pcc added a subscriber: pcc.Jan 23 2019, 4:44 PM

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?

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 added a comment.Jan 24 2019, 11:39 AM
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.

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.