| Message ID | 20220521000400.454525-9-richard.henderson@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | semihosting cleanup | expand |
On Sat, 21 May 2022 at 01:04, Richard Henderson <richard.henderson@linaro.org> wrote: > > Do not store 'err' into errno only to read it back immediately. > Use 'ret' for the return value, not 'reg0'. > > Signed-off-by: Richard Henderson <richard.henderson@linaro.org> > --- > 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 c6bfd4d1ba..b00ed2c6d1 100644 > --- a/semihosting/arm-compat-semi.c > +++ b/semihosting/arm-compat-semi.c > @@ -290,28 +290,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); This should be "ret = ", right? (Otherwise I think this fails to compile. I assume that some later patch has this fix in it.) Otherwise Reviewed-by: Peter Maydell <peter.maydell@linaro.org> thanks -- PMM
On 5/23/22 03:58, Peter Maydell wrote: > On Sat, 21 May 2022 at 01:04, Richard Henderson > <richard.henderson@linaro.org> wrote: >> >> Do not store 'err' into errno only to read it back immediately. >> Use 'ret' for the return value, not 'reg0'. >> >> Signed-off-by: Richard Henderson <richard.henderson@linaro.org> >> --- >> 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 c6bfd4d1ba..b00ed2c6d1 100644 >> --- a/semihosting/arm-compat-semi.c >> +++ b/semihosting/arm-compat-semi.c >> @@ -290,28 +290,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); > > This should be "ret = ", right? (Otherwise I think this fails to > compile. I assume that some later patch has this fix in it.) Eh? No, we're extracting argument reg 0, and then switching on it. Why would it not compile -- I've moved the whole declaration down. r~ > > Otherwise > Reviewed-by: Peter Maydell <peter.maydell@linaro.org> > > thanks > -- PMM
On Mon, 23 May 2022 at 15:58, Richard Henderson <richard.henderson@linaro.org> wrote: > > On 5/23/22 03:58, Peter Maydell wrote: > > On Sat, 21 May 2022 at 01:04, Richard Henderson > > <richard.henderson@linaro.org> wrote: > >> > >> Do not store 'err' into errno only to read it back immediately. > >> Use 'ret' for the return value, not 'reg0'. > >> > >> Signed-off-by: Richard Henderson <richard.henderson@linaro.org> > >> --- > >> 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 c6bfd4d1ba..b00ed2c6d1 100644 > >> --- a/semihosting/arm-compat-semi.c > >> +++ b/semihosting/arm-compat-semi.c > >> @@ -290,28 +290,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); > > > > This should be "ret = ", right? (Otherwise I think this fails to > > compile. I assume that some later patch has this fix in it.) > > Eh? No, we're extracting argument reg 0, and then switching on it. > Why would it not compile -- I've moved the whole declaration down. Yes, you're right -- I misread the diff as doing a rename of 'reg0' to 'ret', but these are different variables and both present both before and after the change. -- PMM
diff --git a/semihosting/arm-compat-semi.c b/semihosting/arm-compat-semi.c index c6bfd4d1ba..b00ed2c6d1 100644 --- a/semihosting/arm-compat-semi.c +++ b/semihosting/arm-compat-semi.c @@ -290,28 +290,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); } static target_ulong common_semi_flen_buf(CPUState *cs)
Do not store 'err' into errno only to read it back immediately. Use 'ret' for the return value, not 'reg0'. Signed-off-by: Richard Henderson <richard.henderson@linaro.org> --- semihosting/arm-compat-semi.c | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-)