This is an archive of the discontinued LLVM Phabricator instance.

[PPC32] Improve the 32-bit PowerPC port
ClosedPublic

Authored by MaskRay on May 26 2019, 8:04 AM.

Details

Summary

Many -static/-no-pie/-shared/-pie applications linked against glibc or musl
should work with this patch. This also helps FreeBSD PowerPC64 to migrate
their lib32 (PR40888).

  • Fix default image base and max page size.
  • Support new-style Secure PLT (see below). Old-style BSS PLT is not implemented, so it is not suitable for FreeBSD rtld now because it doesn't support Secure PLT yet.
  • Support more initial relocation types: R_PPC_ADDR32, R_PPC_REL16*, R_PPC_LOCAL24PC, R_PPC_PLTREL24, and R_PPC_GOT16. The addend of R_PPC_PLTREL24 is special: it decides the call stub PLT type but it should be ignored for the computation of target symbol VA.
  • Support GNU ifunc
  • Support .glink used for lazy PLT resolution in glibc
  • Add a new thunk type: PPC32PltCallStub that is similar to PPC64PltCallStub. It is used by R_PPC_REL24 and R_PPC_PLTREL24.

A PLT stub used in -fPIE/-fPIC usually loads an address relative to
.got2+0x8000 (-fpie/-fpic code uses _GLOBAL_OFFSET_TABLE_ relative
addresses).
Two .got2 sections in two object files have different addresses, thus a PLT stub
can't be shared by two object files. To handle this incompatibility,
change the parameters of Thunk::isCompatibleWith to
const InputSection &, const Relocation &.

PowerPC psABI specified an old-style .plt (BSS PLT) that is both
writable and executable. Linkers don't make separate RW- and RWE segments,
which causes all initially writable memory (think .data) executable.
This is a big security concern so a new PLT scheme (secure PLT) was developed to
address the security issue.

TLS will be implemented in D62940.

glibc older than ~2012 requires .rela.dyn to include .rela.plt, it can
not handle the DT_RELA+DT_RELASZ == DT_JMPREL case correctly. A hack
(not included in this patch) in LinkerScript.cpp addOrphanSections() is
required:

if (Config->EMachine == EM_PPC) {
  // Older glibc assumes .rela.dyn includes .rela.plt
  Add(In.RelaDyn);
  if (In.RelaPlt->isLive() && !In.RelaPlt->Parent)
    In.RelaDyn->getParent()->addSection(In.RelaPlt);
}

Event Timeline

MaskRay created this revision.May 26 2019, 8:04 AM
Herald added a project: Restricted Project. · View Herald Transcript
MaskRay updated this revision to Diff 201448.May 26 2019, 8:14 AM

Fix a typo

ruiu added a subscriber: ruiu.May 27 2019, 3:43 AM

I think that the commit message is a bit too terse. Could you explain the current status of ppc32 if you can? How much is it usable and how much does this patch improve?

MaskRay updated this revision to Diff 202603.Jun 2 2019, 8:23 AM
MaskRay retitled this revision from [PPC] Improve 32-bit PowerPC to [PPC32] Improve 32-bit PowerPC: many applications linked against musl may work with this patch.
MaskRay edited the summary of this revision. (Show Details)
MaskRay removed a subscriber: ruiu.

.

MaskRay updated this revision to Diff 202604.Jun 2 2019, 8:44 AM
MaskRay edited the summary of this revision. (Show Details)

Reword

ruiu added a comment.Jun 2 2019, 9:32 PM

Generally looking great.

