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

[Jordan Rupprecht via Phabricator via llvm-commits]

rupprecht 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
+
----------------
Is `test -e` portable (i.e. on windows)? Just `ls` might be a better option.


================
Comment at: llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp:123
+  if (Err)
+    assert(false);
+  constexpr const char *ELF_NOTE_GNU = "GNU";
----------------
This should probably error instead of assert, otherwise this will cause weird failures depending on debug vs release builds

I don't really understand why it's needed from this comment either -- can you elaborate the comment?

(Probably the libobject code should just be fixed, but I don't see that as a blocker)


================
Comment at: llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp:125
+  constexpr const char *ELF_NOTE_GNU = "GNU";
+  for (const auto &Phdr : unwrapOrError(In.program_headers())) {
+    if (Phdr.p_type != PT_NOTE)
----------------
The ELF dumper in llvm-readobj only looks for notes in the program headers if it's a core file:
https://github.com/llvm-mirror/llvm/blob/master/tools/llvm-readobj/ELFDumper.cpp#L3883
Honestly I'm not sure why it does it though. Do you need to do similar logic here?


================
Comment at: llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp:173
+  sys::path::append(Path,
+                    llvm::toHex(ArrayRef<uint8_t>(BuildIdBytes->begin() + 1,
+                                                  BuildIdBytes->end()),
----------------
BuildIdBytes.getValue().slice(1) instead


================
Comment at: llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp:578
+    linkToBuildIdDir(Config.BuildIdLinkDir, Config.OutputFilename,
+                     Config.BuildIdLinkOutput.getValue(), In);
+  }
----------------
Is the `In` here supposed to be `Out`?


================
Comment at: llvm/tools/llvm-objcopy/ObjcopyOpts.td:167
               HelpText<"Print the version and exit.">;
+defm build_id_link_dir : Eq<"build-id-link-dir", "Hard-link output to <dir>/xx/xxx name derived from hex build ID">,
+                         MetaVarName<"dir">;
----------------
jakehehrlich wrote:
> phosek wrote:
> > rupprecht wrote:
> > > Should this be applied to llvm-strip as well?
> > Yes, I believe we'll need that as well.
> Sounds good, I'll add it.
Maybe you just haven't gotten to it yet, but I still don't see the Stripopts.td change in this patch


================
Comment at: llvm/tools/llvm-objcopy/ObjcopyOpts.td:169
+                         MetaVarName<"dir">;
+defm build_id_link_input : Eq<"build-id-link-input", "Hard-link the input to <dir>/xx/xxx<suffix> name derived from hex build ID">,
+                           MetaVarName<"suffix">;
----------------
This looks long -- you should be able to format this automatically with `git-clang-format --extensions td master`


Repository:
  rL LLVM

https://reviews.llvm.org/D54384