| Message ID | 20231005062610.57351-1-philmd@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | semihosting/arm-compat: Have TARGET_SYS_EXIT[_EXTENDED] return signed | expand |
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>
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~
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
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.
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
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)) { /*
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(-)