From: Julian Seward Date: Mon, 19 Mar 2007 13:38:11 +0000 (+0000) Subject: Document and tidy up one of the more arcane corners of signal X-Git-Tag: svn/VALGRIND_3_3_0~316 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=664364751b17c4c6b10785ae861d578da0674b3d;p=thirdparty%2Fvalgrind.git Document and tidy up one of the more arcane corners of signal handling: why PRE(sys_sigreturn) has to construct a fake syscall return value which, when written back to the guest state, leaves it unchanged. It's only taken me about 3 years to realise why :-) Fixes to ppc platforms to follow. git-svn-id: svn://svn.valgrind.org/valgrind/trunk@6650 --- diff --git a/coregrind/m_syswrap/priv_types_n_macros.h b/coregrind/m_syswrap/priv_types_n_macros.h index 73f50a86ad..e5ce28e8ed 100644 --- a/coregrind/m_syswrap/priv_types_n_macros.h +++ b/coregrind/m_syswrap/priv_types_n_macros.h @@ -94,10 +94,11 @@ typedef SyscallArgLayout; /* Flags describing syscall wrappers */ -#define SfMayBlock (1 << 1) /* may block */ -#define SfPostOnFail (1 << 2) /* call POST() function on failure */ -#define SfPollAfter (1 << 3) /* poll for signals on completion */ -#define SfYieldAfter (1 << 4) /* yield on completion */ +#define SfMayBlock (1 << 1) /* may block */ +#define SfPostOnFail (1 << 2) /* call POST() function on failure */ +#define SfPollAfter (1 << 3) /* poll for signals on completion */ +#define SfYieldAfter (1 << 4) /* yield on completion */ +#define SfNoWriteResult (1 << 5) /* don't write result to guest state */ /* --------------------------------------------------------------------- @@ -312,13 +313,6 @@ static inline UWord getERR ( SyscallStatus* st ) { status->sres = (zzz); \ } while (0) -/* A lamentable kludge */ -#define SET_STATUS_Failure_NO_SANITY_CHECK(zzz) \ - do { Word wzz = (Word)(zzz); \ - status->what = SsFailure; \ - status->val = wzz; \ - } while (0) - #define PRINT(format, args...) \ if (VG_(clo_trace_syscalls)) \ diff --git a/coregrind/m_syswrap/syswrap-amd64-linux.c b/coregrind/m_syswrap/syswrap-amd64-linux.c index 3e3ad81d01..9f61aad08b 100644 --- a/coregrind/m_syswrap/syswrap-amd64-linux.c +++ b/coregrind/m_syswrap/syswrap-amd64-linux.c @@ -451,30 +451,41 @@ PRE(sys_clone) PRE(sys_rt_sigreturn) { + /* This isn't really a syscall at all - it's a misuse of the + syscall mechanism by m_sigframe. VG_(sigframe_create) sets the + return address of the signal frames it creates to be a short + piece of code which does this "syscall". The only purpose of + the syscall is to call VG_(sigframe_destroy), which restores the + thread's registers from the frame and then removes it. + Consequently we must ask the syswrap driver logic not to write + back the syscall "result" as that would overwrite the + just-restored register state. */ + ThreadState* tst; - PRINT("rt_sigreturn ( )"); + PRINT("sys_rt_sigreturn ( )"); vg_assert(VG_(is_valid_tid)(tid)); vg_assert(tid >= 1 && tid < VG_N_THREADS); vg_assert(VG_(is_running_thread)(tid)); - /* Adjust esp to point to start of frame; skip back up over handler + /* Adjust RSP to point to start of frame; skip back up over handler ret addr */ tst = VG_(get_ThreadState)(tid); tst->arch.vex.guest_RSP -= sizeof(Addr); /* This is only so that the RIP is (might be) useful to report if - something goes wrong in the sigreturn */ + something goes wrong in the sigreturn. JRS 20070318: no idea + what this is for */ ML_(fixup_guest_state_to_restart_syscall)(&tst->arch); + /* Restore register state from frame and remove it, as + described above */ VG_(sigframe_destroy)(tid, True); - /* For unclear reasons, it appears we need the syscall to return - without changing %RAX. Since %RAX is the return value, and can - denote either success or failure, we must set up so that the - driver logic copies it back unchanged. Also, note %RAX is of - the guest registers written by VG_(sigframe_destroy). */ - SET_STATUS_from_SysRes( VG_(mk_SysRes_amd64_linux)( tst->arch.vex.guest_RAX ) ); + /* Tell the driver not to update the guest state with the "result", + and set a bogus result to keep it happy. */ + *flags |= SfNoWriteResult; + SET_STATUS_Success(0); /* Check to see if some any signals arose as a result of this. */ *flags |= SfPollAfter; diff --git a/coregrind/m_syswrap/syswrap-main.c b/coregrind/m_syswrap/syswrap-main.c index 053660a848..21f3a35c53 100644 --- a/coregrind/m_syswrap/syswrap-main.c +++ b/coregrind/m_syswrap/syswrap-main.c @@ -858,12 +858,17 @@ void VG_(client_syscall) ( ThreadId tid ) if (sci->status.what == SsComplete && !sci->status.sres.isError) { /* The pre-handler completed the syscall itself, declaring success. */ - PRINT(" --> [pre-success] Success(0x%llx)\n", (ULong)sci->status.sres.res ); - + if (sci->flags & SfNoWriteResult) { + PRINT(" --> [pre-success] NoWriteResult\n"); + } else { + PRINT(" --> [pre-success] Success(0x%llx)\n", + (ULong)sci->status.sres.res ); + } /* In this case the allowable flags are to ask for a signal-poll and/or a yield after the call. Changing the args isn't allowed. */ - vg_assert(0 == (sci->flags & ~(SfPollAfter | SfYieldAfter))); + vg_assert(0 == (sci->flags + & ~(SfPollAfter | SfYieldAfter | SfNoWriteResult))); vg_assert(eq_SyscallArgs(&sci->args, &sci->orig_args)); } @@ -971,7 +976,8 @@ void VG_(client_syscall) ( ThreadId tid ) /* Dump the syscall result back in the guest state. This is a platform-specific action. */ - putSyscallStatusIntoGuestState( &sci->status, &tst->arch.vex ); + if (!(sci->flags & SfNoWriteResult)) + putSyscallStatusIntoGuestState( &sci->status, &tst->arch.vex ); /* Situation now: - the guest state is now correctly modified following the syscall @@ -1022,11 +1028,13 @@ void VG_(post_syscall) (ThreadId tid) /* Validate current syscallInfo entry. In particular we require that the current .status matches what's actually in the guest - state. */ + state. At least in the normal case where we have actually + previously written the result into the guest state. */ vg_assert(sci->status.what == SsComplete); getSyscallStatusFromGuestState( &test_status, &tst->arch.vex ); - vg_assert(eq_SyscallStatus( &sci->status, &test_status )); + if (!(sci->flags & SfNoWriteResult)) + vg_assert(eq_SyscallStatus( &sci->status, &test_status )); /* Ok, looks sane */ /* Get the system call number. Because the pre-handler isn't @@ -1061,7 +1069,8 @@ void VG_(post_syscall) (ThreadId tid) post-handler for sys_open can change the result from success to failure if the kernel supplied a fd that it doesn't like), once again dump the syscall result back in the guest state.*/ - putSyscallStatusIntoGuestState( &sci->status, &tst->arch.vex ); + if (!(sci->flags & SfNoWriteResult)) + putSyscallStatusIntoGuestState( &sci->status, &tst->arch.vex ); /* Do any post-syscall actions required by the tool. */ if (VG_(needs).syscall_wrapper) @@ -1314,7 +1323,8 @@ VG_(fixup_guest_state_after_syscall_interrupted)( ThreadId tid, canonical = convert_SysRes_to_SyscallStatus( VG_(mk_SysRes_Error)( VKI_EINTR ) ); - putSyscallStatusIntoGuestState( &canonical, &th_regs->vex ); + if (!(sci->flags & SfNoWriteResult)) + putSyscallStatusIntoGuestState( &canonical, &th_regs->vex ); sci->status = canonical; VG_(post_syscall)(tid); } @@ -1328,7 +1338,8 @@ VG_(fixup_guest_state_after_syscall_interrupted)( ThreadId tid, if (debug) VG_(printf)(" completed\n"); canonical = convert_SysRes_to_SyscallStatus( sres ); - putSyscallStatusIntoGuestState( &canonical, &th_regs->vex ); + if (!(sci->flags & SfNoWriteResult)) + putSyscallStatusIntoGuestState( &canonical, &th_regs->vex ); sci->status = canonical; VG_(post_syscall)(tid); } diff --git a/coregrind/m_syswrap/syswrap-x86-linux.c b/coregrind/m_syswrap/syswrap-x86-linux.c index 0ac6d38e7f..4c7258accc 100644 --- a/coregrind/m_syswrap/syswrap-x86-linux.c +++ b/coregrind/m_syswrap/syswrap-x86-linux.c @@ -935,8 +935,11 @@ PRE(sys_clone) PRE(sys_sigreturn) { + /* See comments on PRE(sys_rt_sigreturn) in syswrap-amd64-linux.c for + an explanation of what follows. */ + ThreadState* tst; - PRINT("sigreturn ( )"); + PRINT("sys_sigreturn ( )"); vg_assert(VG_(is_valid_tid)(tid)); vg_assert(tid >= 1 && tid < VG_N_THREADS); @@ -946,19 +949,19 @@ PRE(sys_sigreturn) sigreturn sequence's "popl %eax" and handler ret addr */ tst = VG_(get_ThreadState)(tid); tst->arch.vex.guest_ESP -= sizeof(Addr)+sizeof(Word); + /* XXX why does ESP change differ from rt_sigreturn case below? */ /* This is only so that the EIP is (might be) useful to report if something goes wrong in the sigreturn */ ML_(fixup_guest_state_to_restart_syscall)(&tst->arch); + /* Restore register state from frame and remove it */ VG_(sigframe_destroy)(tid, False); - /* For unclear reasons, it appears we need the syscall to return - without changing %EAX. Since %EAX is the return value, and can - denote either success or failure, we must set up so that the - driver logic copies it back unchanged. Also, note %EAX is of - the guest registers written by VG_(sigframe_destroy). */ - SET_STATUS_from_SysRes( VG_(mk_SysRes_x86_linux)( tst->arch.vex.guest_EAX ) ); + /* Tell the driver not to update the guest state with the "result", + and set a bogus result to keep it happy. */ + *flags |= SfNoWriteResult; + SET_STATUS_Success(0); /* Check to see if some any signals arose as a result of this. */ *flags |= SfPollAfter; @@ -966,8 +969,11 @@ PRE(sys_sigreturn) PRE(sys_rt_sigreturn) { + /* See comments on PRE(sys_rt_sigreturn) in syswrap-amd64-linux.c for + an explanation of what follows. */ + ThreadState* tst; - PRINT("rt_sigreturn ( )"); + PRINT("sys_rt_sigreturn ( )"); vg_assert(VG_(is_valid_tid)(tid)); vg_assert(tid >= 1 && tid < VG_N_THREADS); @@ -977,19 +983,19 @@ PRE(sys_rt_sigreturn) ret addr */ tst = VG_(get_ThreadState)(tid); tst->arch.vex.guest_ESP -= sizeof(Addr); + /* XXX why does ESP change differ from sigreturn case above? */ /* This is only so that the EIP is (might be) useful to report if something goes wrong in the sigreturn */ ML_(fixup_guest_state_to_restart_syscall)(&tst->arch); + /* Restore register state from frame and remove it */ VG_(sigframe_destroy)(tid, True); - /* For unclear reasons, it appears we need the syscall to return - without changing %EAX. Since %EAX is the return value, and can - denote either success or failure, we must set up so that the - driver logic copies it back unchanged. Also, note %EAX is of - the guest registers written by VG_(sigframe_destroy). */ - SET_STATUS_from_SysRes( VG_(mk_SysRes_x86_linux)( tst->arch.vex.guest_EAX ) ); + /* Tell the driver not to update the guest state with the "result", + and set a bogus result to keep it happy. */ + *flags |= SfNoWriteResult; + SET_STATUS_Success(0); /* Check to see if some any signals arose as a result of this. */ *flags |= SfPollAfter;