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

[saugustine]

Summary

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

[MaskRay]

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.

[saugustine]

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.

[saugustine]

In D76733#1940199, @MaskRay wrote:

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.

[saugustine]

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

[jhenderson]

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.

319–324

This "pick last" behaviour needs testing.

[saugustine]

Add documentation and otherwise address upstream comments.

[saugustine]

Add missing file.

[saugustine]

clang-format fix

[jhenderson]

llvm/docs/CommandGuide/llvm-symbolizer.rst

154 ↗

(On Diff #252997)

paths -> path

171 ↗

(On Diff #252997)

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]

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]

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

[saugustine]

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]

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.

[jhenderson]

LGTM, with a nit.

llvm/docs/CommandGuide/llvm-symbolizer.rst

154 ↗

(On Diff #253739)

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.

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

+# REQUIRES: x86-registered-target
+
+foo:
+    nop
+
+# RUN: llvm-mc --filetype=obj --triple=x86_64-pc-linux %s -o %t.o -g
+# RUN: llvm-symbolizer 0 --relativenames --obj=%t.o | FileCheck %s -DDIR=%p
+# RUN: llvm-symbolizer 0 -r --obj=%t.o | FileCheck %s -DDIR=%p
+
+# CHECK: [[DIR]]{{\\|/}}relativenames.s:4

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

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));
 
+// -relativenames, -r
+static cl::opt<bool> ClRelativenames("relativenames", cl::init(false),
+                                     cl::desc("Strip the compilation directory from paths"));
+static cl::alias ClRelativenamesShort("r", cl::desc("Alias for -relativenames"),
+                                  cl::NotHidden, cl::aliasopt(ClRelativenames));
+
 // -demangle, -C, -no-demangle
 static cl::opt<bool>
 ClDemangle("demangle", cl::init(true), cl::desc("Demangle function names"));
 static cl::alias
 ClDemangleShort("C", cl::desc("Alias for -demangle"),
                 cl::NotHidden, cl::aliasopt(ClDemangle), cl::Grouping);
 static cl::opt<bool>
 ClNoDemangle("no-demangle", cl::init(false),
int main(int argc, char **argv) {
   Opts.Demangle = ClDemangle;
   Opts.RelativeAddresses = ClUseRelativeAddress;
   Opts.UntagAddresses = ClUntagAddresses;
   Opts.DefaultArch = ClDefaultArch;
   Opts.FallbackDebugPath = ClFallbackDebugPath;
   Opts.DWPName = ClDwpName;
   Opts.DebugFileDirectory = ClDebugFileDirectory;
   Opts.PathStyle = DILineInfoSpecifier::FileLineInfoKind::AbsoluteFilePath;
-  if (ClBasenames)
+  // If both --basenames and --relativenames are specified then pick the last one.
+  if (ClBasenames.getPosition() > ClRelativenames.getPosition())
     Opts.PathStyle = DILineInfoSpecifier::FileLineInfoKind::BaseNameOnly;
+  else if (ClRelativenames)
+    Opts.PathStyle = DILineInfoSpecifier::FileLineInfoKind::RelativeFilePath;
 
   for (const auto &hint : ClDsymHint) {
     if (sys::path::extension(hint) == ".dSYM") {
       Opts.DsymHints.push_back(hint);
     } else {
       errs() << "Warning: invalid dSYM hint: \"" << hint <<
                 "\" (must have the '.dSYM' extension).\n";
     }
   }