[PAL] Rework PAL API value and error returning interface

The old version used special function `DkRaiseFailure` (which in turn called LibOS callback, which saved the error code) to return errors, values were returned directly from functions. This was annoying to deal with, as some functions did not return any value, yet they still used `DkRaiseFailure` to indicate errors. Moreover it abused exception handling interface, to callback into LibOS. The new version uniformly returns values via out-arguments (pointer passed by the caller, where the return value is stored). Errors are returned from functions directly, with `0` indicating successful call. This commit additionally fixes multiple bugs around error code checking. Signed-off-by: Borys Popławski <[email protected]>
24minFan · Feb 25, 2021 · 7f186ba · 7f186ba
1 parent 9e0ac10
commit 7f186ba
Show file tree

Hide file tree

Showing 109 changed files with 1,783 additions and 1,654 deletions.
diff --git a/Documentation/pal/host-abi.rst b/Documentation/pal/host-abi.rst
@@ -248,9 +248,6 @@ applications.
    :project: pal
 
 
-.. doxygendefine:: PAL_STREAM_ERROR
-   :project: pal
-
 Flags used for stream manipulation
 """"""""""""""""""""""""""""""""""
 

diff --git a/LibOS/shim/include/shim_internal.h b/LibOS/shim/include/shim_internal.h
@@ -19,8 +19,6 @@ void* shim_init(int argc, void* args);
 
 /* important macros and static inline functions */
 
-#define PAL_NATIVE_ERRNO() SHIM_TCB_GET(pal_errno)
-
 #define INTERNAL_TID_BASE ((IDTYPE)1 << (sizeof(IDTYPE) * 8 - 1))
 
 static inline bool is_internal_tid(unsigned int tid) {
@@ -177,9 +175,12 @@ bool maybe_emulate_syscall(PAL_CONTEXT* context);
  */
 bool handle_signal(PAL_CONTEXT* context, __sigset_t* old_mask_ptr);
 
-long convert_pal_errno(long err);
-
-#define PAL_ERRNO() convert_pal_errno(PAL_NATIVE_ERRNO())
+/*!
+ * \brief Translate PAL error code into UNIX error code.
+ *
+ * The sign of the error code is preserved.
+ */
+long pal_to_unix_errno(long err);
 
 void warn_unsupported_syscall(unsigned long sysno);
 void debug_print_syscall_before(unsigned long sysno, ...);
@@ -195,11 +196,7 @@ void debug_print_syscall_after(unsigned long sysno, ...);
  * Note that using `clear_event` probably requires external locking to avoid races.
  */
 static inline int create_event(AEVENTTYPE* e) {
-    e->event = DkStreamOpen(URI_PREFIX_PIPE, PAL_ACCESS_RDWR, 0, 0, 0);
-    if (!e->event) {
-        return -PAL_ERRNO();
-    }
-    return 0;
+     return pal_to_unix_errno(DkStreamOpen(URI_PREFIX_PIPE, PAL_ACCESS_RDWR, 0, 0, 0, &e->event));
 }
 
 static inline PAL_HANDLE event_handle(AEVENTTYPE* e) {
@@ -208,7 +205,7 @@ static inline PAL_HANDLE event_handle(AEVENTTYPE* e) {
 
 static inline void destroy_event(AEVENTTYPE* e) {
     if (e->event) {
-        DkObjectClose(e->event);
+        DkObjectClose(e->event); // TODO: handle errors
         e->event = NULL;
     }
 }
@@ -223,15 +220,19 @@ static inline int set_event(AEVENTTYPE* e, size_t n) {
     char bytes[n];
     memset(bytes, '\0', n);
     while (n > 0) {
-        PAL_NUM ret = DkStreamWrite(e->event, 0, n, bytes, NULL);
-        if (ret == PAL_STREAM_ERROR) {
-            int err = PAL_ERRNO();
-            if (err == EINTR || err == EAGAIN || err == EWOULDBLOCK) {
+        size_t size = n;
+        int ret = DkStreamWrite(e->event, 0, &size, bytes, NULL);
+        if (ret < 0) {
+            if (ret == -PAL_ERROR_INTERRUPTED || ret == -PAL_ERROR_TRYAGAIN) {
                 continue;
             }
-            return -err;
+            return pal_to_unix_errno(ret);
         }
-        n -= ret;
+        if (size == 0) {
+            /* This should never happen. */
+            return -EINVAL;
+        }
+        n -= size;
     }
 
     return 0;
@@ -244,14 +245,20 @@ static inline int wait_event(AEVENTTYPE* e) {
         return -EINVAL;
     }
 
-    int err = 0;
+    int ret = 0;
     do {
         char byte;
-        PAL_NUM ret = DkStreamRead(e->event, 0, 1, &byte, NULL, 0);
-        err = ret == PAL_STREAM_ERROR ? PAL_ERRNO() : (ret == 0 ? ENODATA : 0);
-    } while (err == EINTR || err == EAGAIN || err == EWOULDBLOCK);
+        size_t size = 1;
+        ret = DkStreamRead(e->event, 0, &size, &byte, NULL, 0);
+        if (ret < 0) {
+            ret = pal_to_unix_errno(ret);
+        } else if (size == 0) {
+            ret = -ENODATA;
+        }
+    /* XXX(borysp): I think we should actually return both of these. */
+    } while (ret == -EINTR || ret == -EAGAIN);
 
-    return -err;
+    return ret;
 }
 
 static inline int clear_event(AEVENTTYPE* e) {
@@ -266,35 +273,33 @@ static inline int clear_event(AEVENTTYPE* e) {
         PAL_FLG ievent = PAL_WAIT_READ;
         PAL_FLG revent = 0;
 
-        shim_get_tcb()->pal_errno = PAL_ERROR_SUCCESS;
-        PAL_BOL ret = DkStreamsWaitEvents(1, &handle, &ievent, &revent, /*timeout=*/0);
-        if (!ret) {
-            int err = PAL_ERRNO();
-            if (err == EINTR) {
+        int ret = DkStreamsWaitEvents(1, &handle, &ievent, &revent, /*timeout=*/0);
+        if (ret < 0) {
+            if (ret == -PAL_ERROR_INTERRUPTED) {
                 continue;
-            } else if (err == EAGAIN || err == EWOULDBLOCK) {
+            } else if (ret == -PAL_ERROR_TRYAGAIN) {
                 break;
             }
-            return -err;
+            return pal_to_unix_errno(ret);
         }
 
         /* Even if `revent` has `PAL_WAIT_ERROR` marked, let `DkSitreamRead()` report the error
          * below. */
         assert(revent);
 
         char bytes[100];
-        PAL_NUM n = DkStreamRead(e->event, 0, sizeof(bytes), bytes, NULL, 0);
-        if (n == PAL_STREAM_ERROR) {
-            int err = PAL_ERRNO();
-            if (err == EINTR) {
+        size_t n = sizeof(bytes);
+        ret = DkStreamRead(e->event, 0, &n, bytes, NULL, 0);
+        if (ret < 0) {
+            if (ret == -PAL_ERROR_INTERRUPTED) {
                 continue;
-            } else if (err == EAGAIN || err == EWOULDBLOCK) {
+            } else if (ret == -PAL_ERROR_TRYAGAIN) {
                 /* This should not happen, since we polled above... */
                 break;
             }
-            return -err;
+            return pal_to_unix_errno(ret);
         } else if (n == 0) {
-            /* This should not happen, since we polled above... */
+            /* This should not happen, something closed the handle? */
             return -ENODATA;
         }
     }

diff --git a/LibOS/shim/include/shim_lock.h b/LibOS/shim/include/shim_lock.h
@@ -31,12 +31,11 @@ static inline void clear_lock(struct shim_lock* l) {
 
 static inline bool create_lock(struct shim_lock* l) {
     l->owner = 0;
-    l->lock  = DkMutexCreate(0);
-    return l->lock != NULL;
+    return DkMutexCreate(0, &l->lock) == 0;
 }
 
 static inline void destroy_lock(struct shim_lock* l) {
-    DkObjectClose(l->lock);
+    DkObjectClose(l->lock); // TODO: handle errors
     clear_lock(l);
 }
 
@@ -47,7 +46,7 @@ static void lock(struct shim_lock* l) {
 
     assert(l->lock);
 
-    while (!DkSynchronizationObjectWait(l->lock, NO_TIMEOUT))
+    while (DkSynchronizationObjectWait(l->lock, NO_TIMEOUT) < 0)
         /* nop */;
 
     l->owner = get_cur_tid();

diff --git a/LibOS/shim/include/shim_tcb.h b/LibOS/shim/include/shim_tcb.h
@@ -34,7 +34,6 @@ struct shim_tcb {
     /* Scratch space to temporarily store a register. On some architectures (e.g. x86_64 inside
      * an SGX enclave) we lack a way to restore all (or at least some) registers atomically. */
     void*               syscall_scratch_pc;
-    int                 pal_errno;
     struct debug_buf*   debug_buf;
     void*               vma_cache;
 

diff --git a/LibOS/shim/include/shim_thread.h b/LibOS/shim/include/shim_thread.h
@@ -196,7 +196,7 @@ static inline void set_cur_thread(struct shim_thread* thread) {
 static inline void thread_setwait(struct shim_thread** queue, struct shim_thread* thread) {
     if (!thread)
         thread = get_cur_thread();
-    DkEventClear(thread->scheduler_event);
+    DkEventClear(thread->scheduler_event); // TODO: handle errors
     if (queue) {
         get_thread(thread);
         *queue = thread;
@@ -217,13 +217,11 @@ static inline int thread_sleep(uint64_t timeout_us, bool ignore_pending_signals)
         return -EINTR;
     }
 
-    if (!DkSynchronizationObjectWait(event, timeout_us))
-        return -PAL_ERRNO();
-
-    return 0;
+    return pal_to_unix_errno(DkSynchronizationObjectWait(event, timeout_us));
 }
 
 static inline void thread_wakeup(struct shim_thread* thread) {
+    // TODO: handle errors
     DkEventSet(thread->scheduler_event);
 }
 

diff --git a/LibOS/shim/include/shim_vma.h b/LibOS/shim/include/shim_vma.h
@@ -60,7 +60,9 @@ int init_vma(void);
  * if (bkeep_munmap(ptr, len, is_internal, &tmp_vma) < 0) {
  *     handle_errors();
  * }
- * DkVirtualMemoryFree(ptr, len);
+ * if (DkVirtualMemoryFree(ptr, len) < 0) {
+ *     handle_errors();
+ * }
  * bkeep_remove_tmp_vma(tmp_vma);
  *
  * Such a way of freeing is needed, so that no other thread will map the same memory in the window

diff --git a/LibOS/shim/src/bookkeep/shim_handle.c b/LibOS/shim/src/bookkeep/shim_handle.c
@@ -460,7 +460,7 @@ void put_handle(struct shim_handle* hdl) {
 #ifdef DEBUG_REF
             log_debug("handle %p closes PAL handle %p\n", hdl, hdl->pal_handle);
 #endif
-            DkObjectClose(hdl->pal_handle);
+            DkObjectClose(hdl->pal_handle); // TODO: handle errors
             hdl->pal_handle = NULL;
         }
 

diff --git a/LibOS/shim/src/bookkeep/shim_thread.c b/LibOS/shim/src/bookkeep/shim_thread.c
@@ -119,16 +119,18 @@ int alloc_thread_libos_stack(struct shim_thread* thread) {
     }
 
     bool need_mem_free = false;
-    if (DkVirtualMemoryAlloc(addr, SHIM_THREAD_LIBOS_STACK_SIZE, 0, LINUX_PROT_TO_PAL(prot, flags))
-            != addr) {
-        ret = -PAL_ERRNO();
+    ret = DkVirtualMemoryAlloc(&addr, SHIM_THREAD_LIBOS_STACK_SIZE, 0,
+                               LINUX_PROT_TO_PAL(prot, flags));
+    if (ret < 0) {
+        ret = pal_to_unix_errno(ret);
         goto unmap;
     }
     need_mem_free = true;
 
     /* Create a stack guard page. */
-    if (!DkVirtualMemoryProtect(addr, PAGE_SIZE, PAL_PROT_NONE)) {
-        ret = -PAL_ERRNO();
+    ret = DkVirtualMemoryProtect(addr, PAGE_SIZE, PAL_PROT_NONE);
+    if (ret < 0) {
+        ret = pal_to_unix_errno(ret);
         goto unmap;
     }
 
@@ -145,7 +147,9 @@ unmap:;
         BUG();
     }
     if (need_mem_free) {
-        DkVirtualMemoryFree(addr, SHIM_THREAD_LIBOS_STACK_SIZE);
+        if (DkVirtualMemoryFree(addr, SHIM_THREAD_LIBOS_STACK_SIZE) < 0) {
+            BUG();
+        }
     }
     bkeep_remove_tmp_vma(tmp_vma);
     return ret;
@@ -187,15 +191,15 @@ static int init_main_thread(void) {
     set_sig_mask(cur_thread, &set);
     unlock(&cur_thread->lock);
 
-    cur_thread->scheduler_event = DkNotificationEventCreate(PAL_TRUE);
-    if (!cur_thread->scheduler_event) {
+    int ret = DkNotificationEventCreate(PAL_TRUE, &cur_thread->scheduler_event);
+    if (ret < 0) {
         put_thread(cur_thread);
-        return -ENOMEM;
+        return pal_to_unix_errno(ret);;
     }
 
     /* TODO: I believe there is some Pal allocated initial stack which could be reused by the first
      * thread. Tracked: https://github.com/oscarlab/graphene/issues/2140 */
-    int ret = alloc_thread_libos_stack(cur_thread);
+    ret = alloc_thread_libos_stack(cur_thread);
     if (ret < 0) {
         put_thread(cur_thread);
         return ret;
@@ -287,8 +291,8 @@ struct shim_thread* get_new_thread(void) {
 
     unlock(&cur_thread->lock);
 
-    thread->scheduler_event = DkNotificationEventCreate(PAL_TRUE);
-    if (!thread->scheduler_event) {
+    int ret = DkNotificationEventCreate(PAL_TRUE, &thread->scheduler_event);
+    if (ret < 0) {
         put_thread(thread);
         return NULL;
     }
@@ -348,7 +352,9 @@ void put_thread(struct shim_thread* thread) {
                           addr, (char*)addr + SHIM_THREAD_LIBOS_STACK_SIZE);
                 BUG();
             }
-            DkVirtualMemoryFree(addr, SHIM_THREAD_LIBOS_STACK_SIZE);
+            if (DkVirtualMemoryFree(addr, SHIM_THREAD_LIBOS_STACK_SIZE) < 0) {
+                BUG();
+            }
             bkeep_remove_tmp_vma(tmp_vma);
         }
 
@@ -606,9 +612,9 @@ BEGIN_RS_FUNC(thread) {
         return -ENOMEM;
     }
 
-    thread->scheduler_event = DkNotificationEventCreate(PAL_TRUE);
-    if (!thread->scheduler_event) {
-        return -ENOMEM;
+    int ret = DkNotificationEventCreate(PAL_TRUE, &thread->scheduler_event);
+    if (ret < 0) {
+        return pal_to_unix_errno(ret);
     }
 
     if (thread->handle_map) {
@@ -626,7 +632,7 @@ BEGIN_RS_FUNC(thread) {
 
     assert(!get_cur_thread());
 
-    int ret = alloc_thread_libos_stack(thread);
+    ret = alloc_thread_libos_stack(thread);
     if (ret < 0) {
         return ret;
     }

diff --git a/LibOS/shim/src/bookkeep/shim_vma.c b/LibOS/shim/src/bookkeep/shim_vma.c
@@ -295,13 +295,14 @@ static void* _vma_malloc(size_t size) {
         return NULL;
     }
 
-    if (DkVirtualMemoryAlloc(addr, size, 0, PAL_PROT_WRITE | PAL_PROT_READ) != addr) {
+    int ret = DkVirtualMemoryAlloc(&addr, size, 0, PAL_PROT_WRITE | PAL_PROT_READ);
+    if (ret < 0) {
         struct shim_vma* vmas_to_free = NULL;
 
         spinlock_lock(&vma_tree_lock);
         /* Since we are freeing a range we just created, additional vma is not needed. */
-        int ret = _vma_bkeep_remove((uintptr_t)addr, (uintptr_t)addr + size, /*is_internal=*/true,
-                                    NULL, &vmas_to_free);
+        ret = _vma_bkeep_remove((uintptr_t)addr, (uintptr_t)addr + size, /*is_internal=*/true, NULL,
+                                &vmas_to_free);
         spinlock_unlock(&vma_tree_lock);
         if (ret < 0) {
             log_error("Removing a vma we just created failed with %d!\n", ret);
@@ -593,7 +594,7 @@ int init_vma(void) {
 
             int ret = DkRandomBitsRead(&gap, sizeof(gap));
             if (ret < 0) {
-                return -convert_pal_errno(-ret);
+                return pal_to_unix_errno(ret);
             }
 
             /* Resulting distribution is not ideal, but it should not be an issue here. */
@@ -1313,9 +1314,10 @@ BEGIN_RS_FUNC(vma) {
         }
 
         if (need_mapped < vma->addr + vma->length) {
-            if (DkVirtualMemoryAlloc(need_mapped, vma->addr + vma->length - need_mapped,
-                                     /*alloc_type=*/0,
-                                     LINUX_PROT_TO_PAL(vma->prot, /*map_flags=*/0))) {
+            int ret = DkVirtualMemoryAlloc(need_mapped, vma->addr + vma->length - need_mapped,
+                                           /*alloc_type=*/0,
+                                           LINUX_PROT_TO_PAL(vma->prot, /*map_flags=*/0));
+            if (ret >= 0) {
                 need_mapped += vma->length;
             }
         }