[PATCH] D74755: [llvm-objcopy] Attribute an empty section to a segment ending at its address

Wed Feb 26 01:59:57 PST 2020

jhenderson added a comment.

In D74755#1892031 <https://reviews.llvm.org/D74755#1892031>, @MaskRay wrote:

> For the PT_LOAD which include the PT_TLS, note that `p_offset(PT_LOAD) = p_offset(PT_TLS)` on all versions of GNU ld and gold, and lld>=9.0 (precisely, after D56828 <https://reviews.llvm.org/D56828>).

Just because GNU ld, LLD and gold are the most common opensource linkers around doesn't mean that they're the only linkers in existence, and that all outputs will conform to their output styles. Indeed, in our downstream linker script, we have precisely the situation where `p_offset(PT_LOAD) != p_offset(PT_TLS)`, although I admit that I don't think we use any of the functionality affected by this change currently (though we might in the future).

> We will set `ParentSegment` to the PT_TLS if `p_offset(PT_LOAD) < p_offset(PT_TLS)`. GNU strip/objcopy<2.31 does not handle such PT_TLS (there are various other toolchain validation issues) => this makes such a layout quite problematic => it is not worth working around the layout in llvm-objcopy. (Alan Modra kindly worked around lld's old layout in binutils 2.31 but I hope they can drop that hack (https://sourceware.org/bugzilla/show_bug.cgi?id=22829))

Just because there's a bug in GNU objcopy/strip doesn't mean we should try to emulate that bug. This is a good example of where we can (in fact, we already do) and should do better than GNU tools. I think I've been persuaded that an empty section at the end of another segment should be considered part of that segment IFF there is no segment to its right, but I don't agree with breaking parent layout calculations that currently work in all other cases.

>  --------
> 
> It is also possible to place PT_TLS before PT_LOAD to set `ParentSegment` to the PT_TLS, but it is also an impractical scenario. The solution I pick here is consistent with:
> 
>   static bool compareSegmentsByOffset(const Segment *A, const Segment *B) {
>     // Any segment without a parent segment should come before a segment
>     // that has a parent segment.
>     if (A->OriginalOffset < B->OriginalOffset)
>       return true;
>     if (A->OriginalOffset > B->OriginalOffset)
>       return false;
>     return A->Index < B->Index;
>   }

I'm not sure it is consistent with the system as a whole. Note that segments are wholly resolved such that the nested tree is flattened (i.e. no parent segment has a parent segment itself) (see `setParentSegment`). This change would mean that sections don't get this same behaviour (their parent segment would be the "deepest" segment).

> Linkers don't place a PT_TLS before a PT_LOAD. One can use PHDRS command to place a PT_TLS before a PT_LOAD, but we can say that is unsupported by llvm-objcopy.

Why are we at all justified in saying that a perfectly legal PHDRS command is unsupported by llvm-objcopy? This seems extremely unreasonable, especially when we can't even assume that all linkers output that style, as there is nothing in the ELF specification mandating it (PT_LOADs are the only things mentioned in the gABI in relation to program header table ordering). Worse yet, by making the change you're proposing, we're actually breaking behaviour that currently works.

> (A recent binutils change https://sourceware.org/git/?p=binutils-gdb.git;a=commit;h=30fe183248b2523ecff9da36853e2f893c4c4b91 introduced a fairly complex comparison function whose purpose is similar to our's. I think it is an overengineering.)

The fix appears to solve a bug though (the tools couldn't handle a valid input). Whilst the solution might be more complex than required (I don't know whether it is or not), a solution was necessary.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74755/new/

https://reviews.llvm.org/D74755