Page MenuHomePhabricator

[PPC64] V2 abi: Add glink section for lazy symbol resolution.
ClosedPublic

Authored by sfertile on Apr 13 2018, 4:02 PM.

Details

Summary

This patch adds support for .glink resolver stubs from the example implementation in the V2 abi. (Section 4.2.5.3. Procedure Linkage Table)

The .glink section has a common resolver entry point '__glink_PLTresolve' which sets up the environment and then calls a resolver function supplied by the dynamic linker. Following __glink_PLTresolve is a set of branches, one for each function needing lazy resolution, which branch to the common resolver entry point.

Communicating to the dynamic linker where the lazy resolver stubs start is done with the PPC64_GLINK dynamic tag which has to point to the address 32 bytes before the first lazy symbol resolver stub.

In the V2 abi the .glink section is supposed to be merged into the end of the text section, this will be done in a follow up patch.

Diff Detail

Repository
rLLD LLVM Linker

Event Timeline

sfertile created this revision.Apr 13 2018, 4:02 PM

This is a plt by another name, no?

Do you know why it is defined to have another name?

Could it be implemented as a plt section and use the WritePltHeader/writePlt target hooks?

This is a plt by another name, no?

Do you know why it is defined to have another name?

I'm not sure what you mean by 'plt' here. In this abi the .plt section is just the array of addresses of external functions that the dynamic linker fills out at runtime. The .glink section holds the lazy resolution stubs which will setup the environment for the dynamic linker to do so. Then there are also stubs for calling the external functions by loading their address out of the .plt. Am I right to assume by 'plt' you mean a combination of lazy resolver and the call stub?

Could it be implemented as a plt section and use the WritePltHeader/writePlt target hooks?

