Re: Object serialization and final fields

[Mark Wielaard]

Hi Guilhem,

On Fri, 2004-04-02 at 21:52, Guilhem Lavaux wrote:
> Here is the real patch for object serialization. I've added new static 
> methods to VMObjectStreamClass and changed the methods called in 
> ObjectStreamField accordingly. Note that we need to check all exceptions 
> now as the native functions may fail for some other obscure reasons.
>
> 2004-04-02  Guilhem Lavaux <address@hidden>
> 
>       * java/io/ObjectStreamField.java
>       (setBooleanField, setCharField, setByteField, setShortField,
>       setIntField, setLongField, setFloatField, setDoubleField): Use
>       native methods directly to be able to set final fields.
> 
>       * vm/reference/java/io/VMObjectStreamClass.java
>       (setBooleanNative, setCharNative, setByteNative, setShortNative,
>       setIntNative, setLongNative, setFloatNative, setDoubleNative):
>       New methods for serialization to be able to set final fields.
> 
>       * native/jni/java-io/java_io_VMObjectStreamClass.c:
>       Implemented new native methods of java.io.VMObjectStreamClass
>       accordingly.
>
> P.S.: If nobody is against, I'll check it in on sunday.

This looks very nice. Thanks.
Some nitpicks:

- You forgot the addition of setObjectNative() in the ChangeLog entry.
- You should regenerate include/java_io_VMObjectStreamClass.h
  This is automatically done when configuring with
  --enable-regen-headers. But should be mentioned in the
  ChangeLog/patch.
- Please chain exceptions. Don't just get the message, use initCause().
- Also catching Exception will miss Error (and subclasses) so you might
  want to catch Throwable.
- But isn't it simpler to let the setXnative() methods throw
  InternalError() directly (or maybe VirtualMachineError or
  IllegalArgumentException which seem more appropriate).
- Then also don't declare the setXNative() methods throws
  IllegalAccessException. Some are declared as such and some not.
  But it seems this is the only exception that should never be thrown.

Could you also add a short notice to the NEWS file about this VM
interface change. You should at least mention that the standard
implementation assumes that the JNI setXField() methods can be used on
final fields.

BTW. Note to VM/Compiler implementers!
--------------------------------------
We discussed "final" fields a bit more on irc.
We came to the conclusion the the VM spec authors and the JLS spec
authors have a different interpretation of "final".
According to the VM spec a final field is non-private read-only, but
rewritable by the Class defining the field.
According the the JLS spec a final field is single assignable (in a
[static] constructor method).
JNI doesn't seem to define precisely what/how final fields can change.
The above patch depends on the fact that in practice JNI setXField() can
be used to set a final field (this was already the case for the
System.setIn()/setOut()/setError() methods).

The following bug reports are also interesting:

http://developer.java.sun.com/developer/bugParade/bugs/4027808.html
Synopsis: [JNI] SetObjectField modifies a final field value
State: Closed, will not be fixed
Evaluation: JNI does not guard against illegal access to objects [...].

http://developer.java.sun.com/developer/bugParade/bugs/4115974.html
Synopsis: Need to be able to tell JIT not to optimize certain finals.
State: Closed, will not be fixed
Evaluation: These are the only thing which should ever behave this way,
so there's no need for a special interface for this.  The JIT just needs
to handle these specially.

http://developer.java.sun.com/developer/bugParade/bugs/4174797.html
Synopsis: Serialization should not use reflection to set final fields
State: Closed, fixed
Evaluation: [...] Serialization needs to use JNI Set<type>Field Routines
to set fields, instead of Field.set*(). [...]

All this seems a bit unfortunate since it basically means that a runtime
cannot optimize final field access since they can always be changed
through JNI and this "feature" is at least necessary for both
System.setIn/Out/Err() and reflection.

Cheers,

Mark

signature.asc
Description: This is a digitally signed message part