This is an archive of the discontinued LLVM Phabricator instance.

[ELF][PowerPC] Support R_PPC_COPY and R_PPC64_COPY
ClosedPublic

Authored by MaskRay on Jan 22 2020, 11:38 PM.

Diff Detail

Event Timeline

MaskRay created this revision.Jan 22 2020, 11:38 PM

Unit tests: pass. 62087 tests passed, 0 failed and 784 were skipped.

clang-tidy: pass.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

jhenderson added inline comments.Jan 23 2020, 1:37 AM
lld/test/ELF/ppc64-reloc-copy.s
1 ↗(On Diff #239785)

Does it make sense to fold this and the other test together? They're practically identical...

llvm/include/llvm/BinaryFormat/ELFRelocs/PowerPC64.def
30

I think you need an undef here, right?

MaskRay updated this revision to Diff 239923.Jan 23 2020, 9:18 AM
MaskRay marked 3 inline comments as done.

Address review comments

For anyone who is curious about this, copy relocations are especially needed on powerpc32, where they are used to implement global data (such as sharing a pointer between a .so and the main program) in situations where the main program's copy lives in .bss.

Given that FreeBSD libc's API uses this extensively for things such as providing stderr in stdio, this fix is critical on ppc32, as the lack of it prevents anything more complex than /bin/echo from running without crashing due to dereferencing a null pointer.

I am currently running though a powerpc32 buildworld and testing on a G4 so I can see just how much closer that gets us to switching FreeBSD powerpc32 to lld.

On PPC64, the need for copy relocations is much reduced, as generally things go through the TOC in the first place (when using PIC), so it is sufficient to generate an R_PPC64_ADDR64 relocation against the TOC entry, instead of a copy relocation against an offset into .bss.

Unit tests: fail. 62137 tests passed, 5 failed and 811 were skipped.

failed: libc++.std/language_support/cmp/cmp_partialord/partialord.pass.cpp
failed: libc++.std/language_support/cmp/cmp_strongeq/cmp.strongeq.pass.cpp
failed: libc++.std/language_support/cmp/cmp_strongord/strongord.pass.cpp
failed: libc++.std/language_support/cmp/cmp_weakeq/cmp.weakeq.pass.cpp
failed: libc++.std/language_support/cmp/cmp_weakord/weakord.pass.cpp

clang-tidy: pass.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

Aside from a minor nit I have no more comments, looks fine to me.

lld/test/ELF/ppc-reloc-copy.s
10

It was a bit strange to see t1 again.
I'd suggest to rename t1.o, t1.so, t.o and t to something else.
Perhaps use ppc/ppc64 suffixes somewhere?

MaskRay updated this revision to Diff 240105.Jan 23 2020, 11:49 PM

%t1 -> %t1.32 / %t1.64

MaskRay marked an inline comment as done.Jan 23 2020, 11:49 PM
MaskRay added inline comments.
lld/test/ELF/ppc64-reloc-copy.s
1 ↗(On Diff #239785)

Agree.

Placed in ppc-reloc-copy.s

Unit tests: fail. 62153 tests passed, 5 failed and 811 were skipped.

failed: libc++.std/language_support/cmp/cmp_partialord/partialord.pass.cpp
failed: libc++.std/language_support/cmp/cmp_strongeq/cmp.strongeq.pass.cpp
failed: libc++.std/language_support/cmp/cmp_strongord/strongord.pass.cpp
failed: libc++.std/language_support/cmp/cmp_weakeq/cmp.weakeq.pass.cpp
failed: libc++.std/language_support/cmp/cmp_weakord/weakord.pass.cpp

clang-tidy: fail. clang-tidy found 0 errors and 1 warnings. 0 of them are added as review comments below (why?).

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

jhenderson accepted this revision.Jan 24 2020, 1:40 AM

LGTM, but as I'm a bit rusty on some parts of LLD, and know very little about PPC, I can't be particularly confident about the LLD-aspects of this change, so it's probably worth somebody else confirming.

This revision is now accepted and ready to land.Jan 24 2020, 1:40 AM
grimar accepted this revision.Jan 24 2020, 1:44 AM

LGTM. I know nothing about PPC. From LLD code POV this looks fine.

sfertile accepted this revision as: sfertile.Jan 24 2020, 7:15 AM
sfertile added a subscriber: sfertile.

LGTM.

Bdragon28 accepted this revision.Jan 24 2020, 8:26 AM

While I ran into further problems on ppc32, I think it's just because there are further problems on ppc32 (unrelated to this one). At the very least this gets FreeBSD ppc32/lld10 as far as the rescue shell, which is great progress!

This revision was automatically updated to reflect the committed changes.

Thanks to everyone for review!

Cherry picked the commit to release/10.x as f1dab29908d25a4044abff6ffc120c48b20f034d