[PATCH] D67137: [llvm-objcopy] Support --only-keep-debug

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

For clarity, is this change supposed to be instead of D67090 <https://reviews.llvm.org/D67090>? If so, please make sure the docs and help text are updated as part of it!

Also, for my own benefit, would you mind resummarising the expected behaviour of --only-keep-debug. It might be useful to add this as a comment in the code somewhere too.



================
Comment at: llvm/test/tools/llvm-objcopy/ELF/only-keep-debug.test:8
+
+# CHECK:      .note1        NOTE     0000000000000120 000120 000001 00   A 0 0   0
+# CHECK-NEXT: .note2        NOTE     0000000000000121 000121 000001 00   A 0 0   0
----------------
It's not strictly needed, but it would be helpful to include the column headers here too, to make it easier to see which column is which.


================
Comment at: llvm/test/tools/llvm-objcopy/ELF/only-keep-debug.test:49
+    Flags:        [ SHF_ALLOC ]
+    Address:      0x120           # Ensure Address=Offset
+    Content:      01
----------------
If you specify an AddressAlign, you can ensure that this address will remain correct, even if more program headers are added or yaml2obj changes layout slightly. For example, you could specify AddressAlign: 0x400, and then Address: 0x400.


================
Comment at: llvm/test/tools/llvm-objcopy/ELF/only-keep-debug.test:56
+    Content:      02
+  - Name:         .text
+    Type:         SHT_PROGBITS
----------------
What happens if we have a progbits section before the note sections in this segment? NOBITS sections are usually at the end of segments.


================
Comment at: llvm/test/tools/llvm-objcopy/ELF/only-keep-debug.test:61
+    Content:      c3
+  - Name:         .tdata
+    Type:         SHT_PROGBITS
----------------
Do we need to consider the weird layout of .tbss sections at all?


================
Comment at: llvm/test/tools/llvm-objcopy/ELF/only-keep-debug.test:66
+    AddressAlign: 128
+    Size:         7
+  - Name:         .bss
----------------
Any reason for this arbitrary size?


================
Comment at: llvm/test/tools/llvm-objcopy/ELF/only-keep-debug.test:72
+    AddressAlign: 32
+    Size:         63
+  - Name:         .debug_abbrev
----------------
Ditto.


================
Comment at: llvm/test/tools/llvm-objcopy/ELF/only-keep-debug.test:78
+    Type:         SHT_PROGBITS
+    AddressAlign: 8
+    Content:      04
----------------
Does this matter?


================
Comment at: llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp:585-586
+    for (auto &Sec : Obj.sections())
+      if (Sec.Flags & SHF_ALLOC && Sec.Type != SHT_NOTE)
+        Sec.Type = SHT_NOBITS;
+
----------------
If the symbol or string tables have SHF_ALLOC set, this will make them SHT_NOBITS. I'm not sure if that matters or not, but wanted to point it out.


================
Comment at: llvm/tools/llvm-objcopy/ELF/Object.cpp:1961
+    // The first section in a PT_LOAD has to have congruent offset and address
+    // modulo the maximum page size.
+    if (FirstSec && FirstSec == &Sec)
----------------
Maybe worth updating this comment to say that maximum page size is the segment alignment.

Is this comment strictly true? What if there's a hole in the segment before the first section?


================
Comment at: llvm/tools/llvm-objcopy/ELF/Object.cpp:1989
+// Rewrite p_offset and p_filesz of non-empty non-PT_PHDR segments after
+// sh_offset are updated.
+static uint64_t layoutSegmentsForOnlyKeepDebug(std::vector<Segment *> &Segments,
----------------
Nit: sh_offset are -> sh_offset values have been


================
Comment at: llvm/tools/llvm-objcopy/ELF/Object.cpp:2008
+    // than the headers.
+    if (Seg->Offset <= HdrOffset && HdrOffset <= Seg->Offset + Seg->FileSize) {
+      FileSize += Offset - Seg->Offset;
----------------
I think the first part only needs to be `Seg->Offset < HdrOffset`. A segment at the end of the headers doesn't need to be related to them in any way.


================
Comment at: llvm/tools/llvm-objcopy/ELF/Object.cpp:2044
+  if (OnlyKeepDebug) {
+    uint64_t HdrOffset =
+        sizeof(Elf_Ehdr) + llvm::size(Obj.segments()) * sizeof(Elf_Phdr);
----------------
Here and elsewhere, could you rename this to `HdrEnd` or similar please? I thought it was the start of the headers when reading before, which would be the normal situation for an "Offset".


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D67137/new/

https://reviews.llvm.org/D67137