This is an archive of the discontinued LLVM Phabricator instance.

[ELF2] Add a base set of PPC64 relocations
ClosedPublic

Authored by hfinkel on Oct 8 2015, 10:05 AM.

Details

Reviewers
ruiu
rafael
Summary

This is mostly an adaptation of the code in LLVM's lib/ExecutionEngine/RuntimeDyld/RuntimeDyldELF.cpp, and handles a sufficient number of relocations to link a 'hello world' program on big-Endian PPC64/Linux (ELF v1 ABI) (and said program will even run when a few later patches are also applied).

This is obviously still missing test cases; I'm working on those. Other feedback, and suggestions on which tests to emulate, is certainly appreciated.

Diff Detail

Event Timeline

hfinkel updated this revision to Diff 36870.Oct 8 2015, 10:05 AM
hfinkel retitled this revision from to [ELF2] Add a base set of PPC64 relocations.
hfinkel updated this object.
hfinkel added reviewers: ruiu, rafael.
hfinkel added a project: lld.
hfinkel added a subscriber: llvm-commits.
ruiu added inline comments.Oct 8 2015, 10:38 AM
ELF/Target.cpp
206

I'm not entirely sure, but the capitalization rule is usually applyPPCLo (not applyPPClo), no?

206–230

No need for inline. Compilers should take care of them.

208

Arguments must start with uppercase, so Value. But I prefer just V, so that we can write these functions one line.

212

No space before '('

252–253

I wouldn't write zeros beforehand. Instead, you can add as many "0, 0 , 0, ..." as you want at end of Insts to fill the entire space with zeros.

258

Remove this blank line.

267–281

I wouldn't construct Insts. Instead I'd do

write32be(Buf, 0xf810000u); // std %r2 ...
write32be(Buf + 4, 0x3d62000u | apply...)
write32be(Buf + 8, ...
287–293
if (Type != R_PPC64_REL24)
  return false;
return S.isShared() || (S....
294–295

BaseAddr -> Base

298

Rename Addend -> A, just like the ELF spec.

298–299

Offset -> Off

301–321

This switch statement enclosed with if-else-if looks a bit weird. Can you remove outer if and just use switch instead?

318–321

You want to move this piece of code just after "uint8_t *Loc = Buf + Offset;" to return early.

332

This style is not in line with others. Please don't put break after }

358

If you can trigger this assertion by giving broken object files, this shouldn't be an assert() but an error().

360

All variable must starts with uppercase.

393–397

Generally we have lots of very similar expressions to calculate relocation values, we want to write them down as simple as possible. So I don't want to use descriptive names for each temporary variables. Please use one-letter variables or don't use variables at all. For example, I'd write this as

int32_t V = int32(SymVA - Base + Off + A);
if (SignExtend32<32>(V) != V)
  error(...)
write32be(Loc, V);

(By the way, the above error check seems odd. It would never fail, no?)

400–402
write64be(Loc, SymVA - BaseAddr - Offset + Addend);
hfinkel marked 14 inline comments as done.Oct 9 2015, 11:28 PM
hfinkel added inline comments.
ELF/Target.cpp
206

Done.

206–230

Done.

208

Done.

212

Done.

252–253

Done.

258

Done.

267–281

Done.

287–293

Done.

294–295

BaseAddr is consistent with the names used by the other targets (we can certainly rename them all as a separate change).

298

Done.

298–299

Done.

301–321

It is done this way to share the addend-adjustment logic. Early-return: done.

332

Done.

358

Done.

360

Done.

393–397

Done.

You're right (I copied these checks from RuntimeDyldELF.cpp, and I should fix them up there as well).

400–402

I've changed everything to use similar short variable names to those that AArch64 uses.

ruiu edited edge metadata.Oct 10 2015, 8:55 AM

Can you upload the new patch? Thanks!

hfinkel marked 14 inline comments as done.Oct 10 2015, 8:59 AM
In D13562#264243, @ruiu wrote:

Can you upload the new patch? Thanks!

Yes; I'm working on some regression tests now.

hfinkel updated this revision to Diff 37029.Oct 10 2015, 10:10 AM
hfinkel edited edge metadata.

Updated to reflect reviewer comments and add a regression test.

ruiu added a comment.Oct 10 2015, 10:46 AM

Can you upload the new patch? Thanks!

ELF/Target.cpp
232

Add a blank line before this line.

232

This class does not use any members of the class, so let's make it a static non-member function.

250

Blank line.

260

Drop 'u' suffix.

263–264

Can this fit in one line if you use a shorter name for EntryOffset?

270

Blank line.

286

Blank line.

316–321

Write like this to remove the outer if.

case R_PPC64_TOC16: Type = R_PPC64_ADDR16; A -= getTocBase(); break;
case ...
319

(R) & ~3 -> R & ~3

321

You don't need {} here. {} is used in switch-case to create a new scope for local variables, but you didn't define any in this {}.

328

Ditto

349

Remove {}

352

*(L + 3) -> L[3]

353

Ditto

357

Ditto

363

Ditto

376

Ditto

380

Ditto

hfinkel updated this revision to Diff 37067.Oct 11 2015, 3:44 PM

Addressed review comments.

ruiu accepted this revision.Oct 11 2015, 3:57 PM
ruiu edited edge metadata.

LGTM with nits. Thank you for your work for PPC64!

ELF/Target.cpp
218

You can remove &0xffff because least significant 48 bits are already 0.

221

Ditto

This revision is now accepted and ready to land.Oct 11 2015, 3:57 PM
hfinkel closed this revision.Oct 12 2015, 1:58 PM

r250101, thanks!