Re: [PATCH 3/8] newlib: libc: strlen M-profile PACBTI-enablement

["Victor L. Do Nascimento" <victor.donascimento@arm.com>]

Richard Earnshaw <Richard.Earnshaw@foss.arm.com> writes:

> On 05/07/2022 14:58, Victor Do Nascimento wrote:
>> Add function prologue/epilogue to conditionally add BTI landing pads
>> and/or PAC code generation & authentication instructions depending on
>> compilation flags.
>> This patch enables PACBTI for all relevant variants of strlen:
>>       * Newlib for armv8.1-m.main+pacbti
>>       * Newlib for armv8.1-m.main+pacbti+mve
>>       * Newlib-nano
>> ---
>>   newlib/libc/machine/arm/strlen-armv7.S     | 52 ++++++++++++++++++++--
>>   newlib/libc/machine/arm/strlen-stub.c      |  9 ++++
>>   newlib/libc/machine/arm/strlen-thumb2-Os.S | 13 ++++--
>>   3 files changed, 67 insertions(+), 7 deletions(-)
>> diff --git a/newlib/libc/machine/arm/strlen-armv7.S
>> b/newlib/libc/machine/arm/strlen-armv7.S
>> index f3dda0d60..18c8226d0 100644
>> --- a/newlib/libc/machine/arm/strlen-armv7.S
>> +++ b/newlib/libc/machine/arm/strlen-armv7.S
>> @@ -59,6 +59,7 @@
>>      OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.  */
>>     #include "acle-compat.h"
>> +#include "pacbti.h"
>>     	.macro def_fn f p2align=0
>>   	.text
>> @@ -77,7 +78,9 @@
>>   #endif
>>     	/* This code requires Thumb.  */
>> -#if __ARM_ARCH_PROFILE == 'M'
>> +#if __ARM_ARCH_8M_MAIN__
>
> These GCC architecture macros (those that end with '__') aren't portable and are
> essentially deprecated.  What exactly are you trying to achieve here?

It was my attempt at circumventing the .arch directives below.
As these older architecture lack the support for PACBTI instructions, if
we don't prevent the selection of the wrong target architecture, when
said instructions are encountered they cause Newlib compilation to fail.

In particular, I needed to distinguish between armv7e-m and
armv8.1-m.main and chose the __ARM_ARCH_8M_MAIN__ macro out of the
output from using the -dM GCC preprocessor flag.

Will fix.

