[Gc] implement GC_get_stack_base by thr

Discussion:

[Gc] implement GC_get_stack_base by thr_stksegment in solaris

(too old to reply)

Louis Zhuang

2010-04-09 15:33:05 UTC

Hi, I think solaris specific thr_stksegment is the better API to implement
GC_get_stack_base, which avoids worrying SIGSEV from find_limit(). Please review
following patch.

Cheers, Louis

Index: os_dep.c
===================================================================
--- os_dep.c (revision 23713)
+++ os_dep.c (revision 23718)
@@ -1230,6 +1230,22 @@

#endif /* GC_LINUX_THREADS */

+#if defined(GC_SOLARIS_THREADS)
+
+#include <thread.h>
+#include <signal.h>
+
+int GC_get_stack_base(struct GC_stack_base *b)
+{
+ stack_t s;
+ thr_stksegment(&s);
+ b -> mem_base = s.ss_sp;
+ return GC_SUCCESS;
+}
+#define HAVE_GET_STACK_BASE
+
+#endif /* GC_SOLARIS_THREADS */
+
#ifndef HAVE_GET_STACK_BASE
/* Retrieve stack base. */
/* Using the GC_find_limit version is risky. */

Ivan Maidanski

2010-04-10 13:16:53 UTC

Permalink

Post by Louis Zhuang
Hi, I think solaris specific thr_stksegment is the better API to implement
GC_get_stack_base, which avoids worrying SIGSEV from find_limit(). Please review
following patch.

The idea looks good. But the implementation is not so easy. Due to the thr_stksegment bug, it shouldn't be called more than once for the main thread (note that GC_get_stack_base is an API function). Next, thr_stksegment() returned value should be checked (non-zero means error, it is ok to abort in that case). And, add, please, a GC_ASSERT on the returned ss_sp value to be greater than the current stack frame).

Also, thr_stksegment() may be not available (e.g. in case of strict ANSI), so it's good to add a macro turning off thr_stksegment usage (e.g., NO_THR_STKSEGMENT).

It would be also good to submit the patch against the recent CVS (and use GC_get_stack_base() proto definition decorated with GC_API/GC_CALL).

Post by Louis Zhuang
Cheers, Louis
Index: os_dep.c
===================================================================
--- os_dep.c (revision 23713)
+++ os_dep.c (revision 23718)
@@ -1230,6 +1230,22 @@
#endif /* GC_LINUX_THREADS */
+#if defined(GC_SOLARIS_THREADS)
+
+#include <thread.h>
+#include <signal.h>
+
+int GC_get_stack_base(struct GC_stack_base *b)
+{
+ stack_t s;
+ thr_stksegment(&s);
+ b -> mem_base = s.ss_sp;
+ return GC_SUCCESS;
+}
+#define HAVE_GET_STACK_BASE
+
+#endif /* GC_SOLARIS_THREADS */
+
#ifndef HAVE_GET_STACK_BASE
/* Retrieve stack base. */
/* Using the GC_find_limit version is risky. */

Bye.

Boehm, Hans

2010-04-13 21:30:01 UTC

Permalink

From: Ivan Maidanski
Sent: Saturday, April 10, 2010 6:17 AM
To: Louis Zhuang
Subject: Re: [Gc] implement GC_get_stack_base by
thr_stksegment in solaris
Fri, 9 Apr 2010 15:33:05 +0000 (UTC) Louis Zhuang

Post by Louis Zhuang
Hi, I think solaris specific thr_stksegment is the better API to
implement GC_get_stack_base, which avoids worrying SIGSEV from
find_limit(). Please review following patch.

The idea looks good. But the implementation is not so easy.
Due to the thr_stksegment bug,

I'm not sure I understand this. Which bug?

I agree with the other comments, and that this patch looks potentially useful.

Hans

Louis Zhuang

2010-04-13 23:43:48 UTC

Permalink

Post by Louis Zhuang
Hi, I think solaris specific thr_stksegment is the better API to
implement GC_get_stack_base, which avoids worrying SIGSEV from
find_limit(). Please review following patch.

