This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by andrewng on Apr 26 2019, 9:04 AM.

Details

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.

Diff Detail

Repository
rL LLVM

Event Timeline

andrewng created this revision.Apr 26 2019, 9:04 AM

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

phosek added a subscriber: phosek.Apr 29 2019, 5:07 PM

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

MaskRay accepted this revision.EditedApr 29 2019, 10:21 PM

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

This revision is now accepted and ready to land.Apr 29 2019, 10:21 PM

@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 added a comment.EditedApr 30 2019, 2:43 AM

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

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 marked an inline comment as done.Apr 30 2019, 3:22 AM
andrewng added inline comments.
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.

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptApr 30 2019, 5:24 AM