Browse Source

check return codes in semaphore implementation

* note a few other definite and possible bugs
* turn off PAUSE feature, which mostly serves to hide
  error messages in debug spew
Don Porter 7 years ago
parent
commit
9882cad859

+ 1 - 1
LibOS/shim/include/shim_internal.h

@@ -133,7 +133,7 @@ static inline PAL_HANDLE __open_shim_stdio (void)
 int shim_terminate (void);
 
 /* assertions */
-#define USE_PAUSE       1
+#define USE_PAUSE       0
 #define USE_ASSERT      1
 
 static inline void do_pause (void);

+ 3 - 0
LibOS/shim/src/sys/shim_futex.c

@@ -159,6 +159,9 @@ int shim_do_futex (unsigned int * uaddr, int op, int val, void * utime,
                 if (list_empty(&futex->waiters))
                     break;
 
+                // BUG: if the first entry in the list isn't eligible, do we
+                // ever wake anything up? doesn't this check the first entry
+                // over and over?
                 struct futex_waiter * waiter = list_entry(futex->waiters.next,
                                                           struct futex_waiter,
                                                           list);

+ 1 - 0
Pal/src/db_object.c

@@ -100,6 +100,7 @@ DkObjectsWaitAny (PAL_NUM count, PAL_HANDLE * handleArray, PAL_NUM timeout)
     }
 
     for (int i = 0 ; i < count ; i++)
+        // We modify the caller's handleArray?
         if (UNKNOWN_HANDLE(handleArray[i]))
             handleArray[i] = NULL;
 

+ 6 - 2
Pal/src/host/Linux-SGX/db_mutex.c

@@ -115,7 +115,11 @@ int _DkMutexLock (struct mutex_handle * m)
         cpu_relax();
     }
 
+    // Mutex is union of u8 array and u32; this assumes a little-endian machine.
     while (xchg(&m->u, 257) & 1) {
+        // This is broken. The mutex is in enclave memory, the URTS can't
+        // do FUTEX_WAIT on it. This call will always fail and the next level
+        // up needs to retry.
         ret = ocall_futex((int *) m, FUTEX_WAIT, 257, NULL);
         if (ret < 0 &&
             ret != -PAL_ERROR_TRYAGAIN) {
@@ -152,7 +156,7 @@ int _DkMutexUnlock (struct mutex_handle * m)
     m->b.locked = 0;
     barrier();
 
-    /* Spin and try to take lock */
+    /* See if somebody else takes the lock */
     for (i = 0; i < MUTEX_SPINLOCK_TIMES * 2; i++) {
         if (m->b.locked)
             goto success;
@@ -161,7 +165,7 @@ int _DkMutexUnlock (struct mutex_handle * m)
 
     m->b.contended = 0;
 
-    /* We need to wake someone up */
+    /* Nobody took it, we need to wake someone up */
     ocall_futex((int *) m, FUTEX_WAKE, 1, NULL);
 
 success:

+ 6 - 5
Pal/src/host/Linux-SGX/db_semaphore.c

@@ -88,8 +88,7 @@ int _DkSemaphoreAcquire (PAL_HANDLE sem, int count)
     /* optimization: use it as a mutex */
     if (sem->semaphore.max_value == 1) {
         struct mutex_handle * mut = &sem->semaphore.value.mut;
-        _DkMutexLock(mut);
-        return 0;
+        return _DkMutexLock(mut);
     }
 
     if (count > sem->semaphore.max_value)
@@ -156,8 +155,7 @@ int _DkSemaphoreAcquireTimeout (PAL_HANDLE sem, int count, int timeout)
     /* optimization: use it as a mutex */
     if (sem->semaphore.max_value == 1) {
         struct mutex_handle * mut = & sem->semaphore.value.mut;
-        _DkMutexLockTimeout(mut, timeout);
-        return 0;
+        return _DkMutexLockTimeout(mut, timeout);
     }
 
     if (count > sem->semaphore.max_value)
@@ -184,6 +182,7 @@ int _DkSemaphoreAcquireTimeout (PAL_HANDLE sem, int count, int timeout)
         atomic_add (count, (struct atomic_int *) value);
 
     if (!timeout)
+        // BUG: Really? shouldn't we fail if we didn't get the lock?
         return 0;
 
     unsigned long waittime = timeout;
@@ -227,7 +226,9 @@ void _DkSemaphoreRelease (PAL_HANDLE sem, int count)
         struct mutex_handle * mut =
             &sem->semaphore.value.mut;
 
-        _DkMutexUnlock(mut);
+        int ret = _DkMutexUnlock(mut);
+        if (ret < 0)
+            _DkRaiseFailure(ret);
         return;
     }