Page MenuHomePhabricator

[LLD][ELF][ARM] Add support for architecture v6m thunks
ClosedPublic

Authored by peter.smith on Dec 11 2018, 5:28 AM.

Details

Summary

ARM Architecture v6m is used by the smallest microcontrollers such as the cortex-m0. It is Thumb only (no Thumb 2) which prevents it from using the existing Thumb 2 range extension thunks as these use the Thumb 2 movt/movw instructions. Range extension thunks are not usually needed for microcontrollers due to the small amount of flash and ram on the device, however if code is copied from flash into ram then a range extension thunk is required to call that code.

This change adds support for v6m range extension thunks. The procedure call standard APCS permits a thunk to corrupt the intra-procedural scratch register r12 (referred to as ip in the APCS). Most Thumb instructions do not permit access to high registers (r8 - r15) so the thunks must spill some low registers (r0 - r7) to perform the control transfer.

Fixes pr39922 (https://bugs.llvm.org/show_bug.cgi?id=39922)

Diff Detail

Repository
rL LLVM

Event Timeline

peter.smith created this revision.Dec 11 2018, 5:28 AM

I do not know enough about arm to give a sign-off, but the code looks good to me. A minor nit is below.

ELF/Thunks.cpp
787 ↗(On Diff #177695)

You do not need else after return.

eblot added a subscriber: eblot.EditedDec 11 2018, 5:44 AM

Isn't there a copy n' paste leftover?

Thumbv7ABSLongThunk_ -> Thumbv6MABSLongThunk_
Thumbv7ABSLongThunk_ -> Thumbv6MPILongThunk_

peter.smith marked an inline comment as done.

Thanks for the comments. I've applied the suggested change to remove the else.

Yes, thanks for the spot, there is a copy-paste error. My apologies for missing that; now corrected.

efriedma added inline comments.
ELF/Thunks.cpp
570 ↗(On Diff #177700)

IIRC the AAPCS requirement is only 4-byte alignment within a function/thunk (and 8 bytes at ABI boundaries). But this sequence saves four bytes over using ip, I think.

594 ↗(On Diff #177700)

We don't need to maintain 8-byte stack alignment here, and we don't use the second word for the destination address.

It is true that the AAPCS only requires 8-byte stack alignment at procedure call boundaries. I have seen some interrupt handlers assuming 8 byte stack alignment, or at least not considering the case that it might not be, so I thought it better to be safe given that thunks are not easily visible to the programmer. I'm not wedded to the idea though so if you'd prefer I can remove the save of the extra register.

Compilers don't preserve 8-byte alignment within function prologues anyway, in general. For example, LLVM and other compilers frequently generate "push {r4, r5, r6, r7, lr}".

peter.smith edited the summary of this revision. (Show Details)

Ok I've removed the extra register. I've revisited my notes and the instructions that could potentially fault if not 8-byte aligned (ldrd and strd on v5te) don't exist in v6-m anyway.

The thunks look fine.

ELF/Thunks.cpp
785 ↗(On Diff #178022)

Sort of orthogonal to this patch, but maybe worth noting how we know we're targeting v6m here; it isn't obvious to anyone who isn't deeply familiar with the history of ARM architecture versions. (Or maybe it's worth adding an extra boolean to the config?)

Thanks for the comments. I've added some more information to mention where the flags come from and added a reference to InputFiles.cpp where updateSupportedARMFeatures() contains the mapping between architecture and the flags along with some explanation.

ruiu accepted this revision.Dec 14 2018, 4:31 PM

LGTM

This revision is now accepted and ready to land.Dec 14 2018, 4:31 PM
This revision was automatically updated to reflect the committed changes.