Patchwork sim: Don't overwrite stored errno in sim_syscall_multi

login
register
mail settings
Submitter Andrew Burgess
Date Feb. 5, 2018, 11:57 a.m.
Message ID <20180205115701.21260-1-andrew.burgess@embecosm.com>
Download mbox | patch
Permalink /patch/25793/
State New
Headers show

Comments

Andrew Burgess - Feb. 5, 2018, 11:57 a.m.
The patch below hasn't been tested extensively, and not with any of
the currently in tree simulators.  I ran into an issue while working
with an out of tree simulator where I found that in cases where
syscalls were failing, the wrong errno was being reported.

I tracked the issue down to what I think is a generic issue in the
simulator (which is described in the commit message for the change
below).

I've posted the patch at this stage, as it might be useful to others.

To move this forward, is there a specific set of simulator(s) that I
should build against to test this change?

My concern is that the code I'm removing might have originally been
put in place because, somewhere, there is a code path where the
syscall errno is not being stored correctly, and so catching the errno
at a late phase was "fixing" this issue.

If anyone wanted to merge this and try it against their simulators and
report your test results that would be great.

All feedback and suggestions welcome.

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.

Lets consider why reading errno in sim_syscall_multi is a bad idea.
To perform a syscall we call cb_syscall (in syscall.c), there are two
exit paths from this function, these are called FinishSyscall and
ErrorFinish.

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' should have been filled in with an
appropriate value.

However, lets consider a specific syscall 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 call
FinishSyscall.  Notice that in this case, no host syscall may have
been performed, and so reading errno makes absolutely no sense.

This commit removes from sim_syscall_multi the rewriting of
sc->errcode based on the value of errno.

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(-)

Patch

diff --git a/sim/common/sim-syscall.c b/sim/common/sim-syscall.c
index 7923160cb7f..34d2f0ec115 100644
--- a/sim/common/sim-syscall.c
+++ b/sim/common/sim-syscall.c
@@ -97,11 +97,6 @@  sim_syscall_multi (SIM_CPU *cpu, int func, long arg1, long arg2, long arg3,
 
   if (cb_target_to_host_syscall (cb, func) == CB_SYS_exit)
     sim_engine_halt (sd, cpu, NULL, sim_pc_get (cpu), sim_exited, arg1);
-  else if (sc.result == -1)
-    {
-      cb->last_errno = errno;
-      sc.errcode = cb->get_errno (cb);
-    }
 
   *result = sc.result;
   *result2 = sc.result2;