This is an archive of the discontinued LLVM Phabricator instance.

[ELF]: Initial implementation for ARM static linking
AbandonedPublic

Authored by denis-protivensky on Nov 28 2014, 4:30 AM.

Details

Summary

The skeleton code for ELF binary generation for ARM platform.
The code is "almost" able to statically link the simplest case of void main() { return 0; } sample.

Added all needed relocations, GOT and TLS handling.
Some of these may still be improper which causes segfaults.

Diff Detail

Event Timeline

denis-protivensky retitled this revision from to [ELF]: Initial implementation for ARM static linking.
denis-protivensky updated this object.
denis-protivensky edited the test plan for this revision. (Show Details)
denis-protivensky added a project: lld.
denis-protivensky added a subscriber: Unknown Object (MLST).

Provided the diff with extended context.

shankarke requested changes to this revision.Nov 28 2014, 6:53 PM
shankarke added a reviewer: shankarke.
shankarke added a subscriber: shankarke.
shankarke added inline comments.
lib/ReaderWriter/ELF/ARM/ARMELFFile.h
2

80 columns.

104–109

All of the above would need to be done in the relocation handler. We should not interpret the bits in the Reader, this way the Reader is agnostic to the symbol value.

lib/ReaderWriter/ELF/ARM/ARMExecutableWriter.h
37

This should be done only for dynamic executables.

74

same here, only for dynamic executables.

lib/ReaderWriter/ELF/ARM/ARMLinkingContext.h
46

Why is this a dynamic relocation ?

lib/ReaderWriter/ELF/ARM/ARMRelocationPass.cpp
159–162

You may need to manually indent.

lib/ReaderWriter/ELF/ARM/TODO.rst
9

you should also add ARM interoperability.

This revision now requires changes to proceed.Nov 28 2014, 6:53 PM
atanasyan edited edge metadata.Nov 28 2014, 11:34 PM

Just curious, does the only test case cover the most part of new functionality?

lib/ReaderWriter/ELF/ARM/ARMELFFile.h
26

Strange indentation here. I recommend you to run the clang-format other all new files.

104–109

But the relocation handler does not know about used relocation section format (.rel/.rela) so it cannot decide how and where to read an addendum. So I think it is a good approach to read addendums here, construct Reference objects, and call the Reference::addend() method further.

As Simon mentioned, This change needs more tests as it includes changes to handle dynamic linking too.

lib/ReaderWriter/ELF/ARM/ARMELFFile.h
104–109

I have two concerns (a), (b) :-

a) Doing this in the Reader may not be idempotent (Need to consider cases like partial linking and ELF to yam conversions and back to atoms).
b) We may be losing some data because of the transformation in the Reader too, to preserve what was in the object file.

ELFLinkingContext::isRelaOutputFormat() can be used to figure out if the Target is using Rela/Rel. In addition the Target knows the relocation types that it handles.

atanasyan added inline comments.Nov 30 2014, 5:41 AM
lib/ReaderWriter/ELF/ARM/ARMELFFile.h
104–109

In general, I agree with you. As far as I can see ARM relocation addendums can be read in the ARMRelocationHandler class.

MIPS has some unusual relocations like R_MIPS_HI16 / R_MIPS_LO16. These relocations form pairs and to apply each of such relocation we need to know addendums from both paired relocations. The bad thing is that the paired relocations can belong to different atoms. So it is difficult to search a paired relocation in the applyRelocation method. That is why I have to read addendums for MIPS target in the MipsELFFile class. I think Denis’ implementation is inspired by this code.

By the way, if anybody knows a better solution I would be happy to hear it.

In D6446#13, @atanasyan wrote:

Just curious, does the only test case cover the most part of new functionality?

The code is not ready yet, it doesn't even link statically, so I'm not sure if adding test cases won't result in a needless work.

In D6446#14, @shankarke wrote:

As Simon mentioned, This change needs more tests as it includes changes to handle dynamic linking too.

I didn't plan to add support for dynamic linking right now, I'm still working on the static linking.

lib/ReaderWriter/ELF/ARM/ARMELFFile.h
26

Thanks, didn't know about that tool. It revealed couple more formatting misses.

104–109

I used MIPS implementation as a reference, while I initially planned to put addend calculation to the relocation handler. Okay, I'll move the code to the relocation handler.

Here's just a little thing that I don't like in this solution, but it's not critical of course:
some relocations will have Reference::addend field filled, because they are processed and formed in the relocation pass. And other relocations will have Reference::addend empty, and this value will be calculated in-palce. I'm not sure if Reference's addends are used somewhere before the relocation handler, but for me it was more consistent when they all were filled before and came to applyRelocation already formed.

By the way, if anybody knows a better solution I would be happy to hear it.
If there are limitations when putting addend calculation to ELFFile, isn't it better to add a separate pass to process these initial addends (and maybe cross-references between them as for MIPS) for example, after relocation pass?
While it requires more work and additional instance, it looks cleaner, and also removes that little miss I described above for the relocation handler.

