This is an archive of the discontinued LLVM Phabricator instance.

[RFC][ARM][LLD] Initial support for ARM in lld
ClosedPublic

Authored by peter.smith on Jun 3 2016, 2:19 AM.

Details

Reviewers
ruiu
rafael
Summary

Initial support for ARM in lld.

Add support for an ARM Target and the initial set of relocations and PLT entries that are necessary for an ARM only hello world to link. This has been tested against an ARM only sysroot from the 4.2.0 CodeSourcery Lite release.

Tests have been added to test/ELF for the support that has been implemented.

Main limitations:

  • No Thumb support
  • Relocations incomplete
  • No C++ exceptions support
  • No TLS support
  • No range extension or interworking veneer (thunk) support
  • No Build Attribute support
  • No Big-endian support

The deprecated relocations R_ARM_PLT32 and R_ARM_PC24 have been implemented as these are used by the 4.2.0 CodeSourcery Lite release.

For ARM I think the _GLOBAL_OFFSET_TABLE_ symbol should be defined to be the base of the .got section. There is existing code that does a R_ARM_BASE_PREL relocation with respect to _GLOBAL_OFFSET_TABLE_ to form the base of the .got and then add an offset to obtain the entry address.

Diff Detail

Event Timeline

peter.smith updated this revision to Diff 59504.Jun 3 2016, 2:19 AM
peter.smith retitled this revision from to [RFC][ARM][LLD] Initial support for ARM in lld.
peter.smith updated this object.
peter.smith added reviewers: rafael, ruiu.
ruiu edited edge metadata.Jun 3 2016, 1:38 PM

This seems pretty straightforward code to support ARM. I'm happy to hear that you can link a "hello world" for ARM with only this amount of code. You want to have Rafael take a look at it as well -- particularly for relocation handling.

ELF/Driver.cpp
72–76

I want to sort these S== lines alphabetically, but it should be done in a separate patch.

ELF/Target.cpp
1467

Remove the outermost redundant ().

1506

You are not using L1 label in the comment. What is it?

1512

So Dot always points to PLT+16. Is this correct?

1537

I think you can remove this comment -- it is obvious from the relocation name.

1556–1558

Is this what clang-format would format?

1568

clang-format

ELF/Writer.cpp
673 ↗(On Diff #59504)

Format.

674–679 ↗(On Diff #59504)

This seems a bit tricky since we are giving a different meaning to _GLOBAL_OFFSET_TABLE_ only if ARM. I think I understand the issue you are trying to solve. Does one of your tests cover this case (I mean, is there any test that will fail if you remove this code?)

Thank you very much for the comments. I'll post an update on Monday.

peter.smith marked an inline comment as done.Jun 6 2016, 4:08 AM

I've put some answers to the comments inline. I'm working on an updated patch that I'll have ready later today.

ELF/Driver.cpp
72–76

Ok, I'll leave as is for this patch.

ELF/Target.cpp
1467

Ok, will do.

1506

Thinking again, I should have a done a much better job with the comment. What I need to express is that the .word contains the the offset from L1 to the start of the .got.plt section, taking into account the ARM PC bias of +8. In a follow up patch I've come up with a better way of expressing this that is closer to the example given in ELF for the ARM architecture and the code for writePLT:

str lr, [sp,#-4]!
ldr lr, L2

L1: add lr, pc, lr

ldr pc, [lr, #8]

L2: .word &(.got.plt) - L1 - 8

Where &(.got.plt) = L1 + 8 + *L2 hence *L2 = &(.got.plt) - L1 - 8, and L1 = .plt + 8

1512

The 16 comes from combining the offset of 8 of L1 from the start of the .plt section and the 8 pc bias. I've made this is a bit clearer in the revised version.

1537

Ok, comment removed.

1556–1558

I've run clang format and will update the changes so that they conform.

1568

I've run clang format and will update the changes so that they conform.

ELF/Writer.cpp
673 ↗(On Diff #59504)

Will remove, see later comment.

674–679 ↗(On Diff #59504)

This is indeed tricky. I've re-read the relevant sections of ELF for the ARM Architecture which give enough freedom to a linker to special case the R_ARM_BASE_PREL relocation (see later). I've rewrote the entry in getRelExpr to use R_GOTONLY_PC rather than R_PC. This effectively ignores the value of the _GLOBAL_OFFSET_TABLE_ symbol. This is not ideal but it is ok for the initial target of a linux/bsd like system.

By the principle of least surprise I would prefer to define _GLOBAL_OFFSET_TABLE_ to the base of the .got, as is the case in ld.bfd, but I think that is best handled in a separate patch. I will try to find a real world program that depends on _GLOBAL_OFFSET_TABLE_ being defined to be the base of the .got outside the context of R_ARM_BASE_PREL.

  • ELF for the ARM Architecture defines R_ARM_BASE_PREL to be: B(S) + A – P with the special case:

R_ARM_BASE_PREL with the NULL symbol (symbol 0) will give the offset of the GOT origin from the address
of the place.
B(S) means B(S) is the addressing origin of the output segment defining the symbol S. The origin is not required to be the base address of the segment. This value must always be word-aligned.

If I choose B(S) to be the base of the .got section then the relocation works regardless of the value of _GLOBAL_OFFSET_TABLE_.

peter.smith updated this revision to Diff 59704.Jun 6 2016, 5:39 AM
peter.smith edited edge metadata.
peter.smith marked an inline comment as done.

I've updated the patch with:

  • Small corrections applied
  • Made PLT comments and code clearer and closer to the example code in ELF for the ARM Architecture
  • Clang format run over patch
  • R_ARM_BASE_PREL now maps to R_GOTONLY_PC.
  • Removed the setting of _GLOBAL_OFFSET_TABLE_ to the base of the .got section for ARM only.
ruiu added inline comments.Jun 6 2016, 9:07 AM
ELF/Target.cpp
1466

I doubt this is correct. I'd think you want to return just R_PLT_PC and let adjustExpr in Relocations.cpp to relax it later.

1538–1542

Sort

1549–1552

Sort

1575–1579

Sort

1583

Do you have to clear the most significant bit? I think SignExtend64<31> will do that for you.

1596

Use uint64_t instead of int64_t to avoid confusion on right shift.

peter.smith updated this revision to Diff 59741.Jun 6 2016, 9:49 AM

Changes made in light of review comments:

  • always use R_PLT_PC.
  • Rely on SignExtend<31> to clear the top bit for R_ARM_PREL31.
  • Use uint64_t rather than int64_t.
  • sorted in alphabetical order groups of common relocation types in switch statements.
ruiu accepted this revision.Jun 6 2016, 9:56 AM
ruiu edited edge metadata.

LGTM with a few nits.

ELF/Target.cpp
1462–1465

Sort

1585–1586

You can merge this with R_ARM_ABS32 case.

This revision is now accepted and ready to land.Jun 6 2016, 9:56 AM

Thanks for the review, I've taken care of the most recent comments and committed under r271993 + r271994 (missing Requires: arm on one of the tests).

peter.smith closed this revision.Jun 10 2016, 4:49 AM

Apologies I should have closed this review earlier as this was committed under r271993 + r271994 earlier in the week.