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

[Jake Ehrlich via Phabricator via llvm-commits]

jakehehrlich added inline comments.


================
Comment at: llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp:140
+  sys::path::append(Path,
+                    llvm::toHex(ArrayRef<uint8_t>(BuildIdBytes->begin() + 1,
+                                                  BuildIdBytes->end()),
----------------
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.


================
Comment at: llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp:142
+                                                  BuildIdBytes->end()),
+                                true));
+  outs() << "out: " << Config.OutputFilename << " Path: " << Path << "\n";
----------------
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.


================
Comment at: llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp:143
+                                true));
+  outs() << "out: " << Config.OutputFilename << " Path: " << Path << "\n";
+  auto EC = sys::fs::create_hard_link(Config.InputFilename, Path);
----------------
mcgrathr wrote:
> Is this stray debug output?
yes.


================
Comment at: llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp:146
+  if (EC) {
+    // Hard linking failed, try to remove the file first if it exists.
+    if (sys::fs::exists(Path))
----------------
mcgrathr wrote:
> In POSIX terms, this is usually done by checking for errno==EEXIST rather than another filesystem call to see if that was the reason it failed.
create_hard_link doesn't seem to give any better information that could be used to do this across all systems.


================
Comment at: llvm/tools/llvm-objcopy/ELF/Note.h:24
+
+// Notes always have the same format.
+class Note {
----------------
mcgrathr wrote:
> Actually some systems do use 64-bit fields in ELFCLASS64 SHT_NOTE (or PT_NOTE) data.
> 
llvm doesn't handle them and they're outliers.


================
Comment at: llvm/tools/llvm-objcopy/ELF/Note.h:91
+  for (const auto &Note : NoteRange{NoteData}) {
+    if (Note.Type == NT_GNU_BUILD_ID && Note.name() == "GNU") {
+      return Note.desc();
----------------
mcgrathr wrote:
> So this tool does not handle cross-endian file access?
> And what's wrong with ELFFile::notes() anyway?
Ah crap...well that *is* an issue that I should fix I suppose.


Repository:
  rL LLVM

https://reviews.llvm.org/D54384