Patch: FYI: More verifier changes

[Tom Tromey <tromey at redhat dot com>]

I'm checking this in.

* Once again I've moved where bytecode verification is invoked.
  Now it is invoked during class preparation.  Previously in some
  situations we could get a failure because we would need to find the
  superclass of a class which wasn't fully loaded.  Doing verification
  during preparation avoids this problem.

* We now check that `this' has been initialized on return from an
  instance initializer.

* We now allow assignment to a field declared in the current class
  before `this' is fully initialized, per the JVM Spec 2nd Ed.

Verification is still not compiled in by default.
However, we're much closer.  It seems more robust to me -- I'm finding
that the verifier detects gcj bugs more frequently now than it does
bugs in itself.

Tom

Index: ChangeLog
from  Tom Tromey  <tromey@redhat.com>
	* resolve.cc (_Jv_PrepareClass): Verify method here...
	* defineclass.cc (handleMethodsEnd): ... not here.
	* verify.cc (_Jv_BytecodeVerifier::initialize_stack): New method.
	(_Jv_BytecodeVerifier::verify_instructions_0) [op_return]: Ensure
	there are no uninitialized objects.
	(_Jv_BytecodeVerifier::state::this_type): New field.
	(_Jv_BytecodeVerifier::state::state): Initialize this_type.
	(_Jv_BytecodeVerifier::state::copy): Copy this_type.
	(_Jv_BytecodeVerifier::state::merge): Merge this_type.
	(_Jv_BytecodeVerifier::state::check_no_uninitialized_objects):
	Handle this_type.
	(_Jv_BytecodeVerifier::state::check_this_initialized): New
	method.
	(_Jv_BytecodeVerifier::state::set_initialized): Handle this_type.
	(_Jv_BytecodeVerifier::state::set_this_type): New method.
	(_Jv_BytecodeVerifier::verify_instructions_0) [op_putfield]: Allow
	assignment to fields of `this' before another initializer is run.

Index: defineclass.cc
===================================================================
RCS file: /cvs/gcc/gcc/libjava/defineclass.cc,v
retrieving revision 1.26
diff -u -r1.26 defineclass.cc
--- defineclass.cc 2001/12/05 19:28:16 1.26
+++ defineclass.cc 2001/12/10 01:12:51
@@ -940,7 +940,7 @@
       pool_data[super_class].clazz = the_super;
       pool_tags[super_class] = JV_CONSTANT_ResolvedClass;
     }
-	    
+
   // now we've come past the circularity problem, we can 
   // now say that we're loading...
 
@@ -1315,15 +1315,6 @@
 	{
 	  if (def->interpreted_methods[i] == 0)
 	    throw_class_format_error ("method with no code");
-
-	  if (verify)
-	    {
-	      _Jv_InterpMethod *m;
-	      m = (reinterpret_cast<_Jv_InterpMethod *>
-		   (def->interpreted_methods[i]));
-	      // FIXME: enable once verifier is more fully tested.
-	      // _Jv_VerifyMethod (m);
-	    }
 	}
     }
 }
Index: resolve.cc
===================================================================
RCS file: /cvs/gcc/gcc/libjava/resolve.cc,v
retrieving revision 1.26
diff -u -r1.26 resolve.cc
--- resolve.cc 2001/11/05 23:39:54 1.26
+++ resolve.cc 2001/12/10 01:12:52
@@ -578,7 +578,7 @@
 
   // set the instance size for the class
   clz->size_in_bytes = instance_size;
-    
+
   // allocate static memory
   if (static_size != 0)
     {
@@ -628,6 +628,8 @@
       else if (imeth != 0)		// it could be abstract
 	{
 	  _Jv_InterpMethod *im = reinterpret_cast<_Jv_InterpMethod *> (imeth);
+	  // FIXME: enable once verifier is more fully tested.
+	  // _Jv_VerifyMethod (im);
 	  clz->methods[i].ncode = im->ncode ();
 	}
     }
Index: verify.cc
===================================================================
RCS file: /cvs/gcc/gcc/libjava/verify.cc,v
retrieving revision 1.24
diff -u -r1.24 verify.cc
--- verify.cc 2001/12/09 00:14:00 1.24
+++ verify.cc 2001/12/10 01:12:54
@@ -764,6 +764,13 @@
     // This is used to keep a linked list of all the states which
     // require re-verification.  We use the PC to keep track.
     int next;
+    // We keep track of the type of `this' specially.  This is used to
+    // ensure that an instance initializer invokes another initializer
+    // on `this' before returning.  We must keep track of this
+    // specially because otherwise we might be confused by code which
+    // assigns to locals[0] (overwriting `this') and then returns
+    // without really initializing.
+    type this_type;
 
     // INVALID marks a state which is not on the linked list of states
     // requiring reverification.
@@ -772,6 +779,7 @@
     static const int NO_NEXT = -2;
 
     state ()
+      : this_type ()
     {
       stack = NULL;
       locals = NULL;
@@ -779,6 +787,7 @@
     }
 
     state (int max_stack, int max_locals)
+      : this_type ()
     {
       stacktop = 0;
       stackdepth = 0;
@@ -855,6 +864,7 @@
 	    locals[i] = copy->locals[i];
 	  local_changed[i] = copy->local_changed[i];
 	}
