[PATCH] semihosting/arm-compat: Have TARGET_SYS_EXIT[_EXTENDED] return signed

[PATCH] semihosting/arm-compat: Have TARGET_SYS_EXIT[_EXTENDED] return signed

[Philippe Mathieu-Daudé]

Per the "Semihosting for AArch32 and AArch64" spec. v2 (2023Q3) [*]:

  6.5   SYS_EXIT (0x18)
  6.5.2   Entry (64-bit)

    On entry, the PARAMETER REGISTER contains a pointer to
    a two-field argument block:

    . field 1
      The exception type, which is one of the set of reason
      codes in the above tables.

    . field 2
      A subcode, whose meaning depends on the reason code in
      field 1.

    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. [...]

Having libc exit() is declared as:

  LIBRARY
       Standard C Library (libc, -lc)

  SYNOPSIS

       void
       exit(int status);

the status is expected to be signed.

[*] https://github.com/ARM-software/abi-aa/blob/2023q3-release/semihosting/semihosting.rst#652entry-64-bit

Fixes: 7446d35e1d ("target-arm/arm-semi.c: SYS_EXIT on A64 takes a parameter block")
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
Based-on: <20231004120019.93101-1-philmd@linaro.org>
---
 semihosting/arm-compat-semi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/semihosting/arm-compat-semi.c b/semihosting/arm-compat-semi.c
