Tidy up addClassImage API and class <-> image management

Objectively.vs15/Sources/Windowly.c

@@ -32,16 +32,6 @@
 
 static const int64_t DELTA_EPOCH_IN_MICROSECS = 11644473600000000;
 
-long __sync_val_compare_and_swap_(long volatile *Destination, long Exchange, long Comparand) {
-  return InterlockedCompareExchange(Destination, Exchange, Comparand);
-}
-long __sync_add_and_fetch_(long volatile *Append, long Value) {
-  return InterlockedAdd(Append, Value);
-}
-void *__sync_lock_test_and_set_(void *volatile *Target, void *Value) {
-  return InterlockedExchangePointer(Target, Value);
-}
-
 int gettimeofday(struct timeval *tv, struct timezone *tz)
 {
   if (tv)
@@ -133,4 +123,4 @@ int asprintf(char **str, const char *fmt, ...)
   return size;
 }
 
-#pragma endregion asprintf
+#pragma endregion asprintf

Objectively.vs15/Sources/Windowly.h

@@ -88,12 +88,3 @@ OBJECTIVELY_EXPORT int vasprintf(char **str, const char *fmt, va_list args);
 #endif
 
 #define iconv(c0, c1, c2, c3, c4) libiconv(c0, (const char **)c1, c2, c3, c4)
-
-// INTERLOCK STUFF
-OBJECTIVELY_EXPORT long __sync_val_compare_and_swap_(long volatile *Destination, long Exchange, long Comparand);
-OBJECTIVELY_EXPORT long __sync_add_and_fetch_(long volatile *Append, long Value);
-OBJECTIVELY_EXPORT void *__sync_lock_test_and_set_(void *volatile *Target, void *Value);
-
-#define __sync_val_compare_and_swap(c0, c1, c2) __sync_val_compare_and_swap_((volatile long *) c0, c1, c2)
-#define __sync_add_and_fetch(c0, c1) __sync_add_and_fetch_((volatile long *) c0, c1)
-#define __sync_lock_test_and_set(c0, c1) __sync_lock_test_and_set_((void *volatile *) c0, (void *) c1)

Sources/Objectively/Class.c

@@ -25,16 +25,13 @@
 
 #include <assert.h>
 #include <dlfcn.h>
+#include <pthread.h>
 #include <stdlib.h>
 #include <stdio.h>
 #include <string.h>
 
 #if defined(_WIN32)
 #include <Windows.h>
-#elif defined(__APPLE__)
-#include <mach-o/dyld.h>
-#else
-#include <link.h>
 #endif
 
 #if HAVE_UNISTD_H
@@ -49,11 +46,29 @@ size_t _pageSize;
 static Class *_classes;
 
 /**
- * @brief The registered images providing Classes.
+ * @brief Guards the structure of `_classes`. MUST NOT be held across `dlsym`,
+ * `dlopen`, or a Class initializer, each of which can reenter `_initialize`.
  */
-#define MAX_CLASS_IMAGES 8
-static ident _classImages[MAX_CLASS_IMAGES];
-static size_t _classImageCount;
+static pthread_mutex_t _classesLock = PTHREAD_MUTEX_INITIALIZER;
+
+/**
+ * @brief A registered image: the handle the application holds, and the base
+ * address that the Classes it declares record in `Class::image`.
+ */
+typedef struct ClassImage ClassImage;
+struct ClassImage {
+  ident handle;
+  ident image;
+  ClassImage *next;
+};
+
+/**
+ * @brief The registered images providing Classes, most recently added first.
+ * Published atomically rather than under `_classesLock`, which cannot be held
+ * across the `dlsym` this list exists for. A plain list rather than a
+ * MutableArray, because Class is beneath the collections.
+ */
+static ClassImage *_images;
 
 /**
  * @brief Called `atexit` to teardown Objectively.
@@ -80,6 +95,16 @@ static void teardown(void) {
 
     c = next;
   }
+
+  ClassImage *i = _images;
+  while (i) {
+
+    ClassImage *next = i->next;
+
+    free(i);
+
+    i = next;
+  }
 }
 
 /**
@@ -159,12 +184,14 @@ Class *_initialize(const ClassDef *def) {
    * a compound literal with automatic storage. */
   clazz->image = imageForAddress((ident) def->name);
 
