RVM

Thanks Yuval for bringing this to our attention, but I'm not sure the patch should go in. The optimizing compiler folds final fields, for example:

class BooleanHolder {
private final boolean value;
BooleanHolder (boolean val) { this.value = val; }
boolean getValue() {return value;}
}
class Test {
static final BooleanHolder DEBUG = new BooleanHolder(false);
public static void main (String[] args) {
if (DEBUG.getValue()) { System.out.println("Hey we're debugging stuff!"); }
}
}

Ideally in main in the optimizing compiler we'll see that DEBUG can only be one object (assuming the class is initialized) and that value in BooleanHolder has a final value. We take that final value and use it as a constant within the optimizing compiler. This means that the optimizing compiler entirely removes the if(DEBUG...){...} code.

This is not only an important optimization in application code, but we rely quite heavily upon it in the RVM and MMTk to fold away possible costs of having flexible implementations.

What the patch allows is that reflection code would be able to toggle the value of the BooleanHolder which isn't something normal application code would be able to do. This could yield unexpected behavior and given our final field folding is aggressive, we're likely to see errors. It strikes me as cleaner to throw the illegal access exception.

Final fields are a notoriously buggy concept, in particular if this is allowed to escape during object construction then the final field may not have been initialized (see unsafe construction - this is really scary given what our optimizing compiler can do, but it never happens in practice/our regression test set).

The new problem is how to fix this properly? Its certainly the case that Sun's VM lets you do this, but maybe this is because of legacy compatibility and something they couldn't walk away from.

It strikes me that if we were to implement this we'd need to take better control of accessibility and either deny permission to change values that had been folded during optimization or invalidate methods that were compiled assuming final values. The former wouldn't bring us into line with Sun and the latter is likely to have significant runtime cost. Because of this my feeling is this bug should be closed as a "Won't Fix" and that we accept the difference from Sun's JVM. I won't do this immediately so others can post feedback.

There's quite a nice commentary on this here:

http://www.javaspecialists.eu/archive/Issue096.html

Apparently Java 5 allows final fields to be modified by reflection (Java 4 didn't) for serialization purposes and the JIT compiler is allowed to get the wrong value. As our serialization uses our internal reflection methods, I'm not sure if we need final fields to be modifiable to support serialization.

Quite an interesting commentary.

Sun's API documentation states that:

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.

I am not very familiar with the optimising compiler, but I tend to believe it does not fold finals set in the serialisation code. Hence, it seems to me that the fix is consistent with the documented behaviour, and that there is no need to modify the optimising compiler. JikesRVM code does not need modifieable final fields, but application code may use that in the customised readObject method. This seems to be the only portable way to set transient final fields during deserialisation.

And another interesting comment on the subject:

http://www.cs.umd.edu/~pugh/java/memoryModel/archive/2423.html

I believe Yuval is correct that we need to allow this to support deserialization of various forms. If we're serious about running arbitrary Java programs, we have to allow this.

My suggestion would be that we fail the setAccessible for all org.jikesrvm.* and org.MMTk.* classes so we can continue to do the optimization without runtime checks without compromising VM integrity.

We can then provide a command line argument that disables/enables final field chasing outside of the VM code. By default it is enabled, and just like the product JVMs, if the app does something stupid, it sees funny results. Any correct use of this ability to modify final fields should only be happening to objects where we couldn't have applied final field chasing anyways, so I don't see this position as being that risky. But, we can prodive a simple command line flip to problem determination if it ever does happen that we optimize away something that someone then uses reflection to change.

This is a relatively minor thing to fix up (its a bit annoying that accessible is a property of java.lang.reflect.AccessibleObject rather than a VM equivalent) so I've scheduled it for 2.9.3. Its worth noting that the command line switch should only stop folding of non-RVM classes, which will lead to some string comparisons on package names.

Andrew Hughes recently refactored the reflection code in Classpath as projects (such as Jikes RVM and Cacao) had significantly similar implementations that fixing a bug in one meant fixing a bug in all the others separately. I believe Andrew is best placed to evaluate this change and decide where to make the alteration.

Pushing back to 2.9.4. Not a release blocker, but we really should fix this; been open a while.

Resolved in r14471.

reopening so I can modify fix target to 3.0