This is an archive of the discontinued LLVM Phabricator instance.

[PPC64] Remove support for ELF V1 ABI in LLD
ClosedPublic

Authored by syzaara on May 1 2018, 8:01 AM.

Diff Detail

Repository
rLLD LLVM Linker

Event Timeline

syzaara created this revision.May 1 2018, 8:01 AM
ruiu added inline comments.May 1 2018, 8:50 AM
lld/ELF/Arch/PPC64.cpp
93–94 ↗(On Diff #144719)

Please update this as well for LE only.

101–105 ↗(On Diff #144719)

V2 is LE only, no?

137 ↗(On Diff #144719)

It is perhaps better to just do ABI version V" + Twine(Flag) + " is not compatible ... than defining getABIName function.

syzaara added inline comments.May 1 2018, 8:54 AM
lld/ELF/Arch/PPC64.cpp
101–105 ↗(On Diff #144719)

No, there is no restriction that V2 is LE only.

syzaara added inline comments.May 1 2018, 8:56 AM
lld/ELF/Arch/PPC64.cpp
93–94 ↗(On Diff #144719)

We need to keep both LE and BE.

syzaara updated this revision to Diff 144914.May 2 2018, 12:31 PM
ruiu added inline comments.May 3 2018, 10:03 AM
lld/ELF/Arch/PPC64.cpp
101–102 ↗(On Diff #144914)

I don't think you need this assert.

103 ↗(On Diff #144914)

Instead of Config->EKind == ELF64LEKind, please use Config->IsLE.

106 ↗(On Diff #144914)

Perhaps EFlags > 2 is a bit better.

93–94 ↗(On Diff #144719)

Oh okay. I thought PPCv2 ABI is LE only. I stand corrected. I'm curious if compiler tool chain actually supports BE PPCv2 ABI.

lld/ELF/InputSection.cpp
559–560 ↗(On Diff #144914)

You return here only for PPC64. Is this correct? We generally should avoid this because it is sometimes hard to find that only one branch of cascaded if returns immediately. If this is really correct, I'd add

if (Sym.isUndefWeak() && Config->EMachine == EM_PPC64)
  return 0;

but I'm not sure if that's correct.

syzaara added inline comments.May 3 2018, 10:46 AM
lld/ELF/InputSection.cpp
559–560 ↗(On Diff #144914)

This was previously handled with R_PPC_OPD which returned 0 for an undefined weak symbol by checking if the symbol address is zero. Now I have removed R_PPC_OPD and use this relocation, keeping the same return 0 behavior as before.

ruiu added inline comments.May 3 2018, 10:48 AM
lld/ELF/InputSection.cpp
559–560 ↗(On Diff #144914)

Doesn't it conflict with the existing behavior for R_PC? You are changing the default behavior of R_PC. If returning 0 makes sense only for R_PPC_OPD, I think you need to keep it.

syzaara updated this revision to Diff 145052.May 3 2018, 10:57 AM
syzaara updated this revision to Diff 145083.May 3 2018, 1:22 PM
syzaara added inline comments.
lld/ELF/InputSection.cpp
559–560 ↗(On Diff #144914)

Keeping it, but changing name to not include "OPD" which is part of V1 abi

ruiu accepted this revision.May 3 2018, 1:33 PM

LGTM with these changes.

lld/ELF/Arch/PPC64.cpp
105 ↗(On Diff #145083)

Perhaps it is better to return 0 for an invalid version number to prevent cascading error messages. Currently, if you return, say 3, you'll see something like "ABI version 3 is not compatible with ..." error after this error message.

lld/ELF/Relocations.h
59–63 ↗(On Diff #145083)

Sort

This revision is now accepted and ready to land.May 3 2018, 1:33 PM
This revision was automatically updated to reflect the committed changes.
test/ELF/ppc64le-toc-rel.s