[LLD][ELF] Fix getRankProximity to "ignore" not live sections

[andrewng]

Summary

This is a follow up to r358979 which made findOrphanPos only consider
live sections. Unfortunately, this required change to getRankProximity,
used by findOrphanPos, was missed.

[andrewng]

This is how this function used to behave (see r316307), when findOrphanPos ignored non-live sections.

[phosek]

Would it be possible to get this reviewed? I have confirmed that it fixes PR41653.

[MaskRay]

@phosek I think it is better to commit this than reverting D60131 and commiting the two patches again. Rui is probably on holiday..

I think the linker script can be simplified to SECTIONS { .pad : { QUAD(0); } .text : { *(.text) } .ro : { *(.ro) } }. PHDRS is not needed.

Hope @grimar can confirm.

It does fix PR41653.

# fuchsia/zircon/kernel/kernel.ld
    .text.boot0 : {  ## it has no input section thus Sec->Live is false for it. `int Proximity = getRankProximity(Sec, *I);` is -1 which is not expected.

        /*
         * Put some data in, or else the linker makes it a SHT_NOBITS
         * section and that makes objcopy -O binary skip it in the image.
         */
        LONG(0xdeadbeef);
        . += BOOT_HEADER_SIZE - 4;
    } :code

ELF/Writer.cpp

1054 ↗

(On Diff #196860)

Delete the parentheses?

COFF/PDB.cpp contains C && OS ? C->getRVA() - OS->getRVA() : 0 and there is no parentheses.

[grimar]

@MaskRay is right that the linker script can be simplified to SECTIONS { .pad : { QUAD(0); } .text : { *(.text) } .ro : { *(.ro) } }, I think it looks much simpler.

It is strange for me to see !Live output section that is actually in the output though. Let me debug it a bit.

ELF/Writer.cpp

1054 ↗

(On Diff #196860)

I would keep them for readability.

[grimar]

@MaskRay is right that the linker script can be simplified to SECTIONS { .pad : { QUAD(0); } .text : { *(.text) } .ro : { *(.ro) } }, I think it looks much simpler.

I am taking these words back :)
Now, after debugging this, I think the original test case was fine. It allowed to catch a case when .orphan was placed in a different segment for me during
the experiments with the code. So I would not simplify it probably.

In D61197#1483670, @MaskRay wrote:

It does fix PR41653.

# fuchsia/zircon/kernel/kernel.ld
    .text.boot0 : {  ## it has no input section thus Sec->Live is false for it. `int Proximity = getRankProximity(Sec, *I);` is -1 which is not expected.

There is a mistake here. -1 is used in a new code instead int Proximity = getRankProximity(Sec, *I); which never returned -1 I think.

To summarize I am OK with this patch I think.
What I am a bit unhappy with is that Sec->Live is false for the output section that is actually in the output. It was confusing for me.
That started before this patch though. I tried to switch Sec->Live to true for .pad section and experimented with a code a bit but was unable to quickly find the solution.
Perhaps what can we do to avoid the confusion is to comment Live flag for an output section better.
Now it contains comment from input section perpective.

test/ELF/linkerscript/orphan-live-only.s

38 ↗

(On Diff #196860)

What do you think about adding one more .orphan here with "ax" to show it is placed to a different segment?
i.e:

.section .orphan1,"a"
 .byte 0
 
.section .orphan2,"ax"
 .byte 0

[andrewng]

test/ELF/linkerscript/orphan-live-only.s

38 ↗

(On Diff #196860)

I don't mind adding it, although the behaviour of the second orphan is not affected by this patch. However, I guess "no change" can have some value too. I will add it unless someone objects.

lld/trunk/ELF/Writer.cpp

 // The more branches in getSectionRank that match, the more similar they are.
 // Since each branch corresponds to a bit flag, we can just use
 // countLeadingZeros.
 static int getRankProximityAux(OutputSection *A, OutputSection *B) {
   return countLeadingZeros(A->SortRank ^ B->SortRank);
 }
 
 static int getRankProximity(OutputSection *A, BaseCommand *B) {
-  if (auto *Sec = dyn_cast<OutputSection>(B))
-    return getRankProximityAux(A, Sec);
-  return -1;
+  auto *Sec = dyn_cast<OutputSection>(B);
+  return (Sec && Sec->Live) ? getRankProximityAux(A, Sec) : -1;
 }
 
 // When placing orphan sections, we want to place them after symbol assignments
 // so that an orphan after
 //   begin_foo = .;
 //   foo : { *(foo) }
 //   end_foo = .;
 // doesn't break the intended meaning of the begin/end symbols.

lld/trunk/test/ELF/linkerscript/orphan-live-only.s

+# REQUIRES: x86
+# RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-linux %s -o %t.o
+# RUN: echo "PHDRS { \
+# RUN:   exec PT_LOAD FLAGS(0x4 | 0x1); \
+# RUN:   ro   PT_LOAD FLAGS(0x4); \
+# RUN: } \
+# RUN: SECTIONS { \
+# RUN:  .pad : { QUAD(0); } :exec \
+# RUN:  .text : { *(.text) } \
+# RUN:  .ro : { *(.ro) } :ro \
+# RUN: }" > %t.script
+# RUN: ld.lld -o %t --script %t.script %t.o
+# RUN: llvm-readelf -S -l %t | FileCheck %s
+
+## The ".pad" section is not "live" and should be ignored by the
+## orphan placement.
+##
+## Check that the orphan section is placed correctly and belongs to
+## the correct segment.
+
+# CHECK: Section Headers
+# CHECK: .pad
+# CHECK-NEXT: .text
+# CHECK-NEXT: .orphan2
+# CHECK-NEXT: .ro
+# CHECK-NEXT: .orphan1
+
+# CHECK: Segment Sections
+# CHECK-NEXT: .pad .text .orphan2
+# CHECK-NEXT: .ro .orphan1
+
+.section .text,"ax"
+ ret
+
+.section .ro,"a"
+ .byte 0
+
+.section .orphan1,"a"
+ .byte 0
+
+.section .orphan2,"ax"
+ ret