[PATCH] D68094: ELF: Don't merge SHF_LINK_ORDER sections for different output sections in relocatable links.

[Peter Collingbourne via Phabricator via llvm-commits]

pcc added a comment.

In D68094#1685320 <https://reviews.llvm.org/D68094#1685320>, @MaskRay wrote:

> For the record, the original approach does not work for the `ld.lld exidx.o exidx2.o -r -o exidxr.o -T exidx.lds` example at https://reviews.llvm.org/D68094?id=221994#1685161:
>
>   [ 1] .foo              PROGBITS        00000000 000034 000010 00  AX  0   0  1
>   [ 2] .text             PROGBITS        00000000 000044 000004 00  AX  0   0  4
>   [ 3] .ARM.exidx.text.f1 ARM_EXIDX       00000000 000048 000008 00  AL  1   0  4               
>   [ 4] .rel.ARM.exidx.text.f1 REL             00000000 000050 000008 08   I 13   3  4                
>   [ 5] .ARM.exidx.text.f2 ARM_EXIDX       00000000 000058 000008 00  AL  1   0  4
>   [ 6] .rel.ARM.exidx.text.f2 REL             00000000 000060 000010 08   I 13   5  4
>   [ 7] .ARM.attributes   ARM_ATTRIBUTES  00000000 000070 00001e 00      0   0  1
>   [ 8] .ARM.exidx.text.f1 ARM_EXIDX       00000000 000090 000008 00  AL  1   0  4
>   [ 9] .rel.ARM.exidx.text.f1 REL             00000000 000098 000010 08   I 13   8  4
>   [10] .ARM.exidx.text.f2 ARM_EXIDX       00000000 0000a8 000008 00  AL  1   0  4
>   [11] .rel.ARM.exidx.text.f2 REL             00000000 0000b0 000008 08   I 13  10  4
>   
>
> The new approach can cause a segfault.


Yes, or the SHF_LINK_ORDER section can be silently discarded. Thanks for catching that, I've adopted your suggested fix and extended my test to cover it.

> Before this patch, or with the fixed patch, we get the same section layout:
> 
>   [ 1] .foo              PROGBITS        00000000 000034 000010 00  AX  0   0  1
>   [ 2] .text             PROGBITS        00000000 000044 000004 00  AX  0   0  4
>   [ 3] .ARM.exidx.text.f1 ARM_EXIDX       00000000 000048 000010 00  AL  1   0  4
>   [ 4] .rel.ARM.exidx.text.f1 REL             00000000 000058 000018 08   I  9   3  4
>   [ 5] .ARM.exidx.text.f2 ARM_EXIDX       00000000 000070 000010 00  AL  1   0  4
>   [ 6] .rel.ARM.exidx.text.f2 REL             00000000 000080 000018 08   I  9   5  4
>    
> 
> It may still be less than idea because we would expect .ARM.exidx.text.f2 to be placed before .ARM.exidx.text.f1. It is an existing issue though.

Yes, this is a tricky case because we would need to know the section merging rules used in the final link in order to handle it correctly. Peter's option 1 from his first comment would handle this case but I'm not sure whether it makes sense to do that in the presence of linker scripts.

> For the new test `relocatable-linkorder.s`, ld.bfd can combine the two `foo` sections (the behavior before this patch):
> 
>   [ 1] .text             PROGBITS        0000000000000000 000040 000000 00  AX  0   0  4
>   [ 2] .text.f1          PROGBITS        0000000000000000 000040 000001 00  AX  0   0  1
>   [ 3] .text.f2          PROGBITS        0000000000000000 000041 000001 00  AX  0   0  1
>   [ 4] foo               PROGBITS        0000000000000000 000042 000010 00  AL  2   0  1
>    
> 
> It may be worth sending a feature request. I have sent 3 requests based on LLD's treatment of SHF_LINK_ORDER.
> 
> - https://sourceware.org/bugzilla/show_bug.cgi?id=24526 Generalize GC support for SHF_LINK_ORDER
> - https://sourceware.org/bugzilla/show_bug.cgi?id=25021 Garbage collecting non-alloc SHF_LINK_ORDER sections
> - https://sourceware.org/bugzilla/show_bug.cgi?id=25022 Make /DISCARD/ discard SHF_LINK_ORDER sections

I can see if I can find some time to file a feature request.

>> Merging SHF_LINK_ORDER sections can affect semantics if the sh_link
> 
> fields point to different sections.
> 
> The summary can be a bit more detailed about how this affects semantics.

Okay, I'll add more detail to the commit message.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68094