Re: Object serialization and final fields
Mark Wielaard wrote:
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 [EMAIL PROTECTED]
* 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.
Ok.
- 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.
Ok.
- 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.
Ok, I'm modifying this.
- 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.
It was a mistake. First I wanted to make them throw nothing because I
don't really know what may happen in the VM. At some point JNI calls may
raise any type of exceptions. Maybe we may catch all exceptions directly
in the native code and only throw InternalError.
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.
Ok.
The mauve testcase is in CVS (added into
gnu/testlet/java/io/ObjectInputOutput/Test).
Here is the new patch and the new changelog entry (hoping everything is
alright now):
2004-04-06 Guilhem Lavaux [EMAIL PROTECTED]
* java/io/ObjectStreamField.java
(setBooleanField, setCharField, setByteField, setShortField,
setIntField, setLongField, setFloatField, setDoubleField,
setObjectField): 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,
setObjectNative): 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.
* include/java_io_VMObjectStreamClass.h: Regenerated.
* NEWS: Added a warning clause about the VM Interface change.
Cheers,
Guilhem.
Index: NEWS
===
RCS file: /cvsroot/classpath/classpath/NEWS,v
retrieving revision 1.36
diff -u -b -B -r1.36 NEWS
--- NEWS13 Mar 2004 02:18:39 - 1.36
+++ NEWS6 Apr 2004 19:55:20 -
@@ -1,3 +1,11 @@
+New in release 0.09
+
+VM Interface changes:
+
+* GNU Classpath now assumes that JNI calls SetXField can modify final
+fields. This was previously used silently for System.in/out/err and should
+be considered as a feature now.
+
New in release 0.08 (2004/12/03)
* java.util.regexp implementation through gnu.regexp wrappers.
Index: native/jni/java-io/java_io_VMObjectStreamClass.c
===
RCS file:
/cvsroot/classpath/classpath/native/jni/java-io/java_io_VMObjectStreamClass.c,v
retrieving revision 1.3
diff -u -b -B -r1.3 java_io_VMObjectStreamClass.c
--- native/jni/java-io/java_io_VMObjectStreamClass.c29 Mar 2004 07:07:27 -
1.3
+++ native/jni/java-io/java_io_VMObjectStreamClass.c6 Apr 2004 19:55:20 -
@@ -59,3 +59,238 @@
}
return JNI_TRUE;
}
+
+static void throwInternalError(JNIEnv *env)
+{
+ jclass internalErrorClass;
+ jthrowable previousException, newException;
+ jmethodID initException, getMessageID, initCauseID;
+ jstring message;
+
+ internalErrorClass = (*env)-FindClass(env, java/lang/InternalError);
+
Re: Object serialization and final fields
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 [EMAIL PROTECTED] * 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 SettypeField 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 ___ Classpath mailing list [EMAIL PROTECTED] http://mail.gnu.org/mailman/listinfo/classpath
Re: Object serialization and final fields
Hi, On Sun, 2004-04-04 at 20:01, Mark Wielaard wrote: This looks very nice. Thanks. Some nitpicks: [...] Last nitpick. I promise! - Do you have a test case that can be added to Mauve? Thanks, Mark signature.asc Description: This is a digitally signed message part ___ Classpath mailing list [EMAIL PROTECTED] http://mail.gnu.org/mailman/listinfo/classpath
RE: Object serialization and final fields
Mark Wielaard wrote: 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 SettypeField 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. These issues may be addressed in the near future - though it is a bit of a mess. The new Java Memory Model basically allows all the optimisations you want and says that native modification of final fields, beyond use of SetIn/Out/Err, need not cause a change by one thread to become visible in another. In other words any application using this is on its own. There are also special rules for handling the deserialisation issue with finals. See http://www.cs.umd.edu/~pugh/java/memoryModel for details. David Holmes ___ Classpath mailing list [EMAIL PROTECTED] http://mail.gnu.org/mailman/listinfo/classpath
Re: Object serialization and final fields
Hi,
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.
ChangeLog entry:
2004-04-02 Guilhem Lavaux [EMAIL PROTECTED]
* 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.
Cheers,
Guilhem.
P.S.: If nobody is against, I'll check it in on sunday.
Index: java/io/ObjectStreamField.java
===
RCS file: /cvsroot/classpath/classpath/java/io/ObjectStreamField.java,v
retrieving revision 1.14
diff -u -b -B -r1.14 ObjectStreamField.java
--- java/io/ObjectStreamField.java 26 Feb 2004 07:53:15 - 1.14
+++ java/io/ObjectStreamField.java 2 Apr 2004 19:42:15 -
@@ -38,9 +38,9 @@
package java.io;
+import gnu.java.lang.reflect.TypeSignature;
import java.lang.reflect.Field;
import java.lang.reflect.Modifier;
-import gnu.java.lang.reflect.TypeSignature;
import java.security.AccessController;
import java.security.PrivilegedAction;
@@ -64,7 +64,6 @@
{
this (field.getName(), field.getType());
this.field = field;
-toset = !Modifier.isFinal(field.getModifiers());
}
/**
@@ -357,9 +356,9 @@
{
try
{
- field.setBoolean(obj, val);
+ VMObjectStreamClass.setBooleanNative(field, obj, val);
}
-catch(IllegalAccessException x)
+catch(Exception x)
{
throw new InternalError(x.getMessage());
}
@@ -369,9 +368,9 @@
{
try
{
- field.setByte(obj, val);
+ VMObjectStreamClass.setByteNative(field, obj, val);
}
-catch(IllegalAccessException x)
+catch(Exception x)
{
throw new InternalError(x.getMessage());
}
@@ -381,9 +380,9 @@
{
try
{
- field.setChar(obj, val);
+ VMObjectStreamClass.setCharNative(field, obj, val);
}
-catch(IllegalAccessException x)
+catch(Exception x)
{
throw new InternalError(x.getMessage());
}
@@ -393,9 +392,9 @@
{
try
{
- field.setShort(obj, val);
+ VMObjectStreamClass.setShortNative(field, obj, val);
}
-catch(IllegalAccessException x)
+catch(Exception x)
{
throw new InternalError(x.getMessage());
}
@@ -405,9 +404,9 @@
{
try
{
- field.setInt(obj, val);
+ VMObjectStreamClass.setIntNative(field, obj, val);
}
-catch(IllegalAccessException x)
+catch(Exception x)
{
throw new InternalError(x.getMessage());
}
@@ -417,9 +416,9 @@
{
try
{
- field.setLong(obj, val);
+ VMObjectStreamClass.setLongNative(field, obj, val);
}
-catch(IllegalAccessException x)
+catch(Exception x)
{
throw new InternalError(x.getMessage());
}
@@ -429,9 +428,9 @@
{
try
{
- field.setFloat(obj, val);
+ VMObjectStreamClass.setFloatNative(field, obj, val);
}
-catch(IllegalAccessException x)
+catch(Exception x)
{
throw new InternalError(x.getMessage());
}
@@ -441,21 +440,22 @@
{
try
{
- field.setDouble(obj, val);
+ VMObjectStreamClass.setDoubleNative(field, obj, val);
}
-catch(IllegalAccessException x)
+catch(Exception x)
{
throw new InternalError(x.getMessage());
}
}
+
final void setObjectField(Object obj, Object val)
{
try
{
- field.set(obj, val);
+ VMObjectStreamClass.setObjectNative(field, obj, val);
}
-catch(IllegalAccessException x)
+catch(Exception x)
{
throw new InternalError(x.getMessage());
}
Index: native/jni/java-io/java_io_VMObjectStreamClass.c
===
RCS file:
/cvsroot/classpath/classpath/native/jni/java-io/java_io_VMObjectStreamClass.c,v
retrieving revision 1.3
diff -u -b -B -r1.3 java_io_VMObjectStreamClass.c
--- native/jni/java-io/java_io_VMObjectStreamClass.c29 Mar 2004 07:07:27 -
1.3
+++ native/jni/java-io/java_io_VMObjectStreamClass.c2 Apr 2004 19:42:17 -
@@ -59,3 +59,195 @@
}
return JNI_TRUE;
}
+
+static jfieldID getFieldReference(JNIEnv
Re: Object serialization and final fields
Mark Wielaard wrote:
Hi,
Hi Mark !
On Thu, 2004-03-25 at 21:19, Guilhem Lavaux wrote:
Some people has reported failures in kaffe with applications trying to
deserialize objects containing final fields. Apparently it is authorized
in the serialization spec but we cannot rely on
java.lang.reflect.Field to set them. So our only solution is to bypass
the protection in java.lang.reflect.Field by creating new native calls
in ObjectStreamField.
I am proposing the following changes for ObjectStreamField.
It is a bit sad that serialization needs a couple of things that clearly
should have been standard reflection methods/options. Sigh.
I like the idea in general.
Couple of comments/questions.
- ChangeLog entry is missing...
Well, it was not a final patch but just to open up the discussion.
@@ -64,7 +65,7 @@
{
this (field.getName(), field.getType());
this.field = field;
-toset = !Modifier.isFinal(field.getModifiers());
+//toset = !Modifier.isFinal(field.getModifiers());
}
Either remove this or make it a real comment explaining why we don't
test this anymore.
Ok, will do it asap.
/**
@@ -353,11 +354,14 @@
return ObjectStreamField + type + + name + ;
}
+ final private native void setBooleanNative(Object obj, boolean val)
+throws IllegalAccessException;
+
This and the other setXXXNative methods should be moved to
VMObjectStreamClass. You should also provide sample JNI implementations
for this in native/jni/java-io/java_io_VMObjectStreamClass.c
(Note that VmSystem.c has examples for this. See VMSystem
setIn/Out/Err())
Maybe make them static and also provide the field object?
I'll do as requested but concerning java_io_VMObjectStreamClass.c I
would like to propose something else.
After more experimentation, I would have preferred to have some private
internal methods in java.lang.reflect.Field which we could call directly
from native code. These calls should not check whether a field is final
or not but just try to set it. The other usual methods of Field
(get/set) should check the final bit and call the internal methods
afterwards.
If it's not possible I will borrow some code from native Field
implementation.
final void setBooleanField(Object obj, boolean val)
{
try
{
- field.setBoolean(obj, val);
+ setBooleanNative(obj, val);
}
catch(IllegalAccessException x)
{
Do we want to use the new method unconditionally or only when the field
is final?
Would it be useful ? I think it would only eat CPU as we always need to
set fields.
Cheers,
Guilhem.
___
Classpath mailing list
[EMAIL PROTECTED]
http://mail.gnu.org/mailman/listinfo/classpath
Re: Object serialization and final fields
Hi,
On Thu, 2004-03-25 at 21:19, Guilhem Lavaux wrote:
Some people has reported failures in kaffe with applications trying to
deserialize objects containing final fields. Apparently it is authorized
in the serialization spec but we cannot rely on
java.lang.reflect.Field to set them. So our only solution is to bypass
the protection in java.lang.reflect.Field by creating new native calls
in ObjectStreamField.
I am proposing the following changes for ObjectStreamField.
It is a bit sad that serialization needs a couple of things that clearly
should have been standard reflection methods/options. Sigh.
I like the idea in general.
Couple of comments/questions.
- ChangeLog entry is missing...
Index: java/io/ObjectStreamField.java
===
RCS file: /cvsroot/classpath/classpath/java/io/ObjectStreamField.java,v
retrieving revision 1.14
diff -u -b -B -r1.14 ObjectStreamField.java
-- java/io/ObjectStreamField.java 26 Feb 2004 07:53:15 - 1.14
+++ java/io/ObjectStreamField.java25 Mar 2004 20:19:43 -
@@ -38,9 +38,10 @@
package java.io;
+import gnu.java.lang.reflect.TypeSignature;
+
import java.lang.reflect.Field;
import java.lang.reflect.Modifier;
-import gnu.java.lang.reflect.TypeSignature;
import java.security.AccessController;
import java.security.PrivilegedAction;
@@ -64,7 +65,7 @@
{
this (field.getName(), field.getType());
this.field = field;
-toset = !Modifier.isFinal(field.getModifiers());
+//toset = !Modifier.isFinal(field.getModifiers());
}
Either remove this or make it a real comment explaining why we don't
test this anymore.
/**
@@ -353,11 +354,14 @@
return ObjectStreamField + type + + name + ;
}
+ final private native void setBooleanNative(Object obj, boolean val)
+throws IllegalAccessException;
+
This and the other setXXXNative methods should be moved to
VMObjectStreamClass. You should also provide sample JNI implementations
for this in native/jni/java-io/java_io_VMObjectStreamClass.c
(Note that VmSystem.c has examples for this. See VMSystem
setIn/Out/Err())
Maybe make them static and also provide the field object?
final void setBooleanField(Object obj, boolean val)
{
try
{
- field.setBoolean(obj, val);
+ setBooleanNative(obj, val);
}
catch(IllegalAccessException x)
{
Do we want to use the new method unconditionally or only when the field
is final?
Cheers,
Mark
signature.asc
Description: This is a digitally signed message part
___
Classpath mailing list
[EMAIL PROTECTED]
http://mail.gnu.org/mailman/listinfo/classpath
