[PATCHv2] sim: Don't overwrite stored errno in sim_syscall_multi

[Simon Marchi]

On 2018-12-12 18:41, Andrew Burgess wrote:
> I originally posted this patch back in February and figured it might
> be worth giving it a refresh.  Here's the original submission, it
> hasn't changed:
> 
>     https://sourceware.org/ml/gdb-patches/2018-02/msg00062.html
> 
> The sim/ maintainer has been rather absent of late, so I'm hoping
> someone else might approve this.
> 
> I've been using this patch with an out-of-tree RISC-V simulator for
> over a year now without any problems.
> 
> Thanks,
> Andrew
> 
> ---
> 
> The host syscall callback mechanism should take care of updating the
> errcode within the CB_SYSCALL struct, and we should not be adjusting
> the error code once the syscall has completed.  We especially, should
> not be rewriting the syscall errcode based on the value of errno some
> time after running the host syscall, as there is no guarantee that
> errno has not be overwritten.
> 
> To perform a syscall we call cb_syscall (in syscall.c).  To return
> from cb_syscall control passes through one of two exit paths these are
> labeled FinishSyscall and ErrorFinish and are reached using goto
> statements scattered throughout the cb_syscall function.
> 
> In FinishSyscall we store the syscall result in 'sc->result', and the
> error code is transated to target encoding, and stored in
> 'sc->errcode'.
> 
> In ErrorFinish, we again store the syscall result in 'sc->result', and
> fill in 'sc->errcode' by fetching the actual errno from the host with
> the 'cb->get_errno' callback.
> 
> In both cases 'sc->errcode' will have been filled in with an
> appropriate value.
> 
> Further, if we look at a specific syscall example, CB_SYS_open, in
> this case the first thing we do is fetch the path to open from the
> target with 'get_path', if this fails then the errcode is returned,
> and we jump to FinishSyscall.  Notice that in this case, no host
> syscall may have been performed, for example a failure to read the
> path to open out of simulated memory can return EINVAL without
> performing any host syscall.  Given that no host syscall has been
> performed, reading the host errno makes absolutely no sense.
> 
> This commit removes from sim_syscall_multi the rewriting of
> sc->errcode based on the value of errno, and instead relies on the
> value stored in the cb_syscall.
> 
> sim/common/ChangeLog:
> 
> 	* sim-syscall.c (sim_syscall_multi): Don't update sc->errcode at
> 	this point, it should have already been set in cb_syscall.
> ---
>  sim/common/ChangeLog     | 5 +++++
>  sim/common/sim-syscall.c | 5 -----
>  2 files changed, 5 insertions(+), 5 deletions(-)

Hi Andrew,

I took some time to familiarize myself with the issue, I think what you 
say make sense and that your patch is fine, please push.

I would have liked to try it, but none of the currently upstream arches 
that use sim_syscall_multi are easy for me to use (m32r, lm32, mn10300).

Simon