+      this_type = copy->this_type;
       // Don't modify `next'.
     }
 
@@ -870,14 +880,19 @@
       // FIXME: subroutine handling?
     }
 
-    // Merge STATE into this state.  Destructively modifies this state.
-    // Returns true if the new state was in fact changed.  Will throw an
-    // exception if the states are not mergeable.
+    // Merge STATE_OLD into this state.  Destructively modifies this
+    // state.  Returns true if the new state was in fact changed.
+    // Will throw an exception if the states are not mergeable.
     bool merge (state *state_old, bool ret_semantics,
 		int max_locals)
     {
       bool changed = false;
 
+      // Special handling for `this'.  If one or the other is
+      // uninitialized, then the merge is uninitialized.
+      if (this_type.isinitialized ())
+	this_type = state_old->this_type;
+
       // Merge subroutine states.  *THIS and *STATE_OLD must be in the
       // same subroutine.  Also, recursive subroutine calls must be
       // avoided.
@@ -940,8 +955,23 @@
       for (int i = 0; i < max_locals; ++i)
 	if (locals[i].isreference () && ! locals[i].isinitialized ())
 	  verify_fail ("uninitialized object in local variable");
+
+      check_this_initialized ();
+    }
+
+    // Ensure that `this' has been initialized.
+    void check_this_initialized ()
+    {
+      if (this_type.isreference () && ! this_type.isinitialized ())
+	verify_fail ("`this' is uninitialized");
     }
 
+    // Set type of `this'.
+    void set_this_type (const type &k)
+    {
+      this_type = k;
+    }
+
     // Note that a local variable was modified.
     void note_variable (int index)
     {
@@ -957,6 +987,7 @@
 	stack[i].set_initialized (pc);
       for (int i = 0; i < max_locals; ++i)
 	locals[i].set_initialized (pc);
+      this_type.set_initialized (pc);
     }
 
     // Return true if this state is the unmerged result of a `ret'.
@@ -1870,6 +1901,42 @@
       verify_fail ("incompatible return type", start_PC);
   }
 
+  // Initialize the stack for the new method.  Returns true if this
+  // method is an instance initializer.
+  bool initialize_stack ()
+  {
+    int var = 0;
+    bool is_init = false;
+
+    using namespace java::lang::reflect;
+    if (! Modifier::isStatic (current_method->self->accflags))
+      {
+	type kurr (current_class);
+	if (_Jv_equalUtf8Consts (current_method->self->name, gcj::init_name))
+	  {
+	    kurr.set_uninitialized (type::SELF);
+	    is_init = true;
+	  }
+	set_variable (0, kurr);
+	current_state->set_this_type (kurr);
+	++var;
+      }
+
+    // We have to handle wide arguments specially here.
+    int arg_count = _Jv_count_arguments (current_method->self->signature);
+    type arg_types[arg_count];
+    compute_argument_types (current_method->self->signature, arg_types);
+    for (int i = 0; i < arg_count; ++i)
+      {
+	set_variable (var, arg_types[i]);
+	++var;
+	if (arg_types[i].iswide ())
+	  ++var;
+      }
+
+    return is_init;
+  }
+
   void verify_instructions_0 ()
   {
     current_state = new state (current_method->max_stack,
@@ -1878,32 +1945,9 @@
     PC = 0;
     start_PC = 0;
 
-    {
-      int var = 0;
+    // True if we are verifying an instance initializer.
+    bool this_is_init = initialize_stack ();
 
-      using namespace java::lang::reflect;
-      if (! Modifier::isStatic (current_method->self->accflags))
-	{
-	  type kurr (current_class);
-	  if (_Jv_equalUtf8Consts (current_method->self->name, gcj::init_name))
-	    kurr.set_uninitialized (type::SELF);
-	  set_variable (0, kurr);
-	  ++var;
-	}
-
-      // We have to handle wide arguments specially here.
-      int arg_count = _Jv_count_arguments (current_method->self->signature);
-      type arg_types[arg_count];
-      compute_argument_types (current_method->self->signature, arg_types);
-      for (int i = 0; i < arg_count; ++i)
-	{
-	  set_variable (var, arg_types[i]);
-	  ++var;
-	  if (arg_types[i].iswide ())
-	    ++var;
-	}
-    }
-
     states = (state **) _Jv_Malloc (sizeof (state *)
 				    * current_method->code_length);
     for (int i = 0; i < current_method->code_length; ++i)
@@ -2561,6 +2605,10 @@
 	    invalidate_pc ();
 	    break;
 	  case op_return:
+	    // We only need to check this when the return type is
+	    // void, because all instance initializers return void.
+	    if (this_is_init)
+	      current_state->check_this_initialized ();
 	    check_return_type (void_type);
 	    invalidate_pc ();
 	    break;
@@ -2583,6 +2631,13 @@
 	      type klass;
 	      type field = check_field_constant (get_ushort (), &klass);
 	      pop_type (field);
+
+	      // We have an obscure special case here: we can use
+	      // `putfield' on a field declared in this class, even if
+	      // `this' has not yet been initialized.
+	      if (! current_state->this_type.isinitialized ()
+		  && current_state->this_type.pc == type::SELF)
+		klass.set_uninitialized (type::SELF);
 	      pop_type (klass);
 	    }
 	    break;