[Guile-commits] GNU Guile branch, master, updated. v2.1.0-628-gb914b23

[Andy Wingo]

This is an automated email from the git hooks/post-receive script. It was
generated because a ref change was pushed to the repository containing
the project "GNU Guile".

http://git.savannah.gnu.org/cgit/guile.git/commit/?id=b914b236c3d6a6772597278e97b80bdb34129291

The branch, master has been updated
       via  b914b236c3d6a6772597278e97b80bdb34129291 (commit)
      from  b3ae2b5068cbfcb6e9eec7ff96cd936f1c861396 (commit)

Those revisions listed above that are new to this repository have
not appeared on any other notification email; so we list those
revisions in full, below.

- Log -----------------------------------------------------------------
commit b914b236c3d6a6772597278e97b80bdb34129291
Author: Andy Wingo <address@hidden>
Date:   Wed Feb 5 22:52:00 2014 +0100

    Various VM stack management fixes
    
    * libguile/vm.c (vm_increase_sp): New interface, to increase the SP to
      some new level, possibly expanding the stack.
      (vm_push_sp, vm_restore_sp): Intefaces to vm_increase_sp.
      (vm_return_to_continuation): Don't throw an error if there's not
      enough space; instead, expand.
      (vm_reinstate_partial_continuation): Use the new helper.
      (return_unused_stack_to_os): Avoid off-by-one error (sp points to
      valid memory.)
      (scm_call_n): Never write beyond the sp.

-----------------------------------------------------------------------

Summary of changes:
 libguile/vm-engine.c |    6 +-
 libguile/vm.c        |  154 +++++++++++++++++++++++++++++---------------------
 2 files changed, 94 insertions(+), 66 deletions(-)

diff --git a/libguile/vm-engine.c b/libguile/vm-engine.c
index 5edadd3..b5cd095 100644
--- a/libguile/vm-engine.c
+++ b/libguile/vm-engine.c
@@ -195,9 +195,11 @@
 #define ALLOC_FRAME(n)                                              \
   do {                                                              \
     vp->sp = LOCAL_ADDRESS (n - 1);                                 \
-    CHECK_OVERFLOW ();                                              \
     if (vp->sp > vp->sp_max_since_gc)                               \
-      vp->sp_max_since_gc = vp->sp;                                 \
+      {                                                             \
+        vp->sp_max_since_gc = vp->sp;                               \
+        CHECK_OVERFLOW ();                                          \
+      }                                                             \
   } while (0)
 
 /* Reset the current frame to hold N locals.  Used when we know that no
diff --git a/libguile/vm.c b/libguile/vm.c
index 95f12ee..b0918b6 100644
--- a/libguile/vm.c
+++ b/libguile/vm.c
@@ -64,6 +64,36 @@ static SCM sym_debug;
 
 /* #define VM_ENABLE_PARANOID_ASSERTIONS */
 
+static void vm_expand_stack (struct scm_vm *vp) SCM_NOINLINE;
+
+/* RESTORE is for the case where we know we have done a PUSH of equal or
+   greater stack size in the past.  Otherwise PUSH is the thing, which
+   may expand the stack.  */
+enum vm_increase_sp_kind { VM_SP_PUSH, VM_SP_RESTORE };
+
+static inline void
+vm_increase_sp (struct scm_vm *vp, SCM *new_sp, enum vm_increase_sp_kind kind)
+{
+  vp->sp = new_sp;
+  if (new_sp > vp->sp_max_since_gc)
+    {
+      vp->sp_max_since_gc = new_sp;
+      if (kind == VM_SP_PUSH && new_sp >= vp->stack_limit)
+        vm_expand_stack (vp);
+    }
+}
+
+static inline void
+vm_push_sp (struct scm_vm *vp, SCM *new_sp)
+{
+  vm_increase_sp (vp, new_sp, VM_SP_PUSH);
+}
+
+static inline void
+vm_restore_sp (struct scm_vm *vp, SCM *new_sp)
+{
+  vm_increase_sp (vp, new_sp, VM_SP_RESTORE);
+}
 
 
 /*
@@ -114,40 +144,60 @@ vm_return_to_continuation (struct scm_vm *vp, SCM cont, 
size_t n, SCM *argv)
 {
   struct scm_vm_cont *cp;
   SCM *argv_copy;
+  scm_t_ptrdiff reloc;
 
   argv_copy = alloca (n * sizeof(SCM));
   memcpy (argv_copy, argv, n * sizeof(SCM));
 
   cp = SCM_VM_CONT_DATA (cont);
 
-  if (vp->stack_size < cp->stack_size + n + 3)
-    scm_misc_error ("vm-engine", "not enough space to reinstate continuation",
-                    scm_list_1 (cont));
+  /* FIXME: Need to prevent GC while futzing with the stack; otherwise,
+     another thread causing GC may initiate a mark of a stack in an
+     inconsistent state.  */
 
