efence: Fix it all and add a test
authorBarret Rhoden <brho@cs.berkeley.edu>
Tue, 6 Jun 2017 20:54:01 +0000 (16:54 -0400)
committerBarret Rhoden <brho@cs.berkeley.edu>
Wed, 7 Jun 2017 15:38:14 +0000 (11:38 -0400)
To use it, link with -lelectric-fence.  You can do this from your top-level
Makelocal.  e.g. CFLAGS_USER or _TESTS += -lelectric-fence.

This doesn't support C++.  For that, we can look into libduma or something.

The biggest pain was that gcc was optimizing away internalUse and other
flag variables.  That's because it knows about malloc(), but doesn't know
that we are overriding it.  So we have to disable that optimization.

The other thing was locking: first off, our trylock returns a bool (TRUE or
FALSE), not an int (0 or 1).  Second, malloc and free are called from vcore
context, so we can't use a mutex.  Much like with Glibc's malloc lock, we
need to use spin_pdr locks.  Finally, we need to lock inside memalign, not
in the outer functions.  Specifically, someone could be calling memalign
directly and be unprotected.  There's nothing in malloc/calloc/valloc/etc
that needs to be protected.

I also added wrappers for functions like posix_memalign(), and removed the
obnoxious version printing.  I removed tstheap, put its guts in one of our
utests, and added a test for catching faults.

Signed-off-by: Barret Rhoden <brho@cs.berkeley.edu>
tests/Makefile
tests/electric-fence/Makefrag [deleted file]
tests/electric-fence/tstheap.c [deleted file]
user/electric-fence/Makefile
user/electric-fence/efence.c
user/utest/Makefile
user/utest/efence.c [new file with mode: 0644]

