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

Tue Aug 25 19:34:28 PDT 2020

tinti added a subscriber: lhames.
tinti added inline comments.

================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:433
+objdumpIsAbsolute(const Twine &path,
+                  sys::path::Style style = sys::path::Style::native) {
+  if (style == sys::path::Style::windows) {
----------------
tinti wrote:
> jhenderson wrote:
> > You never specify the `style` parameter so unsurprisingly, the check for `style == sys::path::Style::windows` doesn't work, and the test still fails on Windows. You can see this by looking at the test failures listed by the pre-merge checks near the top of this page. You might want a call to `real_style`.
> You are correct. I miss understood the meaning of `Style::native`. I thought it would be set to either `Style::posix` or `Style::native`.
> 
> Unfortunately `real_style` is restricted to https://github.com/llvm-mirror/llvm/blob/master/lib/Support/Path.cpp#L39.
> 
> Thinking about it I can:
> - Add a `#ifdef _WIN32` in `llvm-objdump`. I am really not willing to go with this one but I have seen in other tools such as `llvm-objcopy` [1].
> - Make `real_style` public or be able to query the style outside `llvm::sys::path::*` with a helper function. Doesn't look great also. It seems to be designed this way to avoid such queries.
> - Try another way to answer "was I built on Windows?"
> 
> LLDB has this issue too since GDB does use IS_DIR_ABSOLUTE [3]. It solves it in several ways (usage [4] and [5]):
> 
> - `#ifdef` https://github.com/llvm/llvm-project/blob/master/lldb/source/Utility/FileSpec.cpp#L37
> - `Triple` https://github.com/llvm/llvm-project/blob/master/lldb/source/Utility/FileSpec.cpp#L76
> - indirectly using path https://github.com/llvm/llvm-project/blob/master/lldb/source/Utility/FileSpec.cpp#L310
> 
> With all this in mind I think I would go with `Triple(sys::getProcessTriple()).isOSWindows()` implementation. Correct me if I am wrong but on GNU you can only have one binary for one target whereas LLVM one may have one binary to many targets (I don't know if this applies here).
> 
> Taking into account CROSS-COMPILING between Linux and Windows: My guess would be to behave based on the **running target** rather than the **compile target**.
> 
> Out of ideas. It is becoming a bit complex.
> 
> Two more things. Should I care about `__CYGWIN__`, `__MSDOS__`, `__OS2__`? GNU tools care [2].
> 
> I would answer no. Maybe only care for `__CYGWIN__`.
> 
> ```
> git grep __CYGWIN__ | wc -l
> 43
> git grep __MSDOS__ | wc -l
> 3
> git grep __OS2__ | wc -l
> 3
> ```
> 
> Should I use `isAlpha()` like LLDB? There are some restriction about `<drive letter>:` format [6].
> 
> [1] See https://github.com/llvm/llvm-project/commit/6dc59629570ae6fe1836cff3ff53912917d17af7#diff-a074a8872d4fe193550d7a651ea18cabR229.
> [2] https://sourceware.org/git/?p=binutils-gdb.git;a=blob;f=include/filenames.h;hb=1ab8d928977f4d9f137f972d03e079555d0f29fa#l35
> [3] https://sourceware.org/git/?p=binutils-gdb.git;a=blob;f=gdb/buildsym.c;hb=1ab8d928977f4d9f137f972d03e079555d0f29fa#l519
> [4] https://github.com/llvm/llvm-project/blob/e22f0dabcf976668d35595e17645095e97a8177a/lldb/source/Plugins/SymbolFile/Breakpad/SymbolFileBreakpad.cpp#L653.
> [5] https://github.com/llvm/llvm-project/blob/bc9b6b33a0d5760370e72ae06c520c25aa8d61c6/lldb/source/Utility/FileSpec.cpp#L59
> [6] https://en.wikipedia.org/wiki/Drive_letter_assignment#Common_assignments
Even more complex: https://reviews.llvm.org/D34446

Please see @lhames last comment.

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

https://reviews.llvm.org/D85024