[PATCH] D67137: [llvm-objcopy] Implement --only-keep-debug for ELF

[Fangrui Song via Phabricator via llvm-commits]

MaskRay marked an inline comment as done.
MaskRay added inline comments.


================
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:
> MaskRay wrote:
> > 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.
> The issue is more the dynamic string table. The dynamic string table *should* be SHT_NOBITS but what matters it that the type information changes. Similar things need to happen for .dynamic and such. At the very least I don't want to carry around the assumption that the type information can change.
If changing the section type here does not look good, should I do it in ELFObject.cpp, after

  if (!Config.SetSectionFlags.empty()) {
    for (auto &Sec : Obj.sections()) {
      const auto Iter = Config.SetSectionFlags.find(Sec.Name);
      if (Iter != Config.SetSectionFlags.end()) {
        const SectionFlagsUpdate &SFU = Iter->second;
        setSectionFlagsAndType(Sec, SFU.NewFlags);
      }
    }
  }

?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67137