index 8fc77b7..b8c4095 100644 (file)
@@ -27,7 +27,6 @@ TESTS_EXECS_CPP  = $(patsubst $(TESTS_DIR)/%.cc, \
 include $(TESTS_DIR)/openmp/Makefrag
 include $(TESTS_DIR)/vmm/Makefrag
 include $(TESTS_DIR)/dune/Makefrag
-include $(TESTS_DIR)/electric-fence/Makefrag
 
 TESTS_EXECS = $(TESTS_EXECS_C) $(TESTS_EXECS_CPP)
 
diff --git a/tests/electric-fence/Makefrag b/tests/electric-fence/Makefrag
deleted file mode 100644 (file)
index 532f8e4..0000000
+++ /dev/null
@@ -1,25 +0,0 @@
-ELECTRIC-FENCE_TESTS_DIR = $(TESTS_DIR)/electric-fence
-
-ELECTRIC-FENCE_TESTS_CFLAGS += $(CFLAGS_TESTS) -Wunused -Werror -D__NO_STRING_INLINES
-
-ALL_ELECTRIC-FENCE_TEST_FILES := $(wildcard $(ELECTRIC-FENCE_TESTS_DIR)/*.c)
-
-ELECTRIC-FENCE_TESTS_LDLIBS := $(TESTS_LDLIBS) -lelf -lelectric-fence
-
-ELECTRIC-FENCE_TESTS_SRCS := $(ALL_ELECTRIC-FENCE_TEST_FILES)
-
-ELECTRIC-FENCE_TESTS_LDDEPENDS := $(ELECTRIC-FENCE_TESTS_DIR)/%.c
-
-TESTS_EXECS_C  += $(patsubst $(ELECTRIC-FENCE_TESTS_DIR)/%.c, \
-                      $(OBJDIR)/$(ELECTRIC-FENCE_TESTS_DIR)/%, \
-                      $(ELECTRIC-FENCE_TESTS_SRCS))
-
-STATIC := $(findstring static,$(ELECTRIC-FENCE_TESTS_CFLAGS))
-$(OBJDIR)/$(ELECTRIC-FENCE_TESTS_DIR)/%: $(ELECTRIC-FENCE_TESTS_LDDEPENDS)
-       @echo + cc [ELECTRIC-FENCE_TESTS] $<
-       @mkdir -p $(@D)
-       $(Q)$(CC) $(ELECTRIC-FENCE_TESTS_CFLAGS) -o $@ $< $(ELECTRIC-FENCE_TESTS_LDLIBS)
-       @if [ "$(STATIC)" != "static" ]; then \
-               $(OBJDUMP) -S $@ > $@.asm; \
-               $(NM) -n $@ > $@.sym; \
-       fi
diff --git a/tests/electric-fence/tstheap.c b/tests/electric-fence/tstheap.c
deleted file mode 100644 (file)
index 3daf189..0000000
+++ /dev/null
@@ -1,65 +0,0 @@
-#include <stdlib.h>
-#include <stdio.h>
-#include <math.h>
-#include <limits.h>
-#include "efence.h"
-
-/*
- * This is a simple program to exercise the allocator. It allocates and frees
- * memory in a pseudo-random fashion. It should run silently, using up time
- * and resources on your system until you stop it or until it has gone
- * through TEST_DURATION (or the argument) iterations of the loop.
- */
-
-extern C_LINKAGE double drand48(void); /* For pre-ANSI C systems */
-
-#define        POOL_SIZE       1024
-#define        LARGEST_BUFFER  30000
-#define        TEST_DURATION   1000000
-
-void * pool[POOL_SIZE];
-
-#ifdef FAKE_DRAND48
-/*
- * Add -DFAKE_DRAND48 to your compile flags if your system doesn't
- * provide drand48().
- */
-
-#ifndef        ULONG_MAX
-#define        ULONG_MAX       ~(1L)
-#endif
-
-double
-drand48(void)
-{
-       return (random() / (double)ULONG_MAX);
-}
-#endif
-
-int
-main(int argc, char * * argv)
-{
-       int     count = 0;
-       int     duration = TEST_DURATION;
-
-       if ( argc >= 2 )
-               duration = atoi(argv[1]);
-
-       for ( ; count < duration; count++ ) {
-               void * *        element = &pool[(int)(drand48() * POOL_SIZE)];
-               size_t          size = (size_t)(drand48() * (LARGEST_BUFFER + 1));
-
-               if ( *element ) {
-                       free( *element );
-                       *element = 0;
-               }
-               else if ( size > 0 ) {
-                       *element = malloc(size);
-               }
-       }
-
-       // now let's be dumb.
-       char *c = malloc(4096);
-       c[4097] = 0;
-       return 0;
-}
index 684c989..dd72022 100644 (file)
@@ -1,4 +1,4 @@
 LIBNAME = electric-fence
 INCDIR = .
-CFLAGS_USER += -D__NO_STRING_INLINES
+CFLAGS_USER += -D__NO_STRING_INLINES -fno-builtin-malloc
 include ../Makefrag-user-lib
index 2930400..eaa0dbe 100644 (file)
 #include <pthread.h>
 #include <errno.h>
 
-#ifdef malloc
-#undef malloc
-#endif
-
-#ifdef calloc
-#undef calloc
-#endif
-
-static const char      version[] = "\n  Electric Fence 2.2"
- " Copyright (C) 1987-2014 Bruce Perens.\n";
+#include <parlib/spinlock.h>
+#include <parlib/stdio.h>
 
 /*
  * MEMORY_CREATION_SIZE is the amount of memory to get from the operating
@@ -84,14 +76,6 @@ struct _Slot {
 };
 typedef struct _Slot   Slot;
 
- /*
- * EF_DISABLE_BANNER is a global variable used to control whether
- * Electric Fence prints its usual startup message.  If the value is
- * -1, it will be set from the environment default to 0 at run time.
- */
-int            EF_DISABLE_BANNER = -1;
-
-
 /*
  * EF_ALIGNMENT is a global variable used to control the default alignment
  * of buffers returned by malloc(), calloc(), and realloc(). It is all-caps
@@ -193,30 +177,41 @@ static size_t             bytesPerPage = 0;
  /*
  * mutex to enable multithreaded operation
  */
-static uth_mutex_t mutex;
-static pid_t mutexpid=0;
-static int locknr=0;
-
-
-static void lock() {
-    if (uth_mutex_trylock(&mutex)) {
-       if (mutexpid==getpid()) {
-           locknr++;
-           return;
-       } else {
-           uth_mutex_lock(&mutex);
-       }
-    } 
-    mutexpid=getpid();
-    locknr=1;
+static struct spin_pdr_lock pdr_lock = SPINPDR_INITIALIZER;
+#define RECURSE_UNLOCKED -1
+static long lockholder = RECURSE_UNLOCKED;
+static int lock_depth;
+
+static long who_am_i(void)
+{
+       if (in_vcore_context())
+               return vcore_id();
+       else
+               return (long)current_uthread;
+}
+
+static void lock(void)
+{
+       long me = who_am_i();
+
+       if (!spin_pdr_trylock(&pdr_lock)) {
+               if (lockholder == me) {
+                       lock_depth++;
+                       return;
+               }
+               spin_pdr_lock(&pdr_lock);
+       }
+       lockholder = me;
+       lock_depth = 1;
 }
 
-static void unlock() {
-    locknr--;
-    if (!locknr) {
-       mutexpid=0;
-       uth_mutex_unlock(&mutex);
-    }
+static void unlock(void)
+{
+       lock_depth--;
+       if (!lock_depth) {
+               lockholder = RECURSE_UNLOCKED;
+               spin_pdr_unlock(&pdr_lock);
+       }
 }
 
 /*
@@ -241,16 +236,6 @@ initialize(void)
        char *  string;
        Slot *  slot;
 
-       if ( EF_DISABLE_BANNER == -1 ) {
-               if ( (string = getenv("EF_DISABLE_BANNER")) != 0 )
-                       EF_DISABLE_BANNER = atoi(string);
-               else
-                       EF_DISABLE_BANNER = 0;
-       }
-
-       if ( EF_DISABLE_BANNER == 0 )
-               EF_Print(version);
-
        /*
         * Import the user's environment specification of the default
         * alignment for malloc(). We want that alignment to be under
@@ -437,6 +422,7 @@ memalign(size_t alignment, size_t userSize)
        if ( allocationList == 0 )
                initialize();
 
+       lock();
        if ( userSize == 0 && !EF_ALLOW_MALLOC_0 )
                EF_Abort("Allocating 0 bytes, probably a bug.");
 
@@ -620,6 +606,7 @@ memalign(size_t alignment, size_t userSize)
        if ( !internalUse )
                Page_DenyAccess(allocationList, allocationListSize);
 
+       unlock();
        return address;
 }
 
@@ -811,18 +798,9 @@ realloc(void * oldBuffer, size_t newSize)
 extern C_LINKAGE void *
 malloc(size_t size)
 {
-        void  *allocation;   
-        if ( allocationList == 0 ) {
-               uth_mutex_init(&mutex);
-                initialize();   /* This sets EF_ALIGNMENT */
-        }       
-        lock();
-        allocation=memalign(EF_ALIGNMENT, size); 
-
-        unlock();
-
-       return allocation;
+       if (!allocationList)
+               initialize();   /* This sets EF_ALIGNMENT */
+       return memalign(EF_ALIGNMENT, size);
 }
 
 extern C_LINKAGE char *
@@ -869,11 +847,8 @@ calloc(size_t nelem, size_t elsize)
        size_t  size = nelem * elsize;
         void * allocation;
         
-        lock();
-       
         allocation = malloc(size);
         memset(allocation, 0, size);
-        unlock();
 
        return allocation;
 }
@@ -885,11 +860,22 @@ calloc(size_t nelem, size_t elsize)
 extern C_LINKAGE void *
 valloc (size_t size)
 {
-        void * allocation;
-       
-        lock();
-        allocation= memalign(bytesPerPage, size);
-        unlock();
-       
-        return allocation;
+       return memalign(bytesPerPage, size);
+}
+
+extern C_LINKAGE int
+posix_memalign(void **memptr, size_t alignment, size_t size)
+{
+       *memptr = memalign(alignment, size);
+       if (!*memptr) {
+               errno = ENOMEM;
+               return -1;
+       }
+       return 0;
+}
+
+extern C_LINKAGE void *
+aligned_alloc(size_t alignment, size_t size)
+{
+       return memalign(alignment, size);
 }
index 17089ea..eb50640 100644 (file)
@@ -10,7 +10,7 @@ SRCDIR :=
 OBJDIR := $(SRCDIR)obj
 INCDIR = $(SRCDIR)include
 
-LDLIBS := -lpthread -lbenchutil -lm -liplib -lndblib
+LDLIBS := -lpthread -lbenchutil -lm -liplib -lndblib -lelectric-fence
 
 uc = $(shell echo $(1) | tr a-z A-Z)
 
diff --git a/user/utest/efence.c b/user/utest/efence.c
new file mode 100644 (file)
index 0000000..b1b6229
--- /dev/null
@@ -0,0 +1,97 @@
+#include <utest/utest.h>
+#include <errno.h>
+#include <stdlib.h>
+#include <stdio.h>
+#include <math.h>
+#include <limits.h>
+#include <signal.h>
+#include <pthread.h>
+#include <setjmp.h>
+
+TEST_SUITE("EFENCE");
+
+/* <--- Begin definition of test cases ---> */
+
+/* The guts of this test came from electric fence's tstheap:
+ *
+ *  Electric Fence - Red-Zone memory allocator.
+ *  Bruce Perens, 1988, 1993
+ *
+ *  For email below, drop spaces and <spam-buster> tag.
+ *  MODIFIED:  March 20, 2014 (jric<spam-buster> @ <spam-buster> chegg DOT com)
+ */
+
+#define        POOL_SIZE 1024
+#define        LARGEST_BUFFER 30000
+#define        TEST_DURATION 1000
+
+static void *pool[POOL_SIZE];
+
+bool test_alloc_and_free(void)
+{
+       void **element;
+       size_t size;
+
+       for (int count = 0; count < TEST_DURATION; count++) {
+               element = &pool[(int)(drand48() * POOL_SIZE)];
+               size = (size_t)(drand48() * (LARGEST_BUFFER + 1));
+               if (*element) {
+                       free(*element);
+                       *element = 0;
+               } else if (size > 0) {
+                       *element = malloc(size);
+                       *(uint8_t*)(*element) = 0xab;
+                       *(uint8_t*)(*element + size - 1) = 0xcd;
+               }
+       }
+       /* Surviving without page faulting is success. */
+       return TRUE;
+}
+
+/* The pointer Needs to be volatile, so that blob = malloc() gets assigned
+ * before the fault. */
+static char *volatile blob;
+static jmp_buf save;
+static void *fault_addr;
+
+static void segv_action(int signr, siginfo_t *si, void *arg)
+{
+       fault_addr = si->si_addr;
+       longjmp(save, 1);
+}
+
+static struct sigaction sigact = {.sa_sigaction = segv_action, 0};
+
+bool test_catching_fault(void)
+{
+       pthread_yield();        /* link in pth for intra-thread signals (SIGSEGV) */
+       sigaction(SIGSEGV, &sigact, 0);
+       blob = malloc(PGSIZE);
+       if (!setjmp(save)) {
+               /* First time through, we'll try to pagefault. */
+               blob[PGSIZE + 1] = 0;
+               UT_ASSERT_FMT("Tried to fault, but didn't!", FALSE);
+       }
+       /* Second time, we'll return via setjmp */
+       UT_ASSERT_FMT("Fault addr was %p, should be %p",
+                     fault_addr == blob + PGSIZE + 1, fault_addr,
+                     blob + PGSIZE + 1);
+       return TRUE;
+}
+
+/* <--- End definition of test cases ---> */
+
+struct utest utests[] = {
+       UTEST_REG(alloc_and_free),
+       UTEST_REG(catching_fault),
+};
+int num_utests = sizeof(utests) / sizeof(struct utest);
+
+int main(int argc, char *argv[])
+{
+       // Run test suite passing it all the args as whitelist of what tests to run.
+       char **whitelist = &argv[1];
+       int whitelist_len = argc - 1;
+
+       RUN_TEST_SUITE(utests, num_utests, whitelist, whitelist_len);
+}