[llvm-objcopy] -O binary: do not align physical addresses

[quic-akaryaki]

Summary

llvm-objcopy should not insert padding before a section if its
physical addresses is not aligned to section's alignment. This
behavior will match GNU objcopy and is important for embedded images
where the physical address is used to store the initial data image.
The loader typically will copy this image using a start symbol
created by the linker. If llvm-objcopy inserts padding before such a
section, the symbol address will not match the location in the image.

This commit refines the change in https://reviews.llvm.org/D128961
which intended to align sections which type changed from NOBITS and
their offset may not be aligned. However, it affected all sections.

Fix https://github.com/llvm/llvm-project/issues/62636

[apazos]

Hello @MaskRay and @jhenderson - Can you help review this change? This looks like a bug and lack of compatibility with GNU tools.

[MaskRay]

Your commit message has 4-space indentation. Please remove that. git log -1 --pretty=%B messages don't usually have indentation.

This issue causes the Differential summary to be rendered like a Markdown code block.

https://github.com/llvm/llvm-project/issues/62636

Fix https://github.com/llvm/llvm-project/issues/62636

llvm/test/tools/llvm-objcopy/ELF/binary-paddr.test

292

Add a comment.

## PAddr is not aligned by sh_addralign(.data).

309

Delete the symbols. They are unrelated to the test.

[MaskRay]

is important for embedded images where the physical address is used to store the initial data image. The loader typically will copy this image using a start symbol created by the linker. If llvm-objcopy inserts padding before such a section, the symbol address will not match the location in the image.

Thank you. I don't have much experience with p_paddr. Unaligned p_offset seems a bit strange but I see that it may be used this way...

[jhenderson]

Apologies for not looking at this - I've had a pretty busy few weeks.

[quic-akaryaki]

Fixed the commit message, removed symbols from the test, and added a comment. Thanks, MaskRay.

[MaskRay]

LGTM, will be great to have a sign-up from @jhenderson

llvm/test/tools/llvm-objcopy/ELF/binary-paddr.test

258

I think the comment can be simplified.

" The loader typically will copy this image using a start symbol created by the linker. Since copying is
done by memcpy, the source alignment is not important but the symbol
address must match the initial image location." seems unnecessary to me.

Perhaps just use this:

Without converting the type of a NOBITS section, don't align the offset.
This matches GNU objcopy when the physical address of a section is not aligned.

310

Delete the trailing blank line.

[jhenderson]

I'm not too familiar with the -O binary option and its use-cases, but the change seems reasonable to me. I'm happy to defer to @MaskRay.

[quic-akaryaki]

Updated.

[quic-akaryaki]

Please someone on this review with commit rights commit this change. I don't see outstanding items to complete. Thank you!

[MaskRay]

In D150276#4435411, @quic-akaryaki wrote:

Please someone on this review with commit rights commit this change. I don't see outstanding items to complete. Thank you!

Sure! Which git commit --amend --author=... command shall I use for this patch to give you credits?

[quic-akaryaki]

@MaskRay --author="Alexey Karyakin <[e-mail]>" is fine. Thank you!

[quic-akaryaki]

Fix the endianness issue, which broke big endian PPC tests.

llvm/lib/ObjCopy/ELF/ELFObjcopy.cpp

static uint64_t getSectionFlagsPreserveMask(uint64_t OldFlags,
   const uint64_t PreserveMask =
       (ELF::SHF_COMPRESSED | ELF::SHF_GROUP | ELF::SHF_LINK_ORDER |
        ELF::SHF_MASKOS | ELF::SHF_MASKPROC | ELF::SHF_TLS |
        ELF::SHF_INFO_LINK) &
       ~ELF::SHF_EXCLUDE;
   return (OldFlags & PreserveMask) | (NewFlags & ~PreserveMask);
 }
 
