]> git.ipfire.org Git - thirdparty/valgrind.git/commitdiff
Document and tidy up one of the more arcane corners of signal
authorJulian Seward <jseward@acm.org>
Mon, 19 Mar 2007 13:38:11 +0000 (13:38 +0000)
committerJulian Seward <jseward@acm.org>
Mon, 19 Mar 2007 13:38:11 +0000 (13:38 +0000)
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

coregrind/m_syswrap/priv_types_n_macros.h
coregrind/m_syswrap/syswrap-amd64-linux.c
coregrind/m_syswrap/syswrap-main.c
coregrind/m_syswrap/syswrap-x86-linux.c

index 73f50a86adab743b1c510fba73e9620d56d571e1..e5ce28e8eda836ce71c652ff27a77993de7ba1d1 100644 (file)
@@ -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))                      \
index 3e3ad81d016a0b3f9be952bacfb8ac7453e5e513..9f61aad08b23acf23b68cfde10a347ec390e09e4 100644 (file)
@@ -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;
index 053660a848e37343bc54fcf76642dfc17253ca75..21f3a35c5360d0d89377cb900969f05754450429 100644 (file)
@@ -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);
    } 
index 0ac6d38e7ffc7ec153e0645b9cb36c7a48140cea..4c7258acccf18089d0e772f4c51db03d3794b3c4 100644 (file)
@@ -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;