Page MenuHomePhabricator

Implementation of PLT: handling of IFUNC calls (gnu_indirect_function).
ClosedPublic

Authored by lenykholodov on Feb 23 2015, 9:01 AM.

Details

Summary

This diff includes implementation of linking calls to ifunc functions. It provides ifunc entries in PLT and corresponding relocations (R_ARM_ALU_PC_G0_NC, R_ARM_ALU_PC_G1_NC, R_ARM_LDR_PC_G2 for link-time and R_ARM_IRELATIVE for run-time).

Diff Detail

Repository
rL LLVM

Event Timeline

lenykholodov retitled this revision from to Implementation of PLT: handling of IFUNC calls (gnu_indirect_function)..
lenykholodov updated this object.
lenykholodov edited the test plan for this revision. (Show Details)
lenykholodov added reviewers: Bigcheese, lld.
lenykholodov set the repository for this revision to rL LLVM.
lenykholodov added a project: lld.
lenykholodov added a subscriber: Unknown Object (MLST).Feb 23 2015, 9:02 AM

Can you add testcases for the other relocations that you have in the code ?

Bigcheese requested changes to this revision.Feb 24 2015, 4:58 PM
Bigcheese edited edge metadata.

Looks good, but needs tests for the other relocations too.

This revision now requires changes to proceed.Feb 24 2015, 4:58 PM
lenykholodov updated this revision to Diff 21001.EditedMar 2 2015, 6:06 AM
lenykholodov edited the test plan for this revision. (Show Details)
lenykholodov edited edge metadata.

Tests for relocations R_ARM_ALU_PC_G0_NC, R_ARM_ALU_PC_G1_NC, R_ARM_LDR_PC_G2 have been added. All of them were created in manual mode by editing YAML files. Unfortunately, I couldn't find the way how I could generate these relocations with gcc/clang compilation.

Bigcheese accepted this revision.Mar 2 2015, 9:59 AM
Bigcheese edited edge metadata.

lgtm

This revision is now accepted and ready to land.Mar 2 2015, 9:59 AM

The idea of group relocations is that they allow splitting an address up among couple of instructions without making access to the memory with load instruction. It's the optimization, that won't work for offsets more than 2^28 bits in the way it's used (maybe we should add comments into TODO section about that?)

So the three separated tests for group relocations don't really test anything. They should be tested as a group in one test case:

  • you point all three relocs at the same function address
  • you then show offsets in comments to the content of .rel.plt section as it's done for other tests. Example:
# CHECK: Contents of section .text:                                                                                                                                         
# CHECK: 400084 80b500af fff7f4bf 03461846 80bd00bf                                                                                                                         
#                           ^^ offset = -0x18
  • you then show that summing up these offsets gives the address of the function you're pointing at (again in comments):
#               call site  offset  PC(thm) _Z1fv addr                                                                                                                       
#               0x400088 + (-0x18) + 0x4 = 0x400074

I think in this case it's at least understandable what's happening. And the current edition of these three tests just shows that something is generated by lld, but there's no way to check it.

