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

[Jordan Rupprecht via Phabricator via llvm-commits]

rupprecht added a comment.

cc'ing blaike as a local debug expert -- I'm not familiar with systems using --build-id



================
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
----------------
Does this also work for in-place invocations?


================
Comment at: llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp:140
+  sys::path::append(Path,
+                    llvm::toHex(ArrayRef<uint8_t>(BuildIdBytes->begin() + 1,
+                                                  BuildIdBytes->end()),
----------------
jakehehrlich wrote:
> mcgrathr wrote:
> > Is there a check (and test coverage) for the error case where the build ID is less than two bytes?
> This ArrayRef will be empty in that case. There is however no check to make sure that the build ID isn't empty so the [0] above could fail in theory.
I agree -- this should error (with a test too :) ) if build id is less than 2 bytes


================
Comment at: llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp:142
+                                                  BuildIdBytes->end()),
+                                true));
+  outs() << "out: " << Config.OutputFilename << " Path: " << Path << "\n";
----------------
jakehehrlich wrote:
> mcgrathr wrote:
> > Where's the documentation for the behavior here?
> > The linked-to gdb documentation describes the .build-id/ directory convention in which it looks for xx/xxx.debug, not xx/xxx.
> > This is also what lldb implements (it only ever looks for xx/xxx.debug, never xx/xxx).
> > 
> > If the intent here is to publish the full, unstripped input file as the debug file, then it should have the .debug suffix for the gdb convention.
> > elfutils does look for xx/xxx when looking for an ELF file cold by build ID (which is the use case here, but not something AFAIK that either gdb or lldb supports), and then only looks for xx/xxx.debug if the main file doesn't have debug info.
> > But that's not the common convention.
> > 
> > When objcopy is being used to strip and produce a separate debug file (with SHT_NOBITS replacements for SHF_ALLOC sections, etc.), then what you want to link as xx/xxx.debug is the separate debug file, not the input file.  In that case the best behavior IMHO is to also link xx/xxx to the stripped output file.    That's consistent with e.g. Fedora/CentOS/RHEL debuginfo rpms, which include the separate debug file in /usr/lib/debug/.build-id/xx/xxx.debug and a symlink to the installed (stripped) binary as /usr/lib/debug/.build-id/xx/xxx.
> --only-keep-debug is a nop in llvm-objcopy. There is also no all in one "strip-to-file" option right now. It's certainly doable but it doesn't currently exist. The semantics would be the same weather the input file was copied or we went though the motions.
nit: comment the true here, e.g. `/*LowerCase*/true`


================
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) {
----------------
Why create a hard link instead of copying the file?


================
Comment at: llvm/tools/llvm-objcopy/ELF/Note.h:1
+//===- Notes.h ------------------------------------------------------------===//
+//
----------------
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


================
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">;
----------------
Should this be applied to llvm-strip as well?


Repository:
  rL LLVM

https://reviews.llvm.org/D54384