Fix "346411 MIPS: SysRes::_valEx handling is incorrect"

Specialise type SysRes for mips{32,64}-linux to enable 
meaningful equality comparisons.


git-svn-id: svn://svn.valgrind.org/valgrind/trunk@15404

coregrind/m_libcproc.c

@@ -918,7 +918,7 @@ void VG_(invalidate_icache) ( void *ptr, SizeT nbytes )
 #  elif defined(VGA_mips32) || defined(VGA_mips64)
    SysRes sres = VG_(do_syscall3)(__NR_cacheflush, (UWord) ptr,
                                  (UWord) nbytes, (UWord) 3);
-   vg_assert( sres._isError == 0 );
+   vg_assert( !sr_isError(sres) );
 
 #  elif defined(VGA_tilegx)
    const HChar *start, *end;

coregrind/m_scheduler/ticket-lock-linux.c

@@ -135,8 +135,8 @@ static void acquire_sched_lock(struct sched_lock *p)
       sres = VG_(do_syscall3)(__NR_futex, (UWord)futex,
                               VKI_FUTEX_WAIT | VKI_FUTEX_PRIVATE_FLAG,
                               futex_value);
-      if (sr_isError(sres) && sres._val != VKI_EAGAIN) {
-         VG_(printf)("futex_wait() returned error code %ld\n", sres._val);
+      if (sr_isError(sres) && sr_Err(sres) != VKI_EAGAIN) {
+         VG_(printf)("futex_wait() returned error code %ld\n", sr_Err(sres));
          vg_assert(False);
       }
    }

coregrind/m_syscall.c

@@ -38,11 +38,59 @@
    Building syscall return values.
    ------------------------------------------------------------------ */
 
-#if defined(VGO_linux)
-
 /* Make a SysRes value from a syscall return value.  This is
-   Linux-specific.
+   platform specific. */
 