>> +    /* keep config inherited from -march= */
>> +#elif __ARM_ARCH_PROFILE == 'M'
>>   	.arch   armv7e-m
>>   #else
>>   	.arch	armv6t2
>> @@ -100,8 +103,34 @@
>>   #define tmp2		r5
>>     def_fn	strlen p2align=6
>> +	.fnstart
>> +	.cfi_startproc
>> +	/* common pacbti_prologue macro from pacbti.h not used.
>> +	   handwritten prologue saves one push instruction. */
>> +#if __ARM_FEATURE_PAC_DEFAULT
>> +#if __ARM_FEATURE_BTI_DEFAULT
>> +	pacbti ip, lr, sp
>> +#else
>> +	pac ip, lr, sp
>> +#endif /* __ARM_FEATURE_BTI_DEFAULT */
>> +	push    {r4, r5, ip}
>> +	.save   {r4, r5, ra_auth_code}
>> +	.cfi_def_cfa_offset 12
>> +	.cfi_offset 143, -4
>> +	.cfi_offset 5, -8
>> +	.cfi_offset 4, -12
>> +
>> +#else
>> +#if __ARM_FEATURE_BTI_DEFAULT
>> +	bti
>> +#endif /* __ARM_FEATURE_BTI_DEFAULT */
>> +	push    {r4, r5}
>> +	.save   {r4, r5}
>> +	.cfi_def_cfa_offset 8
>> +	.cfi_offset 5, -4
>> +	.cfi_offset 4, -8
>> +#endif /* __ARM_FEATURE_PAC_DEFAULT */
>>   	pld	[srcin, #0]
>> -	strd	r4, r5, [sp, #-8]!
>>   	bic	src, srcin, #7
>>   	mvn	const_m1, #0
>>   	ands	tmp1, srcin, #7		/* (8 - bytes) to alignment.  */
>> @@ -159,9 +188,22 @@ def_fn	strlen p2align=6
>>   	rev	data1a, data1a
>>   #endif
>>   	clz	data1a, data1a
>> -	ldrd	r4, r5, [sp], #8
>>   	add	result, result, data1a, lsr #3	/* Bits -> Bytes.  */
>> -	bx	lr
>> +#if __ARM_FEATURE_PAC_DEFAULT
>> +	pop	{r4, r5, ip}
>> +	.cfi_restore 4
>> +	.cfi_restore 5
>> +	.cfi_restore 143
>> +	.cfi_def_cfa_offset 0
>> +	aut ip, lr, sp
>> +#else
>> +	ldrd	r4, r5, [sp], #8
>> +	.cfi_restore 4
>> +	.cfi_restore 5
>> +	.cfi_def_cfa_offset 0
>> +#endif /* __ARM_FEATURE_PAC_DEFAULT */
>> +	bx lr
>> +
>>     .Lmisaligned8:
>>   	ldrd	data1a, data1b, [src]
>> @@ -177,4 +219,6 @@ def_fn	strlen p2align=6
>>   	movne	data1a, const_m1
>>   	mov	const_0, #0
>>   	b	.Lstart_realigned
>> +	.cfi_endproc
>> +	.fnend
>>   	.size	strlen, . - strlen
>> diff --git a/newlib/libc/machine/arm/strlen-stub.c b/newlib/libc/machine/arm/strlen-stub.c
>> index fc2daf16f..4a0bb8cbb 100644
>> --- a/newlib/libc/machine/arm/strlen-stub.c
>> +++ b/newlib/libc/machine/arm/strlen-stub.c
>> @@ -58,6 +58,11 @@ strlen (const char* str)
>>          "data .req r3\n\t"
>>          "addr .req r1\n\t"
>>   +#ifdef __ARM_FEATURE_PAC_DEFAULT
>> +       "pac ip, lr, sp\n\t"
>> +       "str ip, [sp, #-4]!\n\t"
>> +#endif
>> +
>>   #ifdef _ISA_ARM_7
>>          "pld [r0]\n\t"
>>   #endif
>> @@ -167,6 +172,10 @@ strlen (const char* str)
>>          "it	ne\n\t"
>>          "addne	len, len, #1\n\t"
>>   # endif
>> +#endif
>> +#ifdef __ARM_FEATURE_PAC_DEFAULT
>> +       "ldr ip, [sp], #4\n\t"
>> +       "aut ip, lr, sp\n\t"
>>   #endif
>>          "bx	lr\n\t");
>>   }
>> diff --git a/newlib/libc/machine/arm/strlen-thumb2-Os.S b/newlib/libc/machine/arm/strlen-thumb2-Os.S
>> index 961f41a0a..823b0310e 100644
>> --- a/newlib/libc/machine/arm/strlen-thumb2-Os.S
>> +++ b/newlib/libc/machine/arm/strlen-thumb2-Os.S
>> @@ -25,6 +25,7 @@
>>      OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.  */
>>     #include "acle-compat.h"
>> +#include "pacbti.h"
>>     	.macro def_fn f p2align=0
>>   	.text
>> @@ -33,8 +34,9 @@
>>   	.type \f, %function
>>   \f:
>>   	.endm
>> -
>> -#if __ARM_ARCH_ISA_THUMB >= 2 && __ARM_ARCH >= 7
>> +#if __ARM_ARCH_8M_MAIN__
>> +	/* keep config inherited from -march= */
>> +#elif __ARM_ARCH_ISA_THUMB >= 2 && __ARM_ARCH >= 7
>>   	.arch   armv7
>>   #else
>>   	.arch	armv6t2
>> @@ -44,11 +46,16 @@
>>   	.syntax unified
>>     def_fn	strlen p2align=1
>> +	.fnstart
>> +	.cfi_startproc
>> +	pacbti_prologue
>>   	mov     r3, r0
>>   1:	ldrb.w  r2, [r3], #1
>>   	cmp     r2, #0
>>   	bne	1b
>>   	subs    r0, r3, r0
>>   	subs    r0, #1
>> -	bx      lr
>> +	pacbti_epilogue
>> +	.cfi_endproc
>> +	.fnend
>>   	.size	strlen, . - strlen