[PATCH] D85024: [llvm-objdump] Implement --prefix option

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/docs/CommandGuide/llvm-objdump.rst:172
+
+  When disassembling, add prefix to absolute paths.
+
----------------
I'd change this text to "When disassembling with the --source option, add prefix to..." (where `--source` is formatted in the same way as other option references - see the reference e.g. to `--disassemble` in the `--source` help text).

I'm also a little confused what is meant by "absolute" paths here. Is it literally gluing the specified prefix onto the front of the path (e.g. "/foo/bar" becomes "prefix/foo/bar")?


================
Comment at: llvm/test/tools/llvm-objdump/X86/source-interleave-absolute-paths.test:2
+; Test --prefix option.
+; UNSUPPORTED: system-windows
+
----------------
Is this option not intended to be supported on Windows? I couldn't figure out GNU objdump's behaviour for Windows, so it's possible it doesn't work, but it would be nice, if we could get it to. One option would be to insert the prefix after the drive letter. Thus `C:\foo` would become `C:\prefix\foo`. It might be necessary to look at GNU's source code to identify its Windows behaviour.


================
Comment at: llvm/test/tools/llvm-objdump/X86/source-interleave-absolute-paths.test:10-13
+; RUN: llvm-objdump --source %t.o 2>&1 | FileCheck %s
+; CHECK:      0000000000000000 <foo>:
+; CHECK-NEXT: ; int foo() {
+; CHECK-NEXT:       0: 55                            pushq   %rbp
----------------
As I said before, I don't think you need the cases where `llvm-objdump` is called without `--prefix`. They are not necessary because `--source` is tested elsewhere. Keeping them in complicates the test making it harder to see what is actually being tested, and also slows it down.

However, I can potentially be persuaded otherwise - I just need to see a justification for this.


================
Comment at: llvm/test/tools/llvm-objdump/X86/source-interleave-absolute-paths.test:17
+; CHECK-PREFIX:      0000000000000000 <foo>:
+; CHECK-PREFIX-NEXT: warning: '{{.*}}.o': failed to find source myprefix{{.*}}/Inputs/source-interleave-x86_64.c
+; CHECK-PREFIX-NEXT:       0: 55                            pushq   %rbp
----------------
What is the purpose of the `{{.*}}` following `myprefix` here? If it's representing `%/p`, then you can use FileCheck's -D option to ensure the output is correct. Furthermore, you can use the same option for the file name. I believe something like the following would work:

```
; RUN: ... | FileCheck %s --check-prefix CHECK-PREFIX -DFILE=%t.o -DROOT=%/p
; CHECK-PREFIX:      0000000000000000 <foo>:
; CHECK-PREFIX-NEXT: warning: '[[FILE]]': failed to find source myprefix[[ROOT]]/Inputs/source-interleave-x86_64.c
```


================
Comment at: llvm/test/tools/llvm-objdump/X86/source-interleave-absolute-paths.test:31-34
+; RUN: llvm-objdump --prefix %p --source %t2.o 2>&1 | FileCheck %s --check-prefix CHECK-MISS-PREFIX-FIX
+; CHECK-MISS-PREFIX-FIX:      0000000000000000 <foo>:
+; CHECK-MISS-PREFIX-FIX-NEXT: ; int foo() {
+; CHECK-MISS-PREFIX-FIX-NEXT:       0: 55                            pushq   %rbp
----------------
I'd make this case the "primary" case (i.e. first in the file), since it is the actually useful one.

You also don't need a unique set of check lines - they are identical to other cases, and thus can be reused (try removing the --check-prefix command - you will see the test still passes).


================
Comment at: llvm/test/tools/llvm-objdump/X86/source-interleave-absolute-paths.test:39-41
+; CHECK-TRAILING1:      0000000000000000 <foo>:
+; CHECK-TRAILING1-NEXT: ; int foo() {
+; CHECK-TRAILING1-NEXT:       0: 55                            pushq   %rbp
----------------
These three checks aren't being used - you are using the `CHECK` cases for this test case (and thus showing that the behaviour is the same as if --prefix wasn't specified). You probably want a comment saying what this case is actually testing (i.e. that `--prefix /` is the same as without specifying `--prefix`).


================
Comment at: llvm/test/tools/llvm-objdump/X86/source-interleave-absolute-paths.test:45
+; CHECK-TRAILING2:      0000000000000000 <foo>:
+; CHECK-TRAILING2-NEXT: warning: '{{.*}}.o': failed to find source myprefix/Inputs/source-interleave-x86_64.c
+; CHECK-TRAILING2-NEXT:       0: 55                            pushq   %rbp
----------------
Why does this case not need the `{{.*}}` that is present in the first "myprefix" case?


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:1033
 
+  // GNU's objdump compatible `--prefix`
+  if (!Prefix.empty()) {
----------------
I don't think you need this comment. Most options are GNU compatible, but even if they weren't, we probably wouldn't highlight them.


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:1035-1037
+    if (llvm::sys::path::is_absolute(LineInfo.FileName)) {
+      llvm::SmallString<128> FilePath;
+      llvm::sys::path::append(FilePath, Prefix, LineInfo.FileName);
----------------
Can you get rid of the `llvm::` prefix here?


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:2900-2903
+  if (!Prefix.empty()) {
+    while (llvm::sys::path::is_separator(Prefix.back()))
+      Prefix.pop_back();
+  }
----------------
I think you can get rid of the outer braces here.


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:2901
+  if (!Prefix.empty()) {
+    while (llvm::sys::path::is_separator(Prefix.back()))
+      Prefix.pop_back();
----------------
Can you get rid of the `llvm::` prefix here? We have a `using namespace llvm` directive at the start of this function.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D85024/new/

https://reviews.llvm.org/D85024