[llvm-objcopy][ELF] Add OriginalType & OriginalFlags

[MaskRay]

Summary

llvm::objcopy::elf::*Section::classof matches Type and Flags, yet Type
and Flags are mutable (setSectionFlagsAndTypes and upcoming
--only-keep-debug feature). Add OriginalType & OriginalFlags to be used
in classof, to prevent classof results from changing.

[MaskRay]

Delete a redundant assignment

[MaskRay]

For some section types, we call their classof (via isa or dyn_cast) before mutation, so using either OriginalType or Type does not matter, e.g. DynamicSymbolTableSection, GroupSection. Some classof methods are unused, e.g. DynamicSection::classof.

[jhenderson]

This seems like a good idea to me. It makes our classof results immutable after all, leading to a more robust program.

One thought I had: did you consider adding just a new "Kind" enum instead, with the various kinds of sections recorded? That would add one field to the section class rather than two. I don't know if the Original* flags really have any other usage, and it would allow us to add further types later on which are not identified by flag/type whilst simplifying the classof functions to a basic comparison.

[MaskRay]

In D69739#1731983, @jhenderson wrote:

This seems like a good idea to me. It makes our classof results immutable after all, leading to a more robust program.

One thought I had: did you consider adding just a new "Kind" enum instead, with the various kinds of sections recorded? That would add one field to the section class rather than two. I don't know if the Original* flags really have any other usage, and it would allow us to add further types later on which are not identified by flag/type whilst simplifying the classof functions to a basic comparison.

We are tackling an expression problem. classof member functions are indeed the only place that OriginalType and OriginalFlag are read. If we introduce SectionBase::Kind as a new member variable, we can avoid OriginalType and OriginalFlag, but the classof logic will have to be centralized in one place, because we need to initialize Kind when a section is created.

[jakehehrlich]

Nice. This will unblock --only-keep-debug. Thanks Fangrui!

[jhenderson]

LGTM

llvm/tools/llvm-objcopy/ELF/Object.h

