[PATCH] Semihosting fix for AArch64 when heapinfo is not provided by debugger

[PATCH] Semihosting fix for AArch64 when heapinfo is not provided by debugger

[Alexander Fedotov]

[-- Attachment #1: Type: text/plain, Size: 112 bytes --]

Use the same approach as in ARM port for case when Debugger not
providing necessary info for semihosting.

Alex

[-- Attachment #2: 0001-Fixed-semihosting-for-AArch64-when-heapinfo-paramete.patch --]
[-- Type: text/x-patch, Size: 4207 bytes --]

From 4f2ac350345f1126fa491dc0a335a9e1da92817e Mon Sep 17 00:00:00 2001
From: Alexander Fedotov <alfedotov@gmail.com>
Date: Wed, 11 Oct 2017 14:52:20 +0300
Subject: [PATCH] Fixed semihosting for AArch64 when heapinfo parameters are
 not provided by debugger

---
 libgloss/aarch64/crt0.S     | 78 ++++++++++++++++++++++++++++++---------------
 libgloss/aarch64/syscalls.c |  7 +++-
 2 files changed, 59 insertions(+), 26 deletions(-)

diff --git a/libgloss/aarch64/crt0.S b/libgloss/aarch64/crt0.S
index f670e03..f831be1 100644
--- a/libgloss/aarch64/crt0.S
+++ b/libgloss/aarch64/crt0.S
@@ -102,33 +102,44 @@
 	ldr	x0, .LC0		/* point at returned values */
 	ldr	x1, [x0, #8]		/* get heap_limit */
 
+	/* Set __heap_limit.  */
+#ifdef __ILP32__
+	/* Sanity check on the __heap_limit.  */
+	tst	x1, #0xffffffff00000000
+	bne	.Linsanepar
+#endif
+	cmp     x1, xzr
+	beq     .LC4
+	adrp	x2, __heap_limit
+	add	x2, x2, #:lo12:__heap_limit
+	str     x1, [x2]
+.LC4:
+
+	ldr	x1, [x0]		/* get heap_base */
 #ifdef __ILP32__
 	/* Sanity check on the heap base.  */
-	ldr	x0, [x0]		/* get heap_base */
-	tst	x0, #0xffffffff00000000
-	beq	1f
-	/* Exit with 1 if the heap base is not within the 32-bit address
-	   space.  */
-	mov	x0, ADP_Stopped_ApplicationExit & 0xff
-	movk	x0, ADP_Stopped_ApplicationExit >> 16, lsl #16
-	adrp	x1, HeapBase	/* Reuse to construct the parameter block.  */
-	add	x1, x1, #:lo12:HeapBase
-	str	x0, [x1]
-	mov	x0, 1
-	str	x0, [x1, #8]
-	mov	w0, #AngelSVC_Reason_ReportException
-	AngelSVCAsm AngelSVC
-1:
-	/* For the sake of safety, set the stack base to the top end of
-	   the 32-bit address space if the returned value from the
-	   Angel API call is larger than or equal to 4 GiB.  */
 	tst	x1, #0xffffffff00000000
-	csinv	w1, w1, wzr, eq
+	bne	.Linsanepar
 #endif
-#else
-	/* Set up the stack pointer to a fixed value.  */
-	ldr     x1, .Lstack
+	cmp     x1, xzr
+	bne     .LC5
+	/* If the heap base value [x0, #0] is 0 then the heap base is actually
+	   at the end of program data (i.e. __end__) */
+	ldr     x1, .LC3
+	str     x1, [x0, #0]
+.LC5:
+	ldr	x1, [x0, #16] /* get stack_base */
+
+#ifdef __ILP32__
+	/* Sanity check on the stack_base.  */
+	tst	x1, #0xffffffff00000000
+	bne	.Linsanepar
+#endif
+	cmp	x1, xzr
+	bne	.LC6
 #endif
+	ldr     x1, .Lstack /*  Set up the stack pointer to a fixed value */
+.LC6:
 
 	/* Ensure quad-word stack alignment.  */
 	and	x0, x1, #~15
@@ -234,6 +245,22 @@
 
 	b	FUNCTION (exit)		/* Cannot return.  */
 
+#if defined (ARM_RDI_MONITOR) && defined (__ILP32__)
+.Linsanepar:
+	/* Exit with 1 if the parameter is not within the 32-bit address
+	   space.  */
+	mov	x1, ADP_Stopped_ApplicationExit & 0xff
+	movk	x1, ADP_Stopped_ApplicationExit >> 16, lsl #16
+	adrp	x0, HeapBase	/* Reuse to construct the parameter block.  */
+	add	x0, x0, #:lo12:HeapBase
+	str	x1, [x0]
+	mov	x1, 1
+	str	x1, [x0, #8]
+	mov	w1, #AngelSVC_Reason_ReportException
+	AngelSVCAsm AngelSVC
+	b       .
+#endif
+
 /* Function initializing exception vector table, flatmap, etc.
    Declared as weak symbol so that user can override this definition
    by linking in their own version of the function.  */
@@ -245,12 +272,13 @@ FUNCTION (_cpu_init_hook):
 #ifdef ARM_RDI_MONITOR
 .LC0:
 	GEN_DWORD HeapBase
-#else
+.LC3:
+	GEN_DWORD __end__
+#endif
 .Lstack:
 	GEN_DWORD __stack
-
 	.weak __stack
-#endif
+
 .LC1:
 	GEN_DWORD __bss_start__
 .LC2:
diff --git a/libgloss/aarch64/syscalls.c b/libgloss/aarch64/syscalls.c
index af206a1..4de4e0d 100644
--- a/libgloss/aarch64/syscalls.c
+++ b/libgloss/aarch64/syscalls.c
@@ -625,6 +625,9 @@ _getpid (int n __attribute__ ((unused)))
   return 1;
 }
 
+/* Heap limit returned from SYS_HEAPINFO Angel semihost call.  */
+ulong __heap_limit __attribute__ ((aligned (8))) = 0xcafedead;
+
 caddr_t
 _sbrk (int incr)
 {
@@ -637,7 +640,9 @@ _sbrk (int incr)
 
   prev_heap_end = heap_end;
 
-  if (heap_end + incr > stack_ptr)
+  if ((heap_end + incr > stack_ptr)
+      /* Honour heap limit if it's valid.  */
+      || ((__heap_limit != 0xcafedead) && (heap_end + incr > __heap_limit)))
     {
       /* Some of the libstdc++-v3 tests rely upon detecting
          out of memory errors, so do not abort here.  */
-- 
2.7.4

Re: [PATCH] Semihosting fix for AArch64 when heapinfo is not provided by debugger

[Corinna Vinschen]

[-- Attachment #1: Type: text/plain, Size: 277 bytes --]

On Oct 11 14:57, Alexander Fedotov wrote:
> Use the same approach as in ARM port for case when Debugger not
> providing necessary info for semihosting.

I'd like to see an ACK from some arm folks.


Thanks,
Corinna

-- 
Corinna Vinschen
Cygwin Maintainer
Red Hat

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] Semihosting fix for AArch64 when heapinfo is not provided by debugger

[Alexander Fedotov]

Any comments/thoughts on this patch ?
It would be great to have this feature in newlib release version.

Alex

On Fri, 13 Oct 2017 at 12:16, Corinna Vinschen <vinschen@redhat.com> wrote:

> On Oct 11 14:57, Alexander Fedotov wrote:
> > Use the same approach as in ARM port for case when Debugger not
> > providing necessary info for semihosting.
>
> I'd like to see an ACK from some arm folks.
>
>
> Thanks,
> Corinna
>
> --
> Corinna Vinschen
> Cygwin Maintainer
> Red Hat
>
-- 
Best regards,
AF

RE: [PATCH] Semihosting fix for AArch64 when heapinfo is not provided by debugger

[Tamar Christina]

Hi Alex,

In principle nothing against the patch. Just a comment

> diff --git a/libgloss/aarch64/crt0.S b/libgloss/aarch64/crt0.S
> index f670e03..f831be1 100644
> --- a/libgloss/aarch64/crt0.S
> +++ b/libgloss/aarch64/crt0.S
> @@ -102,33 +102,44 @@
>  	ldr	x0, .LC0		/* point at returned values */
>  	ldr	x1, [x0, #8]		/* get heap_limit */
>  
> +	/* Set __heap_limit.  */
> +#ifdef __ILP32__
> +	/* Sanity check on the __heap_limit.  */
> +	tst	x1, #0xffffffff00000000
> +	bne	.Linsanepar
> +#endif

if x1 is 0 it won't pass the first test in ILP32 and takes the branch,
I assume you intended it to fall through in that case? So I think this test
should be re-ordered.

> +	cmp     x1, xzr
> +	beq     .LC4
> +	adrp	x2, __heap_limit
> +	add	x2, x2, #:lo12:__heap_limit
> +	str     x1, [x2]
> +.LC4:

Thanks,
Tamar



> -----Original Message-----
> From: newlib-owner@sourceware.org [mailto:newlib-
> owner@sourceware.org] On Behalf Of Alexander Fedotov
> Sent: 19 October 2017 22:41
> To: newlib@sourceware.org
> Subject: Re: [PATCH] Semihosting fix for AArch64 when heapinfo is not
> provided by debugger
> 
> Any comments/thoughts on this patch ?
> It would be great to have this feature in newlib release version.
> 
> Alex
> 
> On Fri, 13 Oct 2017 at 12:16, Corinna Vinschen <vinschen@redhat.com>
> wrote:
> 
> > On Oct 11 14:57, Alexander Fedotov wrote:
> > > Use the same approach as in ARM port for case when Debugger not
> > > providing necessary info for semihosting.
> >
> > I'd like to see an ACK from some arm folks.
> >
> >
> > Thanks,
> > Corinna
> >
> > --
> > Corinna Vinschen
> > Cygwin Maintainer
> > Red Hat
> >
> --
> Best regards,
> AF

Re: [PATCH] Semihosting fix for AArch64 when heapinfo is not provided by debugger

[Richard Earnshaw (lists)]

On 23/10/17 18:23, Tamar Christina wrote:
> Hi Alex,
> 
> In principle nothing against the patch. Just a comment
> 
>> diff --git a/libgloss/aarch64/crt0.S b/libgloss/aarch64/crt0.S
>> index f670e03..f831be1 100644
>> --- a/libgloss/aarch64/crt0.S
>> +++ b/libgloss/aarch64/crt0.S
>> @@ -102,33 +102,44 @@
>>  	ldr	x0, .LC0		/* point at returned values */
>>  	ldr	x1, [x0, #8]		/* get heap_limit */
>>  
>> +	/* Set __heap_limit.  */
>> +#ifdef __ILP32__
>> +	/* Sanity check on the __heap_limit.  */
>> +	tst	x1, #0xffffffff00000000
>> +	bne	.Linsanepar
>> +#endif
> 
> if x1 is 0 it won't pass the first test in ILP32 and takes the branch,
> I assume you intended it to fall through in that case? So I think this test
> should be re-ordered.

Huh?  zero & anything is zero, so the Z bit will be set and the branch
will not be taken.


> 
>> +	cmp     x1, xzr
>> +	beq     .LC4
>> +	adrp	x2, __heap_limit
>> +	add	x2, x2, #:lo12:__heap_limit
>> +	str     x1, [x2]
>> +.LC4:
> 
> Thanks,
> Tamar
> 

Looks basically OK to me.

R.

> 
> 
>> -----Original Message-----
>> From: newlib-owner@sourceware.org [mailto:newlib-
>> owner@sourceware.org] On Behalf Of Alexander Fedotov
>> Sent: 19 October 2017 22:41
>> To: newlib@sourceware.org
>> Subject: Re: [PATCH] Semihosting fix for AArch64 when heapinfo is not
>> provided by debugger
>>
>> Any comments/thoughts on this patch ?
>> It would be great to have this feature in newlib release version.
>>
>> Alex
>>
>> On Fri, 13 Oct 2017 at 12:16, Corinna Vinschen <vinschen@redhat.com>
>> wrote:
>>
>>> On Oct 11 14:57, Alexander Fedotov wrote:
>>>> Use the same approach as in ARM port for case when Debugger not
>>>> providing necessary info for semihosting.
>>>
>>> I'd like to see an ACK from some arm folks.
>>>
>>>
>>> Thanks,
>>> Corinna
>>>
>>> --
>>> Corinna Vinschen
>>> Cygwin Maintainer
>>> Red Hat
>>>
>> --
>> Best regards,
>> AF

RE: [PATCH] Semihosting fix for AArch64 when heapinfo is not provided by debugger

[Tamar Christina]

> -----Original Message-----
> From: Richard Earnshaw (lists) [mailto:Richard.Earnshaw@arm.com]
> Sent: 24 October 2017 16:20
> To: Tamar Christina; Alexander Fedotov; newlib@sourceware.org
> Cc: nd
> Subject: Re: [PATCH] Semihosting fix for AArch64 when heapinfo is not
> provided by debugger
> 
> On 23/10/17 18:23, Tamar Christina wrote:
> > Hi Alex,
> >
> > In principle nothing against the patch. Just a comment
> >
> >> diff --git a/libgloss/aarch64/crt0.S b/libgloss/aarch64/crt0.S index
> >> f670e03..f831be1 100644
> >> --- a/libgloss/aarch64/crt0.S
> >> +++ b/libgloss/aarch64/crt0.S
> >> @@ -102,33 +102,44 @@
> >>  	ldr	x0, .LC0		/* point at returned values */
> >>  	ldr	x1, [x0, #8]		/* get heap_limit */
> >>
> >> +	/* Set __heap_limit.  */
> >> +#ifdef __ILP32__
> >> +	/* Sanity check on the __heap_limit.  */
> >> +	tst	x1, #0xffffffff00000000
> >> +	bne	.Linsanepar
> >> +#endif
> >
> > if x1 is 0 it won't pass the first test in ILP32 and takes the branch,
> > I assume you intended it to fall through in that case? So I think this
> > test should be re-ordered.
> 
> Huh?  zero & anything is zero, so the Z bit will be set and the branch will not
> be taken.
> 

Yes, sorry, I flipped the mask. Disregard my earlier comment.

> 
> >
> >> +	cmp     x1, xzr
> >> +	beq     .LC4
> >> +	adrp	x2, __heap_limit
> >> +	add	x2, x2, #:lo12:__heap_limit
> >> +	str     x1, [x2]
> >> +.LC4:
> >
> > Thanks,
> > Tamar
> >
> 
> Looks basically OK to me.
> 
> R.
> 
> >
> >
> >> -----Original Message-----
> >> From: newlib-owner@sourceware.org [mailto:newlib-
> >> owner@sourceware.org] On Behalf Of Alexander Fedotov
> >> Sent: 19 October 2017 22:41
> >> To: newlib@sourceware.org
> >> Subject: Re: [PATCH] Semihosting fix for AArch64 when heapinfo is not
> >> provided by debugger
> >>
> >> Any comments/thoughts on this patch ?
> >> It would be great to have this feature in newlib release version.
> >>
> >> Alex
> >>
> >> On Fri, 13 Oct 2017 at 12:16, Corinna Vinschen <vinschen@redhat.com>
> >> wrote:
> >>
> >>> On Oct 11 14:57, Alexander Fedotov wrote:
> >>>> Use the same approach as in ARM port for case when Debugger not
> >>>> providing necessary info for semihosting.
> >>>
> >>> I'd like to see an ACK from some arm folks.
> >>>
> >>>
> >>> Thanks,
> >>> Corinna
> >>>
> >>> --
> >>> Corinna Vinschen
> >>> Cygwin Maintainer
> >>> Red Hat
> >>>
> >> --
> >> Best regards,
> >> AF

Re: [PATCH] Semihosting fix for AArch64 when heapinfo is not provided by debugger

[Corinna Vinschen]

[-- Attachment #1: Type: text/plain, Size: 1782 bytes --]

On Oct 24 15:20, Tamar Christina wrote:
> 
> 
> > -----Original Message-----
> > From: Richard Earnshaw (lists) [mailto:Richard.Earnshaw@arm.com]
> > Sent: 24 October 2017 16:20
> > To: Tamar Christina; Alexander Fedotov; newlib@sourceware.org
> > Cc: nd
> > Subject: Re: [PATCH] Semihosting fix for AArch64 when heapinfo is not
> > provided by debugger
> > 
> > On 23/10/17 18:23, Tamar Christina wrote:
> > > Hi Alex,
> > >
> > > In principle nothing against the patch. Just a comment
> > >
> > >> diff --git a/libgloss/aarch64/crt0.S b/libgloss/aarch64/crt0.S index
> > >> f670e03..f831be1 100644
> > >> --- a/libgloss/aarch64/crt0.S
> > >> +++ b/libgloss/aarch64/crt0.S
> > >> @@ -102,33 +102,44 @@
> > >>  	ldr	x0, .LC0		/* point at returned values */
> > >>  	ldr	x1, [x0, #8]		/* get heap_limit */
> > >>
> > >> +	/* Set __heap_limit.  */
> > >> +#ifdef __ILP32__
> > >> +	/* Sanity check on the __heap_limit.  */
> > >> +	tst	x1, #0xffffffff00000000
> > >> +	bne	.Linsanepar
> > >> +#endif
> > >
> > > if x1 is 0 it won't pass the first test in ILP32 and takes the branch,
> > > I assume you intended it to fall through in that case? So I think this
> > > test should be re-ordered.
> > 
> > Huh?  zero & anything is zero, so the Z bit will be set and the branch will not
> > be taken.
> > 
> 
> Yes, sorry, I flipped the mask. Disregard my earlier comment.
> 
> > 
> > >
> > >> +	cmp     x1, xzr
> > >> +	beq     .LC4
> > >> +	adrp	x2, __heap_limit
> > >> +	add	x2, x2, #:lo12:__heap_limit
> > >> +	str     x1, [x2]
> > >> +.LC4:
> > >
> > > Thanks,
> > > Tamar
> > >
> > 
> > Looks basically OK to me.
> > 
> > R.

Thanks, guys.  Pushed.


Thanks,
Corinna

-- 
Corinna Vinschen
Cygwin Maintainer
Red Hat

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]