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

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added inline comments.


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

```
## Only the tail of a segment can be trimmed. .text still occupies space because
## it is followed by .note which is not SHT_NOBITS.
# CHECK2:      Name        Type     Address          Off    Size   ES Flg Lk Inf Al
# CHECK2:      .text       NOBITS   0000000000000200 000200 000001 00  AX  0   0 512
# CHECK2-NEXT: .note       NOTE     0000000000000201 000201 000001 00   A  0   0  0
```


================
Comment at: llvm/test/tools/llvm-objcopy/ELF/only-keep-debug.test:61
+    Content:      c3
+  - Name:         .tdata
+    Type:         SHT_PROGBITS
----------------
jhenderson wrote:
> Do we need to consider the weird layout of .tbss sections at all?
Added .tbss to demonstrate that .tbss can be handled as well.


================
Comment at: llvm/test/tools/llvm-objcopy/ELF/only-keep-debug.test:66
+    AddressAlign: 128
+    Size:         7
+  - Name:         .bss
----------------
jhenderson wrote:
> Any reason for this arbitrary size?
This arbitrary non-zero size is used to show that p_filesz of the containg PT_TLS gets rewritten to 0.


================
Comment at: llvm/test/tools/llvm-objcopy/ELF/only-keep-debug.test:66
+    AddressAlign: 128
+    Size:         7
+  - Name:         .bss
----------------
MaskRay wrote:
> jhenderson wrote:
> > Any reason for this arbitrary size?
> This arbitrary non-zero size is used to show that p_filesz of the containg PT_TLS gets rewritten to 0.
Added a comment.


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


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


================
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;
+
----------------
jakehehrlich wrote:
> jhenderson wrote:
> > 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.
> It matters. Thanks for bringing that up.
For SHF_ALLOC .shstrtab, we will get: `error: e_shstrndx field value 5 in elf header  is not a string table`

GNU objcopy handles the section by dropping SHF_ALLOC. I haven't ever seen a tool that can make e_shstrndx SHF_ALLOC, so it probably does not matter.

-------

Added a test for SHF_ALLOC .symtab and .strtab. The behavior is consistent with GNU objcopy. The ELF spec says this is allowed but I don't think GNU ld or lld can create such components.


================
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)
----------------
jhenderson wrote:
> 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?
This looks stale after I removed MaxPageSize.

Updated the comment.



================
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);
----------------
jakehehrlich wrote:
> jhenderson wrote:
> > 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".
> +1 this was really confusing to me as well. 
Thanks for the suggestion. I agree `HdrEnd` is a much better name.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67137