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

[Roland McGrath via Phabricator via llvm-commits]

mcgrathr 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()),
----------------
Is there a check (and test coverage) for the error case where the build ID is less than two bytes?


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


================
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);
----------------
Is this stray debug output?


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


================
Comment at: llvm/tools/llvm-objcopy/ELF/Note.h:22
+
+static const char *ELF_NOTE_GNU = "GNU";
+
----------------
static is always superfluous inside an anonymous namespace.
This should be `const char *const` or `constexpr` if LLVM allows newer C++.
Except it's unused, so it shouldn't be here at all.


================
Comment at: llvm/tools/llvm-objcopy/ELF/Note.h:24
+
+// Notes always have the same format.
+class Note {
----------------
Actually some systems do use 64-bit fields in ELFCLASS64 SHT_NOTE (or PT_NOTE) data.



================
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();
----------------
So this tool does not handle cross-endian file access?
And what's wrong with ELFFile::notes() anyway?


Repository:
  rL LLVM

https://reviews.llvm.org/D54384