[Object] Create MutableELFObject Class for Doing Mutations on ELFObjectFiles [Part 5]

[abrachet]

Summary

Micro patch to add ability to modify the files header

[jhenderson]

I think you need to be careful that you any methods in ELFObjectFile etc that query the header return the updated value. For example, I see there's a getStartAddress function, which returns the value of e_entry. If you change e_entry, you'll want this to return the updated value, I think.

[abrachet]

Made a virtual getHeader method in ELFObjectFile

[jhenderson]

I'm concerned with the getHeader() call changes in ELFObjectFile, mostly because again it's introducing a very obvious point of failure. What if somebody added a new piece of functionality to the base class to get another piece of data from the header? There's a good change they'll call EF.getHeader(). Say the new function is called getFoo(): if another user then decides they want to call getFoo() on a MutableELFObject, then they'll end up using the unmodified header rather than the modified one, which could cause surprising bugs. I don't have an obvious answer to this, but I'm beginning to wonder if we should be using a different style of inheritance, one that only makes the functions available that we know to be safe from a MutableELFObject's point of view. This might be something like private inheritance. I'm not sure though, as it would mean providing shim functions to re-publicise the "safe" functions we care about. What are your thoughts?

llvm/include/llvm/Object/MutableELFObject.h

363

Why do you need both this and getHeader()?

364

Rename this to getHeader() to ensure sensible function overloading, and have it return a pointer to the header, like the other version.

[abrachet]

Change getHeader() to return a reference not pointer.

[abrachet]

In D66063#1634844, @jhenderson wrote:

I'm concerned with the getHeader() call changes in ELFObjectFile, mostly because again it's introducing a very obvious point of failure. What if somebody added a new piece of functionality to the base class to get another piece of data from the header? There's a good change they'll call EF.getHeader(). Say the new function is called getFoo(): if another user then decides they want to call getFoo() on a MutableELFObject, then they'll end up using the unmodified header rather than the modified one, which could cause surprising bugs. I don't have an obvious answer to this, but I'm beginning to wonder if we should be using a different style of inheritance, one that only makes the functions available that we know to be safe from a MutableELFObject's point of view. This might be something like private inheritance. I'm not sure though, as it would mean providing shim functions to re-publicise the "safe" functions we care about. What are your thoughts?

Hmm that's a lot to think about. I don't love private inheritance because I think it would end up being a lot of boiler plate for the methods that are fine either way, I think.

Certainly it's true that EF.getHeader() would be gravitated towards for future changes. I think if a getFoo() is added and its something that MutableELFObject would want, then either way there needs to be a change either to use getHeader not EF.getHeader() or add getFoo() in MutableELFObject. Of course there's a difference because the bug couldn't exist if getFoo() isn't available.

You would know more about maintaining something like this so I'll go with whatever you (or others) have to say.

llvm/include/llvm/Object/MutableELFObject.h

364

I have moved to have a separate getMutableHeader that returns a mutable reference.

[MaskRay]

llvm/include/llvm/Object/MutableELFObject.h

93

If you use size_t in D64281, you shouldn't need the diff here.

Did you forget to rebase on latest Part 1,2,3?

[grimar]

llvm/include/llvm/Object/ELFObjectFile.h

501

I'd suggest using a reference here and in the other places. It looks more natural
as Header can't be nullptr.

onst Elf_Ehdr &Header = getHeader();

[jhenderson]

I've been trying and failing to come up with a good solution to the design. I only see three options:

1. Adding the getHeader function as this patch does and accepting some fragility. In practice, usages within LLVM are probably fine, because we'll have testing, but it might not work as well for those who wish to use the library in their own tools.
2. Using private inheritance or making the ELFObjectFile a private member of MutableELFObject instead of a base class. This would require MutableELFObject to opt in into functions that we know to be safe to use, and wouldn't allow usage of other functions. This is safer, but could result in a fair amount of extra code. This might need to be combined with 1) too to fix the specific issues this patch addresses.
3. Something more radical involving bigger changes in ELFObjectFile to prevent accessing the wrong header within the base class. I don't have anything specific in mind here at the moment.

Does anybody else have any thoughts on this?

llvm/unittests/Object/MutableELFObjectTest.cpp

352

Why has this changed from your previous (correct) version?

390

Why has this changed from your previous (correct) version?

483

Bad rebase?

616

Bad rebase?

[abrachet]

Addressed comments

[abrachet]

In D66063#1636993, @jhenderson wrote:

I've been trying and failing to come up with a good solution to the design. I only see three options:

1. Adding the getHeader function as this patch does and accepting some fragility. In practice, usages within LLVM are probably fine, because we'll have testing, but it might not work as well for those who wish to use the library in their own tools.
2. Using private inheritance or making the ELFObjectFile a private member of MutableELFObject instead of a base class. This would require MutableELFObject to opt in into functions that we know to be safe to use, and wouldn't allow usage of other functions. This is safer, but could result in a fair amount of extra code. This might need to be combined with 1) too to fix the specific issues this patch addresses.
3. Something more radical involving bigger changes in ELFObjectFile to prevent accessing the wrong header within the base class. I don't have anything specific in mind here at the moment.

Does anybody else have any thoughts on this?

I'll wait to see if anyone else chimes in before working on this

[jhenderson]

llvm/include/llvm/Object/ELFObjectFile.h

385

Adding a comment here saying make sure to use this function instead of calling the EF.getHeader() function directly is probably a good idea.

[rupprecht]

In D66063#1636993, @jhenderson wrote:

2. Using private inheritance or making the ELFObjectFile a private member of MutableELFObject instead of a base class. This would require MutableELFObject to opt in into functions that we know to be safe to use, and wouldn't allow usage of other functions. This is safer, but could result in a fair amount of extra code. This might need to be combined with 1) too to fix the specific issues this patch addresses.

This approach (not private inheritance, but making ELFObjectFile a private member + exposing specific accessors) seems to be the least fragile approach, but also the most verbose one, e.g. you may have to implement/override all the methods of EF that are used in subclasses.

llvm/include/llvm/Object/ELFObjectFile.h

385

Not sure I agree with changing this to a ref; keeping the type as a pointer would probably make this patch (and any potential down-tree fixes) simpler, even if it can't ever be null. I don't feel strongly though.

llvm/include/llvm/Object/MutableELFObject.h

364

This is logically the same thing as getHeader, is there a reason it needs to have a different name?

[abrachet]

In D66063#1639824, @rupprecht wrote:

In D66063#1636993, @jhenderson wrote:

2. Using private inheritance or making the ELFObjectFile a private member of MutableELFObject instead of a base class. This would require MutableELFObject to opt in into functions that we know to be safe to use, and wouldn't allow usage of other functions. This is safer, but could result in a fair amount of extra code. This might need to be combined with 1) too to fix the specific issues this patch addresses.

This approach (not private inheritance, but making ELFObjectFile a private member + exposing specific accessors) seems to be the least fragile approach, but also the most verbose one, e.g. you may have to implement/override all the methods of EF that are used in subclasses.

I have been thinking about this a lot recently. I do really like that MutableELFObject inherits from ELFObjectFile I think it reuses the most amount of code possible while still doing what we want. I think it isn't worth it to use MutableELFObject as a private member that it would be too verbose. Do you think it is sufficiently less fragile to warrant making the larger change? Better to be sure now when the code is much easier to change with all patches still open :)

I did have a question about private inheritance and what is safe to do. I think the most useful methods for llvm-objcopy are actually in the SectionRef SymbolRef and SegmentRef types so really the only methods I can see that need to be publicly exposed are the iterator returning ones. The problem is (not for SegmentRef because I can change that) those keep a pointer to an ObjectFile. From what I have tested, the virtual table is still constructed correctly and we can reinterpret_cast<ObjectFile*>(this) when creating Segment/SymbolRef's and the proper overrides will be called. Is this safe? If not then I guess there would be no difference in how much boilerplate we would need to add between using inheritance and having an ELFObjectFile as a member.

Lastly, how should I go about doing this? Should I switch to private inheritance in this patch or in Part 1 D64281?

[jhenderson]

In D66063#1642202, @abrachet wrote:

I have been thinking about this a lot recently. I do really like that MutableELFObject inherits from ELFObjectFile I think it reuses the most amount of code possible while still doing what we want. I think it isn't worth it to use MutableELFObject as a private member that it would be too verbose. Do you think it is sufficiently less fragile to warrant making the larger change? Better to be sure now when the code is much easier to change with all patches still open :)

I was discussing this issue with a colleague yesterday, to see if there's a clear path forward. He suggested that it might have made more sense to just modify the ELFObjectFile interface directly, rather than adding the MutableELFObject class. That would render the concerns we have a little less of an issue, since it would more clearly be a bug in the base class if the wrong things were called. You could also mess about with more objects to hide away the dangerous methods by doing this. I don't have a strong feeling about any of this either way. I think with sufficient testing and a large comment, you could probably get away without making the changes we've talked about, but it might be worth it still.

I did have a question about private inheritance and what is safe to do. I think the most useful methods for llvm-objcopy are actually in the SectionRef SymbolRef and SegmentRef types so really the only methods I can see that need to be publicly exposed are the iterator returning ones. The problem is (not for SegmentRef because I can change that) those keep a pointer to an ObjectFile. From what I have tested, the virtual table is still constructed correctly and we can reinterpret_cast<ObjectFile*>(this) when creating Segment/SymbolRef's and the proper overrides will be called. Is this safe? If not then I guess there would be no difference in how much boilerplate we would need to add between using inheritance and having an ELFObjectFile as a member.

I'm not massively familiar with how private inheritance works really, but the essential point is that a user of a subclass will never know that it's inheriting from the base class. I believe the virtual table will still exist, if you reinterpret_cast to the base class, and so it's safe enough to work with the base class this way, but it feels a bit wrong to be doing so, as it circumvents the whole point of why we are doing the inheritance this way at all. One advantage of private inheritance over a private member, I believe, is that you should be able to use the using directive to pull base-class functions into the public interface of the sub-class: https://softwareengineering.stackexchange.com/a/244910

Lastly, how should I go about doing this? Should I switch to private inheritance in this patch or in Part 1 D64281?

