Patch: FYI: class reader fixes

[Tom Tromey <tromey at redhat dot com>]

I'm checking this in on the trunk.

This fixes a few more crashes revealed by Jeroen's test case.  Namely:

- We tried to unregister a class whose 'name' field wasn't set,
  resulting in a crash.  (This is fixed in 2 different ways, one up
  front and one in the unregister code; the latter can still be hit
  due to finalization.)
- _Jv_VerifyMethodSignature wasn't checking whether _Jv_VerifyOne
  returned NULL
- We didn't properly handle the case where superclass==0
- read_methods had a very old bug (dating back to the first checkin)
  where it wasn't properly verifying constant pool entries
- The call to wait_for_state in _Jv_FindClass doesn't seem useful,
  and it was causing a crash in the situation where a class refers to
  itself (the lookup should succeed, then the caller will throw)

I think we still don't get the correct answer in all cases for
Jeroen's test.  But, this is a first step.

Tom

Index: ChangeLog
from  Tom Tromey  <tromey@redhat.com>
	* java/lang/natClassLoader.cc (_Jv_UnregisterClass): Handle case
	where class' name is NULL.
	(_Jv_FindClass): Don't wait for class state.
	* java/lang/natVMClassLoader.cc (defineClass): Only unregister if
	name found.
	* include/java-interp.h (_Jv_DefineClass): Updated.
	* defineclass.cc (_Jv_DefineClass): Added 'name_result' argument.
	(struct _Jv_ClassReader): Likewise.
	(found_name): New field.
	(handleClassBegin): Set *found_name.
	(_Jv_VerifyMethodSignature): Handle case where ptr==NULL.
	(handleClassBegin): Throw error if super class not set.
	(read_methods): Correctly call check_tag and prepare_pool_entry.

Index: defineclass.cc
===================================================================
RCS file: /cvs/gcc/gcc/libjava/defineclass.cc,v
retrieving revision 1.46
diff -u -r1.46 defineclass.cc
--- defineclass.cc 15 Jun 2005 19:11:36 -0000 1.46
+++ defineclass.cc 24 Jun 2005 22:21:13 -0000
@@ -70,7 +70,8 @@
  * public or private members here.
  */
 
