[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

2233

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 ↗

(On Diff #259901)

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

13–14 ↗

(On Diff #259901)

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

47 ↗

(On Diff #259901)

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/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() {
   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;
+  std::vector<Segment *> Segments;
   for (const SectionBase &Sec : Obj.allocSections())
     if (Sec.ParentSegment != nullptr)
-      OrderedSegments.push_back(Sec.ParentSegment);
+      Segments.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)
+  if (all_of(Segments, [](const Segment *Seg) { return Seg->PAddr == 0; }))
+    for (Segment *Seg : Segments)
       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 =