[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 with the ``--source`` option, prepend ``prefix`` to absolute paths.
+
----------------



================
Comment at: llvm/test/tools/llvm-objdump/X86/source-interleave-absolute-paths.test:1
+; Test --prefix option.
+; UNSUPPORTED: system-windows
----------------
Please use double coment markers for true comments (as opposed to RUN/CHECK etc lines), i.e. `;;`. This helps them stand out from the actual functional lines of the test. Older tests may not follow this approach, but we can for newer ones.


================
Comment at: llvm/test/tools/llvm-objdump/X86/source-interleave-absolute-paths.test:11-12
+; RUN: llc -o %t2.o -filetype=obj -mtriple=x86_64-pc-linux %t2.ll
+
+
+; Test invalid source interleave fixed by adding the correct prefix.
----------------
No need for a double blank line here and below - just use a single one and make the comments stand out more as suggested above.


================
Comment at: llvm/test/tools/llvm-objdump/X86/source-interleave-absolute-paths.test:15
+
+; RUN: llvm-objdump --prefix %p --source %t.o 2>&1 | FileCheck %s --check-prefix CHECK-MISS-PREFIX-FIX
+; CHECK-MISS-PREFIX-FIX: ; int foo() {
----------------
Hmmm... this is interesting. With llvm-objdump, this test case fails for Windows. However, if I replace llvm-objdump with GNU objdump and run the same invocation, it works. I dug into this a little. The problem is with the meaning of "absolute paths" for Windows - it's ambiguous. C++17's `filesystem::is_absolute`, whose behaviour is matched by `sys::path::is_absolute` essentially says that the path must be unambiguous (i.e. that there must be no drive letter for Windows). However, other languages, e.g. Python, treat a path as absolute with or without the drive letter. GNU objdump appears to follow the Python etc approach, so '/Inputs\foo' is treated as an absolute path and is prefixed.

I think we need extra logic to get this to work on Windows, to ensure GNU compatibility. Basically, you need to  identify the case where a path is absolute within the current drive (i.e. starts with '/', though it might be better, if possible to rely on the underlying functions to do that), and treat that as absolute. What you can't do is modify the behaviour of `sys::path::is_absolute` itself.


================
Comment at: llvm/test/tools/llvm-objdump/X86/source-interleave-absolute-paths.test:25
+
+; Test remove all trailing separators (i.e. '/') in prefix.
+; The prefix '/' is the same as not using the `--prefix` option.
----------------



================
Comment at: llvm/test/tools/llvm-objdump/X86/source-interleave-absolute-paths.test:28-29
+
+; RUN: llvm-objdump --prefix / --source %t2.o 2>&1 | FileCheck %s --check-prefix CHECK-TRAILING1
+; CHECK-TRAILING1: ; int foo() {
+
----------------
You can avoid the duplicate check pattern here, since the output should be the same as earlier.


================
Comment at: llvm/test/tools/llvm-objdump/X86/source-interleave-absolute-paths.test:2
+; Test --prefix option.
+; UNSUPPORTED: system-windows
+
----------------
tinti wrote:
> jhenderson wrote:
> > tinti wrote:
> > > 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`.
> > Thanks, that makes sense to me. If you remove the `UNSUPPORTED` line, the per-merge bot might tell you whether it works on Windows. I also primarily develop on Windows, so can double-check too if you want.
> > 
> > My suspicion is that this might not work, due to how --prefix and absolute paths work on Windows. Once you've implemented --prefix-strip, that one can be supported in a separate test, which will work on Windows then hopefully.
> As far as I understood it should pass.
> 
> Would be awesome if you could test and remove `UNSUPPORTED` if it pass.
I've tested, and it doesn't work - see below for why.


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

https://reviews.llvm.org/D85024