RFC: fix PR 28892

RFC: fix PR 28892

[Tom Tromey]

PR 28892 involves compiling bytecode which has been run through
retroweaver.  Apparently retroweaver will emit bytecode that
initializes static final variables in methods other than <clinit>.
This particular set of constraints doesn't appear to be checked by the
JDK, meaning that this is bytecode which JDK users can run but which
we will reject.

I fixed this by removing the checks.  Any comments (Andrew?) before I
check this in?

This passes our test suite.  It also lets me compiling the test jars
from the PR.

Tom

Index: java/ChangeLog
from  Tom Tromey  <tromey@redhat.com>

	PR java/28892:
	* expr.c (expand_java_field_op): No error for assignments not in
	class initializer or constructor.

Index: java/expr.c
===================================================================
--- java/expr.c	(revision 116140)
+++ java/expr.c	(working copy)
@@ -2846,21 +2846,10 @@
 	  if (DECL_CONTEXT (field_decl) != current_class)
             error ("assignment to final field %q+D not in field's class",
                    field_decl);
-	  else if (FIELD_STATIC (field_decl))
-	    {
-	      if (!DECL_CLINIT_P (current_function_decl))
-		warning (0, "assignment to final static field %q+D not in "
-                         "class initializer",
-                         field_decl);
-	    }
-	  else
-	    {
-	      tree cfndecl_name = DECL_NAME (current_function_decl);
-	      if (! DECL_CONSTRUCTOR_P (current_function_decl)
-		  && !ID_FINIT_P (cfndecl_name))
-                warning (0, "assignment to final field %q+D not in constructor",
-			 field_decl);
-	    }
+	  /* We used to check for assignments to final fields not
+	     occurring in the class initializer or in a constructor
+	     here.  However, this constraint doesn't seem to be
+	     enforced by the JVM.  */
 	}      
 
       if (TREE_THIS_VOLATILE (field_decl))

Re: RFC: fix PR 28892

[Andrew Haley]

Tom Tromey writes:
 > PR 28892 involves compiling bytecode which has been run through
 > retroweaver.  Apparently retroweaver will emit bytecode that
 > initializes static final variables in methods other than <clinit>.
 > This particular set of constraints doesn't appear to be checked by the
 > JDK, meaning that this is bytecode which JDK users can run but which
 > we will reject.
 > 
 > I fixed this by removing the checks.  Any comments (Andrew?) before I
 > check this in?

I'd have made it WONTFIX.  Sadly, however, there seems to be no rule
in the specification that forbids final fields being rewritten in
methods other than clinit.

This makes some optimizations harder.  I think we need a flag to tell
gcj that what we are compiling may not be Java code.  Then we can do
better optimizations when we know that it is.

Andrew.

Re: RFC: fix PR 28892

[Tom Tromey]

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

Andrew> I'd have made it WONTFIX.  Sadly, however, there seems to be no rule
Andrew> in the specification that forbids final fields being rewritten in
Andrew> methods other than clinit.

In this case I don't think the fields are being rewritten -- they are
simply being initialized by a different function.

Andrew> This makes some optimizations harder.  I think we need a flag to tell
Andrew> gcj that what we are compiling may not be Java code.  Then we can do
Andrew> better optimizations when we know that it is.

Unfortunately I think final fields may be changed via reflection.

Tom

Re: RFC: fix PR 28892

[David Daney]

Tom Tromey wrote:
>>>>>>"Andrew" == Andrew Haley <aph@redhat.com> writes:
> 
> 
> Andrew> I'd have made it WONTFIX.  Sadly, however, there seems to be no rule
> Andrew> in the specification that forbids final fields being rewritten in
> Andrew> methods other than clinit.
> 
> In this case I don't think the fields are being rewritten -- they are
> simply being initialized by a different function.
> 
> Andrew> This makes some optimizations harder.  I think we need a flag to tell
> Andrew> gcj that what we are compiling may not be Java code.  Then we can do
> Andrew> better optimizations when we know that it is.
> 
> Unfortunately I think final fields may be changed via reflection.
> 

Is this behavior specified anywhere?

