This is an archive of the discontinued LLVM Phabricator instance.

[llvm-symbolizer] Allow more flexible usage of -e
ClosedPublic

Authored by ikudrin on Apr 3 2019, 5:18 AM.

Details

Summary

'addr2line' allows -e to be grouped with other options; it also allows it to prefix the value. Thus, all the following usages are possible:

  • addr2line -f -e <bin> <addr>
  • addr2line -fe <bin> <addr>
  • addr2line -f e<bin> <addr>
  • addr2line -fe<bin> <addr>

This patch adds the same for llvm-symbolizer.

Diff Detail

Repository
rL LLVM

Event Timeline

ikudrin created this revision.Apr 3 2019, 5:18 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 3 2019, 5:18 AM

Do you think it's worth testing the behaviour of -e in the middle of a group of options e.g. "-Cefi"?

Do you think it's worth testing the behaviour of -e in the middle of a group of options e.g. "-Cefi"?

It's a negative scenario, right? Haven't we already checked that in CommandLineTest.GroupingWithValue?

Do you think it's worth testing the behaviour of -e in the middle of a group of options e.g. "-Cefi"?

It's a negative scenario, right? Haven't we already checked that in CommandLineTest.GroupingWithValue?

The issue is that that unit test only tests that options with the specific set of properties are handled in that way, but doesn't show that the -e option has those properties. I'm in two minds about it though, because it clearly takes a value. I guess I'm wondering how we know that "-Cefi" is/isn't equivalent to "-C -e=fi" without inspecting the code.

ikudrin updated this revision to Diff 193499.Apr 3 2019, 7:59 AM
ikudrin retitled this revision from [llvm-symbolizer] Add grouping for -e to [llvm-symbolizer] Allow more flexible usage of -e.
ikudrin edited the summary of this revision. (Show Details)

It looks like I've just forgotten to enable a prefix form for the switch. Does that resolve the doubts?

This revision is now accepted and ready to land.Apr 3 2019, 8:50 AM
rupprecht accepted this revision.Apr 3 2019, 9:25 AM
This revision was automatically updated to reflect the committed changes.