The idea looks good. But the implementation is not so easy.
Due to the thr_stksegment bug,

I'm not sure I understand this. Which bug?
I agree with the other comments, and that this patch looks potentially useful.
Hans

Sorry for late. I've tested in my solaris 10 box, thr_stksement works
fine in repeated calls. Are you are referring an old bug? I can not
access cvs behind firm firewall so i'll submit patch againt 7.2-alpha4
tomorrow, hopefully it is not too out-of-date to be applied...

Cheers, Louis

Ivan Maidanski

2010-04-14 05:15:17 UTC

Permalink

Post by Louis Zhuang

Post by Louis Zhuang
Hi, I think solaris specific thr_stksegment is the better API to
implement GC_get_stack_base, which avoids worrying SIGSEV from
find_limit(). Please review following patch.

The idea looks good. But the implementation is not so easy.
Due to the thr_stksegment bug,

I'm not sure I understand this? Which bug?

I meant HotSpot bug 4352906. It's unclear how ss_sp value is trashed (and who really does this) after the first call. See:
http://www.google.com/codesearch/p?hl=en#aRIt9pqzOVI/src/os/solaris/vm/os_solaris.hpp&q=thr_stksegment&sa=N&cd=2&ct=rc

On the other hand, implementing the workaround complicates the code - it would require LOCK and GC_lookup_thread() which could return null (since get_main_stack_base is called before GC_thr_init).

Post by Louis Zhuang

I agree with the other comments, and that this patch looks potentially useful.
Hans

You could fetch os_dep.c from:
http://bdwgc.cvs.sourceforge.net/viewvc/*checkout*/bdwgc/bdwgc/os_dep.c

Post by Louis Zhuang
Cheers, Louis

Louis Zhuang

2010-04-14 16:31:07 UTC

Permalink

Post by Ivan Maidanski
I meant HotSpot bug 4352906. It's unclear how ss_sp value is trashed (and who

really does this) after the
http://www.google.com/codesearch/p?hl=en#aRIt9pqzOVI/src/os/solaris/vm/os_solaris.hpp&q=thr_stksegment&sa=N&cd=2&ct=rc

Post by Ivan Maidanski
On the other hand, implementing the workaround complicates the code - it would

require LOCK and

Post by Ivan Maidanski
GC_lookup_thread() which could return null (since get_main_stack_base is

called before GC_thr_init).

It seems a Solaris JDK specific bug. In such situaion, we've cached main stack
base in GC_stackbottom @ misc:845, is it safe to assume we only call
GC_get_stack_base once in primordial thread? Following patch is generated
against latest CVS version. Thanks, Louis

--- os_dep.c.orig 2010-04-14 17:21:55.000000000 +0100
+++ os_dep.c 2010-04-14 17:21:57.000000000 +0100
@@ -1306,6 +1306,30 @@

#endif /* GC_OPENBSD_THREADS */

+#ifdef GC_SOLARIS_THREADS
+
+# define HAVE_GET_STACK_BASE
+
+#include <thread.h>
+#include <signal.h>
+
+/* from http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=4352906 */
+/* thr_stksegment is not reliable for primordial thread under JDK */
+/* However we already cache primordial thread's stack base in */
+/* GC_stackbottom so we are sort of safe */
+GC_API int GC_CALL GC_get_stack_base(struct GC_stack_base *b)
+{
+ stack_t s;
+ if (!thr_stksegment(&s)) {
+ GC_abort("thr_stksegment error");
+ }
+ GC_ASSERT(s.ss_sp > (&s));
+ b -> mem_base = s.ss_sp;
+ return GC_SUCCESS;
+}
+
+#endif /* GC_SOLARIS_THREADS */
+
#ifndef HAVE_GET_STACK_BASE
/* Retrieve stack base. */
/* Using the GC_find_limit version is risky. */

Louis Zhuang

2010-04-14 17:27:43 UTC

Permalink

