[PATCH] ARM + Thumb Segmented Stacks (v2)

[Alex Crichton]

I do not have commit access, so if you could commit for me, it would
be much appreciated!

Thanks again for all your help!

On Wed, Apr 2, 2014 at 6:20 AM, Oliver Stannard <Oliver.Stannard at arm.com> wrote:
> The patch looks good to me now. Do you have commit access? If not, I can commit it for you.
>
> Oliver
>
>> -----Original Message-----
>> From: alexc605 at gmail.com [mailto:alexc605 at gmail.com] On Behalf Of Alex
>> Crichton
>> Sent: 31 March 2014 18:20
>> To: Oliver Stannard
>> Cc: llvm-commits at cs.uiuc.edu
>> Subject: Re: [PATCH] ARM + Thumb Segmented Stacks (v2)
>>
>> 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
>> >
>
> -- 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
>