lib/ReaderWriter/ELF/ARM/ARMExecutableWriter.h
37

This is needed for handling STT_GNU_IFUNC. While this handing is not here, I've already implemented it and want to send as a separate diff.

lib/ReaderWriter/ELF/ARM/ARMLinkingContext.h
46

Because these all marked as dynamic in the ARM ELF reference.

lib/ReaderWriter/ELF/ARM/ARMRelocationPass.cpp
159–162

Will do.

lib/ReaderWriter/ELF/ARM/TODO.rst
9

What does that mean? Can you please explain so I understand what I'm adding?

denis-protivensky edited edge metadata.

Updated:

  • fixed indentations & formatting
  • initial addend calculation is moved to the relocation handler
shankarke requested changes to this revision.Dec 1 2014, 9:21 AM
shankarke edited edge metadata.
shankarke added inline comments.
lib/ReaderWriter/ELF/ARM/ARMExecutableWriter.h
34–35

I couldnt find any place where this gets fixed up.

lib/ReaderWriter/ELF/ARM/ARMLinkingContext.h
33–34

Is the base address correct for ARM ?

lib/ReaderWriter/ELF/ARM/ARMRelocationHandler.cpp
421–422

llvm::report_fatal_error (or) provide a reason with the assert.

lib/ReaderWriter/ELF/ARM/ARMRelocationPass.cpp
146–148

indentation is not right.

224

I dont see any initialization to _PLT0. If you are not supporting dynamic linking with this patch, Please remove dynamic linking code from this patch as its difficult to follow.

242

Why is this empty ?

This revision now requires changes to proceed.Dec 1 2014, 9:21 AM
shankarke added inline comments.Dec 1 2014, 9:23 AM
lib/ReaderWriter/ELF/ARM/TODO.rst
9
denis-protivensky updated this revision to Diff 16806.EditedDec 2 2014, 5:33 AM
denis-protivensky edited edge metadata.

Updated:

  • removed unused code related to PLT processing
  • removed all handlers of dynamic linking (now dynamic linking attempt fails)
  • updated test to link statically
  • corrected debug output formatting & assert
  • corrected TODO list items

Still in question:

  • image's default base address
  • __exidx_start/__exidx_end handling

Need some more tests

a) TLS
b) Check the ARM relocations that you have implemented.

You havent fixed up the exidx_start/end symbol.

I dont see the defsym test testing pieces of ARM functionality that you have implemented.

lib/ReaderWriter/ELF/ARM/ARMRelocationHandler.cpp
102–104

Can you indent some of these manually.

190–193

Isnt this suppose to use a veneer/shim to reach if the target is ARM ?

417–418

I think you should cache this address.

In D6446#25, @shankarke wrote:

Need some more tests

a) TLS
b) Check the ARM relocations that you have implemented.

You havent fixed up the exidx_start/end symbol.

I dont see the defsym test testing pieces of ARM functionality that you have implemented.

Shankar,

Thanks for reviewing this.

The goal of this initial patch is to make a start of ARM ELF generation. The patch doesn't even make static linking possible. All relocation handlres, including TLS handlers, those requiring veneer generation and others come from libc which I'm linking against during manual testing.
I mean that, I can't reproduce these in tests unless making specific samples with TLS variables, or with BL/BLX instructions, and if I start adding them - we end up having not working code in general, with [highly possibly] not working tests, because there's no guarantee that I can implement everything correctly without real testing.

So I propose to add all these things into the TODO list, and also mark them as requiring tests. This is related to TLS handlers, veneers, .exidx section's symbols and others.

lib/ReaderWriter/ELF/ARM/ARMExecutableWriter.h
34–35

It doesn't because my goal is to make the simplest case link statically. I suspect there shouldn't be any exception handling sections, so I decided to drop it for some time. These two atoms needed to make the linker happy and don't break with unresolved symbols error.

I'd prefer adding handling of these later since it at least requires reading exception handling ABI addendum, and maybe writing separate test cases. I noted in the TODO list about the need of .ARM.exidx section handling.

lib/ReaderWriter/ELF/ARM/ARMLinkingContext.h
33–34

That's a good question. This also was a point of concern to me. Didn't manage to find the answer. Do you know where it may be specified?

lib/ReaderWriter/ELF/ARM/TODO.rst
9

Shankar, thank you for the explanation. Added to the TODO list.

denis-protivensky edited edge metadata.

Updated:

  • fixed indentation/formatting again
  • added caching for finding segments by atoms in the layout
  • added fixups for exidx_start/end symbols
wnewton accepted this revision.Dec 4 2014, 4:13 AM
wnewton edited edge metadata.

This looks ok to me as an initial revision, although obviously it is missing functionality and test coverage.

Replaced this with http://reviews.llvm.org/D6716.
This one should be dropped.

lib/ReaderWriter/ELF/ARM/ARMRelocationHandler.cpp
102–104

Ok.

190–193

In general - yes, but it's not implemented in this initial work.

417–418

Good.

denis-protivensky abandoned this revision.Feb 5 2015, 11:26 PM