[RFA/JVMTI] New JVMTI Environment Initialization/Allocation

[RFA/JVMTI] New JVMTI Environment Initialization/Allocation

[Keith Seitz]

[-- Attachment #1: Type: text/plain, Size: 1507 bytes --]

Hi,

This patch rewrites the JVMTI allocation routine so that each JVMTI 
environment requested is allocated anew, since each environment will 
need (at the very least) a place to store event notification information 
(coming to a patch list near you soon).

I've also snuck in another little patch which adds another common test 
macro, this time for JVMTI_ERROR_ILLEGAL_ARGUMENT.

[Heaven help me if my linked list impl is off. I'm not used to not using 
JCL or STL anymore!]

I should mention that I have some tests written for all this new JVMTI 
work, but I need to figure out how to integrate this with existing 
framework. I'll either submit that if I figure it out or ask questions 
if I am defeated.

Keith

ChangeLog
2006-08-31  Keith Seitz  <keiths@redhat.com>

         * include/jvm.h (_Jv_JVMTI_Init): Declare.
         * jvmti.cc (_Jv_JVMTI_Init): New function.
         * prims.cc (_Jv_CreateJavaVM): Initialize JVMTI.

         * jvmti.cc (ILLEGAL_ARGUMENT): New macro.
         (_Jv_JVMTI_Allocate): Use ILLEGAL_ARUMENT.

         * jvmti.cc (_jvmtiEnvironments): New linked list of
         JVMTI environments.
         (FOREACH_ENVIRONMENT): New macro.
         (_envListLock): New object to act as synchronization lock
         for _jvmtiEnvironments.
         (_Jv_JVMTI_DisposeEnvironment): Check for NULL environment.
         Remove the environment from the list of known environments.
         (_Jv_GetJVMTIEnv): Add the new environment to the list
         of known environments.

[-- Attachment #2: jvmti-new-env.patch --]
[-- Type: text/x-patch, Size: 3915 bytes --]

Index: include/jvm.h
===================================================================
--- include/jvm.h	(revision 116604)
+++ include/jvm.h	(working copy)
@@ -573,10 +573,13 @@
 struct _Jv_JavaVM;
 _Jv_JavaVM *_Jv_GetJavaVM (); 
 
-/* Get the JVMTI environment */
+/* Get a JVMTI environment */
 struct _Jv_JVMTIEnv;
 _Jv_JVMTIEnv *_Jv_GetJVMTIEnv (void);
 
+/* Initialize JVMTI */
+extern void _Jv_JVMTI_Init (void);
+
 // Some verification functions from defineclass.cc.
 bool _Jv_VerifyFieldSignature (_Jv_Utf8Const*sig);
 bool _Jv_VerifyMethodSignature (_Jv_Utf8Const*sig);
Index: prims.cc
===================================================================
--- prims.cc	(revision 116604)
+++ prims.cc	(working copy)
@@ -1484,6 +1484,7 @@
   _Jv_platform_initialize ();
 
   _Jv_JNI_Init ();
+  _Jv_JVMTI_Init ();
 
   _Jv_GCInitializeFinalizers (&::gnu::gcj::runtime::FinalizerThread::finalizerReady);
 
Index: jvmti.cc
===================================================================
--- jvmti.cc	(revision 116611)
+++ jvmti.cc	(working copy)
@@ -22,6 +22,7 @@
 #include <gnu/gcj/runtime/BootClassLoader.h>
 #include <java/lang/Class.h>
 #include <java/lang/ClassLoader.h>
+#include <java/lang/Object.h>
 #include <java/lang/Thread.h>
 #include <java/lang/Throwable.h>
 #include <java/lang/VMClassLoader.h>
@@ -39,6 +40,17 @@
   _Jv_ConditionVariable_t condition;
 };
 
+// A simple linked list of all JVMTI environments
+struct jvmti_env_list
+{
+  jvmtiEnv *env;
+  struct jvmti_env_list *next;
+};
+static struct jvmti_env_list *_jvmtiEnvironments = NULL;
+static java::lang::Object *_envListLock = NULL;
+#define FOREACH_ENVIRONMENT(Ele) \
+  for (Ele = _jvmtiEnvironments; Ele != NULL; Ele = Ele->next)
+
 // Some commonly-used checks
 
 #define THREAD_DEFAULT_TO_CURRENT(jthread)				\
@@ -58,6 +70,9 @@
 #define NULL_CHECK(Ptr)					\
   if (Ptr == NULL) return JVMTI_ERROR_NULL_POINTER;
 
+#define ILLEGAL_ARGUMENT(Cond)				\
+  if ((Cond)) return JVMTI_ERROR_ILLEGAL_ARGUMENT
+
 static jvmtiError JNICALL
 _Jv_JVMTI_SuspendThread (MAYBE_UNUSED jvmtiEnv *env, jthread thread)
 {
@@ -195,8 +210,7 @@
 _Jv_JVMTI_Allocate (MAYBE_UNUSED jvmtiEnv *env, jlong size,
 		    unsigned char **result)
 {
-  if (size < 0)
-    return JVMTI_ERROR_ILLEGAL_ARGUMENT;
+  ILLEGAL_ARGUMENT (size < 0);
   NULL_CHECK (result);
   if (size == 0)
     *result = NULL;
@@ -437,7 +451,32 @@
 static jvmtiError JNICALL
 _Jv_JVMTI_DisposeEnvironment (jvmtiEnv *env)
 {
-  // All we need to do is free memory allocated by _Jv_GetJVMTIEnv
+  NULL_CHECK (env);
+
+  if (_jvmtiEnvironments == NULL)
+    return JVMTI_ERROR_INVALID_ENVIRONMENT;
+  else
+    {
+      JvSynchronize dummy (_envListLock);
+      if (_jvmtiEnvironments->env == env)
+	{
+	  _Jv_Free (_jvmtiEnvironments);
+	  _jvmtiEnvironments = _jvmtiEnvironments->next;
+	}
+      else
+	{
+	  struct jvmti_env_list *e = _jvmtiEnvironments; 
+	  while (e->next != NULL && e->next->env != env)
+	    e = e->next;
+	  if (e->next == NULL)
+	    return JVMTI_ERROR_INVALID_ENVIRONMENT;
+
+	  struct jvmti_env_list *next = e->next->next;
+	  _Jv_Free (e->next);
+	  e->next = next;
+	}
+    }
+
   _Jv_Free (env);
   return JVMTI_ERROR_NONE;
 }
@@ -750,5 +789,30 @@
   _Jv_JVMTIEnv *env
     = (_Jv_JVMTIEnv *) _Jv_MallocUnchecked (sizeof (_Jv_JVMTIEnv));
   env->p = &_Jv_JVMTI_Interface;
+
+  {
+    JvSynchronize dummy (_envListLock);
+    struct jvmti_env_list *element
+      = (struct jvmti_env_list *) _Jv_MallocUnchecked (sizeof (struct jvmti_env_list));
+    element->env = env;
+    element->next = NULL;
+
+    if (_jvmtiEnvironments == NULL)
+      _jvmtiEnvironments = element;
+    else
+      {
+	struct jvmti_env_list *e;
+	for (e = _jvmtiEnvironments; e->next != NULL; e = e->next) ;
+	e->next = element;
+      }
+  }
+
   return env;
 }
+
+void
+_Jv_JVMTI_Init ()
+{
+  _jvmtiEnvironments = NULL;
+  _envListLock = new java::lang::Object ();
+}

Re: [RFA/JVMTI] New JVMTI Environment Initialization/Allocation

[Tom Tromey]

>>>>> "Keith" == Keith Seitz <keiths@redhat.com> writes:

Keith> I've also snuck in another little patch which adds another common test
Keith> macro, this time for JVMTI_ERROR_ILLEGAL_ARGUMENT.

I noticed that this macro has no trailing ';' but NULL_CHECK does have
one.  The semicolon should probably be removed from NULL_CHECK.

Keith> [Heaven help me if my linked list impl is off. I'm not used to not
Keith> using JCL or STL anymore!]

Looks good to me.  Please commit.

Keith> +    if (_jvmtiEnvironments == NULL)
Keith> +      _jvmtiEnvironments = element;
Keith> +    else
Keith> +      {
Keith> +	struct jvmti_env_list *e;
Keith> +	for (e = _jvmtiEnvironments; e->next != NULL; e = e->next) ;
Keith> +	e->next = element;
Keith> +      }
Keith> +  }

Do we care about the order of the list?

FWIW, for an empty loop I tend to put the ';' on a line by itself, to
emphasize it.

Tom

Re: [RFA/JVMTI] New JVMTI Environment Initialization/Allocation

[Andrew Haley]

Tom Tromey writes:
 > >>>>> "Keith" == Keith Seitz <keiths@redhat.com> writes:
 > 
 > Keith> I've also snuck in another little patch which adds another common test
 > Keith> macro, this time for JVMTI_ERROR_ILLEGAL_ARGUMENT.
 > 
 > I noticed that this macro has no trailing ';' but NULL_CHECK does have
 > one.  The semicolon should probably be removed from NULL_CHECK.

These macros are dangerous because of hidden dangling else:

#define NULL_CHECK(Ptr)					\
  if (Ptr == NULL) return JVMTI_ERROR_NULL_POINTER;

They should be, e.g.,

#define NULL_CHECK(Ptr)
do						\
  {						\
    if (Ptr == NULL)				\
      return JVMTI_ERROR_NULL_POINTER;		\
  }						\
while (0)

Otherwise, something like

  if (poo)
    NULL_CHECK (p);
  else
    barf ();

may not do as you expected.

Andrew.

Re: [RFA/JVMTI] New JVMTI Environment Initialization/Allocation

[Tom Tromey]

>>>>> "Andrew" == Andrew Haley <aph@redhat.com> writes:

Andrew> These macros are dangerous because of hidden dangling else:

Yeah, I've been assuming that the various check macros would only be
invoked unconditionally.  But... this kind of change is more
future-proof, let's do that.

Tom

Re: [RFA/JVMTI] New JVMTI Environment Initialization/Allocation

[Keith Seitz]

Tom Tromey wrote:

> Keith> I've also snuck in another little patch which adds another common test
> Keith> macro, this time for JVMTI_ERROR_ILLEGAL_ARGUMENT.
> 
> I noticed that this macro has no trailing ';' but NULL_CHECK does have
> one.  The semicolon should probably be removed from NULL_CHECK.

I'll fix that with a follow-up patch which incorporates Andrew's 
suggested changes.

> Keith> +    if (_jvmtiEnvironments == NULL)
> Keith> +      _jvmtiEnvironments = element;
> Keith> +    else
> Keith> +      {
> Keith> +	struct jvmti_env_list *e;
> Keith> +	for (e = _jvmtiEnvironments; e->next != NULL; e = e->next) ;
> Keith> +	e->next = element;
> Keith> +      }
> Keith> +  }
> 
> Do we care about the order of the list?

