Re: [PATCH v2 09/74] semihosting: Return void from do_common_semihosting

[Peter Maydell]

On Tue, 3 May 2022 at 20:58, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> Perform the cleanup in the FIXME comment in common_semi_gdb_syscall.
> Do not modify guest registers until the syscall is complete,
> which in the gdbstub case is asynchronous.
>
> In the synchronous non-gdbstub case, use common_semi_set_ret
> to set the result.  Merge set_swi_errno into common_semi_cb.
> Rely on the latter for combined return value / errno setting.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  semihosting/common-semi.h     |   2 +-
>  linux-user/aarch64/cpu_loop.c |   2 +-
>  linux-user/arm/cpu_loop.c     |   2 +-
>  linux-user/riscv/cpu_loop.c   |   2 +-
>  semihosting/arm-compat-semi.c | 571 ++++++++++++++++------------------
>  target/arm/helper.c           |   4 +-
>  target/arm/m_helper.c         |   2 +-
>  target/riscv/cpu_helper.c     |   2 +-
>  8 files changed, 278 insertions(+), 309 deletions(-)

Oof, this is a big patch. I get that to some extent the
switch from "return the x0 value for callers to set" to
"leaf functions set x0, everything returns void" has to
be done as one patch. But for instance this bit:

> -static inline uint32_t set_swi_errno(CPUState *cs, uint32_t code)
> -{
> -    if (code == (uint32_t)-1) {
> -#ifdef CONFIG_USER_ONLY
> -        TaskState *ts = cs->opaque;
> -
> -        ts->swi_errno = errno;
> -#else
> -        syscall_err = errno;
> -#endif
> -    }
> -    return code;
> -}
> -
>  static inline uint32_t get_swi_errno(CPUState *cs)
>  {
>  #ifdef CONFIG_USER_ONLY
> @@ -290,28 +276,29 @@ static target_ulong common_semi_syscall_len;
>
>  static void common_semi_cb(CPUState *cs, target_ulong ret, target_ulong err)
>  {
> -    target_ulong reg0 = common_semi_arg(cs, 0);
> -
>      if (ret == (target_ulong)-1) {
> -        errno = err;
> -        set_swi_errno(cs, -1);
> -        reg0 = ret;
> +#ifdef CONFIG_USER_ONLY
> +        TaskState *ts = cs->opaque;
> +        ts->swi_errno = err;
> +#else
> +        syscall_err = err;
> +#endif
>      } else {
>          /* Fixup syscalls that use nonstardard return conventions.  */
> +        target_ulong reg0 = common_semi_arg(cs, 0);
>          switch (reg0) {
>          case TARGET_SYS_WRITE:
>          case TARGET_SYS_READ:
> -            reg0 = common_semi_syscall_len - ret;
> +            ret = common_semi_syscall_len - ret;
>              break;
>          case TARGET_SYS_SEEK:
> -            reg0 = 0;
> +            ret = 0;
>              break;
>          default:
> -            reg0 = ret;
>              break;
>          }
>      }
> -    common_semi_set_ret(cs, reg0);
> +    common_semi_set_ret(cs, ret);
>  }

seems like it's just inlining a function, renaming a variable,
moving the point where we initialize it around a little.
That could be its own patch, right?

thanks
-- PMM