This is an archive of the discontinued LLVM Phabricator instance.

ELF: Don't reuse a thunk in a different loadable partition.
ClosedPublic

Authored by pcc on May 23 2019, 8:25 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

pcc created this revision.May 23 2019, 8:25 PM
Herald added a project: Restricted Project. · View Herald Transcript

@pcc There are many partition related revisions. Do you know how to apply them all at once to see the whole picture? I don't know much about arc...

pcc added a comment.May 23 2019, 8:36 PM

I use this script to download patches from Phab (I think there's an arc patch command, but I've never had it work correctly):

#!/bin/sh

wget -O "/tmp/$1.diff" "https://reviews.llvm.org/$1?download=true"

I use it like this: arcdl D12345. Then I just do patch -p0 < /tmp/D12345.diff in my monorepo source tree. I think git apply should work as well, but I normally just use patch.

I think this is the right thing to do. I've left a suggestion to abstract the names a bit as I think that there are some other cases where we don't want to reuse Thunks across OutputSection boundaries.

A theoretical test case

        .section .sec1, "ax", %progbits
        bl far

        .section .sec2, "ax", %progbits
        bl far

        .section .sec3, "ax", %progbits
        .globl far
        .type far, %function
far:    bx lr
SECTIONS {
  .sec1 0x1000 (OVERLAY) : AT (0x1000) { *(.sec1) }
  .sec2 0x1000 (OVERLAY) : AT (0x2000) { *(.sec2) }
  .far  0x10000000 : { *(.sec3) }
}

ld.bfd correctly produces a Thunk in both of .sec1 and .sec2 as .sec1 and .sec2 will not be in memory at the same time. It looks like LLD isn't handling this test case well at the moment. I get relocation out of range errors and .sec1 and .sec2 being given a VMA and LMA of 0. I think that this is because the OVERLAY also has the property of removing SHF_ALLOC from the OutputSection which seems to be having some other side-effects. I'll raise a PR.

lld/ELF/Relocations.cpp
1626 ↗(On Diff #201119)

There are some other cases where we won't want to reuse across an OutputSection boundary, for example when the OutputSections are overlays. Would it be worth extracting into something like if (CompatibleOutputSections(T->getThunkTargetSym()->Section, IS)) where we could add to these conditions.

pcc updated this revision to Diff 201376.May 24 2019, 6:04 PM
pcc marked 2 inline comments as done.
pcc retitled this revision from ELF: Don't reuse a thunk in a different partition. to ELF: Don't reuse a thunk in a different loadable partition..

Address review comments

lld/ELF/Relocations.cpp
1626 ↗(On Diff #201119)

Makes sense. While moving this out to a function I took the opportunity to introduce a small optimization: if the thunk is in the main partition, we can always call it because it will always be loaded.

peter.smith accepted this revision.May 28 2019, 2:28 AM

Thanks for the update. I've marked as accepted as I think only mechanical changes will be needed if D60353 changes Partition representation.

This revision is now accepted and ready to land.May 28 2019, 2:28 AM
ruiu accepted this revision.May 28 2019, 5:30 AM

LGTM

This revision was automatically updated to reflect the committed changes.