Page MenuHomePhabricator

Please use GitHub pull requests for new patches. Phabricator shutdown timeline

[PPC64] Remove support for ELF V1 ABI in LLD

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

Diff Detail

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
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
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
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
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.

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
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
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.
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.

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.

59–63 ↗(On Diff #145083)


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.