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

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added a comment.

In D67137#1725678 <https://reviews.llvm.org/D67137#1725678>, @jakehehrlich wrote:

> I had another thought. Right now the sections first layout algorithm is actually pretty general and doesn't make too much use of being specific to a particular flag. That's a good thing. That bad bit is that we're changing the section type.
>
> As a consequence SHT_NOBITS has to be set for allocated sections in HandleArgs. However there are many allocated section types that require casting and this makes the casting break. In many contexts this winds up being valid but its a non-local contract that SHT_NOBITS is special for the sake of --only-keep debug and all casts have to consider it as potentially special. There are at least two ways out of this
>
> 1. Add a separate type information field to SectionBase that remains unchanged...perhaps OrigionalType so that we can mutate Type. Then change all the classof functions to use OrigionalType. For consistency OrigionalFlags might be nice as well so that we don't have to do this again later.
> 2. Remove the mutation on Type and leave it all functioning as is then add a check on OnlyKeepDebug in section header writing code so that the Type is always written out as SHT_NOBITS if the section is allocated. Additionally inside of the new layout algorithm you'll have to say "if the section is nobits or unallocated" rather than just checking for SHT_NOBITS as we did before.
>
>   I prefer #2 personally but it does make the new layout algorithm `--only-keep-debug` specific and probably won't allow it to be used for any future unforeseen flags that are as difficult as `--only-keep-debug`


Sorry, I missed this comment. I understand your concern regarding the mutability of `llvm::objcopy::elf::SectionBase::Type`. `Type` is used by various `static bool classof(const SectionBase *)` overloads and mutating `Type` can change its result in different stages. My feeling is that we may actually need `OriginalTypes` and `OriginalFlags`, because `Flags` is also used in `classof` methods.

#2 requires us to do:

  --- a/llvm/tools/llvm-objcopy/ELF/Object.cpp
  +++ b/llvm/tools/llvm-objcopy/ELF/Object.cpp
  @@ -70,7 +70,10 @@ template <class ELFT> void ELFWriter<ELFT>::writeShdr(const SectionBase &Sec) {
     uint8_t *B = Buf.getBufferStart() + Sec.HeaderOffset;
     Elf_Shdr &Shdr = *reinterpret_cast<Elf_Shdr *>(B);
     Shdr.sh_name = Sec.NameIndex;
  -  Shdr.sh_type = Sec.Type;
  +  if (OnlyKeepDebug && (Sec.Flags & SHF_ALLOC) && Sec.Type != SHT_NOTE)
  +    Shdr.sh_type = SHT_NOBITS;
  +  else
  +    Shdr.sh_type = Sec.Type;

and add some code to both `layoutSectionsForOnlyKeepDebug` and `layoutSegmentsForOnlyKeepDebug`.

#1 as this patch currently does, can remain simple if we introduce  `OriginalTypes` and `OriginalFlags` (D69739 <https://reviews.llvm.org/D69739>).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67137