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

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/test/tools/llvm-objcopy/bad-build-id.test:4
+
+# CHECK: build ID in file [[T:.*]] is smaller than two bytes.
+
----------------
You don't need to capture "T" here, since you don't use it in a later CHECK. You can do `{{.*}}` instead.


================
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.
The weird syntax still exists: trailing '=' looks like something is missing. I'd prefer a solution to it. Either --build-id-link-output should be a valid switch on its own with no argument, as well as --build-id-link-output=<some value> or some other solution si needed.


================
Comment at: llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp:139
+    return std::move(Err);
+  return make_error<StringError>(
+      "Could not find build ID.",
----------------
jhenderson wrote:
> Use makeStringError, and llvm::errc::invalid_argument, to avoid the std::make_error_code. It might be nice to also mention the object file name in the error message.
Oops, yes, `createStringError`. You don't need the `std::make_error_code` now.


================
Comment at: llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp:132-135
+      if (Note.getType() == NT_GNU_BUILD_ID && Note.getName() == ELF_NOTE_GNU) {
+        return Note.getDesc();
+      }
+      }
----------------
clang-format, and unnecessary braces.


================
Comment at: llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp:145
+findBuildID(const object::ELFObjectFileBase &In) {
+  if (auto *O = dyn_cast<ELFObjectFile<ELF32LE>>(&In)) {
+    return findBuildID(*O->getELFFile());
----------------
I might be missing something, but I think the parentheses here are all unnecessary.


================
Comment at: llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp:164
+          EC.message());
+    return;
+  }
----------------
Remove this return and the parentheses, since `error()` is a no-return function.

P.S. Why do we have separate `error` and `reportError` functions?


================
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.
----------------
Could these two lines be combined to the following?

```
if (auto EC = sys::fs::create_hard_link(ToLink, Path)) {
```


Repository:
  rL LLVM

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

https://reviews.llvm.org/D54384