[llvm-objcopy]Allow llvm-objcopy to be used on an ELF file with no section headers

[jhenderson]

Summary

This patch fixes https://bugs.llvm.org/show_bug.cgi?id=41293 and https://bugs.llvm.org/show_bug.cgi?id=41045. llvm-objcopy assumed that it could always read a section header string table. This isn't the case when the sections were previously all stripped, and the e_shstrndx field was set to 0. This patch fixes this. It also fixes a double space in an error message relating to this issue, and prevents llvm-objcopy from adding extra space for non-existent section headers, meaning that --strip-sections on the output of a previous --strip-sections run produces identical output, simplifying the test.

[grimar]

Few nits/questions from me.

test/tools/llvm-objcopy/ELF/copy-after-strip-sections.test

24

Is this field important?

29

Do you need PAddr for this test?

tools/llvm-objcopy/ELF/Object.cpp

1570

size is a bit strange helper. I do not think it is common to use it instead of .size() call
for a container in LLVM..
I see it is not your initial code, but since you're changing it, what do you think about not using it here at all?

tools/llvm-objcopy/ELF/Object.h

233

Up to you perhaps, but I would suggest to move it out to the constructor body.

[jhenderson]

test/tools/llvm-objcopy/ELF/copy-after-strip-sections.test

24

Not massively, no. But it does force the section to appear at offset 0x1000. I was originally going to check the contents, but I decided not to do that, so I can get rid of it and the Address below.

29

It's not important. Most of our tests just have both. I can remove it and VAddr too, given the other changes.

[jhenderson]

tools/llvm-objcopy/ELF/Object.cpp

1570

The underlying container doesn't currently have a size() method. There are quite a few places in this file that seem to use this helper, so perhaps it should be a different change?

[grimar]

tools/llvm-objcopy/ELF/Object.cpp

1570

Oh, I thought it has. I have no objections then.

[jhenderson]

Address review comments.

[grimar]

LGTM.

[rupprecht]

tools/llvm-objcopy/ELF/Object.h

234

Slightly simpler:

ELFWriter(Object &Obj, Buffer &Buf, bool WSH)
    : Writer(Obj, Buf), WriteSectionHeaders(WSH && Obj.HadShdrs) {}

[jhenderson]

Rebase and address comment.

Unfortunately, this build fails on Linux. Somehow MSVC is able to handle the incomplete type of Object in the ELFWriter constructor, whereas the version of GCC that I'm using doesn't. The Writer classes aren't really part of the Object interface, so I'm going to move them into a separate header.

[jhenderson]

Move constructor definition into .cpp file to avoid dependency on Object issues.

[grimar]

LGTM

test/tools/llvm-objcopy/ELF/copy-after-strip-sections.test

+# llvm-objcopy's --strip-sections removes the section headers. It should be
+# possible to run the tool on the output after this operation. Performing any
+# subsequent stripping operation, or copying the object, should produce
+# identical output.
+
+# RUN: yaml2obj %s -o %t.in
+# RUN: llvm-objcopy %t.in %t.stripped --strip-sections
+# RUN: llvm-objcopy %t.stripped %t.stripped2 --strip-sections
+# RUN: llvm-objcopy %t.stripped2 %t.out
+# RUN: cmp %t.stripped %t.stripped2
+# RUN: cmp %t.stripped %t.out
+
+--- !ELF
+FileHeader:
+  Class:   ELFCLASS64
+  Data:    ELFDATA2LSB
+  Type:    ET_EXEC
+  Machine: EM_X86_64
+Sections:
+  - Name: .text
+    Type: SHT_PROGBITS
+    Content: 'facefeed'
+ProgramHeaders:
+  - Type: PT_LOAD
+    Sections:
+      - Section: .text

tools/llvm-objcopy/ELF/Object.h

private:
   void assignOffsets();
 
   std::unique_ptr<ELFSectionWriter<ELFT>> SecWriter;
 
   size_t totalSize() const;
 
 public:
   virtual ~ELFWriter() {}
-  bool WriteSectionHeaders = true;
+  bool WriteSectionHeaders;
 
   Error finalize() override;
   Error write() override;
