Just aligning segment offsets to segment alignment is incorrect and also wastes more space than is needed. The requirement is that p_offset == p_addr modulo p_align *not* that p_offset == 0 modulo p_align. Generally speaking we've been using p_addr == 0 modulo p_align. In fact yaml2obj can't even produce a valid situation which causes llvm-objcopy to produce incorrect results because alignment and offset were both inherited from the sections the program header covers. This change fixes this bad behavior in llvm-objcopy.
You should probably have a test for the BinaryObject case as well.
In fact yaml2obj can't even produce a valid situation which causes llvm-objcopy to produce incorrect results because alignment and offset were both inherited from the sections the program header covers
Could you explain a bit more what you mean by this? The p_align field for a segment should be completely independent of the align field of its sections.
|15 ↗||(On Diff #119674)|
This might be related to the problem below, but this Address (and that of .data) does not lie within the corresponding segment, according to the VAddr field you specify for the segment, so I'm not sure this makes any sense.
|43–44 ↗||(On Diff #119674)|
Isn't this the wrong behaviour here?
p_offset = 0x230, p_vaddr = 0x1030, p_align = 0x1000, so (p_offset % p_align) != (p_vaddr % p_align).
I'd expect offset to be 0x1030 in this case.
I think this should be the case but I settled for allowing it to be computed from the sections at the request of the reviewer. I have a change in yaml2obj to make this the case up for review. Hopefully it gets reviewed soon.
|2–3 ↗||(On Diff #120873)|
Umm... reading this again, and I realise now that the test is actually testing yaml2obj, not llvm-objcopy!
You are using binary output to %t2, but then using readobj to check %t, which is the output of yaml2obj!
Also, there should really be test cases for both ELF and binary output format cases, since this change affects both.