unofficial mirror of guile-devel@gnu.org 
 help / color / mirror / Atom feed
* [PATCH] Fix continuation problems on IA64.
@ 2008-05-07 23:46 Neil Jerram
  2008-05-08 20:09 ` Ludovic Courtès
  0 siblings, 1 reply; 33+ messages in thread
From: Neil Jerram @ 2008-05-07 23:46 UTC (permalink / raw)
  To: Guile Development

FYI, I finally fixed some problems on IA64 that have been outstanding
for ages (at least about 2 years).  Following is a big spiel about it,
and the actual patch (against 1.8.4 release).  If anyone has any
comments or questions, please let me know.  (And if not, I'll commit
in a couple of days' time.)

Regards,
      Neil


* Specific problems in IA64 make check

** test-unwind

Representation of the relevant dynamic context:

                  non-rewindable
           catch      frame       make cont.
  o----o-----a----------b-------------c
        \
         \             call cont.
          o-----o-----------d

A continuation is captured at (c), with a non-rewindable frame in the
dynamic context at (b).  If a rewind through that frame was attempted,
Guile would throw to the catch at (a).  Then the context unwinds back
past (a), then winds forwards again, and the captured continuation is
called at (d).

We should end up at the catch at (a).  On ia64, we get an "illegal
instruction".

The problem is that Guile does not restore the ia64 register backing
store (RBS) stack (which is saved off when the continuation is
captured) until all the unwinding and rewinding is done.  Therefore,
when the rewind code (scm_i_dowinds) hits the non-rewindable frame at
(b), the RBS stack hasn't yet been restored.  The throw finds the
jmp_buf (for the catch at (a)) correctly from the dynamic context, and
jumps back to (a), but the RBS stack is invalid, hence the illegal
instruction.

This could be fixed by restoring the RBS stack earlier, at the same
point (copy_stack) where the normal stack is restored.  But that
causes a problem in the next test...

** continuations.test

The dynamic context diagram for this case is similar:

                   non-rewindable
  catch                 frame       make cont.
    a----x-----o----------b-------------c
          \
           \    call cont.
            o-------d

The only significant difference is that the catch point (a) is
upstream of where the dynamic context forks.  This means that the RBS
stack at (d) already contains the correct RBS contents for throwing
back to (a), so it doesn't matter whether the RBS stack that was saved
off with the continuation gets restored.

This test passes with the Guile 1.8.4 code, but fails (with an
"illegal instruction") when the code is changed to restore the RBS
stack earlier as described above.

The problem now is that the RBS stack is being restored _too_ early;
specifically when there is still stuff to do that relies on the old
RBS contents.  When a continuation is called, the sequence of relevant
events is:

  (1) Grow the (normal) stack until it is bigger than the (normal)
      stack saved off in the continuation.  (scm_dynthrow, grow_stack)

  (2) scm_i_dowinds calls itself recursively, such that

      (2.1) for each rewind (from (x) to (c)) that will be needed,
            another frame is added to the stack (both normal and RBS),
            with local variables specifying the required rewind; the
            rewinds don't actually happen yet, they will happen when
            the stack unwinds again through these frames

      (2.2) required unwinds - back from where the continuation was
            called (d) to the fork point (x) - are done immediately.

  (3) The normal (i.e. non-RBS) stack that was stored in the
      continuation is restored (i.e. copied on top of the actual
      stack).

      Note that this doesn't overwrite the frames that were added in
      (2.1), because the growth in (1) ensures that the added frames
      are beyond the end of the restored stack.

  (4) ? Restore the RBS stack here too ?

  (5) Return (from copy_stack) through the (2.1) frames, which means
      that the rewinds now happen.

  (6) setcontext (or longjmp) to the context (c) where the
      continuation was captured.

The trouble is that step (1) does not create space in the RBS stack in
the same kind of way that it does for the normal stack.  Therefore, if
the saved (in the continuation) RBS stack is big enough, it can
overwrite the RBS of the (2.1) frames that still need to complete.
This causes an illegal instruction when we return through those frames
and try to perform the rewinds.

* Fix

The key to the fix is that the saved RBS stack only needs to be
restored at some point before the next setcontext call, and that doing
it as close to the setcontext call as possible will avoid bad
interactions with the pre-setcontext stack.  Therefore we do the
restoration at the last possible point, immediately before the next
setcontext call.

The situation is complicated by there being two ways that the next
setcontext call can happen.

  - If the unwinding and rewinding is all successful, the next
    setcontext will be the one from step (6) above.  This is the
    "normal" continuation invocation case.

  - If one of the rewinds throws an error, the next setcontext will
    come from the throw implementation code.  (And the one in step (6)
    will never happen.)  This is the rewind error case.

In the rewind error case, the code calling setcontext knows nothing
about the continuation.  So to cover both cases, we:

  - copy (in step (4) above) the address and length of the
    continuation's saved RBS stack to the current thread state
    (SCM_I_CURRENT_THREAD)

  - modify all setcontext callers so that they check the current
    thread state for a saved RBS stack, and restore it if so before
    calling setcontext.

* Notes

** I think rewinders cannot rely on using any stack data

Unless it can be guaranteed that the data won't go into a register.
I'm not 100% sure about this, but I think it follows from the fact
that the RBS stack is not restored until after the rewinds have
happened.

Note that this isn't a regression caused by the current fix.  In Guile
1.8.4, the RBS stack was restored _after_ the rewinds, and this is
still the case now.

** Most setcontext calls for `throw' don't need to change the RBS stack

