| 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 | expand |
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
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
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
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
> -----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
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
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