lib/ReaderWriter/ELF/ARM/ARMLinkingContext.h
35 ↗(On Diff #21001)

We don't handle dynamic linking yet. Moreover, we don't use any of the relocations in the switch block, so I doubt if this code is needed. You shouldn't add the code that you cannot state is working. Please, remove this function.

66 ↗(On Diff #21001)

The same as for isDynamicRelocation - may be removed.

lib/ReaderWriter/ELF/ARM/ARMRelocationHandler.cpp
312 ↗(On Diff #21001)

This is better to be implemented as a template function taking count of bits in lshift as a template parameter.
Also, I'd rework this function to:

  • calculate shifted result value
  • do what current function does
  • write the reloc with applyArmReloc

So the prototype would be:

template <uint32_t lshift>
static void reloc_R_ARM_ALU_PC_GN_NC(uint8_t *location, uint32_t result);

The calling function should calculate the result by the formula, dump values in the log, and call this function in the end.

348 ↗(On Diff #21001)

There's no T bit in the documentation, so please, update the comment here.

351 ↗(On Diff #21001)

For all these three group relocations:
the offset you receive when making calculations, it may be negative.
The reference ("Group relocations", p.32) says how you should handle this case.
Since you don't handle it, you need to put a check and fire llvm_unreachable in that case, so we may later implement proper handling if needed.

425 ↗(On Diff #21001)

For these, you don't handle initial addend calculation as for other relocation types, so it's inconsistent.

lib/ReaderWriter/ELF/ARM/ARMRelocationPass.cpp
169 ↗(On Diff #21001)

There are more reloc types that can target IFUNC entries. Imagine that someone takes the address of memcpy, so it goes through the ldr instruction (using R_ARM_ABS32 reloc, for example). Other platforms handle most types of relocations with this function just not to miss anything.

250 ↗(On Diff #21001)

You shouldn't pollute the symbol table with this sort of needless symbols. Just put

#ifndef NDEBUG
#endif

guard around.

test/elf/ARM/rel-ifunc.test
7 ↗(On Diff #21001)

I'd really want to see the explanation of where these three instructions in .plt section point at. I think they should point at the same .got entry where value from .rel.plt section points.

10 ↗(On Diff #21001)

You should add .rel.plt section output, and it should point at the .got.plt symbol you specified below.

12 ↗(On Diff #21001)

You'd better show where this address ad004000 is pointing at. This should be somewhere in the .text section defining a function to be substituted (you may add it to the symbol table output for simplicity).

lenykholodov added a reviewer: denis-protivensky.
  • unused methods ARMLinkingContext::isDynamicRelocation and ARMLinkingContext::isRelativeReloc have been removed;
  • imlementations of relocations R_ARM_ALU_PC_G0_NC & R_ARM_ALU_PC_G1_NC have been generalized (see relocR_ARM_ALU_PC_GN_NC function);
  • initial addends have been ignored because they are read during the relocation applying, for group relocation reference.addend() have been used;
  • for negative offsets in group relocations llvm_unreachable has been inserted (full implementation will be done in further patch);
  • additional relocs have been processed with ifunc;
  • global vars scope was changed to static;
  • separate tests for relocations were removed (because of useless);
  • test for ifunc has been changed according to the Denis's comment;
  • comments were fixed.
denis-protivensky requested changes to this revision.Mar 20 2015, 2:07 AM
denis-protivensky edited edge metadata.

You removed tests for separate group relocations, but didn't add one which tests all the three at a time. Can you please do that?

lib/ReaderWriter/ELF/ARM/ARMRelocationHandler.cpp
384 ↗(On Diff #22317)

That's better to be made static_assert

386 ↗(On Diff #22317)

static doesn't make sense. Statics use locks in C++11, so why introduce unneeded internal complexity? If you want to have compile-time variable here, use enum { rshift = 32 - lshift }; but I think that regular variable is enough.

387 ↗(On Diff #22317)

Remove this empty line.

396 ↗(On Diff #22317)

The cast type is wrong. Should be int32_t result = (int32_t)((S + A) - P);

414 ↗(On Diff #22317)

The cast is wrong as above.

432 ↗(On Diff #22317)

The cast is wrong as above.

438 ↗(On Diff #22317)

The result is converted to unsigned while making & operation (because mask is unsigned) then back to signed when assigned. You're also modifying the result value before logging. So I'd propose to move this operation into applyArmReloc:
applyArmReloc(location, (uint32_t)result & mask, mask);

lib/ReaderWriter/ELF/ARM/ARMRelocationPass.cpp
50 ↗(On Diff #22317)

I think this code is already in. You should rebase to the actual sources.

122 ↗(On Diff #22317)

This code is also in the trunk already.

174 ↗(On Diff #22317)

Why don't you handle these two relocs on a subject of IFUNC?

282 ↗(On Diff #22317)

We came to using dyn_cast and assuming that ref.target() cannot be null.

This revision now requires changes to proceed.Mar 20 2015, 2:07 AM
lenykholodov edited edge metadata.
  • small fixes for group relocations (static_assert, using of static specifier, formatting, types conversion);
  • ifunc handling for all available relocations;
  • rebasing with master;
  • new complex test has been added for three group relocs (R_ARM_ALU_PC_G0_NC, R_ARM_ALU_PC_G1_NC, R_ARM_LDR_PC_G2).
denis-protivensky edited edge metadata.

LGTM

This revision is now accepted and ready to land.Mar 24 2015, 10:41 PM

I have no commit permission. I would really appreciate if someone could send this patch on my behalf.

lenykholodov edited edge metadata.
  • generation of veneers for non jump instructions has been disabled;
  • tests have been fixes;
  • missing break has been added to the relocations processing switch.
This revision was automatically updated to reflect the committed changes.