Re: [Arm] SP and SL initialization refactored
"Richard Earnshaw (lists)" <[email protected]>
| Newsgroups | gmane.comp.lib.newlib |
|---|---|
| Message-ID | <[email protected]> |
On 19/03/2019 13:54, Alexander Fedotov wrote: > Hi all > > In continue of threads > https://sourceware.org/ml/newlib/2019/msg00117.html and > https://sourceware.org/ml/newlib/2019/msg00102.html > > I have reworked stack pointer initialization and stack limit as well. > General idea behind of all these changes is to provide hook for > flexibility (including semihosting when debug tap is used) and improve > readability of existing crt0 code. > And now code makes SP initialization in Thumb mode :) > > Any comments are very appreciated. > > Alex > Apologies once again for the delayed review. A small plea: when posting patches, please can you ensure that your attachments are text/patch, or something similar, so that replies will include the contents (and I can view them directly in the email rather than having to mess about extracting the attachment). Before this goes in, the changes will need to be mirrored into newlib/libc/sys/arm. The two should be kept in sync as far as possible. now to the patch... +/* Current Program Status Register (CPSR) + M[4:0] Mode bits. M[4] is always 1 for 32-bit modes. + T[5] 1: Thumb, 0: ARM instruction set + F[6] 1: disables FIQ + I[7] 1: disables IRQ + A[8] 1: disables imprecise aborts + E[9] 0: Little-endian, 1: Big-endian + J[24] 1: Jazelle instruction set + */ Please note in the comment that this only applies to A- and R- profiles (and legacy Arm). +.macro FN_START Please rename to FN_EH_START (and similarly for FN_END) - we already have FUNC_START and the similar name is confusing. + +/******************************************************************************* +* User mode only: This routine makes default target specific Stack +* +-----+ <- SL_sys, Pointer initialization for different processor modes: +* | | SL_usr FIQ, Abort, IRQ, Undefined, Supervisor, System (User) +* | SYS | and setups a default Stack Limit in-case the code has +* | USR | -=0x10000 been compiled with "-mapcs-stack-check" for FIQ and +* | | System (User) modes. +* +-----+ <- initial SP, +* becomes SP_sys Hard-wiring SL value is not ideal, since there is +* and SL_usr currently no support for checking that the heap and +* stack have not collided, or that this default 64k is +* All modes: enough for the program being executed. However, it +* +-----+ <- SL_sys, ensures that this simple crt0 world will not Similarly here. Also this comment wraps in an 80-column window, so please reformat to avoid that. + /* M profile doesn't have CPSR register. */ +#if (__ARM_ARCH_PROFILE != 'M') + /* Following code is compatible for both ARM and Thumb ISA */ + mrs r4, CPSR + tst r4, #(CPSR_M_MASK) /* Test mode bits - in User of all are 0 */ This code is clearly ARM/Thumb2 only. Have you tested that this builds for, say ARM7TDMI in thumb state? If you use a build of gcc configured with --with-multilib-list=aprofile or --with-multilib-list=rmprofile you'll get pretty comprehensive checking of all the variants that need to be supported (beware, it can take quite a long time, even on a fast machine). At the very least, this code needs to be checked on: armv4t Arm and Thumb states armv6-m Thumb state armv7-a Arm and Thumb states armv7-m Thumb state + #ifdef THUMB1_ONLY + adds r2, #128 + adds r2, #128 + mov sl, r2 + #else + add sl, r2, #256 + #endif the ifdef should start in the first column. If you really want to reflect the nesting, then use a number of spaces between the # and the "ifdef". Please can you also clean up the indentation of the instructions: some have one tab indent after the mnemonic, some two. + b lr + FN_END ??? This can't be right, you don't have a label called 'lr'. Probably you meant to use the appropriate return instruction. + /* System (shares regs with User) mode, interrupts disabled */ + mov r1, #(CPSR_M_SYS|CPSR_M_32BIT|CPSR_I_MASK|CPSR_F_MASK) + msr CPSR_c, r1 + mov sp, r3 + This implies dropping support for Armv3 as system mode was added in Armv4. That's probably not an issue now as GCC has also dropped such support, but it should be added to the News. Overall, I quite like the direction this is taking, but there's more work to yet. R. R.