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

Wed Feb 26 10:13:37 PST 2020

MaskRay added a comment.

In D74755#1892936 <https://reviews.llvm.org/D74755#1892936>, @jhenderson wrote:

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

I have rechecked usage of Section::ParentSegment. The only fuzzy thing I find is ihex output:

  static uint64_t sectionPhysicalAddr(const SectionBase *Sec) {
    Segment *Seg = Sec->ParentSegment;
    if (Seg && Seg->Type != ELF::PT_LOAD)
      Seg = nullptr;
    return Seg ? Seg->PAddr + Sec->OriginalOffset - Seg->OriginalOffset
               : Sec->Addr;
  }

This is related to the load memory address (LMA) concept. I started to know LMA with a previous llvm-objcopy `-O binary` change. My recent linker changes related to LMA regions (D74286 <https://reviews.llvm.org/D74286>, D74297 <https://reviews.llvm.org/D74297>) enhanced my understanding, but I confess I still don't know enough.

The situation you deal with (`p_offset(PT_TLS) < p_offset(PT_LOAD)`) is in my mind. I have tried hard thinking of several cases but I can't find a case where the patch can break copy/only-keep-debug usage.

I suggest we start with this patch, improve it along the way when we have more detailed description of the use cases.

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

I don't think it is simply "a bug in GNU objcopy/strip". There is some inherent issues with this layout.

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

Agreed. I have read some binutils code and I totally agree that the related code in bfd/elf.c (assign_file_positions*) and ld/ldlang.c can be largely simplified. For example, a PT_LOAD pass followed by a non-PT_LOAD pass should really be a segment(section) pass followed by a section(segment) pass.
(I came up with the current --only-keep-debug approach after thinking very hard about the lld Writer, the llvm-objcopy layout and bfd/elf.c assign_file_positions*)

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

Sorry, I can't agree with "this patch is breaking behavior that currently works."
(If you have a test case, I'll happily adapt. I can't think of a case this patch can break, though.)

Note that I was imagining how the changed rule could theoretically break future use cases.
I suggest we start thinking about them after we have concrete use cases.

1. Adding a flattening rule can be hardly tested now. 2) Such a rule may be completely rewritten.

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

I am pleased this patch managed to support @yikong's use cases by deleting code:)

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74755