[llvm-objcopy] Don't specialize the all zero p_paddr case

[MaskRay]

Summary

Spotted by https://reviews.llvm.org/D74755#1998673

it looks like OrderedSegments in the function is only used to set the physical address to the virtual address when there are no physical addresses set amongst these sections.

I believe this behavior was copied from https://sourceware.org/git/?p=binutils-gdb.git;a=commit;h=6ffd79000b45e77b3625143932ffbf781b6aecab (2008-05)
The commit was made for very old linkers.
This special rule does not seem useful and remove it can allow us to
delete a large chunk of code.

[jhenderson]

IIRC from what I looked at yesterday, the segments aren't actually used for anything in binary output now, except in the calculation for MinAddr and SecAddr. Assuming that's correct (please check if you're going to do this!), does that mean we could simplify to looping over all Segments in the code you're modifying rather than just those which are ParentSegment instances?

[grimar]

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

2218–2219

all_of -> llvm::all_of
(it is consistent with how LLVM uses algorithms and makes it clear that we use llvm version of a algorithm).

[MaskRay]

Drop a special case to simplify code. No longer a NFC.

[jhenderson]

I believe this behavior was copied from https://sourceware.org/git/?p=binutils-gdb.git;a=commit;h=6ffd79000b45e77b3625143932ffbf781b6aecab (2008-05)

From reading the original review history etc of the change that implemented this in llvm-objcopy, I couldn't see evidence that this is true, except that I recall @jakehehrlich was trying to copy GNU objcopy's behaviour in all things that made some degree of sense. I believe he did this from a mixture of trial and error and reading objcopy code, but I'm not sure.

Assuming that it was to mirror the GNU behaviour, and the GNU behaviour is as you described (I'm getting timeout errors when I go to that link), I'm fine with this, but I'd like @jakehehrlich to chime in if he's still around. If he doesn't do so in the next few days, this can go in.

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

9–10

I'm not sure this sentence is right - I assume you mean p_paddr, not p_paddr.

13–14

If the two outputs should be the same, would simply cmp %t1.out %t0.out1 be simpler?

[grimar]

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

44

nit: perhaps you can remove Align here and above while you are here. It doesn't seem to be useful for this test?

[MaskRay]

Address comments

[jakehehrlich]

https://reviews.llvm.org/D41619 is where I think this stuff was introduced for context.

[jakehehrlich]

So confirmed from reading that this behavoir was just copied by a bit of digging in the code itself. I don't believe a real world use case came up that required this. Reading the bug that Fangrui linked but after that this has my LGTM

[jakehehrlich]

Yeah I don't suspect anyone will be bitten by this, it speeds up this use case, and speeds up all other use cases, and I don't think there was ever a strong reason for adding this specific behavior.

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

-# RUN: yaml2obj %s -o %t
-# RUN: llvm-objcopy -O binary %t %t2
-# RUN: od -t x2 -v %t2 | FileCheck %s --ignore-case
-# RUN: wc -c < %t2 | FileCheck %s --check-prefix=SIZE
+# RUN: yaml2obj -D PADDR=1 %s -o %t1
+# RUN: llvm-objcopy -O binary %t1 %t1.out
+# RUN: od -t x2 -v %t1.out | FileCheck %s --ignore-case
+# RUN: wc -c < %t1.out | FileCheck %s --check-prefix=SIZE
+
+## When all p_paddr fields are 0, GNU objcopy resets LMA to VMA
+## and gives a different output.
+## https://sourceware.org/git/?p=binutils-gdb.git;a=commit;h=6ffd79000b45e77b3625143932ffbf781b6aecab
+## We don't implement this special rule. The p_paddr=0 output is the same as
+## the p_paddr=1 case.
+# RUN: yaml2obj -D PADDR=0 %s -o %t0
+# RUN: llvm-objcopy -O binary %t0 %t0.out
+# RUN: cmp %t1.out %t0.out
 
 !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:    0x0000000000001000
     Content:         "c3c3c3c3"
   - Name:            .data
     Type:            SHT_PROGBITS
     Flags:           [ SHF_ALLOC ]
     Address:         0x1004
     AddressAlign:    0x0000000000000004
     Content:         "3232"
 ProgramHeaders:
   - Type: PT_LOAD
     Flags: [ PF_X, PF_R ]
     VAddr: 0x1000
-    PAddr: 0x0000
-    Align: 0x1000
+    PAddr: [[PADDR]]
     Sections:
       - Section: .text
   - Type: PT_LOAD
     Flags: [ PF_R, PF_W ]
     VAddr: 0x1004
-    PAddr: 0x0000
-    Align: 0x1000
+    PAddr: [[PADDR]]
     Sections:
       - Section: .data
 
-# CHECK: 0000000 c3c3 c3c3 3232
-# SIZE:  6
+# CHECK: 0000000 3232 c3c3
+# SIZE:  4

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

static bool compareSegmentsByOffset(const Segment *A, const Segment *B) {
   // that has a parent segment.
   if (A->OriginalOffset < B->OriginalOffset)
     return true;
   if (A->OriginalOffset > B->OriginalOffset)
     return false;
   return A->Index < B->Index;
 }
 
-static bool compareSegmentsByPAddr(const Segment *A, const Segment *B) {
-  if (A->PAddr < B->PAddr)
-    return true;
-  if (A->PAddr > B->PAddr)
-    return false;
-  return A->Index < B->Index;
-}
-
 void BasicELFBuilder::initFileHeader() {
   Obj->Flags = 0x0;
   Obj->Type = ET_REL;
   Obj->OSABI = ELFOSABI_NONE;
   Obj->ABIVersion = 0;
   Obj->Entry = 0x0;
   Obj->Machine = EM_NONE;
   Obj->Version = 1;

 }
 
 Error BinaryWriter::write() {
   for (const SectionBase &Sec : Obj.allocSections())
     Sec.accept(*SecWriter);
   return Buf.commit();
 }
 
 Error BinaryWriter::finalize() {
-  // 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. We only want to consider the segments
-  // that will affect layout of allocated sections so we only add those.
-  std::vector<Segment *> OrderedSegments;
-  for (const SectionBase &Sec : Obj.allocSections())
-    if (Sec.ParentSegment != nullptr)
-      OrderedSegments.push_back(Sec.ParentSegment);
-
-  // For binary output, we're going to use physical addresses instead of
-  // virtual addresses, since a binary output is used for cases like ROM
-  // loading and physical addresses are intended for ROM loading.
-  // However, if no segment has a physical address, we'll fallback to using
-  // virtual addresses for all.
-  if (all_of(OrderedSegments,
-             [](const Segment *Seg) { return Seg->PAddr == 0; }))
-    for (Segment *Seg : OrderedSegments)
-      Seg->PAddr = Seg->VAddr;
-
-  llvm::stable_sort(OrderedSegments, compareSegmentsByPAddr);
-
-  // Because we add a ParentSegment for each section we might have duplicate
-  // segments in OrderedSegments. If there were duplicates then layoutSegments
-  // would do very strange things.
-  auto End =
-      std::unique(std::begin(OrderedSegments), std::end(OrderedSegments));
-  OrderedSegments.erase(End, std::end(OrderedSegments));
-
   // 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 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.ParentSegment != nullptr)
       Sec.Addr =
           Sec.Offset - Sec.ParentSegment->Offset + Sec.ParentSegment->PAddr;