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

[Jake Ehrlich via Phabricator via llvm-commits]

jakehehrlich added a comment.

So I fixed this issue: https://reviews.llvm.org/D54451

but now I seem to have found a different issue that causes a program failure to happen due to Error being handled improperly. After which we should be using the ELFType iterator implementation.



================
Comment at: llvm/test/tools/llvm-objcopy/build-id-link-dir.test:3
+# RUN: mkdir -p %t-dir
+# RUN: llvm-objcopy --build-id-link-dir=%t-dir %t %t2
+# RUN: cmp %t-dir/4f/cb712aa6387724a9f465a32cd8c14b %t
----------------
rupprecht wrote:
> Does this also work for in-place invocations?
It works it's just odd because you modify the the thing you sent to the build id directory.


================
Comment at: llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp:144
+  outs() << "out: " << Config.OutputFilename << " Path: " << Path << "\n";
+  auto EC = sys::fs::create_hard_link(Config.InputFilename, Path);
+  if (EC) {
----------------
rupprecht wrote:
> Why create a hard link instead of copying the file?
To save space but I guess copying would make the inplace case more sensible. Generally the build system may still need the unstripped file around. For instance, LLD doesn't support linking against binaries stripped with --strip-sections. Because debug binaries total to a rather large amount, not hard-linking would all at once double the size of the build. Currently in Fuchsia, on a lower layer, our build requires ~30gb of space. Of that 10 gb is for unstripped binaries. If we copied we'd suddenly shoot our build size up to 40gb for the smallest part. This isn't a terrible cost mind you but some people have complained about the space requirements. That seems like a high cost to pay to me.


================
Comment at: llvm/tools/llvm-objcopy/ELF/Note.h:1
+//===- Notes.h ------------------------------------------------------------===//
+//
----------------
rupprecht wrote:
> I don't know if you need this file at all. NoteIterator/NoteRange looks like it's reimplementing the note iterator in libobject
> e.g. https://github.com/llvm-mirror/llvm/blob/master/include/llvm/Object/ELF.h#L228
Yeah, I'm rewriting everything in terms of that right now. This file will go away.


================
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">;
----------------
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.


Repository:
  rL LLVM

https://reviews.llvm.org/D54384