[llvm-objcopy] Fix bug in how segment alignment was being handled

[jakehehrlich]

Summary

Just aligning segment offsets to segment alignment is incorrect and also wastes more space than is needed. The requirement is that p_offset == p_addr modulo p_align *not* that p_offset == 0 modulo p_align. Generally speaking we've been using p_addr == 0 modulo p_align. In fact yaml2obj can't even produce a valid situation which causes llvm-objcopy to produce incorrect results because alignment and offset were both inherited from the sections the program header covers. This change fixes this bad behavior in llvm-objcopy.

[jhenderson]

You should probably have a test for the BinaryObject case as well.

In fact yaml2obj can't even produce a valid situation which causes llvm-objcopy to produce incorrect results because alignment and offset were both inherited from the sections the program header covers

Could you explain a bit more what you mean by this? The p_align field for a segment should be completely independent of the align field of its sections.

test/tools/llvm-objcopy/check-addr-offset-align.test

15

This might be related to the problem below, but this Address (and that of .data) does not lie within the corresponding segment, according to the VAddr field you specify for the segment, so I'm not sure this makes any sense.

43–44

Isn't this the wrong behaviour here?

p_offset = 0x230, p_vaddr = 0x1030, p_align = 0x1000, so (p_offset % p_align) != (p_vaddr % p_align).

I'd expect offset to be 0x1030 in this case.

[jakehehrlich]

In D39132#903387, @jhenderson wrote:

You should probably have a test for the BinaryObject case as well.

In fact yaml2obj can't even produce a valid situation which causes llvm-objcopy to produce incorrect results because alignment and offset were both inherited from the sections the program header covers

Could you explain a bit more what you mean by this? The p_align field for a segment should be completely independent of the align field of its sections.

I think this should be the case but I settled for allowing it to be computed from the sections at the request of the reviewer. I have a change in yaml2obj to make this the case up for review. Hopefully it gets reviewed soon.

[jakehehrlich]

Fixed the test case with all the strange addresses that made no sense.

[jhenderson]

test/tools/llvm-objcopy/check-addr-offset-align.test

3–4

Umm... reading this again, and I realise now that the test is actually testing yaml2obj, not llvm-objcopy!

You are using binary output to %t2, but then using readobj to check %t, which is the output of yaml2obj!

Also, there should really be test cases for both ELF and binary output format cases, since this change affects both.

[jakehehrlich]

1. Added test for -O binary case
2. That test still made no sense. It should test the fact that even though the second segment has alignment 0x1000 it can't still have offset 0x1008.

[jakehehrlich]

test/tools/llvm-objcopy/check-addr-offset-align.test

3–4

Yikes! It should be fixed now.

[jhenderson]

LGTM with the comment nits fixed.

tools/llvm-objcopy/Object.cpp

673

There's an extra space here between "that" and "(Offset + ...".

678

"...Offset however so..." -> "...Offset, however, so..."

test/tools/llvm-objcopy/check-addr-offset-align.test