-  ELFWriter(Object &Obj, Buffer &Buf, bool WSH)
-      : Writer(Obj, Buf), WriteSectionHeaders(WSH) {}
+  ELFWriter(Object &Obj, Buffer &Buf, bool WSH);
 };
 
 class BinaryWriter : public Writer {
 private:
   std::unique_ptr<BinarySectionWriter> SecWriter;
 
   uint64_t TotalSize;
 
 public:
public:
   uint8_t ABIVersion;
   uint64_t Entry;
   uint64_t SHOffset;
   uint32_t Type;
   uint32_t Machine;
   uint32_t Version;
   uint32_t Flags;
 
+  bool HadShdrs = true;
   StringTableSection *SectionNames = nullptr;
   SymbolTableSection *SymbolTable = nullptr;
   SectionIndexSection *SectionIndexTable = nullptr;
 
   void sortSections();
   SectionTableRef sections() { return SectionTableRef(Sections); }
   ConstRange<SectionBase> sections() const {
     return make_pointee_range(Sections);

tools/llvm-objcopy/ELF/Object.cpp

template <class ELFT> void ELFBuilder<ELFT>::build() {
 
   readSectionHeaders();
   readProgramHeaders();
 
   uint32_t ShstrIndex = Ehdr.e_shstrndx;
   if (ShstrIndex == SHN_XINDEX)
     ShstrIndex = unwrapOrError(ElfFile.getSection(0))->sh_link;
 
+  if (ShstrIndex == SHN_UNDEF)
+    Obj.HadShdrs = false;
+  else
     Obj.SectionNames =
         Obj.sections().template getSectionOfType<StringTableSection>(
             ShstrIndex,
             "e_shstrndx field value " + Twine(Ehdr.e_shstrndx) +
-              " in elf header " + " is invalid",
+                " in elf header is invalid",
             "e_shstrndx field value " + Twine(Ehdr.e_shstrndx) +
-              " in elf header " + " is not a string table");
+                " in elf header is not a string table");
 }
 
 Writer::~Writer() {}
 
 Reader::~Reader() {}
 
 std::unique_ptr<Object> BinaryReader::create() const {
   return BinaryELFBuilder(MInfo.EMachine, MemBuf).build();
if (Parent == nullptr || Sec.Type == SHT_NOBITS || Sec.Size == 0)
       continue;
     uint64_t Offset =
         Sec.OriginalOffset - Parent->OriginalOffset + Parent->Offset;
     uint8_t *B = Buf.getBufferStart();
     std::memset(B + Offset, 0, Sec.Size);
   }
 }
 
+template <class ELFT>
+ELFWriter<ELFT>::ELFWriter(Object &Obj, Buffer &Buf, bool WSH)
+    : Writer(Obj, Buf), WriteSectionHeaders(WSH && Obj.HadShdrs) {}
+
 Error Object::removeSections(
     std::function<bool(const SectionBase &)> ToRemove) {
 
   auto Iter = std::stable_partition(
       std::begin(Sections), std::end(Sections), [=](const SecPtr &Sec) {
         if (ToRemove(*Sec))
           return false;
         if (auto RelSec = dyn_cast<RelocationSectionBase>(Sec.get())) {
template <class ELFT> void ELFWriter<ELFT>::assignOffsets() {
   if (WriteSectionHeaders)
     Offset = alignTo(Offset, sizeof(Elf_Addr));
   Obj.SHOffset = Offset;
 }
 
 template <class ELFT> size_t ELFWriter<ELFT>::totalSize() const {
   // We already have the section header offset so we can calculate the total
   // size by just adding up the size of each section header.
-  auto NullSectionSize = WriteSectionHeaders ? sizeof(Elf_Shdr) : 0;
-  return Obj.SHOffset + Obj.sections().size() * sizeof(Elf_Shdr) +
-         NullSectionSize;
+  if (!WriteSectionHeaders)
+    return Obj.SHOffset;
+  size_t ShdrCount = Obj.sections().size() + 1; // Includes null shdr.
+  return Obj.SHOffset + ShdrCount * sizeof(Elf_Shdr);
 }
 
 template <class ELFT> Error ELFWriter<ELFT>::write() {
   // Segment data must be written first, so that the ELF header and program
   // header tables can overwrite it, if covered by a segment.
   writeSegmentData();
   writeEhdr();
   writePhdrs();