libpthread: Implement sem_trywait() and sem_getvalue()

src/untrusted/pthread/nc_semaphore.c

 /*
  * Copyright (c) 2011 The Native Client Authors. All rights reserved.
  * Use of this source code is governed by a BSD-style license that can be
  * found in the LICENSE file.
  */
 
 /*
 * Native Client semaphore API
 */
 
@@ skipping 85 matching lines @@
     return 0;
 
   __sync_fetch_and_add(&sem->nwaiters, 1);
   do {
     __nc_irt_futex.futex_wait_abs(&sem->count, 0, NULL);
   } while (!decrement_if_positive(&sem->count));
   __sync_fetch_and_sub(&sem->nwaiters, 1);
   return 0;
 }
 
+int sem_trywait(sem_t *sem) {
+  if (NULL == sem) {

[Roland McGrath, 2013/07/12 21:15:58]

Failing gracefully rather than crashing when passed a bogus sem_t is arguably an
anti-feature.  POSIX makes EINVAL for this case a "may fail" error (vs "shall
fail"), so no conforming application is entitled to rely on it.  Passing a null
pointer here is undefined behavior.  The existing functions diagnosed EINVAL
already and that was historical behavior and so I didn't argue for their
replacement implementations behaving differently.

But these are new functions and there is no possibility of preexisting
applications being broken by a recompile.  So I don't think we should diagnose
EINVAL for null pointers.

Note that glibc doesn't diagnose EINVAL for these cases; it will just crash.

[Mark Seaborn, 2013/07/15 15:51:21]

OK, I've removed the NULL checks.

The NULL check was to be consistent with the existing code, but I've removed
those other NULL checks in a separate change
(https://codereview.chromium.org/18711004/).

+    errno = EINVAL;
+    return -1;
+  }
+
+  if (decrement_if_positive(&sem->count))
+    return 0;
+
+  errno = EAGAIN;
+  return -1;
+}
+
 int sem_post(sem_t *sem) {
   if (NULL == sem) {
     errno = EINVAL;
     return -1;
   }
 
   /* Increment sem->count, checking for overflow. */
   int old_value;
   do {
     old_value = sem->count;
     if (old_value == SEM_VALUE_MAX) {
       errno = EOVERFLOW;
       return -1;
     }
   } while (!__sync_bool_compare_and_swap(&sem->count, old_value,
                                          old_value + 1));
 
   /*
    * We only need to call futex_wake() if there are waiters.  Note
    * that this might do an unnecessary call to futex_wake() if the
    * last waiter has already been woken using futex_wake() but has not
    * yet decremented sem->nwaiters.
    */
   if (sem->nwaiters != 0) {
     int woken_count;
     __nc_irt_futex.futex_wake(&sem->count, 1, &woken_count);
   }
   return 0;
 }
+
+int sem_getvalue(sem_t *sem, int *value) {
+  if (NULL == sem || NULL == value) {

[Roland McGrath, 2013/07/12 21:15:58]

See above about checking |sem| being a null pointer.
There is no excuse whatsoever for checking |value|.
Passing a null pointer is undefined behavior and the best thing to do is crash
(which doesn't require any extra effort to ensure).

[Mark Seaborn, 2013/07/15 15:51:21]

Done.

+    errno = EINVAL;
+    return -1;
+  }
+
+  *value = sem->count;
+  return 0;
+}