In the absence of continuation invocation, the setcontext call in the
throw implementation code always sets context to a place higher up the
same stack (both normal and RBS), hence no stack restoration is
needed.

* Other changes

** Using setcontext for all non-local jumps (for __ia64__)

Along the way, I read a claim somewhere that setcontext was more
reliable than longjmp, in cases where the stack has been manipulated.

I don't now have any reason to believe this, but it seems reasonable
anyway to leave the __ia64__ code using getcontext/setcontext, instead
of setjmp/longjmp.

(I think the only possible argument against this would be performance -
if getcontext was significantly slower than setjmp.  It that proves to
be the case, we should revisit this.)

** Capping RBS base for non-main threads

Somewhere else along the way, I hit a problem in GC, involving the RBS
stack of a non-main thread.  The problem was, in
SCM_MARK_BACKING_STORE, that scm_ia64_register_backing_store_base was
returning a value that was massively greater than the value of
scm_ia64_ar_bsp, leading to a seg fault.  This is because the
implementation of scm_ia64_register_backing_store_base is only valid
for the main thread.  I couldn't find a neat way of getting the true
RBS base of a non-main thread, but one idea is simply to call
scm_ia64_ar_bsp when guilifying a thread, and use the value returned
as an upper bound for that thread's RBS base.  (Note that the RBS
stack grows upwards.)

(Were it not for scm_init_guile, we could be much more definitive
about this.  We could take the value of scm_ia64_ar_bsp as a
definitive base address for the part of the RBS stack that Guile cares
about.  We could also then discard
scm_ia64_register_backing_store_base.)
---
 libguile/ChangeLog       |   35 +++++++++++++++++++++++++++
 libguile/__scm.h         |   18 +++++++++++++-
 libguile/continuations.c |   58 +++++++++++++++++++++------------------------
 libguile/continuations.h |    2 -
 libguile/threads.c       |   20 ++++++++++++++-
 libguile/threads.h       |    5 ++++
 libguile/throw.c         |    6 ++++
 7 files changed, 108 insertions(+), 36 deletions(-)

