Page MenuHomePhabricator

New symbolizer option to print files relative to the compilation directory.
ClosedPublic

Authored by saugustine on Mar 24 2020, 2:38 PM.

Details

Summary

New "--relative" option to allow printing files relative to the compilation directory.

Diff Detail

Event Timeline

saugustine created this revision.Mar 24 2020, 2:38 PM

This change looks good to me. My mere concern is about the option naming. In GNU addr2line, -r has a different meaning, and llvm-symbolizer can be used as llvm-addr2line (it is a crunched executable). If it does not cause too much inconvenience, I will hope we can make llvm-symbolizer and llvm-addr2line have consistent option names.

The clang-format recommendations would change the new options to not match the format of old ones. Not sure the best thing to do there.

This change looks good to me. My mere concern is about the option naming. In GNU addr2line, -r has a different meaning, and llvm-symbolizer can be used as llvm-addr2line (it is a crunched executable). If it does not cause too much inconvenience, I will hope we can make llvm-symbolizer and llvm-addr2line have consistent option names.

What would you choose for a different name? I am open to about anything. I suppose we wouldn't even need a short option at all.

Remove short option. It only existed to be parallel to "-s", but conflicts
with options in gnu addr2line.

With all of our tools with GNU equivalents, we should avoid single letter aliases unless we can get GNU to buy into the new option too. Long names are less of an issue, as it's unlikely we'll end up with a clash.

Please add the option to the llvm-symbolizer and llvm-addr2line docs.

Could you post a quick output comparison please for with and without this option? I'm finding it hard to properly visualise the behaviour from the test.

llvm/tools/llvm-symbolizer/llvm-symbolizer.cpp
81

clang-format appears to be complaining.

83

No need for this comment.

318–324

This "pick last" behaviour needs testing.

saugustine marked 3 inline comments as done.

Add documentation and otherwise address upstream comments.

Add missing file.

clang-format fix

jhenderson added inline comments.Mar 30 2020, 12:57 AM
llvm/docs/CommandGuide/llvm-symbolizer.rst
154

paths -> path

171

Nit: only one blank line, please.

llvm/test/tools/llvm-symbolizer/relativenames.s
9

Something I'm trying to encourage others to do is use '##' for comments, to distinguish them from lit and FileCheck directives.

Also, please add a blank line before this comment to separate it from the regular test case.

13

No need for the -DDIR option here.

15

I'd explicit make this prefix "RELATIVE" to help contrast the last-one-wins cases.

16

This doesn't actually show that the basenames won: by default, check patterns looks for substrings that match, not just full lines (see FileCheck --match-full-lines). You'll want to add at the start the start-of-line regex: {{^}}.

MaskRay added inline comments.Mar 30 2020, 3:50 PM
llvm/tools/llvm-symbolizer/llvm-symbolizer.cpp
80

Double-dashed --relativenames may be better (and is the one in the documentation and --help output)

saugustine marked 7 inline comments as done.

Thanks for the comments, I think this catches them all.

saugustine marked an inline comment as done.Mar 30 2020, 4:16 PM

updated for all comments.

llvm/tools/llvm-symbolizer/llvm-symbolizer.cpp
80

The code as written allows one to specify it either way. Just like basenames.

MaskRay accepted this revision.Mar 30 2020, 4:53 PM

The code change looks good but please wait for @jhenderson's approval.

llvm/tools/llvm-symbolizer/llvm-symbolizer.cpp
80

Yes, this is how llvm:🆑:ParseCommandLineOptions works unless LongOptionsUseDoubleDash is set to true. This is not ideal, though. For references in comments/documentation we should just use the canonical form: --relativenames.

We might choose to set LongOptionsUseDoubleDash for llvm-addr2line to disambiguate option parsing.

This revision is now accepted and ready to land.Mar 30 2020, 4:53 PM
jhenderson accepted this revision.Mar 31 2020, 12:23 AM

LGTM, with a nit.

llvm/docs/CommandGuide/llvm-symbolizer.rst
154

I think, though I suspect opinions may vary, there should be a comma between "--basenames" and "and", to delimit the list properly.

llvm/tools/llvm-symbolizer/llvm-symbolizer.cpp
80

I'd leave as-is in this patch, since the other options are commented in this same style. Arguably, those comments don't add anything, since the option name is listed in the code, so I'd accept a follow-up to delete the comments (and if you want to them omit it from this line in this patch, that's fine by me), or alternatively update them to use double-dashes instead.

This revision was automatically updated to reflect the committed changes.