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

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

I'm out of time. There are probably some more bits in the test for me to look at, but here are some comments to keep you going!



================
Comment at: llvm/docs/CommandGuide/llvm-objdump.rst:170
 
+.. option:: --prefix
+
----------------
Just realised that you need to show that this option takes an argument.


================
Comment at: llvm/docs/CommandGuide/llvm-objdump.rst:172
+
+  When disassembling with the ``--source`` option, add prefix to absolute paths.
+
----------------
A couple of suggestions to further clarify the text.


================
Comment at: llvm/test/tools/llvm-objdump/X86/source-interleave-absolute-paths.test:2
+; Test --prefix option.
+; UNSUPPORTED: system-windows
+
----------------
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.


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

https://reviews.llvm.org/D85024