[llvm-objdump] Implement --prefix-strip option

[tinti]

Summary

The option --prefix-strip is only used when --prefix is not empty.
It removes N initial directories from absolute paths before adding the
prefix.

This matches GNU's objdump behavior.

[tinti]

Fix patch does not apply.

[jhenderson]

llvm/test/tools/llvm-objdump/X86/source-interleave-prefix.test

45

51

Looks like there's one too many blank lines at the end of this file?

Also, I think you should have a few additional test cases:

1. A --prefix-strip value of 0.
2. A --prefix-strip value equal to the number of directory components.
3. A --prefix-strip value greater than the number of components.

llvm/tools/llvm-objdump/llvm-objdump.cpp

361

Are you sure you want int? That implies this can take a negative number. Likely you want something like uint32_t...

1042

Perhaps worth an assert that shows that LineInfo.FileName is not empty.

1046–1052

Have you looked at using the path component iterator functionality provided by Path.h to do this instead? It seems like that would be a preferable solution to your hand-rolled version here.

2996–2997

If you don't use int, then this will be sorted by the command-line library itself.

[tinti]

llvm/test/tools/llvm-objdump/X86/source-interleave-prefix.test

51

I agree.

llvm/tools/llvm-objdump/llvm-objdump.cpp

361

I agree.

1042

Yes. I will place it just after is_absolute_gnu.

1046–1052

Yes I have. But it skips extra separators which is not what GNU does.

https://github.com/llvm/llvm-project/blob/004a264f8c923922ecd34255bcb10f4adaa27ac5/llvm/lib/Support/Path.cpp#L267

To make '/foo//bar/baz' become '/baz' --prefix-strip must be 3.

I will add a test for it too.

[tinti]

Add range tests.
Add extra separator test.
Add assert.
Use uint32_t instead of int.
Comment about not using Path.h iterator.

[jhenderson]

llvm/docs/CommandGuide/llvm-objdump.rst

178

Same in the man page.

llvm/test/tools/llvm-objdump/X86/source-interleave-prefix.test

67

I think it would be worth making this and the following cases positive cases (i.e. where the behaviour results in a valid source interleave), if possible. Also, I think adding to this comment what the path before and after the --prefix-strip would be, as it makes it easier to envision what is going on.

72

Perhaps also worth testing a negative value to show that llvm-objdump reports an error in this case.

llvm/tools/llvm-objdump/llvm-objdump.cpp

364

1045–1054

I think you can avoid converting to a C-string here, by doing something like the following, if I'm not mistaken:

auto StrippedNameStart = LineInfo.FileName.begin();
for (auto Pos = LineInfo.FileName.begin() + 1, End = LineInfo.FileName.end(); Pos != End && Level < PrefixStrip; ++Pos) {
  if (sys::path::is_separator(*Pos)) {
    StrippedNameStart = Pos;
    ++Level;
  }
}
LineInfo.FileName = std::string(Post, LineInfo.FileName.end());

[tinti]

Reword documentation.
Use good result test instead of bad result.
Add invalid value test.
Convert C code to C++.

[tinti]

@jhenderson ping?

[jhenderson]

Sorry for the delay. I've been off sick for much of the week, and am still catching up on things.

llvm/test/tools/llvm-objdump/X86/source-interleave-prefix.test

84

Another one that has occurred to me: testing a non-numeric value, e.g. "--prefix-strip=foo" to show an error is hit. Not 100% certain if it's necessary, but it ensures we have "invalid case" handling in place, in case negative numbers are ever accepted for some reason.

[tinti]

Add invalid text test.

[tinti]

In D96679#2605780, @jhenderson wrote:

Sorry for the delay. I've been off sick for much of the week, and am still catching up on things.

I hope you are well. Be safe.

llvm/test/tools/llvm-objdump/X86/source-interleave-prefix.test

84

I agree.

[jhenderson]

LGTM.

llvm/docs/CommandGuide/llvm-objdump.rst

.. option:: --no-show-raw-insn
 
   When disassembling, do not print the raw bytes of each instruction.
 
 .. option:: --prefix=<prefix>
 
   When disassembling with the :option:`--source` option, prepend ``prefix`` to
   absolute paths.
 
+.. option:: --prefix-strip=<level>
+
+  When disassembling with the :option:`--source` option, strip out ``level``
+  initial directories from absolute paths. It has no effect without
+  :option:`--prefix`.
+
 .. option:: --print-imm-hex
 
   Use hex format when printing immediate values in disassembly output.
 
 .. option:: -S, --source
 
   When disassembling, display source interleaved with the disassembly. Implies
   :option:`--disassemble`.

llvm/docs/llvm-objdump.1

 .It Fl -no-leading-headers
 Print no leading headers.
 .It Fl -no-show-raw-insn
 When disassembling instructions, do not print the instruction bytes.
 .It Fl -prefix Ns = Ns Ar PREFIX
 When disassembling, add
 .Ar PREFIX
 to absolute paths.
+.It Fl -prefix-strip Ns = Ns Ar LEVEL
+When disassembling, strip out
+.Ar LEVEL
+initial directories from absolute paths. It has no effect without
+.Fl -prefix Ns = Ns PREFIX .
 .It Fl -print-imm-hex
 Use hex format for immediate values.
 .It Fl -private-header
 Display only the first format specific file header.
 .It Fl -private-headers
 Display format specific file headers.
 .It Fl r
 Display the relocation entries in the file.