Yes, we do actually. The JVMTI specification says that event 
notifications are posted to environments in the order the environments 
were created. This is about the easiest way for this to happen. I've 
added a comment to explain this.

> FWIW, for an empty loop I tend to put the ';' on a line by itself, to
> emphasize it.

Will do.

Committed, thanks.

Keith

Re: [RFA/JVMTI] New JVMTI Environment Initialization/Allocation

[Keith Seitz]

[-- Attachment #1: Type: text/plain, Size: 399 bytes --]

Andrew Haley wrote:

> These macros are dangerous because of hidden dangling else:

How's this?

Keith

ChangeLog
2006-09-01  Keith Seitz  <keiths@redhat.com>

         * jvmti.cc (THREAD_DEFAULT_TO_CURRENT): Encapsulate in 
do..while loop.
         (THREAD_CHECK_VALID): Likewise.
         (THREAD_CHECK_IS_ALIVE): Likewise.
         (NULL_CHECK): Likewise.
         (ILLEGAL_ARGUMENT): Likewise.


[-- Attachment #2: jvmti-macro-cleanup.patch --]
[-- Type: text/x-patch, Size: 1860 bytes --]

Index: jvmti.cc
===================================================================
--- jvmti.cc	(revision 116635)
+++ jvmti.cc	(working copy)
@@ -56,25 +56,49 @@
 
 // Some commonly-used checks
 
-#define THREAD_DEFAULT_TO_CURRENT(jthread)				\
-  if (jthread == NULL) jthread = java::lang::Thread::currentThread ();
+#define THREAD_DEFAULT_TO_CURRENT(jthread)		\
+  do							\
+    {							\
+      if (jthread == NULL)				\
+	jthread = java::lang::Thread::currentThread ();	\
+    }							\
+  while (0)
 
 #define THREAD_CHECK_VALID(jthread)					\
-  if (!java::lang::Thread::class$.isAssignableFrom (&(jthread->class$))) \
-    return JVMTI_ERROR_INVALID_THREAD;
+  do									\
+    {									\
+      if (!java::lang::Thread::class$.isAssignableFrom (&(jthread->class$))) \
+	return JVMTI_ERROR_INVALID_THREAD;				\
+    }									\
+  while (0)
 
-#define THREAD_CHECK_IS_ALIVE(thread)				\
-  if (!thread->isAlive ()) return JVMTI_ERROR_THREAD_NOT_ALIVE;
+#define THREAD_CHECK_IS_ALIVE(thread)	     \
+  do					     \
+    {					     \
+      if (!thread->isAlive ())		     \
+	return JVMTI_ERROR_THREAD_NOT_ALIVE; \
+    }					     \
+  while (0)
 
 // FIXME: if current phase is not set in Phases,
 // return JVMTI_ERROR_WRONG_PHASE
 #define REQUIRE_PHASE(Env, Phases)
 
-#define NULL_CHECK(Ptr)					\
-  if (Ptr == NULL) return JVMTI_ERROR_NULL_POINTER;
+#define NULL_CHECK(Ptr)				\
+  do						\
+    {						\
+      if (Ptr == NULL)				\
+	return JVMTI_ERROR_NULL_POINTER;	\
+    }						\
+  while (0)
 
-#define ILLEGAL_ARGUMENT(Cond)				\
-  if ((Cond)) return JVMTI_ERROR_ILLEGAL_ARGUMENT
+#define ILLEGAL_ARGUMENT(Cond)			\
+  do						\
+    {						\
+      if ((Cond))				\
+	return JVMTI_ERROR_ILLEGAL_ARGUMENT;	\
+    }						\
+  while (0)
 
 static jvmtiError JNICALL
 _Jv_JVMTI_SuspendThread (MAYBE_UNUSED jvmtiEnv *env, jthread thread)

Re: [RFA/JVMTI] New JVMTI Environment Initialization/Allocation

[Tom Tromey]

>>>>> "Keith" == Keith Seitz <keiths@redhat.com> writes:

>> These macros are dangerous because of hidden dangling else:

Keith> How's this?

Great, thanks.

Tom

Re: [RFA/JVMTI] New JVMTI Environment Initialization/Allocation

[Keith Seitz]

Tom Tromey wrote:
>>>>>> "Keith" == Keith Seitz <keiths@redhat.com> writes:
> 
>>> These macros are dangerous because of hidden dangling else:
> 
> Keith> How's this?
> 
> Great, thanks.

Committed.

Keith