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

[James Henderson via Phabricator via llvm-commits]

jhenderson requested changes to this revision.
jhenderson added a comment.
This revision now requires changes to proceed.

Sorry for not getting to this earlier. It's been a busy week for me.



================
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
----------------
`--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.


================
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
+
----------------
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.


================
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
----------------
Why is this test a separate test? Perhaps naming it differently would help clarify.


================
Comment at: llvm/tools/llvm-objcopy/CopyConfig.cpp:291
+  if (InputArgs.hasArg(OBJCOPY_build_id_link_input))
+    Config.BuildIdLinkInput = InputArgs.getLastArgValue(OBJCOPY_build_id_link_input);
+  if (InputArgs.hasArg(OBJCOPY_build_id_link_output))
----------------
clang-format?


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


================
Comment at: llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp:139
+    return std::move(Err);
+  return make_error<StringError>(
+      "Could not find build ID.",
----------------
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.


================
Comment at: llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp:162
+  if (BuildIdBytes.size() < 2)
+    error("build ID size is too small");
+
----------------
It might be nice to add a little bit more to this error message to make it clear what build ID is too small (e.g. "the build ID in file "foo" is smaller than two bytes").


================
Comment at: llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp:165
+  SmallString<128> Path = Dir;
+  sys::path::append(Path, llvm::toHex(BuildIdBytes[0], true));
+  if (auto EC = sys::fs::create_directories(Path)) {
----------------
/*LowerCase*/


================
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);
----------------
Comment the true here too.


================
Comment at: llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp:182
+    if (EC)
+      error("cannot link " + Path + ": " + EC.message());
+  }
----------------
This should probably mention where it's trying to link to, as well.


================
Comment at: llvm/tools/llvm-objcopy/ObjcopyOpts.td:169
+    : Eq<"build-id-link-dir",
+         "Use as <dir> by --build-id-link-input and --build-id-link-output">,
+      MetaVarName<"dir">;
----------------
Use -> Used
or maybe better yet:
Set directory for --build-id-link-input and --build-id-link-output to <dir>

Using the <dir> meta variable name for the two options in this help text is a bit confusing, given that <dir> is also the name of the meta variable for this option.


Repository:
  rL LLVM

https://reviews.llvm.org/D54384