[RFC PATCH] semihosting/arm-compat: remove heuristic softmmu SYS_HEAPINFO

[RFC PATCH] semihosting/arm-compat: remove heuristic softmmu SYS_HEAPINFO

[Alex Bennée]

The previous numbers were a guess at best. While we could extract the
information from a loaded ELF file via -kernel we could still get
tripped up by self decompressing or relocating code. Besides sane
library code has access to the same symbols in run time to make a
determination of the location of the heap.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Cc: Andrew <astrauss11@gmail.com>
---
 semihosting/arm-compat-semi.c | 19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)

diff --git a/semihosting/arm-compat-semi.c b/semihosting/arm-compat-semi.c
index 1c29146dcf..041b4f6c04 100644
--- a/semihosting/arm-compat-semi.c
+++ b/semihosting/arm-compat-semi.c
@@ -1165,12 +1165,10 @@ target_ulong do_common_semihosting(CPUState *cs)
     case TARGET_SYS_HEAPINFO:
         {
             target_ulong retvals[4];
-            target_ulong limit;
             int i;
 #ifdef CONFIG_USER_ONLY
+            target_ulong limit;
             TaskState *ts = cs->opaque;
-#else
-            target_ulong rambase = common_semi_rambase(cs);
 #endif
 
             GET_ARG(0);
@@ -1201,12 +1199,15 @@ target_ulong do_common_semihosting(CPUState *cs)
             retvals[2] = ts->stack_base;
             retvals[3] = 0; /* Stack limit.  */
 #else
-            limit = current_machine->ram_size;
-            /* TODO: Make this use the limit of the loaded application.  */
-            retvals[0] = rambase + limit / 2;
-            retvals[1] = rambase + limit;
-            retvals[2] = rambase + limit; /* Stack base */
-            retvals[3] = rambase; /* Stack limit.  */
+            /*
+             * Reporting 0 indicates we couldn't calculate the real
+             * values which should force most software to fall back to
+             * using information it has.
+             */
+            retvals[0] = 0; /* Heap Base */
+            retvals[1] = 0; /* Heap Limit */
+            retvals[2] = 0; /* Stack base */
+            retvals[3] = 0; /* Stack limit.  */
 #endif
 
             for (i = 0; i < ARRAY_SIZE(retvals); i++) {
-- 
2.20.1

[RFC PATCH] semihosting/arm-compat: remove heuristic softmmu SYS_HEAPINFO

[Alex Bennée]

The previous numbers were a guess at best. While we could extract the
information from a loaded ELF file via -kernel we could still get
tripped up by self decompressing or relocating code. Besides sane
library code has access to the same symbols in run time to make a
determination of the location of the heap.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Cc: Andrew <astrauss11@gmail.com>
---
 semihosting/arm-compat-semi.c | 19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)

diff --git a/semihosting/arm-compat-semi.c b/semihosting/arm-compat-semi.c
index 1c29146dcf..041b4f6c04 100644
--- a/semihosting/arm-compat-semi.c
+++ b/semihosting/arm-compat-semi.c
@@ -1165,12 +1165,10 @@ target_ulong do_common_semihosting(CPUState *cs)
     case TARGET_SYS_HEAPINFO:
         {
             target_ulong retvals[4];
-            target_ulong limit;
             int i;
 #ifdef CONFIG_USER_ONLY
+            target_ulong limit;
             TaskState *ts = cs->opaque;
-#else
-            target_ulong rambase = common_semi_rambase(cs);
 #endif
 
             GET_ARG(0);
@@ -1201,12 +1199,15 @@ target_ulong do_common_semihosting(CPUState *cs)
             retvals[2] = ts->stack_base;
             retvals[3] = 0; /* Stack limit.  */
 #else
-            limit = current_machine->ram_size;
-            /* TODO: Make this use the limit of the loaded application.  */
-            retvals[0] = rambase + limit / 2;
-            retvals[1] = rambase + limit;
-            retvals[2] = rambase + limit; /* Stack base */
-            retvals[3] = rambase; /* Stack limit.  */
+            /*
+             * Reporting 0 indicates we couldn't calculate the real
+             * values which should force most software to fall back to
+             * using information it has.
+             */
+            retvals[0] = 0; /* Heap Base */
+            retvals[1] = 0; /* Heap Limit */
+            retvals[2] = 0; /* Stack base */
+            retvals[3] = 0; /* Stack limit.  */
 #endif
 
             for (i = 0; i < ARRAY_SIZE(retvals); i++) {
-- 
2.20.1

Re: [RFC PATCH] semihosting/arm-compat: remove heuristic softmmu SYS_HEAPINFO

[Peter Maydell]

On Tue, 1 Jun 2021 at 10:12, Alex Bennée <alex.bennee@linaro.org> wrote:
>
> The previous numbers were a guess at best. While we could extract the
> information from a loaded ELF file via -kernel we could still get
> tripped up by self decompressing or relocating code. Besides sane
> library code has access to the same symbols in run time to make a
> determination of the location of the heap.
>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> Cc: Andrew <astrauss11@gmail.com>
> ---
>  semihosting/arm-compat-semi.c | 19 ++++++++++---------
>  1 file changed, 10 insertions(+), 9 deletions(-)

This seems like a pretty good candidate for breaking existing
working binaries. How much testing against different varieties of
guest-code-using-semihosting have you done ?

thanks
-- PMM

Re: [RFC PATCH] semihosting/arm-compat: remove heuristic softmmu SYS_HEAPINFO

[Peter Maydell]

On Tue, 1 Jun 2021 at 10:12, Alex Bennée <alex.bennee@linaro.org> wrote:
>
> The previous numbers were a guess at best. While we could extract the
> information from a loaded ELF file via -kernel we could still get
> tripped up by self decompressing or relocating code. Besides sane
> library code has access to the same symbols in run time to make a
> determination of the location of the heap.
>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> Cc: Andrew <astrauss11@gmail.com>
> ---
>  semihosting/arm-compat-semi.c | 19 ++++++++++---------
>  1 file changed, 10 insertions(+), 9 deletions(-)

This seems like a pretty good candidate for breaking existing
working binaries. How much testing against different varieties of
guest-code-using-semihosting have you done ?

thanks
-- PMM

Re: [RFC PATCH] semihosting/arm-compat: remove heuristic softmmu SYS_HEAPINFO

[Alex Bennée]

Peter Maydell <peter.maydell@linaro.org> writes:

> On Tue, 1 Jun 2021 at 10:12, Alex Bennée <alex.bennee@linaro.org> wrote:
>>
>> The previous numbers were a guess at best. While we could extract the
>> information from a loaded ELF file via -kernel we could still get
>> tripped up by self decompressing or relocating code. Besides sane
>> library code has access to the same symbols in run time to make a
>> determination of the location of the heap.
>>
>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>> Cc: Andrew <astrauss11@gmail.com>
>> ---
>>  semihosting/arm-compat-semi.c | 19 ++++++++++---------
>>  1 file changed, 10 insertions(+), 9 deletions(-)
>
> This seems like a pretty good candidate for breaking existing
> working binaries. How much testing against different varieties of
> guest-code-using-semihosting have you done ?

None, which is why it's an RFC - but at least one user reported newlib
attempts to use the numbers we gave it rather than falling back to
numbers it knew from the build and getting it wrong. I suspect any code
that doesn't have a fallback path is getting it right more by luck than
judgement though. I'd be curious to hear of code that relies on the
numbers it gets from QEMU.

>
> thanks
> -- PMM


-- 
Alex Bennée

Re: [RFC PATCH] semihosting/arm-compat: remove heuristic softmmu SYS_HEAPINFO

[Alex Bennée]

Peter Maydell <peter.maydell@linaro.org> writes:

> On Tue, 1 Jun 2021 at 10:12, Alex Bennée <alex.bennee@linaro.org> wrote:
>>
>> The previous numbers were a guess at best. While we could extract the
>> information from a loaded ELF file via -kernel we could still get
>> tripped up by self decompressing or relocating code. Besides sane
>> library code has access to the same symbols in run time to make a
>> determination of the location of the heap.
>>
>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>> Cc: Andrew <astrauss11@gmail.com>
>> ---
>>  semihosting/arm-compat-semi.c | 19 ++++++++++---------
>>  1 file changed, 10 insertions(+), 9 deletions(-)
>
> This seems like a pretty good candidate for breaking existing
> working binaries. How much testing against different varieties of
> guest-code-using-semihosting have you done ?

None, which is why it's an RFC - but at least one user reported newlib
attempts to use the numbers we gave it rather than falling back to
numbers it knew from the build and getting it wrong. I suspect any code
that doesn't have a fallback path is getting it right more by luck than
judgement though. I'd be curious to hear of code that relies on the
numbers it gets from QEMU.

>
> thanks
> -- PMM


-- 
Alex Bennée

Re: [RFC PATCH] semihosting/arm-compat: remove heuristic softmmu SYS_HEAPINFO

[Peter Maydell]

On Tue, 1 Jun 2021 at 11:07, Alex Bennée <alex.bennee@linaro.org> wrote:
>
>
> Peter Maydell <peter.maydell@linaro.org> writes:
>
> > On Tue, 1 Jun 2021 at 10:12, Alex Bennée <alex.bennee@linaro.org> wrote:
> >>
> >> The previous numbers were a guess at best. While we could extract the
> >> information from a loaded ELF file via -kernel we could still get
> >> tripped up by self decompressing or relocating code. Besides sane
> >> library code has access to the same symbols in run time to make a
> >> determination of the location of the heap.
> >>
> >> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> >> Cc: Andrew <astrauss11@gmail.com>
> >> ---
> >>  semihosting/arm-compat-semi.c | 19 ++++++++++---------
> >>  1 file changed, 10 insertions(+), 9 deletions(-)
> >
> > This seems like a pretty good candidate for breaking existing
> > working binaries. How much testing against different varieties of
> > guest-code-using-semihosting have you done ?
>
> None, which is why it's an RFC - but at least one user reported newlib
> attempts to use the numbers we gave it rather than falling back to
> numbers it knew from the build and getting it wrong. I suspect any code
> that doesn't have a fallback path is getting it right more by luck than
> judgement though. I'd be curious to hear of code that relies on the
> numbers it gets from QEMU.

Well, newlib is the main one I had in mind -- does it have a fallback
codepath?

-- PMM

Re: [RFC PATCH] semihosting/arm-compat: remove heuristic softmmu SYS_HEAPINFO

[Peter Maydell]

On Tue, 1 Jun 2021 at 11:07, Alex Bennée <alex.bennee@linaro.org> wrote:
>
>
> Peter Maydell <peter.maydell@linaro.org> writes:
>
> > On Tue, 1 Jun 2021 at 10:12, Alex Bennée <alex.bennee@linaro.org> wrote:
> >>
> >> The previous numbers were a guess at best. While we could extract the
> >> information from a loaded ELF file via -kernel we could still get
> >> tripped up by self decompressing or relocating code. Besides sane
> >> library code has access to the same symbols in run time to make a
> >> determination of the location of the heap.
> >>
> >> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> >> Cc: Andrew <astrauss11@gmail.com>
> >> ---
> >>  semihosting/arm-compat-semi.c | 19 ++++++++++---------
> >>  1 file changed, 10 insertions(+), 9 deletions(-)
> >
> > This seems like a pretty good candidate for breaking existing
> > working binaries. How much testing against different varieties of
> > guest-code-using-semihosting have you done ?
>
> None, which is why it's an RFC - but at least one user reported newlib
> attempts to use the numbers we gave it rather than falling back to
> numbers it knew from the build and getting it wrong. I suspect any code
> that doesn't have a fallback path is getting it right more by luck than
> judgement though. I'd be curious to hear of code that relies on the
> numbers it gets from QEMU.

Well, newlib is the main one I had in mind -- does it have a fallback
codepath?

-- PMM

Re: [RFC PATCH] semihosting/arm-compat: remove heuristic softmmu SYS_HEAPINFO

[Andrew Strauss]

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

Yeah, newlib falls back to using the symbols __stack and end to figure out
where to start the stack and heap if 0 is returned by SYS_HEAPINFO.

-- Andrew Strauss

On Tue, Jun 1, 2021 at 6:31 AM Peter Maydell <peter.maydell@linaro.org>
wrote:

> On Tue, 1 Jun 2021 at 11:07, Alex Bennée <alex.bennee@linaro.org> wrote:
> >
> >
> > Peter Maydell <peter.maydell@linaro.org> writes:
> >
> > > On Tue, 1 Jun 2021 at 10:12, Alex Bennée <alex.bennee@linaro.org>
> wrote:
> > >>
> > >> The previous numbers were a guess at best. While we could extract the
> > >> information from a loaded ELF file via -kernel we could still get
> > >> tripped up by self decompressing or relocating code. Besides sane
> > >> library code has access to the same symbols in run time to make a
> > >> determination of the location of the heap.
> > >>
> > >> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> > >> Cc: Andrew <astrauss11@gmail.com>
> > >> ---
> > >>  semihosting/arm-compat-semi.c | 19 ++++++++++---------
> > >>  1 file changed, 10 insertions(+), 9 deletions(-)
> > >
> > > This seems like a pretty good candidate for breaking existing
> > > working binaries. How much testing against different varieties of
> > > guest-code-using-semihosting have you done ?
> >
> > None, which is why it's an RFC - but at least one user reported newlib
> > attempts to use the numbers we gave it rather than falling back to
> > numbers it knew from the build and getting it wrong. I suspect any code
> > that doesn't have a fallback path is getting it right more by luck than
> > judgement though. I'd be curious to hear of code that relies on the
> > numbers it gets from QEMU.
>
> Well, newlib is the main one I had in mind -- does it have a fallback
> codepath?
>
> -- PMM
>

[-- Attachment #2: Type: text/html, Size: 2717 bytes --]

Re: [RFC PATCH] semihosting/arm-compat: remove heuristic softmmu SYS_HEAPINFO

[Andrew Strauss]

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

Yeah, newlib falls back to using the symbols __stack and end to figure out
where to start the stack and heap if 0 is returned by SYS_HEAPINFO.

-- Andrew Strauss

On Tue, Jun 1, 2021 at 6:31 AM Peter Maydell <peter.maydell@linaro.org>
wrote:

> On Tue, 1 Jun 2021 at 11:07, Alex Bennée <alex.bennee@linaro.org> wrote:
> >
> >
> > Peter Maydell <peter.maydell@linaro.org> writes:
> >
> > > On Tue, 1 Jun 2021 at 10:12, Alex Bennée <alex.bennee@linaro.org>
> wrote:
> > >>
> > >> The previous numbers were a guess at best. While we could extract the
> > >> information from a loaded ELF file via -kernel we could still get
> > >> tripped up by self decompressing or relocating code. Besides sane
> > >> library code has access to the same symbols in run time to make a
> > >> determination of the location of the heap.
> > >>
> > >> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> > >> Cc: Andrew <astrauss11@gmail.com>
> > >> ---
> > >>  semihosting/arm-compat-semi.c | 19 ++++++++++---------
> > >>  1 file changed, 10 insertions(+), 9 deletions(-)
> > >
> > > This seems like a pretty good candidate for breaking existing
> > > working binaries. How much testing against different varieties of
> > > guest-code-using-semihosting have you done ?
> >
> > None, which is why it's an RFC - but at least one user reported newlib
> > attempts to use the numbers we gave it rather than falling back to
> > numbers it knew from the build and getting it wrong. I suspect any code
> > that doesn't have a fallback path is getting it right more by luck than
> > judgement though. I'd be curious to hear of code that relies on the
> > numbers it gets from QEMU.
>
> Well, newlib is the main one I had in mind -- does it have a fallback
> codepath?
>
> -- PMM
>

[-- Attachment #2: Type: text/html, Size: 2717 bytes --]

Re: [RFC PATCH] semihosting/arm-compat: remove heuristic softmmu SYS_HEAPINFO

[Andrew Strauss]

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

Tested-by: Andrew Strauss <astrauss11@gmail.com>
Reviewed-by: Andrew Strauss <astrauss11@gmail.com>

On Tue, Jun 1, 2021 at 5:07 AM Alex Bennée <alex.bennee@linaro.org> wrote:

> The previous numbers were a guess at best. While we could extract the
> information from a loaded ELF file via -kernel we could still get
> tripped up by self decompressing or relocating code. Besides sane
> library code has access to the same symbols in run time to make a
> determination of the location of the heap.
>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> Cc: Andrew <astrauss11@gmail.com>
> ---
>  semihosting/arm-compat-semi.c | 19 ++++++++++---------
>  1 file changed, 10 insertions(+), 9 deletions(-)
>
> diff --git a/semihosting/arm-compat-semi.c b/semihosting/arm-compat-semi.c
> index 1c29146dcf..041b4f6c04 100644
> --- a/semihosting/arm-compat-semi.c
> +++ b/semihosting/arm-compat-semi.c
> @@ -1165,12 +1165,10 @@ target_ulong do_common_semihosting(CPUState *cs)
>      case TARGET_SYS_HEAPINFO:
>          {
>              target_ulong retvals[4];
> -            target_ulong limit;
>              int i;
>  #ifdef CONFIG_USER_ONLY
> +            target_ulong limit;
>              TaskState *ts = cs->opaque;
> -#else
> -            target_ulong rambase = common_semi_rambase(cs);
>  #endif
>
>              GET_ARG(0);
> @@ -1201,12 +1199,15 @@ target_ulong do_common_semihosting(CPUState *cs)
>              retvals[2] = ts->stack_base;
>              retvals[3] = 0; /* Stack limit.  */
>  #else
> -            limit = current_machine->ram_size;
> -            /* TODO: Make this use the limit of the loaded application.
> */
> -            retvals[0] = rambase + limit / 2;
> -            retvals[1] = rambase + limit;
> -            retvals[2] = rambase + limit; /* Stack base */
> -            retvals[3] = rambase; /* Stack limit.  */
> +            /*
> +             * Reporting 0 indicates we couldn't calculate the real
> +             * values which should force most software to fall back to
> +             * using information it has.
> +             */
> +            retvals[0] = 0; /* Heap Base */
> +            retvals[1] = 0; /* Heap Limit */
> +            retvals[2] = 0; /* Stack base */
> +            retvals[3] = 0; /* Stack limit.  */
>  #endif
>
>              for (i = 0; i < ARRAY_SIZE(retvals); i++) {
> --
> 2.20.1
>
>

[-- Attachment #2: Type: text/html, Size: 3262 bytes --]

Re: [RFC PATCH] semihosting/arm-compat: remove heuristic softmmu SYS_HEAPINFO

[Andrew Strauss]

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

Tested-by: Andrew Strauss <astrauss11@gmail.com>
Reviewed-by: Andrew Strauss <astrauss11@gmail.com>

On Tue, Jun 1, 2021 at 5:07 AM Alex Bennée <alex.bennee@linaro.org> wrote:

> The previous numbers were a guess at best. While we could extract the
> information from a loaded ELF file via -kernel we could still get
> tripped up by self decompressing or relocating code. Besides sane
> library code has access to the same symbols in run time to make a
> determination of the location of the heap.
>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> Cc: Andrew <astrauss11@gmail.com>
> ---
>  semihosting/arm-compat-semi.c | 19 ++++++++++---------
>  1 file changed, 10 insertions(+), 9 deletions(-)
>
> diff --git a/semihosting/arm-compat-semi.c b/semihosting/arm-compat-semi.c
> index 1c29146dcf..041b4f6c04 100644
> --- a/semihosting/arm-compat-semi.c
> +++ b/semihosting/arm-compat-semi.c
> @@ -1165,12 +1165,10 @@ target_ulong do_common_semihosting(CPUState *cs)
>      case TARGET_SYS_HEAPINFO:
>          {
>              target_ulong retvals[4];
> -            target_ulong limit;
>              int i;
>  #ifdef CONFIG_USER_ONLY
> +            target_ulong limit;
>              TaskState *ts = cs->opaque;
> -#else
> -            target_ulong rambase = common_semi_rambase(cs);
>  #endif
>
>              GET_ARG(0);
> @@ -1201,12 +1199,15 @@ target_ulong do_common_semihosting(CPUState *cs)
>              retvals[2] = ts->stack_base;
>              retvals[3] = 0; /* Stack limit.  */
>  #else
> -            limit = current_machine->ram_size;
> -            /* TODO: Make this use the limit of the loaded application.
> */
> -            retvals[0] = rambase + limit / 2;
> -            retvals[1] = rambase + limit;
> -            retvals[2] = rambase + limit; /* Stack base */
> -            retvals[3] = rambase; /* Stack limit.  */
> +            /*
> +             * Reporting 0 indicates we couldn't calculate the real
> +             * values which should force most software to fall back to
> +             * using information it has.
> +             */
> +            retvals[0] = 0; /* Heap Base */
> +            retvals[1] = 0; /* Heap Limit */
> +            retvals[2] = 0; /* Stack base */
> +            retvals[3] = 0; /* Stack limit.  */
>  #endif
>
>              for (i = 0; i < ARRAY_SIZE(retvals); i++) {
> --
> 2.20.1
>
>

[-- Attachment #2: Type: text/html, Size: 3262 bytes --]