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

[Jake Ehrlich via Phabricator via llvm-commits]

jakehehrlich 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;
+
----------------
MaskRay wrote:
> 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);
>       }
>     }
>   }
> 
> ?
I'd like to avoid changing Type all together. See my original two proposals for how to fix it. One of them is a very minor tweak to the existing code that would only require changing a few lines of this patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67137