[PATCH] D85024: [llvm-objdump] Implement --prefix option
Vinicius Tinti via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Mon Aug 10 04:47:39 PDT 2020
tinti added a comment.
================
Comment at: llvm/docs/CommandGuide/llvm-objdump.rst:172
+
+ When disassembling, add prefix to absolute paths.
+
----------------
jhenderson wrote:
> 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")?
> 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 tried to avoid this here because I saw other examples that don't do this here. I will change.
> 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")?
Yes. It is a dead simple prepend.
The `--prefix` will not touch a path like `../foo/filename.c`. It only touches absolute paths like `/foo/filename.c`.
So with `--prefix bar`:
# `../foo/filename.c` stays the same way.
# `/foo/filename.c` becomes `bar/foo/filename.c`.
================
Comment at: llvm/test/tools/llvm-objdump/X86/source-interleave-absolute-paths.test:2
+; Test --prefix option.
+; UNSUPPORTED: system-windows
+
----------------
jhenderson wrote:
> 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.
> 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.
It is intended to be supported on Windows. The behavior implemented up to now is correct and matches GNU objdump. On Windows path `C:\foo` with prefix `bar` is supposed and will to become `barC:\foo`. It looks wrong but it is not. Now I understand why it is supposed to be that way. Please read below.
There is another option that I have already implemented but not submitted called `--prefix-strip`. `--prefix-strip` receives an integer and strip parts by the separators from the original absolute path. So to make `C:\foo\filename.c` become `C:\prefix\foo\filename.c` one need to use `--prefix C:\prefix\` and `--prefix-strip 1`.
================
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
----------------
jhenderson wrote:
> 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.
The reason I keep that was to make clear the flow of changes done by `--prefix`.
How the untouched output was and what has changed. This can be changed to a comment too.
================
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
----------------
jhenderson wrote:
> 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
> ```
Is to ensure that the output is correct.
I did not know this option on FileCheck. I will use that one.
================
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
----------------
jhenderson wrote:
> 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).
Ok.
I will change.
================
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
----------------
jhenderson wrote:
> Why does this case not need the `{{.*}}` that is present in the first "myprefix" case?
Because in `%t2.o` I did the incomplete prefix (without `%p`).
Complete and correct:
`; RUN: sed -e "s,SRC_COMPDIR,%/p/Inputs,g" %p/Inputs/source-interleave.ll > %t.ll`
Incomplete:
`; RUN: sed -e "s,SRC_COMPDIR,/Inputs,g" %p/Inputs/source-interleave.ll > %t2.ll`
================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:1033
+ // GNU's objdump compatible `--prefix`
+ if (!Prefix.empty()) {
----------------
jhenderson wrote:
> 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.
Ok.
I will remove.
================
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);
----------------
jhenderson wrote:
> Can you get rid of the `llvm::` prefix here?
Ok.
================
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();
+ }
----------------
jhenderson wrote:
> I think you can get rid of the outer braces here.
Ok.
================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:2901
+ if (!Prefix.empty()) {
+ while (llvm::sys::path::is_separator(Prefix.back()))
+ Prefix.pop_back();
----------------
jhenderson wrote:
> Can you get rid of the `llvm::` prefix here? We have a `using namespace llvm` directive at the start of this function.
Ok.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D85024/new/
https://reviews.llvm.org/D85024
More information about the llvm-commits
mailing list