+static void setSectionType(SectionBase &Sec, uint64_t Type) {
+  // If Sec's type is changed from SHT_NOBITS due to --set-section-flags,
+  // Offset may not be aligned. Align it to max(Align, 1).
+  if (Sec.Type == ELF::SHT_NOBITS && Type != ELF::SHT_NOBITS)
+    Sec.Offset = alignTo(Sec.Offset, std::max(Sec.Align, uint64_t(1)));
+  Sec.Type = Type;
+}
+
 static void setSectionFlagsAndType(SectionBase &Sec, SectionFlag Flags) {
   Sec.Flags = getSectionFlagsPreserveMask(Sec.Flags, getNewShfFlags(Flags));
 
   // In GNU objcopy, certain flags promote SHT_NOBITS to SHT_PROGBITS. This rule
   // may promote more non-ALLOC sections than GNU objcopy, but it is fine as
   // non-ALLOC SHT_NOBITS sections do not make much sense.
   if (Sec.Type == SHT_NOBITS &&
       (!(Sec.Flags & ELF::SHF_ALLOC) ||
        Flags & (SectionFlag::SecContents | SectionFlag::SecLoad)))
-    Sec.Type = SHT_PROGBITS;
+    setSectionType(Sec, ELF::SHT_PROGBITS);
 }
 
 static ElfType getOutputElfType(const Binary &Bin) {
   // Infer output ELF type from the input ELF object
   if (isa<ELFObjectFile<ELF32LE>>(Bin))
     return ELFT_ELF32LE;
   if (isa<ELFObjectFile<ELF64LE>>(Bin))
     return ELFT_ELF64LE;
if (!Config.SetSectionFlags.empty() || !Config.SetSectionType.empty()) {
     for (auto &Sec : Obj.sections()) {
       const auto Iter = Config.SetSectionFlags.find(Sec.Name);
       if (Iter != Config.SetSectionFlags.end()) {
         const SectionFlagsUpdate &SFU = Iter->second;
         setSectionFlagsAndType(Sec, SFU.NewFlags);
       }
       auto It2 = Config.SetSectionType.find(Sec.Name);
       if (It2 != Config.SetSectionType.end())
-        Sec.Type = It2->second;
+        setSectionType(Sec, It2->second);
     }
   }
 
   if (!Config.SectionsToRename.empty()) {
     std::vector<RelocationSectionBase *> RelocSections;
     DenseSet<SectionBase *> RenamedSections;
     for (SectionBase &Sec : Obj.sections()) {
       auto *RelocSec = dyn_cast<RelocationSectionBase>(&Sec);

llvm/lib/ObjCopy/ELF/ELFObject.cpp

 
 Error BinaryWriter::finalize() {
   // Compute the section LMA based on its sh_offset and the containing segment's
   // p_offset and p_paddr. Also compute the minimum LMA of all non-empty
   // sections as MinAddr. In the output, the contents between address 0 and
   // MinAddr will be skipped.
   uint64_t MinAddr = UINT64_MAX;
   for (SectionBase &Sec : Obj.allocSections()) {
-    // If Sec's type is changed from SHT_NOBITS due to --set-section-flags,
-    // Offset may not be aligned. Align it to max(Align, 1).
     if (Sec.ParentSegment != nullptr)
-      Sec.Addr = alignTo(Sec.Offset - Sec.ParentSegment->Offset +
-                             Sec.ParentSegment->PAddr,
-                         std::max(Sec.Align, uint64_t(1)));
+      Sec.Addr =
+          Sec.Offset - Sec.ParentSegment->Offset + Sec.ParentSegment->PAddr;
     if (Sec.Type != SHT_NOBITS && Sec.Size > 0)
       MinAddr = std::min(MinAddr, Sec.Addr);
   }
 
   // Now that every section has been laid out we just need to compute the total
   // file size. This might not be the same as the offset returned by
   // layoutSections, because we want to truncate the last segment to the end of
   // its last non-empty section, to match GNU objcopy's behaviour.

llvm/test/tools/llvm-objcopy/ELF/binary-paddr.test

ProgramHeaders:
   - Type:     PT_LOAD
     Flags:    [ PF_R, PF_W, PF_X ]
     Offset:   0x1000
     VAddr:    0x1000
     PAddr:    0x1000
     FileSize: 0x2
     MemSize:  0x13
     Align:    0x1000
+
+## Test that sections do not move relative to their physical addresses if
+## the physical address is not aligned. This behavior matches GNU objcopy
+## and is important for embedded images where the physical address is
+## used to store the initial data image. Without converting the type of a
+## NOBITS section, don't align the offset. This matches GNU objcopy when
+## the physical address of a section is not aligned.
+
+# RUN: yaml2obj --docnum=8 %s -o %t8
+# RUN: llvm-objcopy -O binary --only-section=.text --only-section=.data %t8 %t8.out
+# RUN: od -A x -t x1 %t8.out | FileCheck %s --check-prefix=PHYSUNALIGNED --ignore-case
+
+# PHYSUNALIGNED:      000000 11 22 33 44 55 66 77 88 99 aa bb cc dd ee ff 00
+# PHYSUNALIGNED-NEXT: 000010 11 22 33 44 bd ac cd ab
+
+--- !ELF
+FileHeader:
+  Class:           ELFCLASS64
+  Data:            ELFDATA2LSB
+  Type:            ET_EXEC
+  Machine:         EM_X86_64
+ProgramHeaders:
+  - Type:            PT_LOAD
+    Flags:           [ PF_X, PF_R ]
+    FirstSec:        .text
+    LastSec:         .text
+    VAddr:           0x200000
+    PAddr:           0x200000
+    Align:           0x1000
+  - Type:            PT_LOAD
+    Flags:           [ PF_W, PF_R ]
+    FirstSec:        .data
+    LastSec:         .data
+    VAddr:           0x400000
+    PAddr:           0x200014
+## PAddr is not aligned by sh_addralign(.data)
+    Align:           0x1000
+Sections:
+  - Name:            .text
+    Type:            SHT_PROGBITS
+    Flags:           [ SHF_ALLOC, SHF_EXECINSTR ]
+    Address:         0x200000
+    AddressAlign:    0x1
+    Offset:          0x1000
+    Content:         112233445566778899aabbccddeeff0011223344
+  - Name:            .data
+    Type:            SHT_PROGBITS
+    Flags:           [ SHF_WRITE, SHF_ALLOC ]
+    Address:         0x400000
+    AddressAlign:    0x8
+    Offset:          0x2000
+    Content:         BDACCDAB