Semihosting fix for AArch64 when heapinfo is not provided by debugger

Message ID CAN8C2Cr0d+on0L4pnfX16d=La4R6ViPoDnhQR-4ha_cO-gsdOQ@mail.gmail.com
State New
Headers show
Series
  • Semihosting fix for AArch64 when heapinfo is not provided by debugger
Related show

Commit Message

Alexander Fedotov Oct. 11, 2017, 11:57 a.m.
Use the same approach as in ARM port for case when Debugger not
providing necessary info for semihosting.

Alex

Comments

Corinna Vinschen Oct. 13, 2017, 9:16 a.m. | #1
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
Alexander Fedotov Oct. 19, 2017, 9:41 p.m. | #2
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
Tamar Christina Oct. 23, 2017, 5:23 p.m. | #3
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
Richard Earnshaw (lists) Oct. 24, 2017, 3:19 p.m. | #4
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
Tamar Christina Oct. 24, 2017, 3:20 p.m. | #5
> -----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
Corinna Vinschen Oct. 24, 2017, 3:28 p.m. | #6
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

Patch

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