[PATCH] D41619: [llvm-objcopy] Use physical instead of virtual address when aligning and placing sections in binary

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

Overall design looks okay, but I've got a few comments that need addressing before I give it a LGTM.



================
Comment at: test/tools/llvm-objcopy/binary-paddr.test:1
+# RUN: yaml2obj %s -o %t
+# RUN: llvm-objcopy -O binary %t %t2
----------------
I wonder whether it would be beneficial to have a another test-case here, or possibly extend this one to have a gap in the physical address range, a bit like binary-segment-layout does for virtual addresses. I assume that the binary layout code should then preserve the gap between these two segments.


================
Comment at: tools/llvm-objcopy/Object.cpp:367
 
 static bool compareSegments(const Segment *A, const Segment *B) {
   // Any segment without a parent segment should come before a segment
----------------
I think this function should probably be renamed to "compareSegmentsByOffset" and the new function "compareSegmentsByPAddr" (assuming we don't follow Jake's suggestion). Also, I think "By" sounds better than "Using", but it's incredibly trivial, so I don't mind.


================
Comment at: tools/llvm-objcopy/Object.cpp:377
 
+static bool compareSegmentsUsingPAddr(const Segment *A, const Segment *B) {
+  if (A->PAddr < B->PAddr)
----------------
jakehehrlich wrote:
> I'm wondering if using a member point is ok here because it would let you dedup these two functions. Something like
> 
> ```
> compareSegmentsUsing(uint64_t Segment::*Member, const Segment *A, const Segment *B) {
>   if (A->*Member < B->*Member)
>     return true;
>   if (A->*Member > B->*Member)
>     return false;
>   return A->Index < B->Index;
> }
> ```
> 
> Then to pass the function you can use std::bind(compareSegmentsUsing, &Segment::PAddr)
> 
> This might be discouraged by llvm though since it's so uncommonly used that many people aren't aware of this feature.
I don't think I've ever seen such a feature used before!


================
Comment at: tools/llvm-objcopy/Object.cpp:911-912
+  // 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
----------------
Nit: rom should be capitalized, i.e. ROM, since it's an abbreviation.


================
Comment at: tools/llvm-objcopy/Object.cpp:946
+    // The VAddr and PAddr need to be adjusted so that the alignment is correct
     Seg->VAddr += Diff;
+    Seg->PAddr += Diff;
----------------
Do you need to adjust VAddr any more? As far as I can see, it isn't used at all after PAddr has been initialized to it. Also, the comment above it needs updating to reflect our new usage of PAddr and why we update it.


================
Comment at: tools/llvm-objcopy/Object.cpp:948-950
     // We don't want this to be shifted by alignment so we need to set the
     // alignment to zero.
     Seg->Align = 0;
----------------
I think this block can be removed now, since we don't align segments at all for binary output.


================
Comment at: tools/llvm-objcopy/Object.cpp:954
+      Segment->Offset = Segment->PAddr - LowestPAddr;
+      Offset = std::max(Offset, Segment->Offset + Segment->FileSize);
+    }
----------------
Just want to check, but are NOBITS sections allocated space on disk for binary output? I assume not, but thought it best to double check...


https://reviews.llvm.org/D41619