As this is a structural change that is not specific to this change (as in it doesn't introduce new functionality itself), it makes more sense to do the changes in part 1, if you're going to do them, but we should be sure it's the right thing to do first.

[abrachet]

I was discussing this issue with a colleague yesterday, to see if there's a clear path forward. He suggested that it might have made more sense to just modify the ELFObjectFile interface directly, rather than adding the MutableELFObject class.

So remove MutableELFObject and put it in ELFObjectFile? This might let ELFObjectFile no longer depend on ELFFile but I'm not entirely sure on that. It's also worth noting that as @rupprecht has pointed out many methods are now linear not constant so without doing some changes (could even be a boolean to track whether any modifications have been made) it probably shouldn't be used yet.

You could also mess about with more objects to hide away the dangerous methods by doing this.

Is this in reference to the earlier point about just modifying ELFObjectFile?

I believe the virtual table will still exist, if you reinterpret_cast to the base class, and so it's safe enough to work with the base class this way, but it feels a bit wrong to be doing so, as it circumvents the whole point of why we are doing the inheritance this way at all.

I think it isn't that bad to cast this to a base class if we know the methods that will be called in that context will always be safe to call. It is much more controlled this way but still bugs can happen if the base class methods get change. I don't feel too strongly.

One advantage of private inheritance over a private member, I believe, is that you should be able to use the using directive to pull base-class functions into the public interface of the sub-class:

Oh that's very useful I didn't know about that.

[jhenderson]

In D66063#1644617, @abrachet wrote:

I was discussing this issue with a colleague yesterday, to see if there's a clear path forward. He suggested that it might have made more sense to just modify the ELFObjectFile interface directly, rather than adding the MutableELFObject class.

So remove MutableELFObject and put it in ELFObjectFile? This might let ELFObjectFile no longer depend on ELFFile but I'm not entirely sure on that. It's also worth noting that as @rupprecht has pointed out many methods are now linear not constant so without doing some changes (could even be a boolean to track whether any modifications have been made) it probably shouldn't be used yet.

Yes, I was suggesting that an alternative design might be to simply expand the ELFObjectFile interface, but you raise a good point - I forgot about the complexity issue. We certainly wouldn't want to impair the complexity of the existing ELFObjectFile methods, at least unless we started to make modifications. The point about not relying on ELFFile isn't really the issue here. In fact, the class could for some situations. The important bit is that it is now ELFObjectFile's responsibility to be able to handle mutations, which means that changes to the ELFObjectFile class's code must support that, whereas in the current proposal, changes have to be made to ELFObejctFile's code to support a subclass which it otherwise doesn't care about.

You could also mess about with more objects to hide away the dangerous methods by doing this.

Is this in reference to the earlier point about just modifying ELFObjectFile?

Yes. You could add additional objects that wrap the ELFFile class and return either the original header or a mutated one, similar to what you are doing with MutableELFObject already, but the difference is that the wrapper object in this case goes in the base class, and thus it's never possible to access the dodgy method.

I believe the virtual table will still exist, if you reinterpret_cast to the base class, and so it's safe enough to work with the base class this way, but it feels a bit wrong to be doing so, as it circumvents the whole point of why we are doing the inheritance this way at all.

I think it isn't that bad to cast this to a base class if we know the methods that will be called in that context will always be safe to call. It is much more controlled this way but still bugs can happen if the base class methods get change. I don't feel too strongly.

reinterpret_cast is basically saying "pretend this thing here is actually this thing" which technically works in some situations, but bypasses the whole point of hiding methods via private inheritance. The function might be safe at the moment to call, but it doesn't guarantee it in the future, as the subclass hasn't got tests for that particular case. There may be other subtle correctness issues too.

llvm/include/llvm/Object/ELFObjectFile.h

for (const Elf_Shdr &Sec : *SectionsOrErr) {
 
         Attributes.Parse(Contents, ELFT::TargetEndianness == support::little);
         break;
       }
     }
     return Error::success();
   }
 
+  virtual const Elf_Ehdr &getHeader() const {
+    return *EF.getHeader();
+  }
+
   // This flag is used for classof, to distinguish ELFObjectFile from
   // its subclass. If more subclasses will be created, this flag will
   // have to become an enum.
   bool isDyldELFObject;
 
 public:
   ELFObjectFile(ELFObjectFile<ELFT> &&Other);
   static Expected<ELFObjectFile<ELFT>> create(MemoryBufferRef Object);
public:
 
   Expected<int64_t> getRelocationAddend(DataRefImpl Rel) const override;
 
   uint8_t getBytesInAddress() const override;
   StringRef getFileFormatName() const override;
   Triple::ArchType getArch() const override;
   Expected<uint64_t> getStartAddress() const override;
 
