[Arm] SP and SL initialization refactored

[Richard Earnshaw (lists)]

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.