public:
   IHexWriter(Object &Obj, Buffer &Buf) : Writer(Obj, Buf) {}
 };
 
 class SectionBase {
 public:
   std::string Name;
   Segment *ParentSegment = nullptr;
   uint64_t HeaderOffset;
-  uint64_t OriginalOffset = std::numeric_limits<uint64_t>::max();
   uint32_t Index;
   bool HasSymbol = false;
 
+  uint64_t OriginalFlags = 0;
+  uint64_t OriginalType = ELF::SHT_NULL;
+  uint64_t OriginalOffset = std::numeric_limits<uint64_t>::max();
+
   uint64_t Addr = 0;
   uint64_t Align = 1;
   uint32_t EntrySize = 0;
   uint64_t Flags = 0;
   uint64_t Info = 0;
   uint64_t Link = ELF::SHN_UNDEF;
   uint64_t NameIndex = 0;
   uint64_t Offset = 0;
class OwnedDataSection : public SectionBase {
   MAKE_SEC_WRITER_FRIEND
 
   std::vector<uint8_t> Data;
 
 public:
   OwnedDataSection(StringRef SecName, ArrayRef<uint8_t> Data)
       : Data(std::begin(Data), std::end(Data)) {
     Name = SecName.str();
-    Type = ELF::SHT_PROGBITS;
+    Type = OriginalType = ELF::SHT_PROGBITS;
     Size = Data.size();
     OriginalOffset = std::numeric_limits<uint64_t>::max();
   }
 
   OwnedDataSection(const Twine &SecName, uint64_t SecAddr, uint64_t SecFlags,
                    uint64_t SecOff) {
     Name = SecName.str();
-    Type = ELF::SHT_PROGBITS;
+    Type = OriginalType = ELF::SHT_PROGBITS;
     Addr = SecAddr;
-    Flags = SecFlags;
+    Flags = OriginalFlags = SecFlags;
     OriginalOffset = SecOff;
   }
 
   void appendHexData(StringRef HexData);
   void accept(SectionVisitor &Sec) const override;
   void accept(MutableSectionVisitor &Visitor) override;
 };
 
public:
 
   uint64_t getDecompressedSize() const { return DecompressedSize; }
   uint64_t getDecompressedAlign() const { return DecompressedAlign; }
 
   void accept(SectionVisitor &Visitor) const override;
   void accept(MutableSectionVisitor &Visitor) override;
 
   static bool classof(const SectionBase *S) {
-    return (S->Flags & ELF::SHF_COMPRESSED) ||
+    return (S->OriginalFlags & ELF::SHF_COMPRESSED) ||
            (StringRef(S->Name).startswith(".zdebug"));
   }
 };
 
 class DecompressedSection : public SectionBase {
   MAKE_SEC_WRITER_FRIEND
 
 public:
   explicit DecompressedSection(const CompressedSection &Sec)
       : SectionBase(Sec) {
     Size = Sec.getDecompressedSize();
     Align = Sec.getDecompressedAlign();
-    Flags = (Flags & ~ELF::SHF_COMPRESSED);
+    Flags = OriginalFlags = (Flags & ~ELF::SHF_COMPRESSED);
     if (StringRef(Name).startswith(".zdebug"))
       Name = "." + Name.substr(2);
   }
 
   void accept(SectionVisitor &Visitor) const override;
   void accept(MutableSectionVisitor &Visitor) override;
 };
 
 // There are two types of string tables that can exist, dynamic and not dynamic.
 // In the dynamic case the string table is allocated. Changing a dynamic string
 // table would mean altering virtual addresses and thus the memory image. So
 // dynamic string tables should not have an interface to modify them or
 // reconstruct them. This type lets us reconstruct a string table. To avoid
 // this class being used for dynamic string tables (which has happened) the
 // classof method checks that the particular instance is not allocated. This
 // then agrees with the makeSection method used to construct most sections.
 class StringTableSection : public SectionBase {
   MAKE_SEC_WRITER_FRIEND
 
   StringTableBuilder StrTabBuilder;
 
 public:
   StringTableSection() : StrTabBuilder(StringTableBuilder::ELF) {
-    Type = ELF::SHT_STRTAB;
+    Type = OriginalType = ELF::SHT_STRTAB;
   }
 
   void addString(StringRef Name);
   uint32_t findIndex(StringRef Name) const;
   void prepareForLayout();
   void accept(SectionVisitor &Visitor) const override;
   void accept(MutableSectionVisitor &Visitor) override;
 
   static bool classof(const SectionBase *S) {
-    if (S->Flags & ELF::SHF_ALLOC)
+    if (S->OriginalFlags & ELF::SHF_ALLOC)
       return false;
-    return S->Type == ELF::SHT_STRTAB;
+    return S->OriginalType == ELF::SHT_STRTAB;
   }
 };
 
 // Symbols have a st_shndx field that normally stores an index but occasionally
 // stores a different special value. This enum keeps track of what the st_shndx
 // field means. Most of the values are just copies of the special SHN_* values.
 // SYMBOL_SIMPLE_INDEX means that the st_shndx is just an index of a section.
 enum SymbolShndxType {
public:
   void finalize() override;
   void accept(SectionVisitor &Visitor) const override;
   void accept(MutableSectionVisitor &Visitor) override;
 
   SectionIndexSection() {
     Name = ".symtab_shndx";
     Align = 4;
     EntrySize = 4;
-    Type = ELF::SHT_SYMTAB_SHNDX;
+    Type = OriginalType = ELF::SHT_SYMTAB_SHNDX;
   }
 };
 
 class SymbolTableSection : public SectionBase {
   MAKE_SEC_WRITER_FRIEND
 
   void setStrTab(StringTableSection *StrTab) { SymbolNames = StrTab; }
   void assignIndices();
 
 protected:
   std::vector<std::unique_ptr<Symbol>> Symbols;
   StringTableSection *SymbolNames = nullptr;
   SectionIndexSection *SectionIndexTable = nullptr;
 
   using SymPtr = std::unique_ptr<Symbol>;
 
 public:
-  SymbolTableSection() { Type = ELF::SHT_SYMTAB; }
+  SymbolTableSection() { Type = OriginalType = ELF::SHT_SYMTAB; }
 
   void addSymbol(Twine Name, uint8_t Bind, uint8_t Type, SectionBase *DefinedIn,
                  uint64_t Value, uint8_t Visibility, uint16_t Shndx,
                  uint64_t SymbolSize);
   void prepareForLayout();
   // An 'empty' symbol table still contains a null symbol.
   bool empty() const { return Symbols.size() == 1; }
   void setShndxTable(SectionIndexSection *ShndxTable) {
public:
   void finalize() override;
   void accept(SectionVisitor &Visitor) const override;
   void accept(MutableSectionVisitor &Visitor) override;
   Error removeSymbols(function_ref<bool(const Symbol &)> ToRemove) override;
   void replaceSectionReferences(
       const DenseMap<SectionBase *, SectionBase *> &FromTo) override;
 
   static bool classof(const SectionBase *S) {
-    return S->Type == ELF::SHT_SYMTAB;
+    return S->OriginalType == ELF::SHT_SYMTAB;
   }
 };
 
 struct Relocation {
   Symbol *RelocSymbol = nullptr;
   uint64_t Offset;
   uint64_t Addend;
   uint32_t Type;

 protected:
   SectionBase *SecToApplyRel = nullptr;
 
 public:
   const SectionBase *getSection() const { return SecToApplyRel; }
   void setSection(SectionBase *Sec) { SecToApplyRel = Sec; }
 
   static bool classof(const SectionBase *S) {
-    return S->Type == ELF::SHT_REL || S->Type == ELF::SHT_RELA;
+    return S->OriginalType == ELF::SHT_REL || S->OriginalType == ELF::SHT_RELA;
   }
 };
 
 // Takes the symbol table type to use as a parameter so that we can deduplicate
 // that code between the two symbol table types.
 template <class SymTabType>
 class RelocSectionWithSymtabBase : public RelocationSectionBase {
   void setSymTab(SymTabType *SymTab) { Symbols = SymTab; }
public:
   Error removeSectionReferences(bool AllowBrokenLinks,
       function_ref<bool(const SectionBase *)> ToRemove) override;
   Error removeSymbols(function_ref<bool(const Symbol &)> ToRemove) override;
   void markSymbols() override;
   void replaceSectionReferences(
       const DenseMap<SectionBase *, SectionBase *> &FromTo) override;
 
   static bool classof(const SectionBase *S) {
-    if (S->Flags & ELF::SHF_ALLOC)
+    if (S->OriginalFlags & ELF::SHF_ALLOC)
       return false;
-    return S->Type == ELF::SHT_REL || S->Type == ELF::SHT_RELA;
+    return S->OriginalType == ELF::SHT_REL || S->OriginalType == ELF::SHT_RELA;
   }
 };
 
 // TODO: The way stripping and groups interact is complicated
 // and still needs to be worked on.
 
 class GroupSection : public SectionBase {
   MAKE_SEC_WRITER_FRIEND
public:
   void accept(MutableSectionVisitor &Visitor) override;
   void finalize() override;
   Error removeSymbols(function_ref<bool(const Symbol &)> ToRemove) override;
   void markSymbols() override;
   void replaceSectionReferences(
       const DenseMap<SectionBase *, SectionBase *> &FromTo) override;
 
   static bool classof(const SectionBase *S) {
-    return S->Type == ELF::SHT_GROUP;
+    return S->OriginalType == ELF::SHT_GROUP;
   }
 };
 
 class DynamicSymbolTableSection : public Section {
 public:
   explicit DynamicSymbolTableSection(ArrayRef<uint8_t> Data) : Section(Data) {}
 
   static bool classof(const SectionBase *S) {
-    return S->Type == ELF::SHT_DYNSYM;
+    return S->OriginalType == ELF::SHT_DYNSYM;
   }
 };
 
 class DynamicSection : public Section {
 public:
   explicit DynamicSection(ArrayRef<uint8_t> Data) : Section(Data) {}
 
   static bool classof(const SectionBase *S) {
-    return S->Type == ELF::SHT_DYNAMIC;
+    return S->OriginalType == ELF::SHT_DYNAMIC;
   }
 };
 
 class DynamicRelocationSection
     : public RelocSectionWithSymtabBase<DynamicSymbolTableSection> {
   MAKE_SEC_WRITER_FRIEND
 
 private:
   ArrayRef<uint8_t> Contents;
 
 public:
   explicit DynamicRelocationSection(ArrayRef<uint8_t> Data) : Contents(Data) {}
 
   void accept(SectionVisitor &) const override;
   void accept(MutableSectionVisitor &Visitor) override;
   Error removeSectionReferences(
       bool AllowBrokenLinks,
       function_ref<bool(const SectionBase *)> ToRemove) override;
 
   static bool classof(const SectionBase *S) {
-    if (!(S->Flags & ELF::SHF_ALLOC))
+    if (!(S->OriginalFlags & ELF::SHF_ALLOC))
       return false;
-    return S->Type == ELF::SHT_REL || S->Type == ELF::SHT_RELA;
+    return S->OriginalType == ELF::SHT_REL || S->OriginalType == ELF::SHT_RELA;
   }
 };
 
 class GnuDebugLinkSection : public SectionBase {
   MAKE_SEC_WRITER_FRIEND
 
 private:
   StringRef FileName;

llvm/tools/llvm-objcopy/ELF/Object.cpp

void GnuDebugLinkSection::init(StringRef File) {
   FileName = sys::path::filename(File);
   // The format for the .gnu_debuglink starts with the file name and is
   // followed by a null terminator and then the CRC32 of the file. The CRC32
   // should be 4 byte aligned. So we add the FileName size, a 1 for the null
   // byte, and then finally push the size to alignment and add 4.
   Size = alignTo(FileName.size() + 1, 4) + 4;
   // The CRC32 will only be aligned if we align the whole section.
   Align = 4;
-  Type = ELF::SHT_PROGBITS;
+  Type = OriginalType = ELF::SHT_PROGBITS;
   Name = ".gnu_debuglink";
   // For sections not found in segments, OriginalOffset is only used to
   // establish the order that sections should go in. By using the maximum
   // possible offset we cause this section to wind up at the end.
   OriginalOffset = std::numeric_limits<uint64_t>::max();
 }
 
 GnuDebugLinkSection::GnuDebugLinkSection(StringRef File,
template <class ELFT> void ELFBuilder<ELFT>::readSectionHeaders() {
   uint32_t Index = 0;
   for (const auto &Shdr : unwrapOrError(ElfFile.sections())) {
     if (Index == 0) {
       ++Index;
       continue;
     }
     auto &Sec = makeSection(Shdr);
     Sec.Name = unwrapOrError(ElfFile.getSectionName(&Shdr));
-    Sec.Type = Shdr.sh_type;
-    Sec.Flags = Shdr.sh_flags;
+    Sec.Type = Sec.OriginalType = Shdr.sh_type;
+    Sec.Flags = Sec.OriginalFlags = Shdr.sh_flags;
     Sec.Addr = Shdr.sh_addr;
     Sec.Offset = Shdr.sh_offset;
     Sec.OriginalOffset = Shdr.sh_offset;
     Sec.Size = Shdr.sh_size;
     Sec.Link = Shdr.sh_link;
     Sec.Info = Shdr.sh_info;
     Sec.Align = Shdr.sh_addralign;
     Sec.EntrySize = Shdr.sh_entsize;