-struct _Jv_ClassReader {
+struct _Jv_ClassReader
+{
 
   // do verification?  Currently, there is no option to disable this.
   // This flag just controls the verificaiton done by the class loader;
@@ -104,6 +105,9 @@
   // the classes associated interpreter data.
   _Jv_InterpClass  *def_interp;
 
+  // The name we found.
+  _Jv_Utf8Const **found_name;
+
   /* check that the given number of input bytes are available */
   inline void check (int num)
   {
@@ -219,7 +223,8 @@
   }
 
   _Jv_ClassReader (jclass klass, jbyteArray data, jint offset, jint length,
-		   java::security::ProtectionDomain *pd)
+		   java::security::ProtectionDomain *pd,
+		   _Jv_Utf8Const **name_result)
   {
     if (klass == 0 || length < 0 || offset+length > data->length)
       throw_internal_error ("arguments to _Jv_DefineClass");
@@ -229,6 +234,7 @@
     len    = length;
     pos    = 0;
     def    = klass;
+    found_name = name_result;
 
     def->size_in_bytes = -1;
     def->vtable_method_count = -1;
@@ -279,11 +285,15 @@
    */
 };
 
+// Note that *NAME_RESULT will only be set if the class is registered
+// with the class loader.  This is how the caller can know whether
+// unregistration is require.
 void
 _Jv_DefineClass (jclass klass, jbyteArray data, jint offset, jint length,
-		 java::security::ProtectionDomain *pd)
+		 java::security::ProtectionDomain *pd,
+		 _Jv_Utf8Const **name_result)
 {
-  _Jv_ClassReader reader (klass, data, offset, length, pd);
+  _Jv_ClassReader reader (klass, data, offset, length, pd, name_result);
   reader.parse();
 
   /* that's it! */
@@ -499,9 +509,9 @@
       int attributes_count = read2u ();
       
       check_tag (name_index, JV_CONSTANT_Utf8);
-      prepare_pool_entry (descriptor_index, JV_CONSTANT_Utf8);
+      prepare_pool_entry (name_index, JV_CONSTANT_Utf8);
 
-      check_tag (name_index, JV_CONSTANT_Utf8);
+      check_tag (descriptor_index, JV_CONSTANT_Utf8);
       prepare_pool_entry (descriptor_index, JV_CONSTANT_Utf8);
 
       handleMethod (i, access_flags, name_index,
@@ -930,11 +940,11 @@
   pool_data[this_class].clazz = def;
   pool_tags[this_class] = JV_CONSTANT_ResolvedClass;
 
-  if (super_class == 0 && ! (access_flags & Modifier::INTERFACE))
+  if (super_class == 0)
     {
-      // FIXME: Consider this carefully!  
-      if (! _Jv_equalUtf8Consts (def->name, java::lang::Object::class$.name))
-	throw_no_class_def_found_error ("loading java.lang.Object");
+      // Note that this is ok if we are defining java.lang.Object.
+      // But there is no way to have this class be interpreted.
+      throw_class_format_error ("no superclass reference");
     }
 
   def->state = JV_STATE_PRELOADING;
@@ -946,6 +956,11 @@
   // lock here, as our caller has acquired it.
   _Jv_RegisterInitiatingLoader (def, def->loader);
 
+  // Note that we found a name so that unregistration can happen if
+  // needed.
+  *found_name = def->name;
+
+  jclass the_super = NULL;
   if (super_class != 0)
     {
       // Load the superclass.
@@ -953,8 +968,7 @@
       _Jv_Utf8Const* super_name = pool_data[super_class].utf8; 
 
       // Load the superclass using our defining loader.
-      jclass the_super = _Jv_FindClass (super_name,
-					def->loader);
+      jclass the_super = _Jv_FindClass (super_name, def->loader);
 
       // This will establish that we are allowed to be a subclass,
       // and check for class circularity error.
@@ -1547,7 +1561,7 @@
   while (ptr && UTF8_PEEK (ptr, limit) != ')')
     ptr = _Jv_VerifyOne (ptr, limit, false);
 
-  if (UTF8_GET (ptr, limit) != ')')
+  if (! ptr || UTF8_GET (ptr, limit) != ')')
     return false;
 
   // get the return type
Index: java/lang/natClassLoader.cc
===================================================================
RCS file: /cvs/gcc/gcc/libjava/java/lang/natClassLoader.cc,v
retrieving revision 1.83
diff -u -r1.83 natClassLoader.cc
--- java/lang/natClassLoader.cc 19 May 2005 22:24:26 -0000 1.83
+++ java/lang/natClassLoader.cc 24 Jun 2005 22:21:13 -0000
@@ -107,6 +107,10 @@
 void
 _Jv_UnregisterClass (jclass the_class)
 {
+  // This can happen if the class could not be defined properly.
+  if (! the_class->name)
+    return;
+
   JvSynchronize sync (&java::lang::Class::class$);
   jint hash = HASH_UTF(the_class->name);
 
@@ -328,12 +332,6 @@
 	    }
 	}
     }
-  else
-    {
-      // We need classes to be in the hash while we're loading, so
-      // that they can refer to themselves.
-      _Jv_Linker::wait_for_state (klass, JV_STATE_LOADED);
-    }
 
   return klass;
 }
Index: java/lang/natVMClassLoader.cc
===================================================================
RCS file: /cvs/gcc/gcc/libjava/java/lang/natVMClassLoader.cc,v
retrieving revision 1.13
diff -u -r1.13 natVMClassLoader.cc
--- java/lang/natVMClassLoader.cc 12 May 2005 01:27:15 -0000 1.13
+++ java/lang/natVMClassLoader.cc 24 Jun 2005 22:21:13 -0000
@@ -71,16 +71,18 @@
 	  klass->name = name2;
 	}
 
+      _Jv_Utf8Const *found_name = NULL;
       try
 	{
-	  _Jv_DefineClass (klass, data, offset, length, pd);
+	  _Jv_DefineClass (klass, data, offset, length, pd, &found_name);
 	}
       catch (java::lang::Throwable *ex)
 	{
 	  klass->state = JV_STATE_ERROR;
 	  klass->notifyAll ();
 
-	  _Jv_UnregisterInitiatingLoader (klass, klass->loader);
+	  if (found_name != NULL)
+	    _Jv_UnregisterInitiatingLoader (klass, klass->loader);
 
 	  // If EX is not a ClassNotFoundException, that's ok, because we
 	  // account for the possibility in defineClass().
Index: include/java-interp.h
===================================================================
RCS file: /cvs/gcc/gcc/libjava/include/java-interp.h,v
retrieving revision 1.29
diff -u -r1.29 java-interp.h
--- include/java-interp.h 17 Mar 2005 00:18:48 -0000 1.29
+++ include/java-interp.h 24 Jun 2005 22:21:13 -0000
@@ -36,7 +36,8 @@
 
 void _Jv_InitInterpreter ();
 void _Jv_DefineClass (jclass, jbyteArray, jint, jint,
-		      java::security::ProtectionDomain *);
+		      java::security::ProtectionDomain *,
+		      _Jv_Utf8Const **);
 
 void _Jv_InitField (jobject, jclass, int);
 void * _Jv_AllocMethodInvocation (jsize size);