-  vp->sp = cp->sp;
-  vp->fp = cp->fp;
+  /* We know that there is enough space for the continuation, because we
+     captured it in the past.  However there may have been an expansion
+     since the capture, so we may have to re-link the frame
+     pointers.  */
+  reloc = (vp->stack_base - (cp->stack_base - cp->reloc));
+  vp->fp = cp->fp + reloc;
   memcpy (vp->stack_base, cp->stack_base, cp->stack_size * sizeof (SCM));
+  vm_restore_sp (vp, cp->sp + reloc);
+
+  if (reloc)
+    {
+      SCM *fp = vp->fp;
+      while (fp)
+        {
+          SCM *next_fp = SCM_FRAME_DYNAMIC_LINK (fp);
+          if (next_fp)
+            {
+              next_fp += reloc;
+              SCM_FRAME_SET_DYNAMIC_LINK (fp, next_fp);
+            }
+          fp = next_fp;
+        }
+    }
+
+  /* Now we have the continuation properly copied over.  We just need to
+     copy the arguments.  It is not guaranteed that there is actually
+     space for the arguments, though, so we have to bump the SP first.  */
+  vm_push_sp (vp, vp->sp + 3 + n);
 
+  /* Now copy on an empty frame and the return values, as the
+     continuation expects.  */
   {
+    SCM *base = vp->sp + 1 - 3 - n;
     size_t i;
 
-    /* Push on an empty frame, as the continuation expects.  */
     for (i = 0; i < 3; i++)
-      {
-        vp->sp++;
-        *vp->sp = SCM_BOOL_F;
-      }
+      base[i] = SCM_BOOL_F;
 
-    /* Push the return values.  */
     for (i = 0; i < n; i++)
-      {
-        vp->sp++;
-        *vp->sp = argv_copy[i];
-      }
-    if (vp->sp > vp->sp_max_since_gc)
-      vp->sp_max_since_gc = vp->sp;
-    vp->ip = cp->ra;
+      base[i + 3] = argv_copy[i];
   }
+
+  vp->ip = cp->ra;
 }
 
 static struct scm_vm * thread_vm (scm_i_thread *t);
@@ -292,8 +342,6 @@ vm_abort (struct scm_vm *vp, SCM tag,
   scm_c_abort (vp, tag, nstack + tail_len, argv, current_registers);
 }
 