+# RUN: yaml2obj %s -o %t
+# RUN: llvm-objcopy -O binary %t %t2
+# RUN: llvm-readobj -program-headers %t | 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:    0x0000000000000008
+    Content:         "c3c3c3c3"
+  - Name:            .data
+    Type:            SHT_PROGBITS
+    Flags:           [ SHF_ALLOC ]
+    Address:         0x1008
+    AddressAlign:    0x0000000000000008
+    Content:         "3232"
+ProgramHeaders:
+  - Type: PT_LOAD
+    Flags: [ PF_X, PF_R ]
+    VAddr: 0x1030
+    PAddr: 0x1030
+    Align: 0x1000
+    Sections:
+      - Section: .text
+  - Type: PT_LOAD
+    Flags: [ PF_R, PF_W ]
+    VAddr: 0x1038
+    PAddr: 0x1038
+    Align: 0x1000
+    Sections:
+      - Section: .data
+
+#CHECK:     ProgramHeaders [
+#CHECK-NEXT:   ProgramHeader {
+#CHECK-NEXT:    Type: PT_LOAD
+#CHECK-NEXT:    Offset: 0x230
+#CHECK-NEXT:    VirtualAddress: 0x1030
+#CHECK-NEXT:    PhysicalAddress: 0x1030
+#CHECK-NEXT:    FileSize: 4
+#CHECK-NEXT:    MemSize: 4
+#CHECK-NEXT:    Flags [
+#CHECK-NEXT:      PF_R
+#CHECK-NEXT:      PF_X
+#CHECK-NEXT:    ]
+#CHECK-NEXT:    Alignment: 4096
+#CHECK-NEXT:  }
+#CHECK-NEXT:  ProgramHeader {
+#CHECK-NEXT:    Type: PT_LOAD
+#CHECK-NEXT:    Offset: 0x238
+#CHECK-NEXT:    VirtualAddress: 0x1038
+#CHECK-NEXT:    PhysicalAddress: 0x1038
+#CHECK-NEXT:    FileSize: 2
+#CHECK-NEXT:    MemSize: 2
+#CHECK-NEXT:    Flags [
+#CHECK-NEXT:      PF_R
+#CHECK-NEXT:      PF_W
+#CHECK-NEXT:    ]
+#CHECK-NEXT:    Alignment: 4096
+#CHECK-NEXT:  }
+#CHECK-NEXT:]

tools/llvm-objcopy/Object.cpp

template <class ELFT> void ELFObject<ELFT>::sortSections() {
   // possible while meeting that demand however.
   auto CompareSections = [](const SecPtr &A, const SecPtr &B) {
     return A->OriginalOffset < B->OriginalOffset;
   };
   std::stable_sort(std::begin(this->Sections), std::end(this->Sections),
                    CompareSections);
 }
 
+static uint64_t alignToAddr(uint64_t Offset, uint64_t Addr, uint64_t Align) {
+  // Calculate Diff such that  (Offset + Diff) & -Align == Addr & -Align.
+  if (Align == 0)
+    Align = 1;
+  auto Diff =
+      static_cast<int64_t>(Addr % Align) - static_cast<int64_t>(Offset % Align);
+  // We only want to add to Offset however so if Diff < 0 we can add Align and
+  // (Offset + Diff) & -Align == Addr & -Align will still hold.
+  if (Diff < 0)
+    Diff += Align;
+  return Offset + Diff;
+}
+
 template <class ELFT> void ELFObject<ELFT>::assignOffsets() {
   // We need a temporary list of segments that has a special order to it
   // so that we know that anytime ->ParentSegment is set that segment has
   // already had it's offset properly set.
   std::vector<Segment *> OrderedSegments;
   for (auto &Segment : this->Segments)
     OrderedSegments.push_back(Segment.get());
   auto CompareSegments = [](const Segment *A, const Segment *B) {
for (auto &Segment : OrderedSegments) {
     // such that a parent segment will always come before a child segment in
     // OrderedSegments. This means that the Offset of the ParentSegment should
     // 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);
+      Offset = alignToAddr(Offset, Segment->VAddr, Segment->Align);
       Segment->Offset = Offset;
     }
     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
template <class ELFT> void BinaryObject<ELFT>::finalize() {
   };
   std::sort(std::begin(this->Segments), std::end(this->Segments),
             CompareSegments);
 
   uint64_t Offset = 0;
   for (auto &Segment : this->Segments) {
     if (Segment->Type == llvm::ELF::PT_LOAD &&
         Segment->firstSection() != nullptr) {
-      Offset = alignTo(Offset, Segment->Align);
+      Offset = alignToAddr(Offset, Segment->VAddr, Segment->Align);
       Segment->Offset = Offset;
       Offset += Segment->FileSize;
     }
   }
   TotalSize = Offset;
 }
 
 template class Object<ELF64LE>;