ELF/Options.td
30 ↗(On Diff #202604)

nit: powerpc32 → PowerPC32

311 ↗(On Diff #202604)

Ditto

ELF/SyntheticSections.cpp
1042

Can you add return here? Or maybe you want to make this a switch whose each "case" ends with a return.

1043

This magic number needs a comment.

2260

nit: add parentheses before ?.

3217

Can you add a comment?

MaskRay updated this revision to Diff 202669.Jun 3 2019, 2:19 AM

Add some support for BSS-PLT ABI

Will add some more tests.

MaskRay marked 7 inline comments as done.Jun 3 2019, 2:56 AM
MaskRay added inline comments.
ELF/SyntheticSections.cpp
2260

I think PowerPC32 doesn't need .glink. I'll just delete this change.

MaskRay updated this revision to Diff 202677.Jun 3 2019, 2:56 AM
MaskRay marked an inline comment as done.

Address review comments

ruiu added a comment.Jun 4 2019, 5:22 AM

What is a motivation to support the old-style PLT? If a program really expects that the PLT is in a writable segment, something is wrong with the program...

What is a motivation to support the old-style PLT? If a program really expects that the PLT is in a writable segment, something is wrong with the program...

I didn't want to support BSS PLT (old-style PLT).

For Secure PLT, I can get musl -no-pie/-pie/-shared and glibc -no-pie work with this patch.

For BSS PLT, because musl doesn't support it, I have to debug glibc in qemu-system-ppc -hda debian_wheezy_powerpc_standard.qcow2, which is a pain... (glibc cannot be compiled without -O2 => the quality of debug info is at gcc's mercy).

On FreeBSD's side, I believe its rtld doesn't support Secure PLT.

@adalava @emaste It would be great if you can migrate from BSS PLT to Secure PLT (-msecure-plt; default in gcc if it is configured with --enable-secure-plt; many Linux gcc packages are configured this way).

MaskRay updated this revision to Diff 203113.Jun 5 2019, 3:27 AM
MaskRay retitled this revision from [PPC32] Improve 32-bit PowerPC: many applications linked against musl may work with this patch to [PPC32] Improve 32-bit PowerPC.
MaskRay edited the summary of this revision. (Show Details)
MaskRay edited reviewers, added: grimar, ruiu; removed: espindola.
MaskRay edited subscribers, added: kib; removed: ruiu.

Ready for review.

Many -no-pie/-pie/-shared linked against musl or glibc should work now.

ruiu added inline comments.Jun 5 2019, 8:48 PM
ELF/Arch/PPC.cpp
105

Is this a nop? Can you add a comment?

ELF/SyntheticSections.cpp
2275

Hmm, so we don't call writePlt() if a target is PPC32?

ELF/Thunks.cpp
243 ↗(On Diff #203113)

nit: usually we write public members before private members.

709 ↗(On Diff #203113)

nit: flip the condition and return early so that the indentation is one level shallower?

717–718 ↗(On Diff #203113)

format.

MaskRay marked 5 inline comments as done.Jun 5 2019, 11:10 PM
MaskRay added inline comments.
ELF/SyntheticSections.cpp
2275

On PPC32, how PLT works is different from targets (it is still similar to PPC64). I've added more comments in PPC::writePltHeader.

MaskRay updated this revision to Diff 203287.Jun 5 2019, 11:10 PM

Address review comments

ruiu added inline comments.Jun 5 2019, 11:18 PM
ELF/SyntheticSections.cpp
2275

So, this is not really a PLT header but a PPC32-specific mechanism. Is writePltHeader a correct name? Maybe we should rename this writePPC32PltResolver or something like that?

MaskRay updated this revision to Diff 203290.Jun 5 2019, 11:37 PM

Also add R_PPC_ADDR16

MaskRay marked an inline comment as done.Jun 5 2019, 11:46 PM
MaskRay added inline comments.
ELF/SyntheticSections.cpp
2275

.glink is indeed a PPC-specific mechanism (PPC64 moves the "footer" (PLTresolve) to the "header" so it looks more conventional).

The PPC class is in an unnamed namespace so I cannot access it from SyntheticSections.cpp. If you still think writePPC32PltResolver is the way to go, I can extract it from the class and place it into the lld::elf:: namespace. I still feel it may not worth the trouble.

namespace {
class PPC final : public TargetInfo {
public:
...
  void writePltHeader(uint8_t *Buf) const override;
ruiu added inline comments.Jun 5 2019, 11:56 PM
ELF/SyntheticSections.cpp
2275

I feel like a PPC32-specific knowledge such as the actual machine instruction code leaks here, which is I think not ideal. How about moving the code to write b PLTresolve to writePltHeader and rename it writePPC32PltSection? Currently, a concrete implementation to write the first half of PPC32's PLT section is in this function while the other is in writePltHeader, which is not actually a header but a trailer. Nothing is really complicated, but there are perhaps too many things that are slightly confusing.

MaskRay updated this revision to Diff 203295.Jun 6 2019, 12:32 AM

Add Target::writePPC32PltSection and let PPC::writePPC32PltSection override it.
This is to isolate the PPC32 knowledge.

ruiu added inline comments.Jun 6 2019, 12:36 AM
ELF/Arch/PPC.cpp
31

To make it clear that we write a PLT section in a different way, I'd define writePlt and writePltHeader which just call llvm_unreachable().

ELF/Target.h
45 ↗(On Diff #203295)

This function doesn't have to be in this class because no dynamic dispatching is necessary. Only PPC32 implements this as its name implies. I'd make this a non-member function.

MaskRay updated this revision to Diff 203297.Jun 6 2019, 12:42 AM

Forgot to add Buf += 4 * NumEntries;

MaskRay updated this revision to Diff 203303.Jun 6 2019, 1:38 AM
MaskRay marked 3 inline comments as done.

Remove Target::writePPC32GlinkSection()
Add elf::writePPC32GlinkSection()

MaskRay marked 2 inline comments as done.Jun 6 2019, 1:38 AM
ruiu accepted this revision.Jun 6 2019, 1:40 AM

LGTM

This revision is now accepted and ready to land.Jun 6 2019, 1:40 AM

I cross compiled libc-test with powerpc-linux-gnu-gcc + lld.

% cd ~/libc-test # config.mak is below
% make > /tmp/libc-test.lld
# /tmp/libc-test.bfd was created in a similar way
% grep -c FAIL /tmp/libc-test.bfd /tmp/libc-test.lld
/tmp/libc-test.bfd:112
/tmp/libc-test.lld:112

So lld linked libc-test doesn't have more failures than bfd linked libc-test :)
(Tests ran under qemu-static-ppc. The situation may get better on real hardware.)

CROSS_COMPILE=powerpc-linux-gnu-
CFLAGS += -mlong-double-64 -msecure-plt
CC = powerpc-linux-gnu-gcc

CFLAGS += -specs ~/musl/powerpc/lib/musl-gcc.specs
LDFLAGS += -specs ~/musl/powerpc/lib/musl-gcc.specs
...
LDFLAGS += -fuse-ld=gold -g  # my /usr/powerpc-linux-gnu/bin/ld.gold points to lld... gcc 9 starts to understand -fuse-ld=lld
MaskRay updated this revision to Diff 203322.Jun 6 2019, 3:36 AM
MaskRay retitled this revision from [PPC32] Improve 32-bit PowerPC to [PPC32] Improve the 32-bit PowerPC port.
MaskRay edited the summary of this revision. (Show Details)

Implement R_PPC_GLOB_DAT and improve a R_PPC_GOT16 test while testing libc-test with glibc

Great job, @MaskRay !!

By initial tests, it seems that the PPC 32 bit FreeBSD "loader/bootloader" is now working fine when compiled with clang-8 and linked with LLD-9 with this patch. It is a static-stand-alone binary that doesn't depend on *crt*stuff.

For the 32 compatibility packages I now receive the error: "<unknown>:0: error: Undefined temporary symbol" when compiling everything with -msecure-plt. I'll retest with D62940 patch applyed

MaskRay updated this revision to Diff 203389.Jun 6 2019, 9:43 AM
MaskRay edited the summary of this revision. (Show Details)

Add more tests

This revision was automatically updated to reflect the committed changes.