Refine semaphore for correctness

This commit improves robustness with better overflow handling and error
detection. The implementation maintains FIFO ordering and blocking
semantics while being more efficient in the common case and more robust
in error conditions.

Author: jserv

kernel/semaphore.c

@@ -12,6 +12,7 @@
 #include <sys/task.h>
 
 #include "private/error.h"
+#include "private/utils.h"
 
 /* Semaphore Control Block structure. */
 struct sem_t {
@@ -26,36 +27,47 @@ struct sem_t {
 
 static inline bool sem_is_valid(const sem_t *s)
 {
-    return (s && s->magic == SEM_MAGIC && s->wait_q);
+    return s && s->magic == SEM_MAGIC && s->wait_q && s->max_waiters > 0 &&
+           s->count >= 0 && s->count <= SEM_MAX_COUNT;
+}
+
+static inline void sem_invalidate(sem_t *s)
+{
+    if (s) {
+        s->magic = 0xDEADBEEF; /* Clear magic to prevent reuse */
+        s->count = -1;
+        s->max_waiters = 0;
+    }
 }
 
 sem_t *mo_sem_create(uint16_t max_waiters, int32_t initial_count)
 {
     /* Enhanced input validation */
-    if (!max_waiters || initial_count < 0 || initial_count > SEM_MAX_COUNT)
+    if (unlikely(!max_waiters || initial_count < 0 ||
+                 initial_count > SEM_MAX_COUNT))
         return NULL;
 
     sem_t *sem = malloc(sizeof(sem_t));
-    if (!sem)
+    if (unlikely(!sem))
         return NULL;
 
-    /* Initialize structure to known state */
+    /* Initialize structure to known safe state */
     sem->wait_q = NULL;
     sem->count = 0;
     sem->max_waiters = 0;
     sem->magic = 0;
 
     /* Create wait queue */
     sem->wait_q = queue_create(max_waiters);
-    if (!sem->wait_q) {
+    if (unlikely(!sem->wait_q)) {
         free(sem);
         return NULL;
     }
 
     /* Initialize remaining fields atomically */
     sem->count = initial_count;
     sem->max_waiters = max_waiters;
-    sem->magic = SEM_MAGIC; /* Mark as valid last */
+    sem->magic = SEM_MAGIC; /* Mark as valid last to prevent races */
 
     return sem;
 }
@@ -65,19 +77,19 @@ int32_t mo_sem_destroy(sem_t *s)
     if (!s)
         return ERR_OK; /* Destroying NULL is a no-op, not an error */
 
-    if (!sem_is_valid(s))
+    if (unlikely(!sem_is_valid(s)))
         return ERR_FAIL;
 
     NOSCHED_ENTER();
 
     /* Check if any tasks are waiting - unsafe to destroy if so */
-    if (queue_count(s->wait_q) > 0) {
+    if (unlikely(queue_count(s->wait_q) > 0)) {
         NOSCHED_LEAVE();
         return ERR_TASK_BUSY;
     }
 
-    /* Invalidate the semaphore to prevent further use */
-    s->magic = 0;
+    /* Atomically invalidate the semaphore to prevent further use */
+    sem_invalidate(s);
     queue_t *wait_q = s->wait_q;
     s->wait_q = NULL;
 
@@ -91,23 +103,23 @@ int32_t mo_sem_destroy(sem_t *s)
 
 void mo_sem_wait(sem_t *s)
 {
-    if (!sem_is_valid(s)) {
+    if (unlikely(!sem_is_valid(s))) {
         /* Invalid semaphore - this is a programming error */
         panic(ERR_SEM_OPERATION);
     }
 
     NOSCHED_ENTER();
 
-    /* Fast path: resource available and no waiters (preserves FIFO) */
-    if (s->count > 0 && queue_count(s->wait_q) == 0) {
+    /* Fast path: resource available and no waiters (preserves FIFO ordering) */
+    if (likely(s->count > 0 && queue_count(s->wait_q) == 0)) {
         s->count--;
         NOSCHED_LEAVE();
         return;
     }
 
     /* Slow path: must wait for resource */
-    /* Verify wait queue has capacity (should never fail for valid semaphore) */
-    if (queue_count(s->wait_q) >= s->max_waiters) {
+    /* Verify wait queue has capacity before attempting to block */
+    if (unlikely(queue_count(s->wait_q) >= s->max_waiters)) {
         NOSCHED_LEAVE();
         panic(ERR_SEM_OPERATION); /* Queue overflow - system error */
     }
@@ -120,22 +132,22 @@ void mo_sem_wait(sem_t *s)
      */
     _sched_block(s->wait_q);
 
-    /* When we return here, we have been awakened and have acquired the
-     * semaphore. The task that signaled us did NOT increment the count - the
-     * "token" was passed directly to us, so no further action is needed.
+    /* When we return here, we have been awakened and acquired the semaphore.
+     * The signaling task passed the "token" directly to us without incrementing
+     * the count, so no further action is needed.
      */
 }
 
 int32_t mo_sem_trywait(sem_t *s)
 {
-    if (!sem_is_valid(s))
+    if (unlikely(!sem_is_valid(s)))
         return ERR_FAIL;
 
     int32_t result = ERR_FAIL;
 
     NOSCHED_ENTER();
 
-    /* Only succeed if resource is available AND no waiters (preserves FIFO) */
+    /* Only succeed if resource available AND no waiters (preserves FIFO) */
     if (s->count > 0 && queue_count(s->wait_q) == 0) {
         s->count--;
         result = ERR_OK;
@@ -147,7 +159,7 @@ int32_t mo_sem_trywait(sem_t *s)
 
 void mo_sem_signal(sem_t *s)
 {
-    if (!sem_is_valid(s)) {
+    if (unlikely(!sem_is_valid(s))) {
         /* Invalid semaphore - this is a programming error */
         panic(ERR_SEM_OPERATION);
     }
@@ -157,13 +169,13 @@ void mo_sem_signal(sem_t *s)
 
     NOSCHED_ENTER();
 
-    /* Check if any tasks are waiting */
+    /* Check if any tasks are waiting for resources */
     if (queue_count(s->wait_q) > 0) {
         /* Wake up the oldest waiting task (FIFO order) */
         awakened_task = queue_dequeue(s->wait_q);
-        if (awakened_task) {
-            /* Validate the awakened task before changing its state */
-            if (awakened_task->state == TASK_BLOCKED) {
+        if (likely(awakened_task)) {
+            /* Validate awakened task state consistency */
+            if (likely(awakened_task->state == TASK_BLOCKED)) {
                 awakened_task->state = TASK_READY;
                 should_yield = true;
             } else {
@@ -172,39 +184,46 @@ void mo_sem_signal(sem_t *s)
             }
         }
         /* Note: count is NOT incremented - the "token" is passed directly to
-         * the awakened task to prevent race conditions.
+         * the awakened task to prevent race conditions where the count could
+         * be decremented by another task between our increment and the
+         * awakened task's execution.
          */
     } else {
-        /* No waiting tasks - increment available count */
-        if (s->count < SEM_MAX_COUNT)
+        /* No waiting tasks - increment available resource count */
+        if (likely(s->count < SEM_MAX_COUNT))
             s->count++;
-        /* Silently ignore overflow - semaphore remains at max count */
+
+        /* Silently ignore overflow - semaphore remains at max count.
+         * This prevents wraparound while maintaining system stability.
+         */
     }
 
     NOSCHED_LEAVE();
 
-    /* Yield outside critical section to allow awakened task to run.
-     * This improves responsiveness if the awakened task has higher priority.
+    /* Yield outside critical section if we awakened a task.
+     * This improves system responsiveness by allowing the awakened task to run
+     * immediately if it has higher priority.
      */
     if (should_yield)
         mo_task_yield();
 }
 
 int32_t mo_sem_getvalue(sem_t *s)
 {
-    if (!sem_is_valid(s))
+    if (unlikely(!sem_is_valid(s)))
         return -1;
 
-    /* This is inherently racy - the value may change immediately after being
-     * read. The volatile keyword ensures we read the current value, but does
-     * not provide atomicity across multiple operations.
+    /* This read is inherently racy - the value may change immediately after
+     * being read. The volatile keyword ensures we read the current value from
+     * memory, but does not provide atomicity across multiple operations.
+     * Callers should not rely on this value for synchronization decisions.
      */
     return s->count;
 }
 
 int32_t mo_sem_waiting_count(sem_t *s)
 {
-    if (!sem_is_valid(s))
+    if (unlikely(!sem_is_valid(s)))
         return -1;
 
     int32_t count;