[PATCH] ARM + Thumb Segmented Stacks (v2)

[Alex Crichton]

Thanks again so much for your help and bearing with me!

I've updated with the t2LDR_POST opcode, using the coprocessor on
Thumb2 (but not Thumb1), tweaking the report_fatal_error condition
(just removing the "!Thumb" clause), and formatted such that no more
lines are over 80 chars.

On Mon, Mar 31, 2014 at 7:01 AM, Oliver Stannard
<Oliver.Stannard at arm.com> wrote:
>> -----Original Message-----
>> >> I've attempted to implement (5), but if I chose the wrong opcodes,
>> >> just let me know.
>> >
>> > Most of them look correct, but the "pop {lr}" instruction is now
>> using
>> > ARM::tPOP, which corresponds to encoding T1, which can only load into
>> > registers r0-r7 and pc. (The reason of the difference between the
>> Thumb1
>> > push and pop instructions is that it can be used for efficient
>> function
>> > epilogues, popping the lr straight back into the pc.) I see two
>> options:
>> > * Use encoding T3, which can be any register except for sp. This is a
>> > Thumb2-only instruction, so this code could not run on Thumb1-only
>> > processors. I think this is roughly what you had before.
>> > * Use POP encoding T1 to pop into r4, followed by MOV encoding T1 to
>> move
>> > from r4 to lr. Both of these instructions are available on Thumb1.
>> >
>> > Since Thumb1FrameLowering inherits from ARMFrameLowering, your code
>> is
>> > currently being run for Thumb1, but emitting code which is not valid
>> Thumb1.
>> > Because of this, you will need to either ensure you only use Thumb1
>> > instructions when generating code for Thumb1 or Thumb2, or add an
>> > implementation of adjustForSegmentedStacks to Thumb1FrameLowering. It
>> is
>> > fine if this implementation just throws report_fatal_error for now,
>> if you
>> > don't want to support Thumb1 yet.
>>
>> If this is the only instruction in question, I think it's easy enough
>> to support both. I switched Thumb1 to using "pop {r4}; mov lr, r4",
>> and I added a "pop {lr}" for Thumb2, but the only instruction I found
>> was t2LDMIA_UPD. You expressed size concerns about this previously,
>> but I was unable to find opcodes like t2POP or t3POP. Is there one for
>> the instruction you were mentioning?
>
> Sorry, I've been using the ARM encoding names, LLVM has different names for the encodings. I think you want ARM::t2LDR_POST, there are example of its use at ARMFrameLowering.cpp:{1192,881}. t2LDMIA_UPD emits the invalid encoding of LDM with one register. You can use llvm-mc to find out the LLVM name of the instruction that you want like this:
>
> $ llvm-mc -triple thumbv7-none-eabi -assemble -show-inst <<< 'pop   {lr}'
>         .text
>         ldr     lr, [sp], #4            @ <MCInst #2456 t2LDR_POST
>                                         @  <MCOperand Reg:10>
>                                         @  <MCOperand Reg:12>
>                                         @  <MCOperand Reg:12>
>                                         @  <MCOperand Imm:4>
>                                         @  <MCOperand Imm:14>
>                                         @  <MCOperand Reg:0>>
>
>> I've also added a thumb2-specific test (just a small one) to make sure
>> the code path is hit. I also accidentally left out a test case for ARM
>> in the previous patches which I included in this patch (sorry about
>> that!)
>>
>> >> According to the original author [1], apparently some Thumb units
>> >> don't have the coprocessor that the ARM version uses. The location
>> of
>> >> the stack limit seems like it can be all over the place and is
>> pretty
>> >> dependent to the platform that it's running on, so I'm not quite
>> sure
>> >> what the best way is to reconcile all these without adding a zillion
>> >> configuration options.
>> >
>> > This may not be too important an issue at the moment, as rust does
>> not
>> > support thumb (as far as I can tell, [2] does not contain thumb and
>> the arm
>> > directory does not have any thumb assembly). Do you know of anyone
>> using
>> > this for thumb, and what platform they are targeting? However,
>> switching
>> > based on the instruction set (or any other hardware feature) is
>> really not
>> > the right way to do this, as this is a platform decision.
>>
>> For now, we have a contributor who is exploring Thumb, but I think
>> right now they're predominately working without the standard library
>> and/or just having a dummy __morestack stub. They haven't upstreamed
>> their work just yet.
>>
>> I am unaware of what platforms they're targeting, I'd have to get more
>> information from them. Perhaps this could continue to use the
>> coprocessor for the stack limit, and further patches could tweak it
>> for various platforms?
>
> Yes, it seems that the best option for now is to use the coprocessor for Thumb2 and ARM, and the STACK_LIMIT symbol for Thumb1. It looks like the coprocessor register you are using is part of the Virtual Memory System Architecture (VMSA) extension, which is only available for A-class processors. However, as far as I am aware, Linux and Android require an MMU (except for thinks like uCLinux), so I think we can assume the existence of that co-processor if the platform is Linux or Android.
>
> 8)
>> if (!Thumb && !ST->isTargetAndroid() && !ST->isTargetLinux())
>>   report_fatal_error("Segmented stacks not supported on this platfrom.");
>
> I think you should check this condition, it looks like it will not trigger for "thumb and not (linux or android)". Also the comment says you do not support Thumb1, but you do now.
>
> 9)
> There are a few long lines (>80 chars), clang-format can fix these and other coding style issues for you.
>
> Oliver
>
> -- IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium.  Thank you.
>
> ARM Limited, Registered office 110 Fulbourn Road, Cambridge CB1 9NJ, Registered in England & Wales, Company No:  2557590
> ARM Holdings plc, Registered office 110 Fulbourn Road, Cambridge CB1 9NJ, Registered in England & Wales, Company No:  2548782
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: arm.patch
Type: application/octet-stream
Size: 44511 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140331/0fd665ed/attachment.obj>