llvm/test/tools/llvm-objdump/X86/source-interleave-prefix.test

 ; RUN:   FileCheck %s --check-prefix=CHECK-BROKEN-PREFIX -DFILE=%t-missing-prefix.o -DPREFIX=''
 
 ;; All trailing separators on the prefix are discarded.
 ;; The prefix 'myprefix//' is converted to 'myprefix'.
 
 ; RUN: llvm-objdump --prefix myprefix// --source %t-missing-prefix.o 2>&1 | \
 ; RUN:   FileCheck %s --check-prefix=CHECK-BROKEN-PREFIX -DFILE=%t-missing-prefix.o -DPREFIX=myprefix
 
+;; Test invalid source interleave fixed by adding the correct prefix and
+;; stripping out an extra directory in path.
+
+; RUN: sed -e "s,SRC_COMPDIR,/extra/Inputs,g" %p/Inputs/source-interleave.ll > %t-extra-path-prefix.ll
+; RUN: llc -o %t-extra-path-prefix.o -filetype=obj -mtriple=x86_64-pc-linux %t-extra-path-prefix.ll
+; RUN: llvm-objdump --prefix %p --prefix-strip 1 --source %t-extra-path-prefix.o 2>&1 | \
+; RUN:   FileCheck %s --check-prefix=CHECK-MISSING-PREFIX-FIX
+

llvm/tools/llvm-objdump/llvm-objdump.h

 extern cl::opt<bool> DisassembleAll;
 extern cl::opt<DIDumpType> DwarfDumpType;
 extern cl::list<std::string> FilterSections;
 extern cl::list<std::string> MAttrs;
 extern cl::opt<std::string> MCPU;
 extern cl::opt<bool> NoShowRawInsn;
 extern cl::opt<bool> NoLeadingAddr;
 extern cl::opt<std::string> Prefix;
+extern cl::opt<int> PrefixStrip;
 extern cl::opt<bool> PrintImmHex;
 extern cl::opt<bool> PrivateHeaders;
 extern cl::opt<bool> Relocations;
 extern cl::opt<bool> SectionHeaders;
 extern cl::opt<bool> SectionContents;
 extern cl::opt<bool> SymbolDescription;
 extern cl::opt<bool> SymbolTable;
 extern cl::opt<std::string> TripleName;

llvm/tools/llvm-objdump/llvm-objdump.cpp

static cl::opt<bool>
     Wide("wide", cl::desc("Ignored for compatibility with GNU objdump"),
          cl::cat(ObjdumpCat));
 static cl::alias WideShort("w", cl::Grouping, cl::aliasopt(Wide));
 
 cl::opt<std::string> objdump::Prefix("prefix",
                                      cl::desc("Add prefix to absolute paths"),
                                      cl::cat(ObjdumpCat));
 
+cl::opt<int>
+    objdump::PrefixStrip("prefix-strip",
+                         cl::desc("Strip out initial directories from absolute "
+                                  "paths. It has no effect without --prefix"),
+                         cl::init(0), cl::cat(ObjdumpCat));
+
 enum DebugVarsFormat {
   DVDisabled,
   DVUnicode,
   DVASCII,
 };
 
 static cl::opt<DebugVarsFormat> DbgVariables(
     "debug-vars", cl::init(DVDisabled),
if (ExpectedLineInfo) {
     WarnedInvalidDebugInfo = true;
     // TODO Untested.
     reportWarning("failed to parse debug information: " +
                       toString(ExpectedLineInfo.takeError()),
                   ObjectFilename);
   }
 
   if (!Prefix.empty() && sys::path::is_absolute_gnu(LineInfo.FileName)) {
+    if (PrefixStrip > 0) {
+      int Level = 0;
+      const char *FileName = LineInfo.FileName.c_str();
+
+      // FileName has at least one character since is_absolute_gnu is false
+      // for an empty string.
+      for (const char *S = FileName + 1; *S != '\0' && Level < PrefixStrip;
+           ++S) {
+        if (sys::path::is_separator(*S)) {
+          FileName = S;
+          ++Level;
+        }
+      }
+
+      LineInfo.FileName = std::string(FileName);
+    }
+
     SmallString<128> FilePath;
     sys::path::append(FilePath, Prefix, LineInfo.FileName);
 
     LineInfo.FileName = std::string(FilePath);
   }
 
   if (PrintLines)
     printLines(OS, LineInfo, Delimiter, LVP);
int main(int argc, char **argv) {
 
   cl::ParseCommandLineOptions(argc, argv, "llvm object file dumper\n", nullptr,
                               /*EnvVar=*/nullptr,
                               /*LongOptionsUseDoubleDash=*/true);
 
   if (StartAddress >= StopAddress)
     reportCmdLineError("start address should be less than stop address");
 
+  if (PrefixStrip < 0)
+    reportCmdLineError("prefix strip must be non-negative");
+
   ToolName = argv[0];
 
   // Defaults to a.out if no filenames specified.
   if (InputFilenames.empty())
     InputFilenames.push_back("a.out");
 
   // Removes trailing separators from prefix.
   while (!Prefix.empty() && sys::path::is_separator(Prefix.back()))