This section is actually supposed to get merged into the end of the .text section (I'm planning to work on that as a follow on though) . Currently we are writing the call stubs in the .plt section, but we will have to fix that by writing the call stubs directly into the .text section and writing what we currently emit to the .got.plt section to the .plt section.

This is a plt by another name, no?

Do you know why it is defined to have another name?

I'm not sure what you mean by 'plt' here. In this abi the .plt section is just the array of addresses of external functions that the dynamic linker fills out at runtime. The .glink section holds the lazy resolution stubs which will setup the environment for the dynamic linker to do so. Then there are also stubs for calling the external functions by loading their address out of the .plt. Am I right to assume by 'plt' you mean a combination of lazy resolver and the call stub?

The question is why there is such a difference of terminology. At least of x86_64/x86/aarch64 the the array of addresses is the .got.plt section and the .plt section is what has the stubs. Just like .glink it has a special first entry. That is:

pp64 v2 | others
.glink | .plt
.plt | .got.plt

This is a plt by another name, no?

Do you know why it is defined to have another name?

I'm not sure what you mean by 'plt' here. In this abi the .plt section is just the array of addresses of external functions that the dynamic linker fills out at runtime. The .glink section holds the lazy resolution stubs which will setup the environment for the dynamic linker to do so. Then there are also stubs for calling the external functions by loading their address out of the .plt. Am I right to assume by 'plt' you mean a combination of lazy resolver and the call stub?

The question is why there is such a difference of terminology. At least of x86_64/x86/aarch64 the the array of addresses is the .got.plt section and the .plt section is what has the stubs. Just like .glink it has a special first entry. That is:

pp64 v2 | others
.glink | .plt
.plt | .got.plt

Thanks for clarifying. I knew about the .plt <--> .got.plt name mismatch for the array of function addresses, but I had though the stubs being emitted in the .plt section for the other targets were the equivalent of the call stubs that we are currently emitting in the .plt section on PPC64 right now.

Could it be implemented as a plt section and use the WritePltHeader/writePlt target hooks?

Yeah, this seems like a good fit, as long as there is no objection to renaming GotPltSection and PltSection for EM_PPC64.

ruiu added a comment.Apr 19 2018, 10:09 AM

Yeah, this seems like a good fit, as long as there is no objection to renaming GotPltSection and PltSection for EM_PPC64.

I don't have any objection. The name mismatch is very unfortunate, but if they only have different names, that'd be straightforward.

grimar added a subscriber: grimar.Apr 23 2018, 1:59 AM
grimar added inline comments.
ELF/SyntheticSections.cpp
1207

because

1209

Did you mean Config->EMachine == EM_PPC64 && Config->IsLE ?

Given the comment, should Config->IsLE check be omitted?

Actually, it seems it could be the following then.

if (InX::Glink && !InX::Glink->empty()) { ...

2715

This assert does not make much sense actually. With or without it we are going to crash
anyways.

2725

Seems to be excessive check for me. I would expect we remove the synthetic section from
the output if it is an empty one. Hence I think we should never reach writeTo for an empty section.

2749

You use these only in a single place, so I would suggest to inline them.

ELF/Writer.cpp
400 ↗(On Diff #142475)

Please remove the excessive empty line.

I've moved the lazy resolver stubs to .plt section as suggested and will hopefully post the updated patch shortly. Everything seems to be working as expected. I had to re implement the plt call-stubs as thunks before I could do that move, but that was something that was going to have to be done eventually anyways.

ELF/SyntheticSections.cpp
1209

Yeah I meant Config->EMachine == EM_PPC64 && Config->IsLE, but what I should be checking is Config->EMachine == EM_PPC64 && Config->EFlags == 2.

I've since added the eflag check I want in https://reviews.llvm.org/D46076

sfertile updated this revision to Diff 144823.May 1 2018, 7:28 PM

Moved the lazy resolver stubs to the .plt section, and renamed the ,plt and .got.plt sections.

I've looked at the ifunc test we had and there are a number of things broken with the ppc ifunc implementation so I've XFAIL'ed it rather then simply update it. Our ifunc problems can be fixed in a separate patch.

sfertile edited the summary of this revision. (Show Details)May 1 2018, 7:30 PM
ruiu added inline comments.May 3 2018, 10:12 AM
ELF/Arch/PPC64.cpp
196

Looks like you set 60 to PltHeaderSize but you are using only the first 56 bytes. Is this intentional?

ELF/SyntheticSections.cpp
882–886

BSS-ish .got.plt looks pretty odd. Is this really what the spec says? It at least needs some explanation in comment for a reader.

885

This also needs an explanation.

1042–1043

Since you are using this only once, I'd inline it and remove this function.

ELF/Target.h
160–162 ↗(On Diff #144823)

We often inline this kind of magic numbers with some comments if they are used only once, especially when they are a constant specified by the spec.

sfertile added inline comments.May 4 2018, 1:55 PM
ELF/Arch/PPC64.cpp
196

I'm writing a 64 bit offset to Buf + 52, so the whole 60 bytes are used.

ELF/SyntheticSections.cpp
882–886

yeah this is odd, I don't know why its made to look like a .bss section but it is specified by the abi (both V1 and V2 64 bit abis as well as the 32 bit abi I believe)

ruiu added inline comments.May 4 2018, 2:59 PM
ELF/Arch/PPC64.cpp
196

Ah. I stand corrected!

ELF/SyntheticSections.cpp
882–886

Can you add this comment as a code comment? I believe honest comment like that will be useful in the future.

sfertile updated this revision to Diff 145621.May 7 2018, 6:58 PM
sfertile edited the summary of this revision. (Show Details)

Updated based on review comments.

ruiu added inline comments.May 7 2018, 7:04 PM
ELF/SyntheticSections.cpp
880

nit: accross -> across, abis -> ABIs, abi -> ABI

882–886

I confirmed https://members.openpowerfoundation.org/document/dl/576 p.93 says that .plt is SHT_NOBITS.

914–928

I think what you are doing is obvious, so please explain why.

1210

Please avoid auto.

1212

I think this format does not follow the LLVM coding style. Maybe you want to use clang-format.

1935

Indentation

sfertile marked 6 inline comments as done.May 7 2018, 7:04 PM
sfertile added inline comments.
ELF/SyntheticSections.cpp
882–886

Yep, added.

1209

Zaara committed a patch that removes support for the V1 abi, so I no longer need to check the abi version here.

sfertile updated this revision to Diff 145731.May 8 2018, 11:02 AM
sfertile marked an inline comment as done.

Addressed review comments.

ruiu accepted this revision.May 8 2018, 11:07 AM

LGTM

This revision is now accepted and ready to land.May 8 2018, 11:07 AM
sfertile marked 4 inline comments as done.May 8 2018, 11:12 AM
sfertile added inline comments.
ELF/SyntheticSections.cpp
930–932

I used clang-format on this but didn't like the result:

Config->EMachine == EM_ARM
    ? ".got"
    : Config->EMachine == EM_PPC64 ? ".plt"
                                   : ".got.plt") {}

So I manually formatted, does anyone have a preference on this?

1935

I believe the indentation here is correct. The line I added is an argument to the SyntheticSection constructor so it lines up with the first arg on the previous line. The following line is a different member initializer and so it lines up with the SyntheticSection constructor.

ruiu added inline comments.May 8 2018, 11:23 AM
ELF/SyntheticSections.cpp
930–932

Maybe, I wouldn't fix what clang-format formatted by hand, because otherwise when I edit it next time, clang-format would try to change the format again, and I'd have to fix it by hand again. It seems to be better to not fight against the tool.

That said, I might factor this out as a static (file local) function getIgotPltName() which returns a StringRef for a chosen target, so that I don't need to write a cascading ?: expressionsas an initializer expression.

sfertile added inline comments.May 8 2018, 11:40 AM
ELF/SyntheticSections.cpp
930–932

Good idea. I'll update it.

This revision was automatically updated to reflect the committed changes.