Page MenuHomePhabricator

Find PLT entries for x86, x86_64, and AArch64
ClosedPublic

Authored by jgalenson on Aug 2 2018, 3:40 PM.

Details

Summary

This adds a new method to ELFObjectFileBase that returns the symbols and addresses of PLT entries.

This design was suggested by pcc and eugenis in https://reviews.llvm.org/D49383.

Diff Detail

Repository
rL LLVM

Event Timeline

jgalenson created this revision.Aug 2 2018, 3:40 PM

I'm not at all familiar with ELF or parsing x86 and ARM, so I'd appreciate feedback on how this works and if there's a way to do it more cleanly (especially the parsing).

I'm also not sure how to test this. The two child patches both use the new APIs and have tests for themselves, so perhaps that's good enough?

paulsemel added inline comments.
lib/Object/ELFObjectFile.cpp
357–360 ↗(On Diff #158845)

As explained here, this is in my opinion not the right way to get the plt/got etc..

jgalenson added inline comments.Aug 8 2018, 8:35 AM
lib/Object/ELFObjectFile.cpp
357–360 ↗(On Diff #158845)

It seems like we decided over there to keep it this way.

Friendly ping.

pcc added inline comments.Aug 13 2018, 5:29 PM
lib/Object/ELFObjectFile.cpp
340 ↗(On Diff #158845)

Maybe this should be named JumpSlotReloc?

lib/Target/AArch64/MCTargetDesc/AArch64MCTargetDesc.cpp
165 ↗(On Diff #158845)

You could use the support::endian::read32le function here (and elsewhere in the patch).

170 ↗(On Diff #158845)

To avoid excessive indentation I'd write this with an early continue.

if (Insn >> 31 != 1 || ((Insn >> 24) & 0x1f) != 0x10)
  continue;
uint64_t Imm = ...;

I might also simplify the condition like this:

if ((Insn & 0x9f000000) != 0x90000000)
  continue;
176 ↗(On Diff #158845)

I'd write out the instruction: ldr Xt, [Xn, #pimm] so that it's clear which ldr variant you're matching for.

lib/Target/X86/MCTargetDesc/X86MCTargetDesc.cpp
449 ↗(On Diff #158845)

I wouldn't try to match the push here (and in the 64-bit version) since it's not essential for decoding the plt entry. It's also theoretically possible for it to be missing, e.g. a linker would be able to avoid emitting it when passed -z now.

450 ↗(On Diff #158845)

I think you also need to be able to decode the 0xff 0x25 form here, it's used in non-PIC executables.

jgalenson updated this revision to Diff 160597.Aug 14 2018, 8:30 AM
jgalenson marked 4 inline comments as done.
jgalenson added inline comments.
lib/Target/AArch64/MCTargetDesc/AArch64MCTargetDesc.cpp
165 ↗(On Diff #158845)

Ooh, I didn't know about that. Thanks!

170 ↗(On Diff #158845)

Good ideas. Thanks.

lib/Target/X86/MCTargetDesc/X86MCTargetDesc.cpp
449 ↗(On Diff #158845)

I didn't know that. Thanks.

450 ↗(On Diff #158845)

I extended the condition to handle that case. But does anything else change with that form, or just these bits of the opcode?

pcc added inline comments.Aug 14 2018, 2:24 PM
lib/Object/ELFObjectFile.cpp
357–368 ↗(On Diff #160597)

I don't think these find_if calls will do what you expect if the section is not found. In that case find_if will return the end iterator, and dereferencing it is UB.

I'd rewrite this as a single loop over sections().

lib/Target/AArch64/MCTargetDesc/AArch64MCTargetDesc.cpp
171 ↗(On Diff #160597)

Move the declaration of Imm onto this line.

lib/Target/X86/MCTargetDesc/X86MCTargetDesc.cpp
450 ↗(On Diff #158845)

0xff 0x25 is similar to the equivalent 64-bit instruction, except that in 32-bit mode the immediate is not PC-relative. So I think you will need a separate block of code to handle it.

jgalenson updated this revision to Diff 160832.Aug 15 2018, 9:35 AM
jgalenson marked 3 inline comments as done.
pcc accepted this revision.Aug 16 2018, 1:27 PM

LGTM

This revision is now accepted and ready to land.Aug 16 2018, 1:27 PM
This revision was automatically updated to reflect the committed changes.