[PATCH] D54384: [llvm-objcopy] Add --build-id-link-dir flag

Tue Nov 27 12:54:44 PST 2018

jakehehrlich added inline comments.

================
Comment at: llvm/test/tools/llvm-objcopy/build-id-link-dir.test:5
+# RUN: llvm-objcopy --build-id-link-dir=%t-dir %t %t2
+# RUN: not test -e %t-dir/4f/cb712aa6387724a9f465a32cd8c14b
+
----------------
jhenderson wrote:
> rupprecht wrote:
> > Is `test -e` portable (i.e. on windows)? Just `ls` might be a better option.
> I can't verify for all Windows versions, but on mine, it is part of GnuWin32 tools, which is a prerequisite, I believe.
I have a windows box with a minimal install for this exact reason. I'll try it later to make sure it works and report back.

================
Comment at: llvm/test/tools/llvm-objcopy/build-id-link-dir.test:11
+
+# RUN: llvm-objcopy --build-id-link-dir=%t-dir --build-id-link-output= --strip-sections %t %t4
+# RUN: cmp %t-dir/4f/cb712aa6387724a9f465a32cd8c14b %t4
----------------
jhenderson wrote:
> `--build-id-link-output= ...` looks weird and I don't think should be valid syntax. Could it not just be `--build-id-link-output`?
> 
> If specifying an output is valid, there should be a test for that...
> 
> Basically, I'm a little confused as to what is going on here and in the next test.
Setting an output is valid. It's just that stripped binaries traditionally have no extension. an empty "=" is commonly used to pass an empty string and is valid for all "=" arguments across llvm from the argument parser's perspective. This is equivalent to `--build-id-link-output ""`

The imagined use case for --build-idlink-output that uses a non-empty string would be if you're not stripping, you're adding or removing something and you need ".debug" afterwards. Or using "--only-keep-debug" despite that not having a proper implementation at this exact moment. The example for this would be `llvm-objcopy --build-id-link-output=.debug --only-keep-debug <input> <output>` there's also this informally proposed idea that has floated around a few people here of adding a third ".dwp" file to the build id directory but I think people wanted to put that feature directly in llvm-dwp. Also in that case you're more or less directly using llvm-objcopy just to do the copy to the build id directory and no other operation.

================
Comment at: llvm/test/tools/llvm-objcopy/no-build-id2.test:1
+# RUN: yaml2obj %s > %t
+# RUN: not llvm-objcopy --build-id-link-dir=%t-dir --build-id-link-input=.debug %t 2>&1 >/dev/null | FileCheck %s
----------------
jhenderson wrote:
> Why is this test a separate test? Perhaps naming it differently would help clarify.
The intent was for one to have notes but no build id entry and the other to just not have notes. Those would need two different yaml files. I guess I forgot to add that. They've been added now.

================
Comment at: llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp:125
+    llvm_unreachable("This error should have been a success");
+  constexpr const char *ELF_NOTE_GNU = "GNU";
+  for (const auto &Phdr : unwrapOrError(In.program_headers())) {
----------------
jhenderson wrote:
> Why the constexpr here? Pretty sure it's not going to achieve much. Also, I don't think this is the correct case for constants in LLVM - I think it's supposed to be the same as other local variables.
In other code bases I've been working in constexpr has been prefered. As for the case correctness. This is a defined constant in system's <elf.h> but not in llvm's BinaryFormat header for elf. I'm adding it here as it would normally appear in the header. I'll remove the constexpr but I'd like to keep the name.

================
Comment at: llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp:179
+    if (sys::fs::exists(Path))
+      sys::fs::remove(Path, true);
+    EC = sys::fs::create_hard_link(ToLink, Path);
----------------
jhenderson wrote:
> Comment the true here too.
I realized it wasn't needed and dropped in instead. I commented the other one as requested however.

================
Comment at: llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp:164
+          EC.message());
+    return;
+  }
----------------
jhenderson wrote:
> Remove this return and the parentheses, since `error()` is a no-return function.
> 
> P.S. Why do we have separate `error` and `reportError` functions?
1) Because I copied the error handeling functions from other places in the very first commit and we've just kept it so far
2) They all do slightly different things.  Though I think I'm not consistent about when I use the Error form of reportError and when I manually use error() with an Error

================
Comment at: llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp:170-171
+  Path += Suffix;
+  auto EC = sys::fs::create_hard_link(ToLink, Path);
+  if (EC) {
+    // Hard linking failed, try to remove the file first if it exists.
----------------
jhenderson wrote:
> Could these two lines be combined to the following?
> 
> ```
> if (auto EC = sys::fs::create_hard_link(ToLink, Path)) {
> ```
yep. Thanks!

Repository:
  rL LLVM

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

https://reviews.llvm.org/D54384