-  /* Link before publishing, and relaxed on the way in: the CAS is a read-modify-
-   * write, so it joins the release sequence of the push it displaces, and a
-   * reader acquiring the head synchronizes with every publisher behind it. The
-   * head is only copied here, never dereferenced, so there is nothing to acquire. */
-  clazz->next = __atomic_load_n(&_classes, __ATOMIC_RELAXED);
-  while (!__atomic_compare_exchange_n(&_classes, &clazz->next, clazz, 1, __ATOMIC_RELEASE, __ATOMIC_RELAXED)) ;
+  /* Taken here rather than around the whole function: def.initialize above can
+   * reach other archetypes, and so this, before that Class is published. */
+  pthread_mutex_lock(&_classesLock);
+
+  clazz->next = _classes;
+  _classes = clazz;
+
+  pthread_mutex_unlock(&_classesLock);
 
   return clazz;
 }
@@ -207,105 +234,106 @@ ident _cast(const Class *clazz, const ident obj) {
   return (ident) obj;
 }
 
-/**
- * @return The base address of the image behind `handle`, or `NULL`.
- * @remarks There is no one call for this. Windows hands out the module itself as
- * the handle, which `GetModuleFileName` confirms rather than assumes; glibc
- * answers from the link map; and macOS, which has neither, is left with matching
- * the handle against the loaded images.
- */
-static ident imageForHandle(ident handle) {
+void addClassImage(ident handle, const ident address) {
 
   assert(handle);
+  assert(address);
 
-#if defined(_WIN32)
-  char path[MAX_PATH];
-  if (GetModuleFileNameA((HMODULE) handle, path, sizeof(path))) {
-    return handle;
+  const ident image = imageForAddress(address);
+  if (image == NULL) {
+    fprintf(stderr, "%s: no image contains %p\n", __func__, address);
+    abort();
   }
-#elif defined(__APPLE__)
-  for (uint32_t i = 0; i < _dyld_image_count(); i++) {
-
-    /* RTLD_LOCAL is not the default on macOS, and dlopen of an image already
-     * loaded promotes it to the global namespace, which would put every image in
-     * the process there - undoing the isolation the caller loaded it for. */
-    ident image = dlopen(_dyld_get_image_name(i), RTLD_LAZY | RTLD_NOLOAD | RTLD_LOCAL);
-    if (image == NULL) {
-      continue;
-    }
 
-    dlclose(image);
+  ClassImage *classImage = calloc(1, sizeof(ClassImage));
+  assert(classImage);
 
-    if (image == handle) {
-      return (ident) _dyld_get_image_header(i);
-    }
-  }
-#else
-  struct link_map *map;
-  if (dlinfo(handle, RTLD_DI_LINKMAP, &map) == 0 && map) {
-    return (ident) map->l_addr;
-  }
-#endif
+  classImage->handle = handle;
+  classImage->image = image;
 
-  return NULL;
+  /* Published the same way a Class is, and for the same reason: classForName
+   * walks this list on any thread. */
+  classImage->next = __atomic_load_n(&_images, __ATOMIC_RELAXED);
+  while (!__atomic_compare_exchange_n(&_images, &classImage->next, classImage, 1,
+      __ATOMIC_RELEASE, __ATOMIC_RELAXED)) ;
 }
 
 void removeClassImage(ident handle) {
 
   assert(handle);
 
-  for (size_t i = 0; i < _classImageCount; i++) {
-    if (_classImages[i] == handle) {
-      memmove(_classImages + i, _classImages + i + 1, (_classImageCount - i - 1) * sizeof(ident));
-      _classImages[--_classImageCount] = NULL;
+  /* Held from the search to the last unlink, so that retiring an image and
+   * dropping its Classes is one operation, and a second call for the same handle
+   * finds it already gone. Nothing here reaches the loader. */
+  pthread_mutex_lock(&_classesLock);
+
+  ident image = NULL;
+
+  /* Retired in place rather than unlinked, so that a concurrent classForName
+   * parked on this node still has a next to follow, and never reads a node that
+   * has been freed. Retiring is a single store of the handle it matches on, so
+   * that walk sees this image or does not, and never half of it. Retired nodes
+   * are freed at teardown; reusing one would put a newly registered image where
+   * the retired one sat, and lookup order is newest first. */
+  for (ClassImage *i = __atomic_load_n(&_images, __ATOMIC_ACQUIRE); i; i = i->next) {
+    if (__atomic_load_n(&i->handle, __ATOMIC_ACQUIRE) == handle) {
+      image = i->image;
+      __atomic_store_n(&i->handle, NULL, __ATOMIC_RELEASE);
       break;
     }
   }
 
-  const ident image = imageForHandle(handle);
   if (image == NULL) {
-    return;
+    fprintf(stderr, "%s: %p was never registered\n", __func__, handle);
+    abort();
   }
 
-  Class **link = &_classes;
-  while (*link) {
-    Class *clazz = *link;
+  Class **classes = &_classes;
+  while (*classes) {
+    Class *clazz = *classes;
 
     if (clazz->image == image) {
-      *link = clazz->next;
+      *classes = clazz->next;
       clazz->next = NULL;
     } else {
-      link = &clazz->next;
+      classes = &clazz->next;
     }
   }
-}
 
-void addClassImage(ident handle) {
-
-  assert(handle);
-  assert(_classImageCount < MAX_CLASS_IMAGES);
-
-  _classImages[_classImageCount++] = handle;
+  pthread_mutex_unlock(&_classesLock);
 }
 
 Class *classForName(const char *name) {
 
   if (name) {
-    Class *c = __atomic_load_n(&_classes, __ATOMIC_ACQUIRE);
+    pthread_mutex_lock(&_classesLock);
+
+    Class *c = _classes;
     while (c) {
       if (strcmp(name, c->def.name) == 0) {
-        return c;
+        break;
       }
       c = c->next;
     }
 
+    pthread_mutex_unlock(&_classesLock);
+
+    if (c) {
+      return c;
+    }
+
     char *s;
     if (asprintf(&s, "_%s", name) > 0) {
       Class *clazz = NULL;
       Class *(*archetype)(void) = NULL;
 
-      for (size_t i = _classImageCount; i > 0 && archetype == NULL; i--) {
-        archetype = dlsym(_classImages[i - 1], s);
+      for (ClassImage *i = __atomic_load_n(&_images, __ATOMIC_ACQUIRE);
+          i && archetype == NULL; i = i->next) {
+
+        ident handle = __atomic_load_n(&i->handle, __ATOMIC_ACQUIRE);
+        if (handle) {
+          archetype = dlsym(handle, s);
+        }
       }
 
       if (archetype == NULL) {

Sources/Objectively/Class.h

@@ -138,6 +138,8 @@ OBJECTIVELY_EXPORT ident _cast(const Class *clazz, const ident obj);
 /**
  * @brief Registers an image that provides Classes, e.g. a plugin.
  * @param handle A handle from `dlopen`.
+ * @param address Any address within that image, such as the entry point the
+ * application resolved from `handle`.
  * @details `classForName` resolves a name it has not yet initialized through the
  * process-wide namespace, which holds only images loaded `RTLD_GLOBAL`, and which
  * Windows does not have at all. An application that loads Classes from a plugin
@@ -147,8 +149,12 @@ OBJECTIVELY_EXPORT ident _cast(const Class *clazz, const ident obj);
  * loaded plugin answers ahead of the one it replaced.
  * @remarks Resolution is by Objectively's own convention, the Class name
  * prefixed with an underscore, so nothing about the image has to be declared.
+ * @remarks `address` is what identifies the image. Classes record the base
+ * address of the image that declared them, and the platforms report that for an
+ * address, not for a handle, so this takes one that the caller can vouch for.
+ * Aborts if no loaded image contains it.
  */
-OBJECTIVELY_EXPORT void addClassImage(ident handle);
+OBJECTIVELY_EXPORT void addClassImage(ident handle, const ident address);
 
 /**
  * @brief Unregisters an image, and every Class it declared.
@@ -168,6 +174,9 @@ OBJECTIVELY_EXPORT void addClassImage(ident handle);
  * and its interface, which are not reclaimed.
  * @remarks MUST be called while the handle is still open, and only once nothing
  * instantiated from that image survives.
+ * @remarks Aborts if `handle` was not registered, rather than leaving the
+ * Classes it declared behind, which is the failure this exists to prevent. Two
+ * calls for the same handle abort on the second, whichever order they arrive in.
  */
 OBJECTIVELY_EXPORT void removeClassImage(ident handle);