-  unsigned getPlatformFlags() const override { return EF.getHeader()->e_flags; }
+  unsigned getPlatformFlags() const override { return getHeader().e_flags; }
 
   const ELFFile<ELFT> *getELFFile() const { return &EF; }
 
   bool isDyldType() const { return isDyldELFObject; }
   static bool classof(const Binary *v) {
     return v->getType() == getELFType(ELFT::TargetEndianness == support::little,
                                       ELFT::Is64Bits);
   }

 
 template <class ELFT>
 uint64_t ELFObjectFile<ELFT>::getSymbolValueImpl(DataRefImpl Symb) const {
   const Elf_Sym *ESym = getSymbol(Symb);
   uint64_t Ret = ESym->st_value;
   if (ESym->st_shndx == ELF::SHN_ABS)
     return Ret;
 
-  const Elf_Ehdr *Header = EF.getHeader();
+  const Elf_Ehdr *Header = &getHeader();
   // Clear the ARM/Thumb or microMIPS indicator flag.
   if ((Header->e_machine == ELF::EM_ARM || Header->e_machine == ELF::EM_MIPS) &&
       ESym->getType() == ELF::STT_FUNC)
     Ret &= ~1;
 
   return Ret;
 }
 
 template <class ELFT>
 Expected<uint64_t>
 ELFObjectFile<ELFT>::getSymbolAddress(DataRefImpl Symb) const {
   uint64_t Result = getSymbolValue(Symb);
   const Elf_Sym *ESym = getSymbol(Symb);
   switch (ESym->st_shndx) {
   case ELF::SHN_COMMON:
   case ELF::SHN_UNDEF:
   case ELF::SHN_ABS:
     return Result;
   }
 
-  const Elf_Ehdr *Header = EF.getHeader();
+  const Elf_Ehdr *Header = &getHeader();
   auto SymTabOrErr = EF.getSection(Symb.d.a);
   if (!SymTabOrErr)
     return SymTabOrErr.takeError();
   const Elf_Shdr *SymTab = *SymTabOrErr;
 
   if (Header->e_type == ELF::ET_REL) {
     auto SectionOrErr = EF.getSection(ESym, SymTab, ShndxTable);
     if (!SectionOrErr)
uint32_t ELFObjectFile<ELFT>::getSymbolAlignment(DataRefImpl Symb) const {
   const Elf_Sym *Sym = getSymbol(Symb);
   if (Sym->st_shndx == ELF::SHN_COMMON)
     return Sym->st_value;
   return 0;
 }
 
 template <class ELFT>
 uint16_t ELFObjectFile<ELFT>::getEMachine() const {
-  return EF.getHeader()->e_machine;
+  return getHeader().e_machine;
 }
 
 template <class ELFT> uint16_t ELFObjectFile<ELFT>::getEType() const {
-  return EF.getHeader()->e_type;
+  return getHeader().e_type;
 }
 
 template <class ELFT>
 uint64_t ELFObjectFile<ELFT>::getSymbolSize(DataRefImpl Sym) const {
   return getSymbol(Sym)->st_size;
 }
 
 template <class ELFT>
uint32_t ELFObjectFile<ELFT>::getSymbolFlags(DataRefImpl Sym) const {
 
   auto DotSymtabSecSyms = EF.symbols(DotSymtabSec);
   if (DotSymtabSecSyms && ESym == (*DotSymtabSecSyms).begin())
     Result |= SymbolRef::SF_FormatSpecific;
   auto DotDynSymSecSyms = EF.symbols(DotDynSymSec);
   if (DotDynSymSecSyms && ESym == (*DotDynSymSecSyms).begin())
     Result |= SymbolRef::SF_FormatSpecific;
 
-  if (EF.getHeader()->e_machine == ELF::EM_ARM) {
+  if (getHeader().e_machine == ELF::EM_ARM) {
     if (Expected<StringRef> NameOrErr = getSymbolName(Sym)) {
       StringRef Name = *NameOrErr;
       if (Name.startswith("$d") || Name.startswith("$t") ||
           Name.startswith("$a"))
         Result |= SymbolRef::SF_FormatSpecific;
     } else {
       // TODO: Actually report errors helpfully.
       consumeError(NameOrErr.takeError());

 template <class ELFT>
 relocation_iterator
 ELFObjectFile<ELFT>::section_rel_begin(DataRefImpl Sec) const {
   DataRefImpl RelData;
   auto SectionsOrErr = EF.sections();
   if (!SectionsOrErr)
     return relocation_iterator(RelocationRef());
   uintptr_t SHT = reinterpret_cast<uintptr_t>((*SectionsOrErr).begin());
-  RelData.d.a = (Sec.p - SHT) / EF.getHeader()->e_shentsize;
+  RelData.d.a = (Sec.p - SHT) / getHeader().e_shentsize;
   RelData.d.b = 0;
   return relocation_iterator(RelocationRef(RelData, this));
 }
 
 template <class ELFT>
 relocation_iterator
 ELFObjectFile<ELFT>::section_rel_end(DataRefImpl Sec) const {
   const Elf_Shdr *S = reinterpret_cast<const Elf_Shdr *>(Sec.p);
ELFObjectFile<ELFT>::section_rel_end(DataRefImpl Sec) const {
 
   RelData.d.b += S->sh_size / S->sh_entsize;
   return relocation_iterator(RelocationRef(RelData, this));
 }
 
 template <class ELFT>
 section_iterator
 ELFObjectFile<ELFT>::getRelocatedSection(DataRefImpl Sec) const {
-  if (EF.getHeader()->e_type != ELF::ET_REL)
+  if (getHeader().e_type != ELF::ET_REL)
     return section_end();
 
   const Elf_Shdr *EShdr = getSection(Sec);
   uintX_t Type = EShdr->sh_type;
   if (Type != ELF::SHT_REL && Type != ELF::SHT_RELA)
     return section_end();
 
   auto R = EF.getSection(EShdr->sh_info);
uint64_t ELFObjectFile<ELFT>::getRelocationType(DataRefImpl Rel) const {
   if (sec->sh_type == ELF::SHT_REL)
     return getRel(Rel)->getType(EF.isMips64EL());
   else
     return getRela(Rel)->getType(EF.isMips64EL());
 }
 
 template <class ELFT>
 StringRef ELFObjectFile<ELFT>::getRelocationTypeName(uint32_t Type) const {
-  return getELFRelocationTypeName(EF.getHeader()->e_machine, Type);
+  return getELFRelocationTypeName(getHeader().e_machine, Type);
 }
 
 template <class ELFT>
 void ELFObjectFile<ELFT>::getRelocationTypeName(
     DataRefImpl Rel, SmallVectorImpl<char> &Result) const {
   uint32_t type = getRelocationType(Rel);
   EF.getRelocationTypeName(type, Result);
 }

 template <class ELFT>
 uint8_t ELFObjectFile<ELFT>::getBytesInAddress() const {
   return ELFT::Is64Bits ? 8 : 4;
 }
 
 template <class ELFT>
 StringRef ELFObjectFile<ELFT>::getFileFormatName() const {
   bool IsLittleEndian = ELFT::TargetEndianness == support::little;
-  switch (EF.getHeader()->e_ident[ELF::EI_CLASS]) {
+  switch (getHeader().e_ident[ELF::EI_CLASS]) {
   case ELF::ELFCLASS32:
-    switch (EF.getHeader()->e_machine) {
+    switch (getHeader().e_machine) {
     case ELF::EM_386:
       return "ELF32-i386";
     case ELF::EM_IAMCU:
       return "ELF32-iamcu";
     case ELF::EM_X86_64:
       return "ELF32-x86-64";
     case ELF::EM_ARM:
       return (IsLittleEndian ? "ELF32-arm-little" : "ELF32-arm-big");
case ELF::ELFCLASS32:
     case ELF::EM_SPARC32PLUS:
       return "ELF32-sparc";
     case ELF::EM_AMDGPU:
       return "ELF32-amdgpu";
     default:
       return "ELF32-unknown";
     }
   case ELF::ELFCLASS64:
-    switch (EF.getHeader()->e_machine) {
+    switch (getHeader().e_machine) {
     case ELF::EM_386:
       return "ELF64-i386";
     case ELF::EM_X86_64:
       return "ELF64-x86-64";
     case ELF::EM_AARCH64:
       return (IsLittleEndian ? "ELF64-aarch64-little" : "ELF64-aarch64-big");
     case ELF::EM_PPC64:
       return "ELF64-ppc64";
StringRef ELFObjectFile<ELFT>::getFileFormatName() const {
   default:
     // FIXME: Proper error handling.
     report_fatal_error("Invalid ELFCLASS!");
   }
 }
 
 template <class ELFT> Triple::ArchType ELFObjectFile<ELFT>::getArch() const {
   bool IsLittleEndian = ELFT::TargetEndianness == support::little;
-  switch (EF.getHeader()->e_machine) {
+  switch (getHeader().e_machine) {
   case ELF::EM_386:
   case ELF::EM_IAMCU:
     return Triple::x86;
   case ELF::EM_X86_64:
     return Triple::x86_64;
   case ELF::EM_AARCH64:
     return IsLittleEndian ? Triple::aarch64 : Triple::aarch64_be;
   case ELF::EM_ARM:
     return Triple::arm;
   case ELF::EM_AVR:
     return Triple::avr;
   case ELF::EM_HEXAGON:
     return Triple::hexagon;
   case ELF::EM_LANAI:
     return Triple::lanai;
   case ELF::EM_MIPS:
-    switch (EF.getHeader()->e_ident[ELF::EI_CLASS]) {
+    switch (getHeader().e_ident[ELF::EI_CLASS]) {
     case ELF::ELFCLASS32:
       return IsLittleEndian ? Triple::mipsel : Triple::mips;
     case ELF::ELFCLASS64:
       return IsLittleEndian ? Triple::mips64el : Triple::mips64;
     default:
       report_fatal_error("Invalid ELFCLASS!");
     }
   case ELF::EM_MSP430:
     return Triple::msp430;
   case ELF::EM_PPC:
     return Triple::ppc;
   case ELF::EM_PPC64:
     return IsLittleEndian ? Triple::ppc64le : Triple::ppc64;
   case ELF::EM_RISCV:
-    switch (EF.getHeader()->e_ident[ELF::EI_CLASS]) {
+    switch (getHeader().e_ident[ELF::EI_CLASS]) {
     case ELF::ELFCLASS32:
       return Triple::riscv32;
     case ELF::ELFCLASS64:
       return Triple::riscv64;
     default:
       report_fatal_error("Invalid ELFCLASS!");
     }
   case ELF::EM_S390:
     return Triple::systemz;
 
   case ELF::EM_SPARC:
   case ELF::EM_SPARC32PLUS:
     return IsLittleEndian ? Triple::sparcel : Triple::sparc;
   case ELF::EM_SPARCV9:
     return Triple::sparcv9;
 
   case ELF::EM_AMDGPU: {
     if (!IsLittleEndian)
       return Triple::UnknownArch;
 
-    unsigned MACH = EF.getHeader()->e_flags & ELF::EF_AMDGPU_MACH;
+    unsigned MACH = getHeader().e_flags & ELF::EF_AMDGPU_MACH;
     if (MACH >= ELF::EF_AMDGPU_MACH_R600_FIRST &&
         MACH <= ELF::EF_AMDGPU_MACH_R600_LAST)
       return Triple::r600;
     if (MACH >= ELF::EF_AMDGPU_MACH_AMDGCN_FIRST &&
         MACH <= ELF::EF_AMDGPU_MACH_AMDGCN_LAST)
       return Triple::amdgcn;
 
     return Triple::UnknownArch;
   }
 
   case ELF::EM_BPF:
     return IsLittleEndian ? Triple::bpfel : Triple::bpfeb;
 
   default:
     return Triple::UnknownArch;
   }
 }
 
 template <class ELFT>
 Expected<uint64_t> ELFObjectFile<ELFT>::getStartAddress() const {
-  return EF.getHeader()->e_entry;
+  return getHeader().e_entry;
 }
 
 template <class ELFT>
 ELFObjectFileBase::elf_symbol_iterator_range
 ELFObjectFile<ELFT>::getDynamicSymbolIterators() const {
   return make_range(dynamic_symbol_begin(), dynamic_symbol_end());
 }
 
 template <class ELFT> bool ELFObjectFile<ELFT>::isRelocatableObject() const {
-  return EF.getHeader()->e_type == ELF::ET_REL;
+  return getHeader().e_type == ELF::ET_REL;
 }
 
 } // end namespace object
 } // end namespace llvm
 
 #endif // LLVM_OBJECT_ELFOBJECTFILE_H

llvm/include/llvm/Object/MutableELFObject.h

template <typename ELFT> class MutableELFObject : public ELFObjectFile<ELFT> {
   /// would otherwise need to be: entries with no modifications never get
   /// copied and the only overhead for those is an index. Entries which get
   /// modified can have richer types during program executation than are
   /// allowed by the object file standard.
   template <typename OrigType, typename NewType> class MutableTable {
     struct MappingType {
       enum MappedType { Original, New, Removed };
 
-      size_t Index;
+      uint64_t Index;
       MappedType Type;
 
-      MappingType(size_t Index, MappedType Type) : Index(Index), Type(Type) {}
+      MappingType(uint64_t Index, MappedType Type) : Index(Index), Type(Type) {}
 
-      operator size_t() const { return Index; }
+      operator uint64_t() const { return Index; }
     };
 
     ArrayRef<OrigType> OriginalValues;
     std::vector<NewType> NewValues;
     std::vector<MappingType> Mappings;
 
   public:
     explicit MutableTable(ArrayRef<OrigType> TheOriginalValues)
         : OriginalValues(TheOriginalValues) {
-      size_t Count = 0;
+      uint64_t Count = 0;
       std::transform(OriginalValues.begin(), OriginalValues.end(),
                      std::back_inserter(Mappings), [&Count](const OrigType &) {
                        return MappingType(Count++, MappingType::Original);
                      });
     }
 
     /// Get the OrigType at index Index regardless of whether it is an OrigType
     /// or NewType. In the case that Mappings[Index].Type == New, call NewTypes
     /// operator OrigType to make the proper conversion.
-    const OrigType &operator[](size_t Index) const {
+    const OrigType &operator[](uint64_t Index) const {
       assert(Index < Mappings.size() && "Out of bounds");
       assert(Mappings[Index].Type != MappingType::Removed);
       if (Mappings[Index].Type == MappingType::New)
         return static_cast<const OrigType &>(NewValues[Mappings[Index]]);
       return OriginalValues[Mappings[Index]];
     }
 
     /// Get the OrigType at index Index. This method ignores any changes made
     /// and always returns the OrigType from its original state at its original
     /// index.
-    const OrigType &getOriginal(size_t Index) const {
+    const OrigType &getOriginal(uint64_t Index) const {
       assert(Index < OriginalValues.size() && "Out of bounds");
       assert(Mappings[Index].Type != MappingType::Removed);
       return OriginalValues[Index];
     }
 
-    /// Gets the index of the next entry that hasn't been removed after \param
-    /// CurrentIndex. If there is no entry after \param CurrentIndex which has
-    /// not been removed, this returns the index past the last entry.
-    size_t getNextIndex(size_t CurrentIndex) const {
-      assert(CurrentIndex < Mappings.size());
-      return *llvm::find_if(
-          seq(CurrentIndex + 1, Mappings.size()), [this](size_t Index) {
-            return Mappings[Index].Type != MappingType::Removed;
-          });
+    /// Gets next index that hasn't been removed after \param CurrentIndex.
+    uint64_t getNextIndex(uint64_t CurrentIndex) const {
+      auto Range = seq(CurrentIndex + 1, (uint64_t)Mappings.size());
+      return *std::find_if(Range.begin(), Range.end(), [this](uint64_t Index) {
     }
 
     /// Returns the first index which has not been removed.
-    size_t getFirstIndex() const {
+    uint64_t getFirstIndex() const {
       return Mappings[0].Type != MappingType::Removed ? 0 : getNextIndex(0);
     }
 
     /// Counts the number of entries that haven't been removed between 0 and
     /// \param Index.
-    size_t getEffectiveIndex(size_t Index) const {
-      size_t Ret = 0;
-      for (size_t I = 0; I < Index; ++I)
+    uint64_t getEffectiveIndex(uint64_t Index) const {
+      uint64_t Ret = 0;
+      for (uint64_t I = 0; I < Index; ++I)
         if (Mappings[I].Type != MappingType::Removed)
           ++Ret;
       return Ret;
     }
 
-    /// Returns the index of the last element. This is different to size()
-    /// because size() returns the number of entries that haven't been removed.
+    /// different to size() because size() returns the number of entries that
+    /// haven't been removed.
     size_t getEndIndex() const { return Mappings.size(); }
 
     /// Removes the entry at \param Index.
-    void remove(size_t Index) {
+    void remove(uint64_t Index) {
       assert(Index < Mappings.size() && "Out of bounds");
       assert(Mappings[Index].Type != MappingType::Removed &&
              "Entry already removed");
       Mappings[Index].Type = MappingType::Removed;
     }
 
     /// Add's \param Value to the end of the table.
     void add(NewType &&Value) {
       NewValues.emplace_back(std::move(Value));
       Mappings.emplace_back(NewValues.size() - 1, MappingType::New);
     }
 
     /// If the entry at index Index has already been made mutable, this returns
     /// a reference to that. Otherwise, this replaces the current entry at the
     /// specified index with a NewType constructued with Arguments.
     template <typename... Args>
-    NewType &makeMutable(size_t Index, Args &&... Arguments) {
+    NewType &makeMutable(uint64_t Index, Args &&... Arguments) {
       assert(Index < Mappings.size() && "Out of bounds");
       assert(Mappings[Index].Type != MappingType::Removed);
       if (Mappings[Index].Type == MappingType::New)
         return NewValues[Mappings[Index]];
       NewValues.emplace_back(Arguments...);
       Mappings[Index] = MappingType(NewValues.size() - 1, MappingType::New);
       return NewValues.back();
     }
 
     /// Returns a pointer to the NewType if the entry at the specified index
     /// has had makeMutable called on it. Otherwise this method returns nullptr.
-    const NewType *getConstIfNew(size_t Index) const {
+    const NewType *getConstIfNew(uint64_t Index) const {
       assert(Index < Mappings.size() && "Out of bounds");
       assert(Mappings[Index].Type != MappingType::Removed);
       return Mappings[Index].Type == MappingType::New
                  ? &NewValues[Mappings[Index]]
                  : nullptr;
     }
 
     /// Return the number of elements in the table.
     size_t size() const {
       return llvm::count_if(Mappings, [](MappingType &Mapping) {
         return Mapping.Type != MappingType::Removed;
       });
     }
 
     size_t originalSize() const { return OriginalValues.size(); }
   };
 
   using Elf_Shdr = typename ELFT::Shdr;
   using Elf_Ehdr = typename ELFT::Ehdr;
   using Elf_Sym = typename ELFT::Sym;
 
   using MutableSymbolTable = MutableTable<Elf_Sym, MutableELFSymbol<ELFT>>;
 
+  Elf_Ehdr FileHeader;
   MutableTable<Elf_Shdr, MutableELFSection<ELFT>> Sections;
   MutableSymbolTable Symbols;
   MutableSymbolTable DynSymbols;
 
-  const Elf_Ehdr &getHeader() const {
-    return *reinterpret_cast<const Elf_Ehdr *>(this->base());
-  }
-
   /// Many getSection* methods in ELFObjectFile use getSection to get the
   /// the header associated with that section. This override returns a valid
   /// section header whether the section has been modified or not.
   const Elf_Shdr *getSection(DataRefImpl Sec) const override {
     return &Sections[Sec.p];
   }
 
   const Elf_Sym *getSymbol(DataRefImpl Sym) const override {
template <typename ELFT> class MutableELFObject : public ELFObjectFile<ELFT> {
   }
 
   static DataRefImpl toDataRef(uintptr_t Ptr);
   static DataRefImpl toDataRef(uint32_t A, uint32_t B);
 
 public:
   explicit MutableELFObject(ELFObjectFile<ELFT> &&B)
       : ELFObjectFile<ELFT>(std::move(B)),
+        FileHeader(*reinterpret_cast<const Elf_Ehdr *>(this->base())),
         Sections(ArrayRef<Elf_Shdr>(reinterpret_cast<const Elf_Shdr *>(
                                         this->base() + getHeader().e_shoff),
                                     getHeader().e_shnum)),
         Symbols(findSymbolTable(ELF::SHT_SYMTAB)),
         DynSymbols(findSymbolTable(ELF::SHT_DYNSYM)) {}
 
   section_iterator section_begin() const override {
     return section_iterator(
void addSection(MutableELFSection<ELFT> &&Sec) {
     Sections.add(std::move(Sec));
   }
 
   /// Removes a symbol.
   void removeSymbol(SymbolRef Sym) {
     assert(Sections.getOriginal(Sym.getRawDataRefImpl().d.a).sh_type ==
                ELF::SHT_SYMTAB &&
            "Not pointing to symbol table");
-    assert(Sym.getRawDataRefImpl().d.b && "Cannot remove index 0 symbol");
     Symbols.remove(Sym.getRawDataRefImpl().d.b);
   }
 
   /// Removes a section.
   void removeSection(SectionRef Sec) {
-    assert(Sec.getRawDataRefImpl().p && "Cannot remove index 0 section");
     Sections.remove(Sec.getRawDataRefImpl().p);
   }
+
+  const Elf_Ehdr &getHeader() const override { return FileHeader; }
+  Elf_Ehdr &getMutableHeader() { return FileHeader; }
 };
 
 template <typename ELFT>
 void MutableELFObject<ELFT>::moveSectionNext(DataRefImpl &Sec) const {
   Sec.p = Sections.getNextIndex(Sec.p);
 }
 
 template <typename ELFT>

llvm/unittests/Object/MutableELFObjectTest.cpp

- Name:    test
     Value:   2
     Binding: STB_GLOBAL)");
 
   ASSERT_THAT_EXPECTED(ErrOrObj, Succeeded());
   auto *ELFObjFile = dyn_cast<ELFObjectFile<ELF64LE>>(ErrOrObj->get());
   ASSERT_TRUE(ELFObjFile);
   MutableELFObject<ELF64LE> MutableObject(std::move(*ELFObjFile));
 
-  symbol_iterator TestSym(++MutableObject.symbol_begin());
+  auto TestSym = symbol_iterator(++MutableObject.symbol_begin());
   Expected<MutableELFSymbol<ELF64LE> &> MutSymOrErr =
       MutableObject.getMutableSymbol(*TestSym);
   ASSERT_THAT_EXPECTED(MutSymOrErr, Succeeded());
   EXPECT_EQ(MutSymOrErr->Header.st_value, 2u);
   MutSymOrErr->Header.st_value = 5;
   EXPECT_EQ(TestSym->getValue(), 5u);
 
   EXPECT_EQ(MutSymOrErr->Name, "test");
- Name:    test
     Value:   0x1234
     Binding: STB_GLOBAL)");
 
   ASSERT_THAT_EXPECTED(ErrOrObj, Succeeded());
   auto *ELFObjFile = dyn_cast<ELFObjectFile<ELF64LE>>(ErrOrObj->get());
   ASSERT_TRUE(ELFObjFile);
   MutableELFObject<ELF64LE> MutableObject(std::move(*ELFObjFile));
 
-  symbol_iterator DynSym(++MutableObject.dynamic_symbol_begin());
+  auto DynSym = symbol_iterator(++MutableObject.dynamic_symbol_begin());
   EXPECT_EQ(DynSym->getValue(), 0x1234U);
   EXPECT_TRUE(DynSym->getFlags() & SymbolRef::SF_Global);
   auto NameOrErr = DynSym->getName();
   ASSERT_THAT_EXPECTED(NameOrErr, Succeeded());
   EXPECT_EQ(*NameOrErr, "test");
 }
 
 // Test removeSymbol method.
- Name:    .remove_me_too
     Type:    SHT_PROGBITS)");
 
   ASSERT_THAT_EXPECTED(ErrOrObj, Succeeded());
   auto *ELFObjFile = dyn_cast<ELFObjectFile<ELF64LE>>(ErrOrObj->get());
   ASSERT_TRUE(ELFObjFile);
   MutableELFObject<ELF64LE> MutableObject(std::move(*ELFObjFile));
 
   // 4 additional sections, index 0, .symtab, .strtab and .shstrtab are added
-  // by yaml2obj.
+  // yaml2obj.
   EXPECT_EQ(
       std::distance(MutableObject.section_begin(), MutableObject.section_end()),
       6);
 
-  std::vector<StringRef> SectionNames;
-  llvm::transform(MutableObject.sections(), std::back_inserter(SectionNames),
-                  [](SectionRef Sec) {
-                    StringRef Name;
-                    Sec.getName(Name);
-                    return Name;
-                  });
-  EXPECT_THAT(SectionNames,
-              ::testing::ElementsAre("", ".remove_me", ".remove_me_too",
-                                     ".symtab", ".strtab", ".shstrtab"));
-
   auto Iter = ++MutableObject.section_begin();
   StringRef Name;
   ASSERT_FALSE(Iter->getName(Name));
   EXPECT_EQ(Name, ".remove_me");
 
   MutableObject.removeSection(*Iter);
 
   EXPECT_EQ(
EXPECT_EQ(
       1);
 
   Elf_Sym_Impl<ELF64LE> Sym;
   for (size_t I = 0; I < 100; ++I)
     MutableObject.addSymbol({Sym, ""});
 
   EXPECT_EQ(
       std::distance(MutableObject.symbol_begin(), MutableObject.symbol_end()),
-      101);
+      10001);
 }
 
 // Add and then remove a symbol.
 TEST(MutableELFObject, AddRemoveSymbol) {
   SmallString<0> Storage;
   Expected<std::unique_ptr<ObjectFile>> ErrOrObj = yaml2ObjectFile(Storage, R"(
 --- !ELF
 FileHeader:
FileHeader:
 
   MutableObject.addSection({Header, "", OwningArrayRef<uint8_t>(0)});
   MutableObject.addSection({Header, "", OwningArrayRef<uint8_t>(0)});
   Header.sh_size = 100;
   MutableObject.addSection({Header, "", OwningArrayRef<uint8_t>(0)});
   EXPECT_EQ(Distance(), 8);
   EXPECT_EQ(IterAtIndex(7)->getSize(), 100u);
 }
+
+TEST(MutableELFObject, UpdateHeader) {
+  SmallString<0> Storage;
+  Expected<std::unique_ptr<ObjectFile>> ErrOrObj = yaml2ObjectFile(Storage, R"(
+--- !ELF
+FileHeader:
+  Class:     ELFCLASS64
+  Data:      ELFDATA2LSB
+  Type:      ET_REL
+  Machine:   EM_X86_64)");
+
+  ASSERT_THAT_EXPECTED(ErrOrObj, Succeeded());
+  auto *ELFObjFile = dyn_cast<ELFObjectFile<ELF64LE>>(ErrOrObj->get());
+  ASSERT_TRUE(ELFObjFile);
+  MutableELFObject<ELF64LE> MutableObject(std::move(*ELFObjFile));
+
+  Elf_Ehdr_Impl<ELF64LE> &Header = MutableObject.getMutableHeader();
+  EXPECT_EQ(Header.e_type, ELF::ET_REL);
+  Header.e_type = ELF::ET_EXEC;
+  EXPECT_EQ(MutableObject.getHeader().e_type, ELF::ET_EXEC);
+  EXPECT_FALSE(MutableObject.isRelocatableObject());
+
+  Header.e_type = ELF::ET_REL;
+  EXPECT_TRUE(MutableObject.isRelocatableObject());
+
+  Header.e_flags = 100;
+  EXPECT_EQ(MutableObject.getPlatformFlags(), 100u);
+
+  Header.e_entry = 5;
+  Expected<uint64_t> StartAddr = MutableObject.getStartAddress();
+  ASSERT_THAT_EXPECTED(StartAddr, Succeeded());
+  EXPECT_EQ(*StartAddr, 5u);
+}