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

[Fangrui Song via llvm-commits]

Author: Fangrui Song
Date: 2020-04-27T11:20:41-07:00
New Revision: fd624e623d34dd0e812b7fdce73bdff15f456826

URL: https://github.com/llvm/llvm-project/commit/fd624e623d34dd0e812b7fdce73bdff15f456826
DIFF: https://github.com/llvm/llvm-project/commit/fd624e623d34dd0e812b7fdce73bdff15f456826.diff

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

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 some corner cases of very old linkers.
This special rule does not seem useful and remove it can allow us to
delete a large chunk of code.

Reviewed By: jhenderson, jakehehrlich

Differential Revision: https://reviews.llvm.org/D78786

Added: 
    

Modified: 
    llvm/test/tools/llvm-objcopy/ELF/binary-no-paddr.test
    llvm/tools/llvm-objcopy/ELF/Object.cpp

Removed: 
    


################################################################################
diff  --git a/llvm/test/tools/llvm-objcopy/ELF/binary-no-paddr.test b/llvm/test/tools/llvm-objcopy/ELF/binary-no-paddr.test
index c492785a8b40..99cf19fad9c4 100644
--- a/llvm/test/tools/llvm-objcopy/ELF/binary-no-paddr.test
+++ b/llvm/test/tools/llvm-objcopy/ELF/binary-no-paddr.test
@@ -1,7 +1,16 @@
-# 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 
diff erent 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:
@@ -26,17 +35,15 @@ 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

diff  --git a/llvm/tools/llvm-objcopy/ELF/Object.cpp b/llvm/tools/llvm-objcopy/ELF/Object.cpp
index bc590fad4bed..3808e92d0b42 100644
--- a/llvm/tools/llvm-objcopy/ELF/Object.cpp
+++ b/llvm/tools/llvm-objcopy/ELF/Object.cpp
@@ -1101,14 +1101,6 @@ static bool compareSegmentsByOffset(const Segment *A, const Segment *B) {
   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;
@@ -2224,34 +2216,6 @@ Error BinaryWriter::write() {
 }
 
 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