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

Tue Aug 25 09:13:17 PDT 2020

tinti marked 4 inline comments as done.
tinti added inline comments.

================
Comment at: llvm/test/tools/llvm-objdump/X86/source-interleave-absolute-paths.test:29
+; RUN: llvm-objdump --prefix myprefix/// --source %t.o 2>&1 | FileCheck %s --check-prefix CHECK-TRAILING -DFILE=%t.o
+; CHECK-TRAILING: warning: '[[FILE]]': failed to find source myprefix/Inputs/source-interleave-x86_64.c
----------------
jhenderson wrote:
> You don't need `CHECK-TRAILING`. You can use `CHECK-BROKEN-PREFIX` with `-D ROOT=''`.
In this case I believe I need.

`CHECK-BROKEN-PREFIX` uses `%t2`.
`CHECK-TRAILING` uses `%t`.

================
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) {
----------------
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

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85024