-static void vm_expand_stack (struct scm_vm *vp) SCM_NOINLINE;
-
 static void
 vm_reinstate_partial_continuation (struct scm_vm *vp, SCM cont,
                                    size_t n, SCM *argv,
@@ -310,48 +358,28 @@ vm_reinstate_partial_continuation (struct scm_vm *vp, SCM 
cont,
 
   cp = SCM_VM_CONT_DATA (cont);
 
-  while (1)
-    {
-      scm_t_ptrdiff saved_stack_height = vp->sp - vp->stack_base;
+  vm_push_sp (vp, SCM_FRAME_LOCALS_ADDRESS (vp->fp) + cp->stack_size + n - 1);
 
-      base = SCM_FRAME_LOCALS_ADDRESS (vp->fp);
-      reloc = cp->reloc + (base - cp->stack_base);
-
-      vp->sp = base + cp->stack_size + n + 1;
-      if (vp->sp < vp->stack_limit)
-        break;
-
-      vm_expand_stack (vp);
-      vp->sp = vp->stack_base + saved_stack_height;
-    }
-
-#define RELOC(scm_p)                                           \
-  (((SCM *) (scm_p)) + reloc)
+  base = SCM_FRAME_LOCALS_ADDRESS (vp->fp);
+  reloc = cp->reloc + (base - cp->stack_base);
 
   memcpy (base, cp->stack_base, cp->stack_size * sizeof (SCM));
 
+  vp->fp = cp->fp + reloc;
+  vp->ip = cp->ra;
+
   /* now relocate frame pointers */
   {
     SCM *fp;
-    for (fp = RELOC (cp->fp);
+    for (fp = vp->fp;
          SCM_FRAME_LOWER_ADDRESS (fp) > base;
          fp = SCM_FRAME_DYNAMIC_LINK (fp))
-      SCM_FRAME_SET_DYNAMIC_LINK (fp, RELOC (SCM_FRAME_DYNAMIC_LINK (fp)));
+      SCM_FRAME_SET_DYNAMIC_LINK (fp, SCM_FRAME_DYNAMIC_LINK (fp) + reloc);
   }
 
-  vp->sp = base - 1 + cp->stack_size;
-  vp->fp = RELOC (cp->fp);
-  vp->ip = cp->ra;
-
   /* Push the arguments. */
   for (i = 0; i < n; i++)
-    {
-      vp->sp++;
-      *vp->sp = argv_copy[i];
-    }
-
-  if (vp->sp > vp->sp_max_since_gc)
-      vp->sp_max_since_gc = vp->sp;
+    vp->sp[i + 1 - n] = argv_copy[i];
 
   /* The prompt captured a slice of the dynamic stack.  Here we wind
      those entries onto the current thread's stack.  We also have to
@@ -371,7 +399,6 @@ vm_reinstate_partial_continuation (struct scm_vm *vp, SCM 
cont,
           scm_dynstack_wind_1 (dynstack, walk);
       }
   }
-#undef RELOC
 }
 
 
@@ -810,8 +837,11 @@ static void
 return_unused_stack_to_os (struct scm_vm *vp)
 {
 #if HAVE_SYS_MMAN_H
-  scm_t_uintptr start = (scm_t_uintptr) vp->sp;
-  scm_t_uintptr end = (scm_t_uintptr) vp->sp_max_since_gc;
+  scm_t_uintptr start = (scm_t_uintptr) (vp->sp + 1);
+  scm_t_uintptr end = (scm_t_uintptr) vp->stack_limit;
+  /* The second condition is needed to protect against wrap-around.  */
+  if (vp->sp_max_since_gc < vp->stack_limit && vp->sp < vp->sp_max_since_gc)
+    end = (scm_t_uintptr) (vp->sp_max_since_gc + 1);
 
   start = ((start - 1U) | (page_size - 1U)) + 1U; /* round up */
   end = ((end - 1U) | (page_size - 1U)) + 1U; /* round up */
@@ -937,6 +967,8 @@ vm_expand_stack (struct scm_vm *vp)
       abort ();
     }
 
+  /* FIXME: Prevent GC while we expand the stack, to ensure that a
+     stack marker can trace the stack.  */
   if (stack_size > vp->stack_size)
     {
       SCM *old_stack;
@@ -955,7 +987,8 @@ vm_expand_stack (struct scm_vm *vp)
       if (reloc)
         {
           SCM *fp;
-          vp->fp += reloc;
+          if (vp->fp)
+            vp->fp += reloc;
           vp->sp += reloc;
           vp->sp_max_since_gc += reloc;
           fp = vp->fp;
@@ -1027,13 +1060,10 @@ scm_call_n (SCM proc, SCM *argv, size_t nargs)
 
   SCM_CHECK_STACK;
 
-  /* Check that we have enough space: 3 words for the boot
-     continuation, 3 + nargs for the procedure application, and 3 for
-     setting up a new frame.  */
-  base_frame_size = 3 + 3 + nargs + 3;
-  vp->sp += base_frame_size;
-  if (vp->sp >= vp->stack_limit)
-    vm_expand_stack (vp);
+  /* Check that we have enough space: 3 words for the boot continuation,
+     and 3 + nargs for the procedure application.  */
+  base_frame_size = 3 + 3 + nargs;
+  vm_push_sp (vp, vp->sp + base_frame_size);
   base = vp->sp + 1 - base_frame_size;
 
   /* Since it's possible to receive the arguments on the stack itself,
@@ -1054,10 +1084,6 @@ scm_call_n (SCM proc, SCM *argv, size_t nargs)
   base[4] = SCM_PACK (vp->ip); /* ra */
   base[5] = proc;
   vp->fp = &base[5];
-  vp->sp = &SCM_FRAME_LOCAL (vp->fp, nargs);
-
-  if (vp->sp > vp->sp_max_since_gc)
-    vp->sp_max_since_gc = vp->sp;
 
   {
     int resume = SCM_I_SETJMP (registers);


hooks/post-receive
-- 
GNU Guile