I have not carefully read JLS 3, but IIRC in the good old days, it was 
kind of unspecified behavior.

David Daney

Re: RFC: fix PR 28892

[Andrew Haley]

David Daney writes:
 > Tom Tromey wrote:
 > >>>>>>"Andrew" == Andrew Haley <aph@redhat.com> writes:
 > > 
 > > 
 > > Andrew> I'd have made it WONTFIX.  Sadly, however, there seems to be no rule
 > > Andrew> in the specification that forbids final fields being rewritten in
 > > Andrew> methods other than clinit.
 > > 
 > > In this case I don't think the fields are being rewritten -- they are
 > > simply being initialized by a different function.
 > > 
 > > Andrew> This makes some optimizations harder.  I think we need a flag to tell
 > > Andrew> gcj that what we are compiling may not be Java code.  Then we can do
 > > Andrew> better optimizations when we know that it is.
 > > 
 > > Unfortunately I think final fields may be changed via reflection.
 > > 
 > 
 > Is this behavior specified anywhere?
 > 
 > I have not carefully read JLS 3, but IIRC in the good old days, it was 
 > kind of unspecified behavior.

Well, if you can find that in JLS 2 you're better at reading specs
than me!  There are some restrictions on putfield, but I can't see
that one.

Andrew.

Re: RFC: fix PR 28892

[David Daney]

Andrew Haley wrote:
> David Daney writes:
>  > Tom Tromey wrote:
>  > >>>>>>"Andrew" == Andrew Haley <aph@redhat.com> writes:
>  > > 
>  > > 
>  > > Andrew> I'd have made it WONTFIX.  Sadly, however, there seems to be no rule
>  > > Andrew> in the specification that forbids final fields being rewritten in
>  > > Andrew> methods other than clinit.
>  > > 
>  > > In this case I don't think the fields are being rewritten -- they are
>  > > simply being initialized by a different function.
>  > > 
>  > > Andrew> This makes some optimizations harder.  I think we need a flag to tell
>  > > Andrew> gcj that what we are compiling may not be Java code.  Then we can do
>  > > Andrew> better optimizations when we know that it is.
>  > > 
>  > > Unfortunately I think final fields may be changed via reflection.
>  > > 
>  > 
>  > Is this behavior specified anywhere?
>  > 
>  > I have not carefully read JLS 3, but IIRC in the good old days, it was 
>  > kind of unspecified behavior.
> 
> Well, if you can find that in JLS 2 you're better at reading specs
> than me!  There are some restrictions on putfield, but I can't see
> that one.

Well I think I found the relevant documentation.  The JLS defers to the 
API documentation for things in the standard library.

Look in: http://java.sun.com/j2se/1.5.0/docs/api/

The documentation for java.lang.reflect.Field.set(Object,Object) says:

"If the underlying field is final, the method throws an 
IllegalAccessException unless setAccessible(true) has succeeded for this 
field and this field is non-static. Setting a final field in this way is 
meaningful only during deserialization or reconstruction of instances of 
classes with blank final fields, before they are made available for 
access by other parts of a program. Use in any other context may have 
unpredictable effects, including cases in which other parts of a program 
continue to use the original value of this field."

Earlier versions of the JDK specified that an IllegalAccessException was 
thrown unconditionally, but I think behaved in the manner documented for 
  JDK 1.5.

If you try to modify a final field directly (after it has been set) the 
verifier must reject the code.  So the only option would be to use 
reflection, for which it is only supposed to work in a very narrow set 
of circumstances.

David Daney.

Re: RFC: fix PR 28892

[Tom Tromey]

>>>>> "David" == David Daney <ddaney@avtrex.com> writes:

David> If you try to modify a final field directly (after it has been set)
David> the verifier must reject the code.  So the only option would be to use
David> reflection, for which it is only supposed to work in a very narrow set
David> of circumstances.

.. and the special cases of System.{in,out,err}, via the set* methods.

But, yeah, I'd forgotten about the non-static constraint.
In any case, I think this only applies to subsequent sets.  I don't
think there is any requirement that the initial set of a static final
variable be done in <clinit>.

Tom