Post by Louis Zhuang
It seems a Solaris JDK specific bug. In such situaion, we've cached main stack
GC_get_stack_base once in primordial thread? Following patch is generated
against latest CVS version. Thanks, Louis

Pls apply following patch with ANSI C guard the same as solaris header.
Rgds, Louis

--- os_dep.c.orig 2010-04-14 18:25:50.000000000 +0100
+++ os_dep.c 2010-04-14 18:25:55.000000000 +0100
@@ -1306,6 +1306,31 @@

#endif /* GC_OPENBSD_THREADS */

+#if defined(GC_SOLARIS_THREADS) \
+ && (!defined(_STRICT_STDC) || defined(__EXTENSIONS__))
+
+# define HAVE_GET_STACK_BASE
+
+#include <thread.h>
+#include <signal.h>
+
+/* from http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=4352906 */
+/* thr_stksegment is not reliable for primordial thread under JDK */
+/* However we already cache primordial thread's stack base in */
+/* GC_stackbottom so we are sort of safe */
+GC_API int GC_CALL GC_get_stack_base(struct GC_stack_base *b)
+{
+ stack_t s;
+ if (!thr_stksegment(&s)) {
+ GC_abort("thr_stksegment error");
+ }
+ GC_ASSERT(s.ss_sp > (&s));
+ b -> mem_base = s.ss_sp;
+ return GC_SUCCESS;
+}
+
+#endif /* GC_SOLARIS_THREADS */
+
#ifndef HAVE_GET_STACK_BASE
/* Retrieve stack base. */
/* Using the GC_find_limit version is risky. */

Ivan Maidanski

2010-04-15 06:36:57 UTC

Permalink

Post by Louis Zhuang

It seems a Solaris JDK specific bug. In such situation, we've cached main stack
GC_get_stack_base once in primordial thread? Following patch is generated
against latest CVS version. Thanks, Louis

This is a public function so we can't assume it is never called by the user for the main thread. (I can't make a decision myself whether it's ok to ignore the bug or add a workaround (caching+locking) for it.)

Post by Louis Zhuang
Pls apply following patch with ANSI C guard the same as solaris header.
Rgds, Louis

Is the whole bdwgc really compilable with -D _STRICT_STDC on Solaris? (If not then there is no need for testing _STRICT_STDC/__EXTENSIONS__).

Post by Louis Zhuang
--- os_dep.c.orig 2010-04-14 18:25:50.000000000 +0100
+++ os_dep.c 2010-04-14 18:25:55.000000000 +0100
@@ -1306,6 +1306,31 @@
#endif /* GC_OPENBSD_THREADS */
+#if defined(GC_SOLARIS_THREADS) \
+ && (!defined(_STRICT_STDC) || defined(__EXTENSIONS__))

I think it's better to have a macro to fall back to the default strategy
(e.g. NO_THR_STKSEGMENT).

Post by Louis Zhuang
+
+# define HAVE_GET_STACK_BASE
+
+#include <thread.h>
+#include <signal.h>
+
+/* from http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=4352906 */
+/* thr_stksegment is not reliable for primordial thread under JDK */
+/* However we already cache primordial thread's stack base in */
+/* GC_stackbottom so we are sort of safe */
+GC_API int GC_CALL GC_get_stack_base(struct GC_stack_base *b)
+{
+ stack_t s;
+ if (!thr_stksegment(&s)) {

According to the spec, 0 means no error.

Post by Louis Zhuang
+ GC_abort("thr_stksegment error");
+ }
+ GC_ASSERT(s.ss_sp > (&s));

It's "nicer" to write: GC_ASSERT(&s HOTTER_THAN s.ss_sp);

Post by Louis Zhuang
+ b -> mem_base = s.ss_sp;
+ return GC_SUCCESS;
+}
+
+#endif /* GC_SOLARIS_THREADS */
+
#ifndef HAVE_GET_STACK_BASE
/* Retrieve stack base. */
/* Using the GC_find_limit version is risky. */