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

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added inline comments.


================
Comment at: llvm/tools/llvm-objcopy/ELF/Object.cpp:1964
+    if (FirstSec && FirstSec == &Sec)
+      Off = alignTo(Off, MaxPageSize, Sec.Addr);
+
----------------
jakehehrlich wrote:
> Why not use Sec.ParentSegment->Align instead of MaxPageSize?
Thanks. Deleting MaxPageSize can simplify the code a bit.


================
Comment at: llvm/tools/llvm-objcopy/ELF/Object.cpp:1979-1982
+      if (Sec.Type & SHF_ALLOC)
+        Off = Sec.Addr - FirstSec->Addr + FirstSec->Offset;
+      else
+        Off = Sec.OriginalOffset - FirstSec->OriginalOffset + FirstSec->Offset;
----------------
jakehehrlich wrote:
> jakehehrlich wrote:
> > Flags not Type
> I think only the non-allocated branch is needed here.
> 
> The first branch only makes sense if we already know that Sec.Type != SHT_NOBITS which we do because of the previous check for that above. The second branch makes complete since in both the allocated and non-allocated cases within the same segment.
I agree. For non-first sections in a segment, we can just use its relative sh_offset distance to the first section.

I added it probably to emulate GNU objcopy. Reading it again, their choice looks weird and I don't think an executable that is linked by a linker (instead of hand-crafted) may need `Off = Sec.Addr - FirstSec->Addr + FirstSec->Offset`. I'll simplify the logic here.


================
Comment at: llvm/tools/llvm-objcopy/ELF/Object.cpp:1997
+      continue;
+    uint64_t Offset = Seg->Offset;
+    if (const SectionBase *Sec = Seg->firstSection())
----------------
jakehehrlich wrote:
> In the case where a Segment doesn't have a section this seems likely to go very poorly. For instance image a file with all the debug information after the allocated stuff but with a section-less segment in between the allocated content and the debug information. That would I think cause the offset of this segment to overlap with debug information which would be *very* bad. Perhaps we can add all of the section-less segments to a list and then assign them all MaxOffset with a FileSize of zero?
GNU objcopy seems to set p_offset of such PT_LOAD to 0. Rewriting p_offset and p_filesz is to make symbolization tools that rely on file offsets happy. I think such segments are not useful to the tool. p_filesz=MaxOffset probably is not better than p_filesz=old_p_filesz, so I will leave them unchanged.

Added another test.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67137