[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.

Looks good to me, with the suggested changes. Just get Jake to give one final approval to address his concern in the test.



================
Comment at: test/tools/llvm-objcopy/binary-paddr.test:35
+    Flags: [ PF_R, PF_W ]
+    VAddr: 0x2000
+    PAddr: 0x1008
----------------
owenpshaw wrote:
> jakehehrlich wrote:
> > We don't know where the .data section will end up only that it will have 4-byte alignment It's possible (even quite likely) for the the offset of the .data section (which decides the offset of the program header unfortunately, not the other way around) to be such that the p_offset % p_align != p_vaddr % p_align. This is still not a valid ELF file (at least it isn't assured).
> I guess I'm missing something with this alignment stuff.  Having possibly invalid values in the elf doesn't seem to affect what we're testing, but I agree it'd be good to use proper input if possible.  What values would make it valid?
Maybe try PAddr 0x4000 and PVaddr 0x2000, and then use .data AddrAlign of 0x1000?


================
Comment at: tools/llvm-objcopy/Object.cpp:994
     Seg->FileSize -= Diff;
-    Seg->MemSize -= Diff;
-    // The VAddr needs to be adjusted so that the alignment is correct as well
-    Seg->VAddr += Diff;
-    Seg->PAddr = Seg->VAddr;
-    // We don't want this to be shifted by alignment so we need to set the
-    // alignment to zero.
-    Seg->Align = 0;
+    // The PAddr needs to be increased to remove the gap
+    Seg->PAddr += Diff;
----------------
Nit: full stop at end of comment.

Also, what gap? I assume you mean the gap at the start of the file, so I'd say "remove the gap before the first section."


https://reviews.llvm.org/D41619