index 0033a1e018..c419d0c33a 100644
--- a/semihosting/arm-compat-semi.c
+++ b/semihosting/arm-compat-semi.c
@@ -725,7 +725,7 @@ void do_common_semihosting(CPUState *cs)
     case TARGET_SYS_EXIT:
     case TARGET_SYS_EXIT_EXTENDED:
     {
-        uint32_t ret;
+        int ret;
 
         if (common_semi_sys_exit_extended(cs, nr)) {
             /*
-- 
2.41.0

Re: [PATCH] semihosting/arm-compat: Have TARGET_SYS_EXIT[_EXTENDED] return signed

[Alex Bennée]

Philippe Mathieu-Daudé <philmd@linaro.org> writes:

> Per the "Semihosting for AArch32 and AArch64" spec. v2 (2023Q3) [*]:
>
>   6.5   SYS_EXIT (0x18)
>   6.5.2   Entry (64-bit)
>
>     On entry, the PARAMETER REGISTER contains a pointer to
>     a two-field argument block:
>
>     . field 1
>       The exception type, which is one of the set of reason
>       codes in the above tables.
>
>     . field 2
>       A subcode, whose meaning depends on the reason code in
>       field 1.
>
>     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. [...]
>
> Having libc exit() is declared as:
>
>   LIBRARY
>        Standard C Library (libc, -lc)
>
>   SYNOPSIS
>
>        void
>        exit(int status);
>
> the status is expected to be signed.
>
> [*] https://github.com/ARM-software/abi-aa/blob/2023q3-release/semihosting/semihosting.rst#652entry-64-bit
>
> Fixes: 7446d35e1d ("target-arm/arm-semi.c: SYS_EXIT on A64 takes a parameter block")
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Acked-by: Alex Bennée <alex.bennee@linaro.org>

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Re: [PATCH] semihosting/arm-compat: Have TARGET_SYS_EXIT[_EXTENDED] return signed

[Richard Henderson]

On 10/4/23 23:26, Philippe Mathieu-Daudé wrote:
> Per the "Semihosting for AArch32 and AArch64" spec. v2 (2023Q3) [*]:
> 
>    6.5   SYS_EXIT (0x18)
>    6.5.2   Entry (64-bit)
> 
>      On entry, the PARAMETER REGISTER contains a pointer to
>      a two-field argument block:
> 
>      . field 1
>        The exception type, which is one of the set of reason
>        codes in the above tables.
> 
>      . field 2
>        A subcode, whose meaning depends on the reason code in
>        field 1.
> 
>      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. [...]
> 
> Having libc exit() is declared as:
> 
>    LIBRARY
>         Standard C Library (libc, -lc)
> 
>    SYNOPSIS
> 
>         void
>         exit(int status);
> 
> the status is expected to be signed.

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

Re: [PATCH] semihosting/arm-compat: Have TARGET_SYS_EXIT[_EXTENDED] return signed

[Peter Maydell]

On Thu, 5 Oct 2023 at 07:27, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> Per the "Semihosting for AArch32 and AArch64" spec. v2 (2023Q3) [*]:
>
>   6.5   SYS_EXIT (0x18)
>   6.5.2   Entry (64-bit)
>
>     On entry, the PARAMETER REGISTER contains a pointer to
>     a two-field argument block:
>
>     . field 1
>       The exception type, which is one of the set of reason
>       codes in the above tables.
>
>     . field 2
>       A subcode, whose meaning depends on the reason code in
>       field 1.
>
>     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. [...]
>
> Having libc exit() is declared as:
>
>   LIBRARY
>        Standard C Library (libc, -lc)
>
>   SYNOPSIS
>
>        void
>        exit(int status);
>
> the status is expected to be signed.
>
> [*] https://github.com/ARM-software/abi-aa/blob/2023q3-release/semihosting/semihosting.rst#652entry-64-bit

Is this actually a visible change in behaviour? It makes
more sense to use 'int', I agree, but unless I'm confused
about C type conversions then I don't think it actually
changes the result in any case, does it?  Given we start with a
guest 64 or 32 bit signed integer value and put it into a
'target_ulong' (arg1), it doesn't seem to me to make a
difference whether we put it into a 'uint32_t' or an
'int' (ret) before passing it to either exit() or
gdb_exit() (which both take 'int')...

-- PMM

Re: [PATCH] semihosting/arm-compat: Have TARGET_SYS_EXIT[_EXTENDED] return signed

[Philippe Mathieu-Daudé]

Hi Peter,

On 16/10/23 18:08, Peter Maydell wrote:
> On Thu, 5 Oct 2023 at 07:27, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>>
>> Per the "Semihosting for AArch32 and AArch64" spec. v2 (2023Q3) [*]:
>>
>>    6.5   SYS_EXIT (0x18)
>>    6.5.2   Entry (64-bit)
>>
>>      On entry, the PARAMETER REGISTER contains a pointer to
>>      a two-field argument block:
>>
>>      . field 1
>>        The exception type, which is one of the set of reason
>>        codes in the above tables.
>>
>>      . field 2
>>        A subcode, whose meaning depends on the reason code in
>>        field 1.
>>
>>      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. [...]
>>
>> Having libc exit() is declared as:
>>
>>    LIBRARY
>>         Standard C Library (libc, -lc)
>>
>>    SYNOPSIS
>>
>>         void
>>         exit(int status);
>>
>> the status is expected to be signed.
>>
>> [*] https://github.com/ARM-software/abi-aa/blob/2023q3-release/semihosting/semihosting.rst#652entry-64-bit
> 
> Is this actually a visible change in behaviour? It makes
> more sense to use 'int', I agree, but unless I'm confused
> about C type conversions then I don't think it actually
> changes the result in any case, does it?  Given we start with a
> guest 64 or 32 bit signed integer value and put it into a
> 'target_ulong' (arg1), it doesn't seem to me to make a
> difference whether we put it into a 'uint32_t' or an
> 'int' (ret) before passing it to either exit() or
> gdb_exit() (which both take 'int')...

There should be no behavioral change, it is a cleanup
to avoid asking "why are we using a uint32_t here?" in
future reviews. Do you rather I mention it in the commit
description?

Regards,

Phil.

Re: [PATCH] semihosting/arm-compat: Have TARGET_SYS_EXIT[_EXTENDED] return signed

[Peter Maydell]

On Tue, 17 Oct 2023 at 12:32, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> Hi Peter,
>
> On 16/10/23 18:08, Peter Maydell wrote:
> > On Thu, 5 Oct 2023 at 07:27, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
> >>
> >> Per the "Semihosting for AArch32 and AArch64" spec. v2 (2023Q3) [*]:
> >>
> >>    6.5   SYS_EXIT (0x18)
> >>    6.5.2   Entry (64-bit)
> >>
> >>      On entry, the PARAMETER REGISTER contains a pointer to
> >>      a two-field argument block:
> >>
> >>      . field 1
> >>        The exception type, which is one of the set of reason
> >>        codes in the above tables.
> >>
> >>      . field 2
> >>        A subcode, whose meaning depends on the reason code in
> >>        field 1.
> >>
> >>      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. [...]
> >>
> >> Having libc exit() is declared as:
> >>
> >>    LIBRARY
> >>         Standard C Library (libc, -lc)
> >>
> >>    SYNOPSIS
> >>
> >>         void
> >>         exit(int status);
> >>
> >> the status is expected to be signed.
> >>
> >> [*] https://github.com/ARM-software/abi-aa/blob/2023q3-release/semihosting/semihosting.rst#652entry-64-bit
> >
> > Is this actually a visible change in behaviour? It makes
> > more sense to use 'int', I agree, but unless I'm confused
> > about C type conversions then I don't think it actually
> > changes the result in any case, does it?  Given we start with a
> > guest 64 or 32 bit signed integer value and put it into a
> > 'target_ulong' (arg1), it doesn't seem to me to make a
> > difference whether we put it into a 'uint32_t' or an
> > 'int' (ret) before passing it to either exit() or
> > gdb_exit() (which both take 'int')...
>
> There should be no behavioral change, it is a cleanup
> to avoid asking "why are we using a uint32_t here?" in
> future reviews. Do you rather I mention it in the commit
> description?

Yeah, I think it would be best to say specifically in
the commit message that it's not a behaviour change.
I tend to think that Fixes: tags imply that we're fixing
an actual problem in the original commit (and eg that
we might want to consider backporting the change), so I
would also drop that tag.

(I would have just fixed this up on applying this, but this
patch depends on some other one that isn't upstream yet.)

thanks
-- PMM