Page MenuHomePhabricator

[lld][ARM] Add relocations to perform function calls
ClosedPublic

Authored by denis-protivensky on Jan 27 2015, 11:57 PM.

Details

Summary

Added relocations to perform function calls with and without passing arguments.
ARM-only, Thumb-only and mixed mode code generations are supported.
Only simple veneers (direct instruction modification) are supported as ARM-Thumb interwork.

Diff Detail

Event Timeline

denis-protivensky retitled this revision from to [lld][ARM] Add relocations to perform function calls.
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).
wnewton added inline comments.Jan 28 2015, 1:42 AM
lib/ReaderWriter/ELF/ARM/ARMRelocationHandler.cpp
113

I guess this assert and the one in applyArmReloc are the points where error checking happens. It would be good to use the error checking functionality instead as the asserts will get removed in release builds.

161

"addressesThumb" is IMO a more descriptive name than "exchanges" so this variable does not help me to read the code.

lib/ReaderWriter/ELF/ARM/ARMRelocationHandler.cpp
113

This assert prevents from programmer's mistake when they specify a mask that doesn't correspond to bits in the result.
I agree that error checking is needed (may be added later), but this assert serves for another purpose.

161

I also doubted about this name, but I'd like to introduce some variable to point that addressing Thumb (or ARM) code for some instructions means 'exchange instruction set'. Any ideas about better name?

wnewton added inline comments.Jan 28 2015, 2:33 AM
lib/ReaderWriter/ELF/ARM/ARMRelocationHandler.cpp
161

switchMode?

lib/ReaderWriter/ELF/ARM/ARMRelocationHandler.cpp
161

Thanks! This sounds much better. Will update shortly.

Renamed exchanges => switchMode.

Calling ARM code from thumb needs a veneer right ?

You would need to create a stub when going through all the atoms if that is the case.

Shankar,

In general case - you're right, the stub is needed to interwork between ARM and Thumb code.
The current implementation covers a subset of the interwork where non-conditional jumps of B/BL forms are used, and they are fixed by changing to BLX instructions (works for ARMv5 and above).

Stub generation should be determined and done in the relocation pass, and it doesn't deny currently implemented instruction fixups, but supplements them.
I'm currently working on the stub generation, but I'd like this part of work with instruction fixups to be accepted. Is it ok?

This revision is now accepted and ready to land.Jan 28 2015, 6:28 AM
wnewton accepted this revision.Feb 2 2015, 8:17 AM
wnewton edited edge metadata.

LGTM

ruiu edited edge metadata.Feb 2 2015, 10:38 AM

I'm sorry, this thread is slipped from my inbox for some reason.
Reviewing...

ruiu accepted this revision.Feb 2 2015, 10:54 AM
ruiu edited edge metadata.

LGTM with nits.

lib/ReaderWriter/ELF/ARM/ARMRelocationHandler.cpp
19

Do we need this macro? It would be more readable if we just write

uint64_t T = flag;

instead of

DECLARE_T(flag);
35

Is this intentional that you used signed integers instead of unsigned integers in this function? I'd use uint16_t when I do bit manipulation.

170

It's just a suggestion and shouldn't be mixed with this patch, but if you define DEBUG_TYPE like this

#define DEBUG_TYPE "ARM"

then you can just use DEBUG(...) instead of DEBUG_WITH_TYPE("ARM", ...).

295

Indentation.

lib/ReaderWriter/ELF/ARM/ARMRelocationHandler.cpp
19

Indeed, the implicit conversion from bool should give proper values.

35

Agreed, will update all of these places while merging.

170

Thanks, got you.

295

Will fix.

lib/ReaderWriter/ELF/ARM/ARMRelocationHandler.cpp
35

I just looked carefully into the code and realized that using signed integers is a proper way of handling addends, because they may be negative numbers as well.
As for MOV addend of Thumb instruction you mentioned, it's formed from exactly 16 bits, and the value is a signed integer, which may be negative (as the reference says). So using signed integers is perfectly fine for all relocation types.

ruiu added inline comments.Feb 2 2015, 11:24 PM
lib/ReaderWriter/ELF/ARM/ARMRelocationHandler.cpp
35

I'd think the final result type, which is a signed integer type, can be different from types used for intermediate computation. We shuffle bits by shifts and bitmasks to form a 16 bit pattern here. All intermediate types except the result can be unsigned, no? Looks like all right bit shifts in this function aren't used to extend sign bit.

lib/ReaderWriter/ELF/ARM/ARMRelocationHandler.cpp
35

Okay, I'll change all value initialization and intermediate result calculations to operate with unsigned ints of the same size.

This revision was automatically updated to reflect the committed changes.