[llvm-objcopy] Fix major layout bugs in llvm-objcopy

[jakehehrlich]

Summary

Somehow a few massive errors slipped though the cracks of testing.

1. The code in Segment::finalize was left over from the old layout algorithm. In certain situations this would cause very strange issues with segment layout. For instance in the shift-segments.test case it would cause the second segment to have the same offset as the first.
2. In debugging this I discovered another issue. Namely section alignment was not being computed based on Section->Align but instead Section->Offset which is bizarre and makes no sense. I have no clue how it worked in the first place. This issue is also fixed
3. Fixing #2 exposed a bug where things were not being written past the end of the file that technically should have been. This was because in certain cases (like overlapping-segments) the end of the file wouldn't always be bumped if the offset could be chosen relative to an existing segment that already had it's offset chosen. For fully nested segments this is fine but for overlapping segments this leaves the end of the file short. So I changed how the offset is bumped when looping though segments.

[jhenderson]

I'm equally at a loss as to how the align bug wasn't picked up earlier. I think it would be wise to take a look and see which test(s) should be covering this case, and why they weren't failing before.

test/tools/llvm-objcopy/segment-shift.test

56 ↗

(On Diff #117242)

This address looks wrong. Shouldn't it be 0x3000, i.e. the address of .text3?

tools/llvm-objcopy/Object.cpp

33 ↗

(On Diff #117242)

I don't think we need this function at all anymore.

[jhenderson]

test/tools/llvm-objcopy/segment-shift.test

44 ↗

(On Diff #117242)

Actually, the same goes for this segment too - the address looks wrong.

[jakehehrlich]

1. Removed all traces of Segment::finalize
2. Added proper addresses to program headers in shift test to make sure addresses are also being tested

test/tools/llvm-objcopy/segment-shift.test

56 ↗

(On Diff #117242)

In yaml2obj the addresses of the program headers default to 0x0 and the user has to manually add them (this "feature" of yaml2obj is my fault). I'll add the desired addresses to make things consistent

[jhenderson]

LGTM.

llvm/trunk/test/tools/llvm-objcopy/segment-shift.test

+# RUN: yaml2obj %s -o %t
+# RUN: llvm-objcopy %t %t2
+# RUN: llvm-readobj -program-headers %t2 | FileCheck %s
+
+!ELF
+FileHeader:
+  Class:           ELFCLASS64
+  Data:            ELFDATA2LSB
+  Type:            ET_EXEC
+  Machine:         EM_X86_64
+Sections:
+  - Name:            .text
+    Type:            SHT_PROGBITS
+    Flags:           [ SHF_ALLOC, SHF_EXECINSTR ]
+    Address:         0x1000
+    AddressAlign:    0x1000
+    Size:            0x1000
+  - Name:            .text2
+    Type:            SHT_PROGBITS
+    Flags:           [ SHF_ALLOC, SHF_EXECINSTR ]
+    Address:         0x2000
+    AddressAlign:    0x1000
+    Size:            0x1000
+  - Name:            .text3
+    Type:            SHT_PROGBITS
+    Flags:           [ SHF_ALLOC, SHF_EXECINSTR ]
+    Address:         0x3000
+    AddressAlign:    0x1000
+    Size:            0x1000
+ProgramHeaders:
+  - Type: PT_LOAD
+    Flags: [ PF_R ]
+    VAddr: 0x1000
+    PAddr: 0x1000
+    Sections:
+      - Section: .text
+  - Type: PT_LOAD
+    Flags: [ PF_X, PF_R ]
+    VAddr: 0x3000
+    PAddr: 0x3000
+    Sections:
+      - Section: .text3
+
+# CHECK:     ProgramHeaders [
+# CHECK-NEXT:  ProgramHeader {
+# CHECK-NEXT:    Type: PT_LOAD (0x1)
+# CHECK-NEXT:    Offset: 0x1000
+# CHECK-NEXT:    VirtualAddress: 0x1000
+# CHECK-NEXT:    PhysicalAddress: 0x1000
+# CHECK-NEXT:    FileSize: 4096
+# CHECK-NEXT:    MemSize: 4096
+# CHECK-NEXT:    Flags [ (0x4)
+# CHECK-NEXT:      PF_R (0x4)
+# CHECK-NEXT:    ]
+# CHECK-NEXT:    Alignment: 4096
+# CHECK-NEXT:  }
+# CHECK-NEXT:  ProgramHeader {
+# CHECK-NEXT:    Type: PT_LOAD (0x1)
+# CHECK-NEXT:    Offset: 0x2000
+# CHECK-NEXT:    VirtualAddress: 0x3000
+# CHECK-NEXT:    PhysicalAddress: 0x3000
+# CHECK-NEXT:    FileSize: 4096
+# CHECK-NEXT:    MemSize: 4096
+# CHECK-NEXT:    Flags [ (0x5)
+# CHECK-NEXT:      PF_R (0x4)
+# CHECK-NEXT:      PF_X (0x1)
+# CHECK-NEXT:    ]
+# CHECK-NEXT:    Alignment: 4096
+# CHECK-NEXT:  }
+# CHECK-NEXT:]

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

public:
   uint64_t PAddr;
   uint64_t Type;
   uint64_t VAddr;
 
   uint64_t OriginalOffset;
   Segment *ParentSegment = nullptr;
 
   Segment(llvm::ArrayRef<uint8_t> Data) : Contents(Data) {}
-  void finalize();
   const SectionBase *firstSection() const {
     if (!Sections.empty())
       return *Sections.begin();
     return nullptr;
   }
   void addSection(const SectionBase *sec) { Sections.insert(sec); }
   template <class ELFT> void writeHeader(llvm::FileOutputBuffer &Out) const;
   void writeSegment(llvm::FileOutputBuffer &Out) const;

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

template <class ELFT> void Segment::writeHeader(FileOutputBuffer &Out) const {
   Phdr.p_offset = Offset;
   Phdr.p_vaddr = VAddr;
   Phdr.p_paddr = PAddr;
   Phdr.p_filesz = FileSize;
   Phdr.p_memsz = MemSize;
   Phdr.p_align = Align;
 }
 
-void Segment::finalize() {
-  auto FirstSec = firstSection();
-  if (FirstSec) {
-    // It is possible for a gap to be at the begining of a segment. Because of
-    // this we need to compute the new offset based on how large this gap was
-    // in the source file. Section layout should have already ensured that this
-    // space is not used for something else.
-    uint64_t OriginalOffset = Offset;
-    Offset = FirstSec->Offset - (FirstSec->OriginalOffset - OriginalOffset);
-  }
-}
-
 void Segment::writeSegment(FileOutputBuffer &Out) const {
   uint8_t *Buf = Out.getBufferStart() + Offset;
   // We want to maintain segments' interstitial data and contents exactly.
   // This lets us just copy segments directly.
   std::copy(std::begin(Contents), std::end(Contents), Buf);
 }
 
 void SectionBase::initialize(SectionTableRef SecTable) {}
for (auto &Segment : OrderedSegments) {
     // already be set and we can set our offset relative to it.
     if (Segment->ParentSegment != nullptr) {
       auto Parent = Segment->ParentSegment;
       Segment->Offset =
           Parent->Offset + Segment->OriginalOffset - Parent->OriginalOffset;
     } else {
       Offset = alignTo(Offset, Segment->Align == 0 ? 1 : Segment->Align);
       Segment->Offset = Offset;
-      Offset += Segment->FileSize;
     }
+    Offset = std::max(Offset, Segment->Offset + Segment->FileSize);
   }
   // Now the offset of every segment has been set we can assign the offsets
   // of each section. For sections that are covered by a segment we should use
   // the segment's original offset and the section's original offset to compute
   // the offset from the start of the segment. Using the offset from the start
   // of the segment we can assign a new offset to the section. For sections not
   // covered by segments we can just bump Offset to the next valid location.
   uint32_t Index = 1;
   for (auto &Section : this->Sections) {
     Section->Index = Index++;
     if (Section->ParentSegment != nullptr) {
       auto Segment = Section->ParentSegment;
       Section->Offset =
           Segment->Offset + (Section->OriginalOffset - Segment->OriginalOffset);
     } else {
-      Offset = alignTo(Offset, Section->Offset);
+      Offset = alignTo(Offset, Section->Align == 0 ? 1 : Section->Align);
       Section->Offset = Offset;
       if (Section->Type != SHT_NOBITS)
         Offset += Section->Size;
     }
   }
 
   Offset = alignTo(Offset, sizeof(typename ELFT::Addr));
   this->SHOffset = Offset;
template <class ELFT> void ELFObject<ELFT>::finalize() {
   // remaining issues.
   uint64_t Offset = this->SHOffset + sizeof(Elf_Shdr);
   for (auto &Section : this->Sections) {
     Section->HeaderOffset = Offset;
     Offset += sizeof(Elf_Shdr);
     Section->NameIndex = this->SectionNames->findIndex(Section->Name);
     Section->finalize();
   }
-
-  for (auto &Segment : this->Segments)
-    Segment->finalize();
 }
 
 template <class ELFT> size_t BinaryObject<ELFT>::totalSize() const {
   return TotalSize;
 }
 
 template <class ELFT>
 void BinaryObject<ELFT>::write(FileOutputBuffer &Out) const {
   for (auto &Segment : this->Segments) {
     // GNU objcopy does not output segments that do not cover a section. Such
     // segments can sometimes be produced by LLD due to how LLD handles PT_PHDR.
     if (Segment->Type == llvm::ELF::PT_LOAD &&
         Segment->firstSection() != nullptr) {
       Segment->writeSegment(Out);
     }
   }
 }
 
 template <class ELFT> void BinaryObject<ELFT>::finalize() {
-  for (auto &Segment : this->Segments)
-    Segment->finalize();
 
   // Put all segments in offset order.
   auto CompareSegments = [](const SegPtr &A, const SegPtr &B) {
     return A->Offset < B->Offset;
   };
   std::sort(std::begin(this->Segments), std::end(this->Segments),
             CompareSegments);