[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
Tue Jan 9 15:46:57 PST 2018


jakehehrlich added inline comments.


================
Comment at: test/tools/llvm-objcopy/basic-align-copy.test:1
 # RUN: yaml2obj %s -o %t
 # RUN: llvm-objcopy -O binary %t %t2
----------------
Can you rename this file to "binary-segment-layout" or something that that relates to what the test now actually tests since it no longer tests that alignment is respected.


================
Comment at: test/tools/llvm-objcopy/basic-align-copy.test:33
     Flags: [ PF_R ]
+    VAddr: 0x08
     Sections:
----------------
This will violate the p_addr == p_offset (MOD p_align) requirement as is. If you don't want these to be on seperate pages change the Address Align of the sections. Ideally we would have small AddressAlign's and large ProgramHeader alignment but unfortunately I haven't updated the layout strategy of yaml2obj to make that work properly yet so I've been getting by with this. 


================
Comment at: tools/llvm-objcopy/Object.cpp:756
+    return Offset;
+  uint64_t LowestPAddr = Segments[0]->PAddr;
+
----------------
Unfortunately the lowest PAddr will not necessarily come from the Segment with the lowest offset. You'll need to use std::min_element to find the one you're looking for. In fact because the order that segments might need to be laid out in might differ from the order they're presented to this function in it's not clear this function should be used. I *think* this winds up working because in practice the only Segments in the Segments vector here are PT_LOAD segments are not nested inside of anything so consequently the only case that will run is the "Offset = Segment->PAddr - LowestPAddr;".. More over if that's the only case that's ever going to be hit I don't think we need to embed it in such a complicated function as this one. I think the code is different enough now that we can just inline the layout algorithm for the binary case in BinaryObject<ELFT>::finalize() and remove the UsePAddr argument and the if condition. Two simple more obviously correct algorithms are better than one 
slightly more complicated algorithm. Especially when it comes layout (which has historically been the hardest thing to get right)


================
Comment at: tools/llvm-objcopy/Object.cpp:948
 
-  uint64_t Offset = LayoutSegments(OrderedSegments, 0);
+  uint64_t Offset = LayoutSegments(OrderedSegments, 0, true);
 
----------------
I think you should inline the new algorithm for binary segment layout here and LayoutSegments can just be for ELF file layout.


https://reviews.llvm.org/D41619





More information about the llvm-commits mailing list