Page MenuHomePhabricator

[ELF][PPC64] Remove unneeded PPC64PCRelLongBranchThunk
ClosedPublic

Authored by MaskRay on Nov 26 2021, 12:41 PM.

Details

Summary

This reverts the PPC64PCRelLongBranchThunk part from D86706.
PPC64PCRelLongBranchThunk is the same as PPC64R12SetupStub.

Use __gep_setup_ instead of __long_branch_pcrel_ for the stub symbol name
as it more closely indicates the operation.
(Note: GNU ld uses *.long_branch.* and *.plt_branch.*).

Diff Detail

Event Timeline

MaskRay created this revision.Nov 26 2021, 12:41 PM
MaskRay requested review of this revision.Nov 26 2021, 12:41 PM
Herald added a project: Restricted Project. · View Herald TranscriptNov 26 2021, 12:41 PM
MaskRay retitled this revision from [ELF][PPC64] Remove unneeded PPC64PCRelLongBranchThunk. NFC to [ELF][PPC64] Remove unneeded PPC64PCRelLongBranchThunk.Nov 26 2021, 12:41 PM

How important is it for the thunk symbol names to match ld.bfd/ld.gold?

lld/ELF/Thunks.cpp
986

I am really not a fan of the name. The reason we emit this has nothing to do with long branches and the code we emit is not necessarily PC-Relative.

We emit this thunk when we need to branch to a GEP (Global Entry Point) of local function Function1 simply because the caller doesn't maintain a TOC pointer and Function1 does.

The existing name makes sense because the GEP assumes that r12 contains the address of the GEP itself (i.e. at the first instruction in the GEP, r12 == pc).

How important is it for the thunk symbol names to match ld.bfd/ld.gold?

Not important. If linkers can have a common name which is reasonable to both parties, I am happy to do so so that users would need to learn less stuff.

PPC64R12SetupStub is not only used by NOTOC->TOC code, but is also used by NOTOC->NOTOC long branches, so __long_branch_pcrel is not that bad. That said, if __gep_setup makes more sense, I am happy to use it, too.

nemanjai accepted this revision.Nov 30 2021, 4:42 AM

PPC64R12SetupStub is not only used by NOTOC->TOC code, but is also used by NOTOC->NOTOC long branches, so __long_branch_pcrel is not that bad. That said, if __gep_setup makes more sense, I am happy to use it, too.

Ah yes, that's a good point. In fact, that is the more common use for this thunk (I neglected the fact that we are removing the check for st_other > 1).

LGTM.

This revision is now accepted and ready to land.Nov 30 2021, 4:42 AM
NeHuang accepted this revision.Nov 30 2021, 8:44 AM
This comment was removed by NeHuang.
lld/ELF/Thunks.cpp
986

+1, the existing name makes more sense which is consistent the functionality of the thunk. Would be better we keep __gep_setup_ as the name.

NeHuang added inline comments.Nov 30 2021, 8:54 AM
lld/ELF/Thunks.cpp
986

Just noticed (s.stOther >> 5) > 1 got removed. LGTM and approved. Thank you for fixing it,

MaskRay updated this revision to Diff 390764.Nov 30 2021, 11:30 AM
MaskRay marked 3 inline comments as done.

Pick __gep_setup_ instead of __long_branch_pcrel_

MaskRay edited the summary of this revision. (Show Details)Nov 30 2021, 11:31 AM
This revision was landed with ongoing or failed builds.Nov 30 2021, 11:33 AM
This revision was automatically updated to reflect the committed changes.