diff --git a/libguile/ChangeLog b/libguile/ChangeLog
index 28c439b..58dd248 100644
--- a/libguile/ChangeLog
+++ b/libguile/ChangeLog
@@ -1,3 +1,38 @@
+2008-05-08  Neil Jerram  <neil@ossau.uklinux.net>
+
+	* throw.c (scm_ithrow): For IA64 add a return statement, to
+	appease GCC.
+
+	* threads.h (scm_i_thread): New IA64 fields:
+	register_backing_store_base and pending_rbs_continuation.
+
+	* threads.c (guilify_self_1): For IA64: cap RBS base address at
+	the current value of scm_ia64_ar_bsp, and store the capped value
+	in thread state.
+	(SCM_MARK_BACKING_STORE): Use thread->register_backing_store_base
+	instead of scm_ia64_register_backing_store_base().
+	(scm_threads_mark_stacks): Add "&" in "&t->regs", so that the code
+	works both for jmp_buf defined as an array, and jmp_buf defined as
+	a struct.
+
+	* continuations.h (scm_t_contregs): Remove `fresh' and `ctx'
+	fields; these are now inside the IA64 definition of `jmp_buf'.
+
+	* continuations.c (scm_make_continuation): Simplify, by moving
+	some of the IA64 code inside the definition of "setjmp", and by
+	some obvious commonizations.  For IA64 register backing store
+	(RBS) stack base, use thread->register_backing_store_base instead
+	of scm_ia64_register_backing_store_base().
+	(copy_stack): For IA64, store pointer to continuation being
+	invoked in thread state, so we can restore the continuation's RBS
+	stack just before the next setcontext call.
+	(copy_stack_and_call): Don't restore RBS stack explicitly here.
+	It will be restored, if appropriate, inside the longjmp call.
+	(scm_ia64_longjmp): New function.
+
+	* __scm.h (setjmp, longjmp, jmp_buf): For IA64, implement using
+	getcontext and setcontext.
+
 2008-02-16  Ludovic Courtès  <ludo@gnu.org>
 
 	* gc_os_dep.c: Add NetBSD/alpha support.  Patch by Greg Troxel
diff --git a/libguile/__scm.h b/libguile/__scm.h
index 3d6d9a7..b198f9d 100644
--- a/libguile/__scm.h
+++ b/libguile/__scm.h
@@ -402,7 +402,23 @@
 #  define setjmp setjump
 #  define longjmp longjump
 # else				/* ndef _CRAY1 */
-#  include <setjmp.h>
+#  if defined (__ia64__)
+/* For IA64, emulate the setjmp API using getcontext. */
+#   include <signal.h>
+#   include <ucontext.h>
+    typedef struct {
+      ucontext_t ctx;
+      int fresh;
+    } jmp_buf;
+#   define setjmp(JB)				        \
+      ( (JB).fresh = 1,				        \
+        getcontext (&((JB).ctx)),			\
+        ((JB).fresh ? ((JB).fresh = 0, 0) : 1) )
+#   define longjmp(JB,VAL) scm_ia64_longjmp (&(JB), VAL)
+    void scm_ia64_longjmp (jmp_buf *, int);
+#  else                 	/* ndef __ia64__ */
+#   include <setjmp.h>
+#  endif			/* ndef __ia64__ */
 # endif				/* ndef _CRAY1 */
 #endif				/* ndef vms */
 
diff --git a/libguile/continuations.c b/libguile/continuations.c
index 39785a5..80a2790 100644
--- a/libguile/continuations.c
+++ b/libguile/continuations.c
@@ -124,47 +124,30 @@ scm_make_continuation (int *first)
   continuation->offset = continuation->stack - src;
   memcpy (continuation->stack, src, sizeof (SCM_STACKITEM) * stack_size);
 
-#ifdef __ia64__
-  continuation->fresh = 1;
-  getcontext (&continuation->ctx);
-  if (continuation->fresh)
+  *first = !setjmp (continuation->jmpbuf);
+  if (*first)
     {
+#ifdef __ia64__
       continuation->backing_store_size =
-	(char *) scm_ia64_ar_bsp(&continuation->ctx)
+	(char *) scm_ia64_ar_bsp(&continuation->jmpbuf.ctx)
 	-
-	(char *) scm_ia64_register_backing_store_base ();
+	(char *) thread->register_backing_store_base;
       continuation->backing_store = NULL;
       continuation->backing_store = 
         scm_gc_malloc (continuation->backing_store_size,
 		       "continuation backing store");
       memcpy (continuation->backing_store, 
-              (void *) scm_ia64_register_backing_store_base (), 
+              (void *) thread->register_backing_store_base, 
               continuation->backing_store_size);
-      *first = 1;
-      continuation->fresh = 0;
+#endif /* __ia64__ */
       return cont;
     }
   else
     {
       SCM ret = continuation->throw_value;
-      *first = 0;
       continuation->throw_value = SCM_BOOL_F;
       return ret;
     }
-#else /* !__ia64__ */
-  if (setjmp (continuation->jmpbuf))
-    {
-      SCM ret = continuation->throw_value;
-      *first = 0;
-      continuation->throw_value = SCM_BOOL_F;
-      return ret;
-    }
-  else
-    {
-      *first = 1;
-      return cont;
-    }
-#endif /* !__ia64__ */
 }
 #undef FUNC_NAME
 
@@ -218,6 +201,9 @@ copy_stack (void *data)
   copy_stack_data *d = (copy_stack_data *)data;
   memcpy (d->dst, d->continuation->stack,
 	  sizeof (SCM_STACKITEM) * d->continuation->num_stack_items);
+#ifdef __ia64__
+  SCM_I_CURRENT_THREAD->pending_rbs_continuation = d->continuation;
+#endif
 }
 
 static void
@@ -235,16 +221,26 @@ copy_stack_and_call (scm_t_contregs *continuation, SCM val,
   scm_i_set_last_debug_frame (continuation->dframe);
 
   continuation->throw_value = val;
-#ifdef __ia64__
-  memcpy (scm_ia64_register_backing_store_base (),
-          continuation->backing_store,
-          continuation->backing_store_size);
-  setcontext (&continuation->ctx);
-#else
   longjmp (continuation->jmpbuf, 1);
-#endif
 }
 
+#ifdef __ia64__
+void
+scm_ia64_longjmp (jmp_buf *JB, int VAL)
+{
+  scm_i_thread *t = SCM_I_CURRENT_THREAD;
+
+  if (t->pending_rbs_continuation)
+    {
+      memcpy (t->register_backing_store_base,
+	      t->pending_rbs_continuation->backing_store,
+	      t->pending_rbs_continuation->backing_store_size);
+      t->pending_rbs_continuation = NULL;
+    }
+  setcontext (&JB->ctx);
+}
+#endif
+
 /* Call grow_stack until the stack space is large enough, then, as the current
  * stack frame might get overwritten, let copy_stack_and_call perform the
  * actual copying and continuation calling.
diff --git a/libguile/continuations.h b/libguile/continuations.h
index 0274c1b..f6fb96a 100644
--- a/libguile/continuations.h
+++ b/libguile/continuations.h
@@ -46,8 +46,6 @@ typedef struct
   jmp_buf jmpbuf;
   SCM dynenv;
 #ifdef __ia64__
-  ucontext_t ctx;
-  int fresh;
   void *backing_store;
   unsigned long backing_store_size;
 #endif /* __ia64__ */
diff --git a/libguile/threads.c b/libguile/threads.c
index 858a1eb..609fc99 100644
--- a/libguile/threads.c
+++ b/libguile/threads.c
@@ -423,6 +423,22 @@ guilify_self_1 (SCM_STACKITEM *base)
   t->pending_asyncs = 1;
   t->last_debug_frame = NULL;
   t->base = base;
+#ifdef __ia64__
+  /* Calculate and store off the base of this thread's register
+     backing store (RBS).  Unfortunately our implementation(s) of
+     scm_ia64_register_backing_store_base are only reliable for the
+     main thread.  For other threads, therefore, find out the current
+     top of the RBS, and use that as a maximum. */
+  t->register_backing_store_base = scm_ia64_register_backing_store_base ();
+  {
+    ucontext_t ctx;
+    void *bsp;
+    getcontext (&ctx);
+    bsp = scm_ia64_ar_bsp (&ctx);
+    if (t->register_backing_store_base > bsp)
+      t->register_backing_store_base = bsp;
+  }
+#endif
   t->continuation_root = SCM_EOL;
   t->continuation_base = base;
   scm_i_pthread_cond_init (&t->sleep_cond, NULL);
@@ -1350,7 +1366,7 @@ SCM_DEFINE (scm_broadcast_condition_variable, "broadcast-condition-variable", 1,
     scm_mark_locations ((SCM_STACKITEM *) &ctx.uc_mcontext,           \
       ((size_t) (sizeof (SCM_STACKITEM) - 1 + sizeof ctx.uc_mcontext) \
        / sizeof (SCM_STACKITEM)));                                    \
-    bot = (SCM_STACKITEM *) scm_ia64_register_backing_store_base ();  \
+    bot = (SCM_STACKITEM *) SCM_I_CURRENT_THREAD->register_backing_store_base;  \
     top = (SCM_STACKITEM *) scm_ia64_ar_bsp (&ctx);                   \
     scm_mark_locations (bot, top - bot); } while (0)
 #else
@@ -1374,7 +1390,7 @@ scm_threads_mark_stacks (void)
 #else
       scm_mark_locations (t->top, t->base - t->top);
 #endif
-      scm_mark_locations ((SCM_STACKITEM *) t->regs,
+      scm_mark_locations ((SCM_STACKITEM *) &t->regs,
 			  ((size_t) sizeof(t->regs)
 			   / sizeof (SCM_STACKITEM)));
     }
diff --git a/libguile/threads.h b/libguile/threads.h
index 09939b0..d58a0fb 100644
--- a/libguile/threads.h
+++ b/libguile/threads.h
@@ -28,6 +28,7 @@
 #include "libguile/root.h"
 #include "libguile/iselect.h"
 #include "libguile/dynwind.h"
+#include "libguile/continuations.h"
 
 #if SCM_USE_PTHREAD_THREADS
 #include "libguile/pthread-threads.h"
@@ -107,6 +108,10 @@ typedef struct scm_i_thread {
   SCM_STACKITEM *base;
   SCM_STACKITEM *top;
   jmp_buf regs;
+#ifdef __ia64__
+  void *register_backing_store_base;
+  scm_t_contregs *pending_rbs_continuation;
+#endif
 
 } scm_i_thread;
 
diff --git a/libguile/throw.c b/libguile/throw.c
index 1c25463..119d0bd 100644
--- a/libguile/throw.c
+++ b/libguile/throw.c
@@ -824,6 +824,12 @@ scm_ithrow (SCM key, SCM args, int noreturn SCM_UNUSED)
   /* Otherwise, it's some random piece of junk.  */
   else
     abort ();
+
+#ifdef __ia64__
+  /* On IA64, we #define longjmp as setcontext, and GCC appears not to
+     know that that doesn't return. */
+  return SCM_UNSPECIFIED;
+#endif
 }
 
 
-- 
1.5.4.2






^ permalink raw reply related	[flat|nested] 33+ messages in thread
end of thread, other threads:[~2008-06-23 12:20 UTC | newest]

Thread overview: 33+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2008-05-07 23:46 [PATCH] Fix continuation problems on IA64 Neil Jerram
2008-05-08 20:09 ` Ludovic Courtès
2008-05-08 21:29   ` Neil Jerram
2008-05-09  8:15     ` Ludovic Courtès
2008-05-09 22:19       ` Neil Jerram
2008-05-11  3:06         ` Ludovic Courtès
2008-05-12 21:02           ` Neil Jerram
2008-05-14  3:45             ` Internal visibility Ludovic Courtès
2008-05-27 21:32               ` Ludovic Courtès
2008-05-31 21:23                 ` Ludovic Courtès
2008-06-01  8:00                   ` Neil Jerram
2008-06-01 11:05                     ` Ludovic Courtès
2008-06-01 20:48                       ` Neil Jerram
2008-06-01 22:02                         ` Ludovic Courtès
2008-06-01 20:24                   ` Han-Wen Nienhuys
2008-06-09 18:10                     ` Neil Jerram
2008-06-10  1:51                       ` Han-Wen Nienhuys
2008-06-10  7:37                         ` Ludovic Courtès
2008-06-10  8:04                           ` Thien-Thi Nguyen
2008-06-10 12:09                             ` Ludovic Courtès
2008-06-11  7:49                               ` Thien-Thi Nguyen
2008-06-11 12:20                                 ` Ludovic Courtès
2008-06-12 20:45                                   ` Mike Gran
2008-06-23 12:20                                     ` Ludovic Courtès
2008-06-11  4:05                           ` Han-Wen Nienhuys
2008-06-11  7:18                             ` Ludovic Courtès
2008-06-11  7:24                             ` Clinton Ebadi
2008-06-11  7:39                               ` Ludovic Courtès
2008-06-11 16:09                               ` Han-Wen Nienhuys
2008-06-23 12:18                                 ` Ludovic Courtès
2008-06-11  7:37                             ` Neil Jerram
2008-05-12 22:18     ` [PATCH] Fix continuation problems on IA64 Neil Jerram
2008-05-14  2:55       ` Ludovic Courtès

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).