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

[Jake Ehrlich via Phabricator via llvm-commits]

jakehehrlich added a comment.

This looks highly promising to me BTW.



================
Comment at: llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp:568-571
+  if (Config.OnlyKeepDebug)
+    for (auto &Sec : Obj.sections())
+      if (Sec.Flags & SHF_ALLOC && Sec.Type != SHT_NOTE)
+        Sec.Type = SHT_NOBITS;
----------------
I think this should happen last since it might affect the behavoir of other parts.


================
Comment at: llvm/tools/llvm-objcopy/ELF/Object.cpp:1951
+// occupy no space in the file.
+static uint64_t layoutSectionsForOnlyKeepDebug(Object &Obj, uint64_t Off,
+                                               uint64_t MaxPageSize) {
----------------
Funny enough this looks almost identical to the original layout algorithm used and is on of the primary reasons firstSection was exposed to begin with. Just a funny historical detail.


================
Comment at: llvm/tools/llvm-objcopy/ELF/Object.cpp:1964
+    if (FirstSec && FirstSec == &Sec)
+      Off = alignTo(Off, MaxPageSize, Sec.Addr);
+
----------------
Why not use Sec.ParentSegment->Align instead of MaxPageSize?


================
Comment at: llvm/tools/llvm-objcopy/ELF/Object.cpp:1974-1975
+
+    if (!FirstSec)
+      Off = Sec.Align ? alignTo(Off, Sec.Align) : Off;
+    else if (FirstSec != &Sec) {
----------------
Can you add a comment explaining that FirstSec is nullptr generally means that we're in a non-allocated section.


================
Comment at: llvm/tools/llvm-objcopy/ELF/Object.cpp:1979
+      // sh_offset for non-SHF_ALLOC sections.
+      if (Sec.Type & SHF_ALLOC)
+        Off = Sec.Addr - FirstSec->Addr + FirstSec->Offset;
----------------
Flags not Type


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


================
Comment at: llvm/tools/llvm-objcopy/ELF/Object.cpp:2008-2014
+    // If the segment includes EHDR or PHDR, don't make it smaller than the
+    // headers.
+    if (Seg->Offset <= HdrOffset && HdrOffset <= Seg->Offset + Seg->FileSize) {
+      FileSize += Offset - Seg->Offset;
+      Offset = Seg->Offset;
+      FileSize = std::max(FileSize, HdrOffset - Offset);
+    }
----------------
This whole check feels really strange to me. Why would this happen? Why do we need to increase the size of the headers like this?


================
Comment at: llvm/tools/llvm-objcopy/ELF/Object.h:344
   bool WriteSectionHeaders;
+  bool OnlyKeepDebug;
 
----------------
Please add a note here that this selects a layout algorithm and is *not* an instance of the config leaking into the ELFWriter which should never occur.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67137