RE: patch to fix null-pointer failure in libjava/boehm.cc

["Boehm, Hans" <hans dot boehm at hp dot com>]

This patch also looks basically safe to me, but I have some
general concerns about this code.

I don't really understand the class loading side of this, but
I wonder about memory ordering issues here.
I'm not sure anything here is even declared volatile.  Thus it seems
to me that wherever the flags field in the _Jv_Field structure
is set, the setting of that flag can be arbitrarily reordered
by the compiler?

Presumably setting this field doesn't acquire the allocation lock?
so the affected code can be stopped at any instruction?  And I think
this patch changed the ordering of the reads for the flags and u.addr
field?

Things are currently actually a bit worse than this, since we mark
from finalizable objects with the allocation lock held, but the world
not stopped.  (The theory here is that this only looks at
objects that are inaccessible, and can only become accessible again
through an operation that requires the allocation lock.  I'm getting
increasingly nervous about that theory.)

Hans

> -----Original Message-----
> From: java-patches-owner@gcc.gnu.org 
> [mailto:java-patches-owner@gcc.gnu.org] On Behalf Of Per Bothner
> Sent: Saturday, March 26, 2005 12:21 PM
> To: java-patches@gcc.gnu.org
> Subject: patch to fix null-pointer failure in libjava/boehm.cc
> 
> 
> I don't know if this is quite right.  I don't know this code 
> at all, and I'm curious if there was a reason for 
> dereferencing field->u.addr twice in the original code.
> 
> Is this appropriate for 4.0 - or perhaps 4.0.1?  Perhaps if 
> Hans and others more familiar with this code agree it is 
> safe.  At least this minimal version should be definitely safe:
> -	      if (JvFieldIsRef (field) && field->isResolved())
> +	      if (JvFieldIsRef (field) && p && field->isResolved())
> 
> Here is the analysis of the bug; 
> http://gcc.gnu.org/ml/java/2005-03/msg00141.html
> -- 
> 	--Per Bothner
> per@bothner.com   http://per.bothner.com/
>