+#if defined(VGP_mips32_linux) || defined(VGP_mips64_linux)
+
+SysRes VG_(mk_SysRes_mips32_linux) ( UWord v0, UWord v1, UWord a3 ) {
+   /* MIPS uses a3 != 0 to flag an error */
+   SysRes res;
+   res._isError = (a3 != (UWord)0);
+   res._val     = v0;
+   res._valEx   = v1;
+   return res;
+}
+
+SysRes VG_(mk_SysRes_mips64_linux) ( ULong v0, ULong v1, ULong a3 ) {
+   /* MIPS uses a3 != 0 to flag an error */
+   SysRes res;
+   res._isError = (a3 != (ULong)0);
+   res._val     = v0;
+   res._valEx   = v1;
+   return res;
+}
+
+/* Generic constructors. */
+SysRes VG_(mk_SysRes_Error) ( UWord err ) {
+   SysRes r;
+   r._isError = True;
+   r._val     = err;
+   r._valEx   = 0;
+   return r;
+}
+
+SysRes VG_(mk_SysRes_Success) ( UWord res ) {
+   SysRes r;
+   r._isError = False;
+   r._val     = res;
+   r._valEx   = 0;
+   return r;
+}
+
+SysRes VG_(mk_SysRes_SuccessEx) ( UWord res, UWord resEx ) {
+   SysRes r;
+   r._isError = False;
+   r._val     = res;
+   r._valEx   = resEx;
+   return r;
+}
+
+
+#elif defined(VGO_linux) \
+      && !defined(VGP_mips32_linux) && !defined(VGP_mips64_linux)
+
+/*
    From:
    http://sources.redhat.com/cgi-bin/cvsweb.cgi/libc/sysdeps/unix/sysv/
    linux/i386/sysdep.h?
@@ -144,32 +192,9 @@ SysRes VG_(mk_SysRes_arm64_linux) ( Long val ) {
    return res;
 }
 
-#if defined(VGA_mips64) || defined(VGA_mips32)
-/* MIPS uses a3 != 0 to flag an error */
-SysRes VG_(mk_SysRes_mips32_linux) ( UWord v0, UWord v1, UWord a3 ) {
-   SysRes res;
-   res._isError = (a3 != (UWord)0);
-   res._val     = v0;
-   res._valEx   = v1;
-   return res;
-}
-
-/* MIPS uses a3 != 0 to flag an error */
-SysRes VG_(mk_SysRes_mips64_linux) ( ULong v0, ULong v1, ULong a3 ) {
-   SysRes res;
-   res._isError = (a3 != (ULong)0);
-   res._val     = v0;
-   res._valEx   = v1;
-   return res;
-}
-#endif
-
 /* Generic constructors. */
 SysRes VG_(mk_SysRes_Error) ( UWord err ) {
    SysRes r;
-#if defined(VGA_mips64) || defined(VGA_mips32)
-   r._valEx   = 0;
-#endif
    r._isError = True;
    r._val     = err;
    return r;
@@ -177,9 +202,6 @@ SysRes VG_(mk_SysRes_Error) ( UWord err ) {
 
 SysRes VG_(mk_SysRes_Success) ( UWord res ) {
    SysRes r;
-#if defined(VGA_mips64) || defined(VGA_mips32)
-   r._valEx   = 0;
-#endif
    r._isError = False;
    r._val     = res;
    return r;

coregrind/m_syswrap/syswrap-main.c

@@ -369,10 +369,10 @@ Bool eq_SyscallArgs ( SyscallArgs* a1, SyscallArgs* a2 )
 }
 
 static
-Bool eq_SyscallStatus ( SyscallStatus* s1, SyscallStatus* s2 )
+Bool eq_SyscallStatus ( UInt sysno, SyscallStatus* s1, SyscallStatus* s2 )
 {
    /* was: return s1->what == s2->what && sr_EQ( s1->sres, s2->sres ); */
-   if (s1->what == s2->what && sr_EQ( s1->sres, s2->sres ))
+   if (s1->what == s2->what && sr_EQ( sysno, s1->sres, s2->sres ))
       return True;
 #  if defined(VGO_darwin)
    /* Darwin-specific debugging guff */
@@ -1858,9 +1858,15 @@ void VG_(post_syscall) (ThreadId tid)
       previously written the result into the guest state. */
    vg_assert(sci->status.what == SsComplete);
 
+   /* Get the system call number.  Because the pre-handler isn't
+      allowed to mess with it, it should be the same for both the
+      original and potentially-modified args. */
+   vg_assert(sci->args.sysno == sci->orig_args.sysno);
+   sysno = sci->args.sysno;
+
    getSyscallStatusFromGuestState( &test_status, &tst->arch.vex );
    if (!(sci->flags & SfNoWriteResult))
-      vg_assert(eq_SyscallStatus( &sci->status, &test_status ));
+      vg_assert(eq_SyscallStatus( sysno, &sci->status, &test_status ));
    /* Failure of the above assertion on Darwin can indicate a problem
       in the syscall wrappers that pre-fail or pre-succeed the
       syscall, by calling SET_STATUS_Success or SET_STATUS_Failure,
@@ -1872,18 +1878,12 @@ void VG_(post_syscall) (ThreadId tid)
       comment is completely irrelevant. */
    /* Ok, looks sane */
 
-   /* Get the system call number.  Because the pre-handler isn't
-      allowed to mess with it, it should be the same for both the
-      original and potentially-modified args. */
-   vg_assert(sci->args.sysno == sci->orig_args.sysno);
-   sysno = sci->args.sysno;
-   ent = get_syscall_entry(sysno);
-
    /* pre: status == Complete (asserted above) */
    /* Consider either success or failure.  Now run the post handler if:
       - it exists, and
       - Success or (Failure and PostOnFail is set)
    */
+   ent = get_syscall_entry(sysno);
    if (ent->after
        && ((!sr_isError(sci->status.sres))
            || (sr_isError(sci->status.sres)

coregrind/m_vkiscnums.c

@@ -52,6 +52,19 @@ const HChar* VG_(sysnum_string)(Word sysnum)
    return buf;
 }
 
+/* include/pub_tool_basics.h hardcodes the following syscall numbers
+   on mips{32,64}-linux so as to avoid a module cycle.  We make that
+   safe here by causing the build to fail if those numbers should ever
+   change.  See comments in function sr_EQ in the mips{32,64}-linux
+   section of include/pub_tool_basics.h for more details. */
+#if defined(VGP_mips32_linux)
+STATIC_ASSERT(__NR_pipe  == 4042);
+STATIC_ASSERT(__NR_pipe2 == 4328);
+#elsif defined(VGP_mips64_linux)
+STATIC_ASSERT(__NR_pipe  == 5021);
+STATIC_ASSERT(__NR_pipe2 == 5287);
+#endif
+
 //---------------------------------------------------------------------------
 #elif defined(VGO_darwin)
 //---------------------------------------------------------------------------

coregrind/pub_core_syscall.h

@@ -90,6 +90,11 @@ extern SysRes VG_(mk_SysRes_tilegx_linux)( Long val );
 extern SysRes VG_(mk_SysRes_Error)       ( UWord val );
 extern SysRes VG_(mk_SysRes_Success)     ( UWord val );
 
+#if defined(VGP_mips32_linux) || defined(VGP_mips64_linux)
+/* On Linux/MIPS, VG_(mk_SysRes_Success) sets the second result word
+   to zero.  Here is a version that allows setting both values. */
+extern SysRes VG_(mk_SysRes_SuccessEx)   ( UWord val, UWord valEx );
+#endif
 
 
 /* Return a string which gives the name of an error value.  Note,

include/pub_tool_basics.h

@@ -130,11 +130,19 @@ typedef  struct { UWord uw1; UWord uw2; }  UWordPair;
 typedef UInt ThreadId;
 
 /* An abstraction of syscall return values.
-   Linux:
+   Linux/MIPS32 and Linux/MIPS64:
+      When _isError == False, 
+         _val and possible _valEx hold the return value.  Whether
+         _valEx actually holds a valid value depends on which syscall
+         this SysRes holds of the result of.
+      When _isError == True,  
+         _val holds the error code.
+
+   Linux/other:
       When _isError == False, 
          _val holds the return value.
       When _isError == True,  
-         _err holds the error code.
+         _val holds the error code.
 
    Darwin:
       Interpretation depends on _mode:
@@ -150,16 +158,24 @@ typedef UInt ThreadId;
          update both {R,E}DX and {R,E}AX (in guest state) given a SysRes,
          if we're required to.
 */
-#if defined(VGO_linux)
+#if defined(VGP_mips32_linux) || defined(VGP_mips64_linux)
 typedef
    struct {
       Bool  _isError;
       UWord _val;
-#if defined(VGA_mips64) || defined(VGA_mips32)
       UWord _valEx;
-#endif
    }
    SysRes;
+
+#elif defined(VGO_linux) \
+      && !defined(VGP_mips32_linux) && !defined(VGP_mips64_linux)
+typedef
+   struct {
+      Bool  _isError;
+      UWord _val;
+   }
+   SysRes;
+
 #elif defined(VGO_darwin)
 typedef
    enum { 
@@ -176,6 +192,7 @@ typedef
       SysResMode _mode;
    }
    SysRes;
+
 #else
 #  error "Unknown OS"
 #endif
@@ -183,7 +200,7 @@ typedef
 
 /* ---- And now some basic accessor functions for it. ---- */
 
-#if defined(VGO_linux)
+#if defined(VGP_mips32_linux) || defined(VGP_mips64_linux)
 
 static inline Bool sr_isError ( SysRes sr ) {
    return sr._isError;
@@ -191,18 +208,52 @@ static inline Bool sr_isError ( SysRes sr ) {
 static inline UWord sr_Res ( SysRes sr ) {
    return sr._isError ? 0 : sr._val;
 }
-#if defined(VGA_mips64) || defined(VGA_mips32)
 static inline UWord sr_ResEx ( SysRes sr ) {
    return sr._isError ? 0 : sr._valEx;
 }
-#endif
 static inline UWord sr_Err ( SysRes sr ) {
    return sr._isError ? sr._val : 0;
 }
-// FIXME: this function needs to be fixed for MIPS
-static inline Bool sr_EQ ( SysRes sr1, SysRes sr2 ) {
+static inline Bool sr_EQ ( UInt sysno, SysRes sr1, SysRes sr2 ) {
+   /* This uglyness of hardcoding syscall numbers is necessary to
+      avoid having this header file be dependant on
+      include/vki/vki-scnums-mips{32,64}-linux.h.  It seems pretty
+      safe given that it is inconceivable that the syscall numbers
+      for such simple syscalls would ever change.  To make it 
+      really safe, coregrind/m_vkiscnums.c static-asserts that these
+      syscall numbers haven't changed, so that the build wil simply
+      fail if they ever do. */
+#  if defined(VGP_mips32_linux)
+   const UInt __nr_Linux = 4000;
+   const UInt __nr_pipe  = __nr_Linux + 42;
+   const UInt __nr_pipe2 = __nr_Linux + 328;
+#  else
+   const UInt __nr_Linux = 5000;
+   const UInt __nr_pipe  = __nr_Linux + 21;
+   const UInt __nr_pipe2 = __nr_Linux + 287;
+#  endif
+   Bool useEx = sysno == __nr_pipe || sysno == __nr_pipe2;
    return sr1._val == sr2._val
-       && sr1._isError == sr2._isError;
+          && (useEx ? (sr1._valEx == sr2._valEx) : True)
+          && sr1._isError == sr2._isError;
+}
+
+#elif defined(VGO_linux) \
+      && !defined(VGP_mips32_linux) && !defined(VGP_mips64_linux)
+
+static inline Bool sr_isError ( SysRes sr ) {
+   return sr._isError;
+}
+static inline UWord sr_Res ( SysRes sr ) {
+   return sr._isError ? 0 : sr._val;
+}
+static inline UWord sr_Err ( SysRes sr ) {
+   return sr._isError ? sr._val : 0;
+}
+static inline Bool sr_EQ ( UInt sysno, SysRes sr1, SysRes sr2 ) {
+   /* sysno is ignored for Linux/not-MIPS */
+   return sr1._val == sr2._val
+          && sr1._isError == sr2._isError;
 }
 
 #elif defined(VGO_darwin)
@@ -259,7 +310,8 @@ static inline UWord sr_Err ( SysRes sr ) {
    }
 }
 
-static inline Bool sr_EQ ( SysRes sr1, SysRes sr2 ) {
+static inline Bool sr_EQ ( UInt sysno, SysRes sr1, SysRes sr2 ) {
+   /* sysno is ignored for Darwin */
    return sr1._mode == sr2._mode
           && sr1._wLO == sr2._wLO && sr1._wHI == sr2._wHI;
 }