Re: [PATCH v2 18/22] semihosting/arm-compat: Clean up local variable shadowing

[Philippe Mathieu-Daudé]

Hi Peter, Alex,

On 8/9/23 14:36, Peter Maydell wrote:
> On Mon, 4 Sept 2023 at 17:15, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
> > Fix:
> >
> >    semihosting/arm-compat-semi.c: In function ‘do_common_semihosting’:
> >    semihosting/arm-compat-semi.c:379:13: warning: declaration of ‘ret’ shadows 
> > a previous local [-Wshadow=local]
> >      379 |         int ret, err = 0;
> >          |             ^~~
> >    semihosting/arm-compat-semi.c:370:14: note: shadowed declaration is here
> >      370 |     uint32_t ret;
> >          |              ^~~
> >    semihosting/arm-compat-semi.c:682:27: warning: declaration of ‘ret’ shadows 
> > a previous local [-Wshadow=local]
> >      682 |                 abi_ulong ret;
> >          |                           ^~~
> >    semihosting/arm-compat-semi.c:370:9: note: shadowed declaration is here
> >      370 |     int ret;
> >          |         ^~~
> >
> > Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> > ---
> >   semihosting/arm-compat-semi.c | 9 +++------
> >   1 file changed, 3 insertions(+), 6 deletions(-)
>
> If I'm reading the code correctly, the top level 'ret' variable
> is only used by the SYS_EXIT case currently. So rather than
> changing the type of it I think it would be better to remove
> it and have a variable at tighter scope for SYS_EXIT. I
> think that's easier to read than this kind of "single variable
> used for multiple purposes at different places within a
> long function".

Yes you are right.

Now looking at it, we currently use a uint32_t type, but per
https://github.com/ARM-software/abi-aa/blob/main/semihosting/semihosting.rst#652entry-64-bit:

  In particular, if field 1 is ADP_Stopped_ApplicationExit then
  field 2 is an exit status code, as passed to the C standard library
  exit() function. A simulator receiving this request must notify a
  connected debugger, if present, and then exit with the specified
  status.

exit() is declared as:

LIBRARY
     Standard C Library (libc, -lc)

SYNOPSIS

     void
     exit(int status);

So it expects a signed status code, but we convert it to unsigned...
Alex, shouldn't we use a 'int' type here instead of 'uint32_t'?

> > diff --git a/semihosting/arm-compat-semi.c b/semihosting/arm-compat-semi.c
> > index 564fe17f75..85852a15b8 100644
> > --- a/semihosting/arm-compat-semi.c
> > +++ b/semihosting/arm-compat-semi.c
> > @@ -367,7 +367,7 @@ void do_common_semihosting(CPUState *cs)
> >       target_ulong ul_ret;
> >       char * s;
> >       int nr;
> > -    uint32_t ret;
> > +    int ret;
> >       int64_t elapsed;