Re: [ ruby-Bugs-11134 ] Init_stack and ruby_init_stack fail to reinit stack (threads problem?)

Hi,

At Sat, 26 May 2007 14:29:55 +0900,
Gonzalo Garramuno wrote in [ruby-core:11252]:
> Personally, I'd keep Init_stack() and ruby_init_stack(), but
> expose only ruby_init_stack() in ruby.h (as is now).
> I'd add a new int (boolean) parameter to ruby_init_stack()
> to allow avoiding the STACK_CHECK().  This would break
> backwards compatability of this function, but as it has only
> recently been introduced that should not be too terrible.
> I'd merge Init_stack() to call ruby_init_stack() to avoid
> code duplication.

It does solve nothing.

The point of this problem is that ruby interpreter is not
thread-safe as a library, so that you must not call it in
native-threads other than the native-thread on which ruby
itself is running.  Instead, you have to send any requests to
the native-thread and process them in it.

> Finally, I'd probably remove the 1Mb clamping of the stack
> space that is used for win32/linux (not sure what its
> purpose is, either -- seems like if you are reading
> getrlimit values you should use them as is).

To try to catch stack overflow.

> A better patch would involve merging Init_stack and
> ruby_init_stack by having one call the other.

Sounds reasonable.

> For ruby1.9:
> 
> I'm not too familiar with the insides of ruby1.9 yet, so my
> question first is how does each thread keep knowledge of its
> own stack?  
> Currently the gc.c in ruby1.9 seems almost identical to 1.8
> in this aspect, so I'm a tad puzzled how this is working at
> all now.  It seems like YARV as it is right now is not safe
> at all for embedding.

It does know about the native-threads which are created in
ruby.  Therefore, you can't store VALUEs on the stacks of other
threads.


Index: eval.c
===================================================================
--- eval.c	(revision 12390)
+++ eval.c	(working copy)
@@ -1330,5 +1330,4 @@ char **rb_origenviron;
 
 void rb_call_inits _((void));
-void Init_stack _((VALUE*));
 void Init_heap _((void));
 void Init_ext _((void));
Index: gc.c
===================================================================
--- gc.c	(revision 12390)
+++ gc.c	(working copy)
@@ -1451,7 +1451,19 @@ ruby_set_stack_size(size)
 
 void
-Init_stack(addr)
+ruby_init_stack(addr
+#ifdef __ia64
+		, bsp
+#endif
+    )
     VALUE *addr;
+#ifdef __ia64
+    void *bsp;
+#endif
 {
+    size_t size = 0;
+#if defined(_WIN32) || defined(__CYGWIN__)
+    MEMORY_BASIC_INFORMATION m;
+#endif
+
 #ifdef __ia64
     if (rb_gc_register_stack_start == 0) {
@@ -1468,14 +1480,10 @@ Init_stack(addr)
 # endif
     }
-    {
-        VALUE *bsp = (VALUE*)rb_ia64_bsp();
-        if (rb_gc_register_stack_start == 0 ||
-            bsp < rb_gc_register_stack_start) {
-            rb_gc_register_stack_start = bsp;
-        }
+    if (rb_gc_register_stack_start == 0 ||
+	bsp < rb_gc_register_stack_start) {
+	rb_gc_register_stack_start = bsp;
     }
 #endif
 #if defined(_WIN32) || defined(__CYGWIN__)
-    MEMORY_BASIC_INFORMATION m;
     memset(&m, 0, sizeof(m));
     VirtualQuery(&m, &m, sizeof(m));
@@ -1500,61 +1508,22 @@ Init_stack(addr)
     rb_gc_stack_start = addr;
 #endif
-#ifdef HAVE_GETRLIMIT
+
+    /* set STACK_LEVEL_MAX */
+#if defined(_WIN32) || defined(__CYGWIN__)
+    size = (char *)mi.BaseAddress - (char *)mi.AllocationBase;
+#elif defined HAVE_GETRLIMIT
     {
 	struct rlimit rlim;
 
 	if (getrlimit(RLIMIT_STACK, &rlim) == 0) {
-	    unsigned int space = rlim.rlim_cur/5;
-
-	    if (space > 1024*1024) space = 1024*1024;
-	    STACK_LEVEL_MAX = (rlim.rlim_cur - space) / sizeof(VALUE);
+	    size = rlim.rlim_cur;
 	}
     }
 #endif
-}
-
-void ruby_init_stack(VALUE *addr
-#ifdef __ia64
-    , void *bsp
-#endif
-    )
-{
-    if (!rb_gc_stack_start ||
-        STACK_UPPER(&addr,
-                    rb_gc_stack_start > addr,
-                    rb_gc_stack_start < addr)) {
-        rb_gc_stack_start = addr;
+    if (size) {
+	size_t space = size / 5;
+	if (space > 1024*1024) space = 1024*1024;
+	STACK_LEVEL_MAX = (size - space) / sizeof(VALUE);
     }
-#ifdef __ia64
-    if (!rb_gc_register_stack_start ||
-        (VALUE*)bsp < rb_gc_register_stack_start) {
-        rb_gc_register_stack_start = (VALUE*)bsp;
-    }
-#endif
-#ifdef HAVE_GETRLIMIT
-    {
-        struct rlimit rlim;
-
-        if (getrlimit(RLIMIT_STACK, &rlim) == 0) {
-            unsigned int space = rlim.rlim_cur/5;
-
-            if (space > 1024*1024) space = 1024*1024;
-            STACK_LEVEL_MAX = (rlim.rlim_cur - space) / sizeof(VALUE);
-        }
-    }
-#elif defined _WIN32
-    {
-	MEMORY_BASIC_INFORMATION mi;
-	DWORD size;
-	DWORD space;
-
-	if (VirtualQuery(&mi, &mi, sizeof(mi))) {
-	    size = (char *)mi.BaseAddress - (char *)mi.AllocationBase;
-	    space = size / 5;
-	    if (space > 1024*1024) space = 1024*1024;
-	    STACK_LEVEL_MAX = (size - space) / sizeof(VALUE);
-	}
-    }
-#endif
 }
 
Index: ruby.h
===================================================================
--- ruby.h	(revision 12390)
+++ ruby.h	(working copy)
@@ -581,13 +581,13 @@ VALUE rb_require _((const char*));
 #ifdef __ia64
 void ruby_init_stack(VALUE*, void*);
-#define RUBY_INIT_STACK \
-    VALUE variable_in_this_stack_frame; \
-    ruby_init_stack(&variable_in_this_stack_frame, rb_ia64_bsp());
+#define Init_stack(addr) ruby_init_stack(addr, rb_ia64_bsp())
 #else
 void ruby_init_stack(VALUE*);
+#define Init_stack(addr) ruby_init_stack(addr)
+#endif
 #define RUBY_INIT_STACK \
     VALUE variable_in_this_stack_frame; \
-    ruby_init_stack(&variable_in_this_stack_frame);
-#endif
+    Init_stack(&variable_in_this_stack_frame);
+
 void ruby_init _((void));
 void ruby_options _((int, char**));


-- 
Nobu Nakada