Re: Package-private access checking in GCJ 3.3

[Ranjit Mathew <rmathew at hotmail dot com>]

> > > Ranjit> I have only "bubblestrapped" with this change and tested
> > > Ranjit> against very simple testcases.
> > >
> > > Make sure you rebuild libgcj from scratch.  We may have library bugs
> > > that we don't know of due to this compiler bug.
> >
> > Yes, I realise that. I will try it tonight and let you
> > know if I face any problems.
> 
> I tried building libgcj with this modification
> and the very first class (java.lang.Class) caused
> it to ICE - it ICEs consistently at line 284
> of java/lang/Class.java (in 3.3) in method
> desiredAssertionStatus( ) where it returns
> the value from desiredAssertionStatus( ) (a
> package-private static final method) in
> java/lang/VMClassLoader.

The problem was that not_accessible_p( ) can also be called 
with MEMBER set to a TYPE_DECL, which has a NULL_TREE for a
DECL_CONTEXT. I am appending a version of my kludge that
works around this problem. It also makes use of a new
java_protstring_lookup( ) function similar to 
java_accstring_lookup( ) that is used with not_accessible_p( )
to report errors on "actual" access violations.

I am not happy with this kludge as I have a feeling that
it is working around the real problem and, more
importantly, since it *still* does not solve the related 
problem not stopping the access of a generously accessible 
member in a class with default access. For example, it will 
silently  pass an access to (say) foo.Bar.SNAFU or foo.Bar.snafu( ) 
from a class in another package where foo.Bar has default 
access but SNAFU and snafu( ) have either public or 
protected access.

GCJ 3.3 complains about package-private class access
only in some special cases like "x = new foo.Bar( );" but
not in (say) "x = new foo.Bar( ).snafu( )".

In any case, as Tom predicted, this kludge still exposes
a number of illegal accesses to package-private members 
in libgcj. Obviously, these *MUST* be fixed before 
package-private access checking is enabled in the compiler. 

These are:

 1. java.lang.String illegally uses the members "value"
    and "count" in gnu.gcj.runtime.StringBuffer.

 2. java.lang.VMThrowable illegally calls "stackTraceAddrs( )"
    in gnu.gcj.runtime.StackTrace.

 3. gnu.gcj.runtime.NameFinder illegally uses the constructor
    in java.lang.StackTraceElement.

 4. gnu.java.nio.{Byte,Char,Double,Float,Int,Long,Short}BufferImpl
    illegally use the respective constructors in base classes
    java.nio.{Byte,Char,Double,Float,Int,Long,Short}Buffer.

 5. gnu.java.nio.ByteBufferImpl illegally uses "backing_buffer"
    from java.nio.ByteBuffer.

 6. java.awt.image.ColorModel illegally uses "numComponents" in
    java.awt.color.ColorSpace.

 7. gnu.awt.xlib.XEventLoop illegally uses the constructor in
    gnu.gcj.xlib.XAnyEvent.

As Andrew had suggested, I bypassed these simply by making
the members being accessed protected/public. These have to
be resolved in a better manner and with more thought though.

I did not see any new failures in the default testsuite (i.e.
without Mauve and Jacks).

Ranjit.

Index: parse.y
===================================================================
--- parse.y	2003-06-08 20:04:39.000000000 +0530
+++ parse.y	2003-06-22 16:14:36.000000000 +0530
@@ -72,4 +72,5 @@ definitions and other extensions.  */
 /* Local function prototypes */
 static char *java_accstring_lookup PARAMS ((int));
+static char *java_protstring_lookup PARAMS ((int));
 static void  classitf_redefinition_error PARAMS ((const char *,tree, tree, tree));
 static void  variable_redefinition_error PARAMS ((tree, tree, tree, int));
@@ -3243,4 +3244,20 @@ java_accstring_lookup (flags)
 }
 
+static char *
+java_protstring_lookup (flags)
+     int flags;
+{
+  static char buffer [10];
+#define COPY_RETURN(S) {strcpy (buffer, S); return buffer;}
+
+  if (flags & ACC_PUBLIC) COPY_RETURN ("public");
+  if (flags & ACC_PRIVATE) COPY_RETURN ("private");
+  if (flags & ACC_PROTECTED) COPY_RETURN ("protected");
+
+  COPY_RETURN ("default");
+
+#undef COPY_RETURN
+}
+
 /* Issuing error messages upon redefinition of classes, interfaces or
    variables. */
@@ -9859,5 +9876,5 @@ resolve_qualified_expression_name (wfl, 
 	      parse_error_context
 		(qual_wfl, "Can't access %s field `%s.%s' from `%s'",
-		 java_accstring_lookup (get_access_flags_from_decl (decl)),
+		 java_protstring_lookup (get_access_flags_from_decl (decl)),
 		 GET_TYPE_NAME (type),
 		 IDENTIFIER_POINTER (DECL_NAME (decl)),
@@ -9989,5 +10006,5 @@ resolve_qualified_expression_name (wfl, 
 		    (qual_wfl,
 		     "Can't access %s field `%s.%s' from `%s'",
-		     java_accstring_lookup
+		     java_protstring_lookup
 		       (get_access_flags_from_decl (field_decl)),
 		     GET_TYPE_NAME (type),
@@ -10141,9 +10158,10 @@ not_accessible_p (reference, member, whe
     }
 
-  /* Default access are permitted only when occurring within the
-     package in which the type (REFERENCE) is declared. In other words,
-     REFERENCE is defined in the current package */
-  if (ctxp->package)
-    return !class_in_current_package (reference);
+  /* Default access is permitted only when occurring from within the
+     package in which MEMBER is defined. */
+  if (TREE_CODE (member) == TYPE_DECL)
+    return !class_in_current_package (TREE_TYPE (member));
+  else
+    return !class_in_current_package (DECL_CONTEXT (member));
 
   /* Otherwise, access is granted */
@@ -10530,5 +10548,5 @@ patch_method_invocation (patch, primary,
       const char *const fct_name = IDENTIFIER_POINTER (DECL_NAME (list));
       const char *const access =
-	java_accstring_lookup (get_access_flags_from_decl (list));
+	java_protstring_lookup (get_access_flags_from_decl (list));
       const char *const klass =
 	IDENTIFIER_POINTER (DECL_NAME (TYPE_NAME (DECL_CONTEXT (list))));