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

Jake Ehrlich via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Wed Jan 10 10:16:44 PST 2018


jakehehrlich added inline comments.


================
Comment at: test/tools/llvm-objcopy/binary-paddr.test:1
+# RUN: yaml2obj %s -o %t
+# RUN: llvm-objcopy -O binary %t %t2
----------------
jhenderson wrote:
> 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.
That seems like a good additional thing to add. The gap should be preserved for segments but not for sections that are outside of segments.


================
Comment at: tools/llvm-objcopy/Object.cpp:377
 
+static bool compareSegmentsUsingPAddr(const Segment *A, const Segment *B) {
+  if (A->PAddr < B->PAddr)
----------------
jhenderson wrote:
> 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!
It's honestly what you get when you follow the very basic deduplication principal "make the parts that are different a parameter" but it's very very rarely used. Even this case is kind of academic THB.


================
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;
----------------
jhenderson wrote:
> 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.
Actually yeah, I should have caught this. Following that logic there was never really a reason to update MemSize either.


================
Comment at: tools/llvm-objcopy/Object.cpp:954
+      Segment->Offset = Segment->PAddr - LowestPAddr;
+      Offset = std::max(Offset, Segment->Offset + Segment->FileSize);
+    }
----------------
jhenderson wrote:
> 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...
If they're inside a segment and not at the end of the file then space is allocated to them. If they're not in a segment or if they're at the end of the last segment then they're left out.


https://reviews.llvm.org/D41619





More information about the llvm-commits mailing list