[9] RFR (S): 8157181: Compilers accept modification of final fields outside initializer methods

[Vladimir Ivanov]

>> Also, instead of bailing out the whole compilation, you can emit an
>> access as if the field access isn't linked yet (C1 supports that by
>> setting field offset to -1). The JVM should throw an appropriate
>> exception when trying to link the field.
>
> I tried that (by setting the variable 'offset' in
> GraphBuilder::access_field to -1. But C1 will generate an access to an
> offset of -1; executing that code results in data corruption.
Have you set needs_patching as well?

src/share/vm/c1/c1_GraphBuilder.cpp [1]

void GraphBuilder::access_field(Bytecodes::Code code) {
...
   const bool needs_patching = !holder->is_loaded() ||
                               !field->will_link(method()->holder(), 
code) ||
                               PatchALot;
...
   const int offset = !needs_patching ? field->offset() : -1;
...
     case Bytecodes::_putstatic: {
...
       append(new StoreField(append(obj), offset, field, val, true, 
state_before, needs_patching));

Otherwise, it's a bug in C1 and patching is broken for unlinked field 
accesses.

>>> It seems to me that the interpeter is affected by a related problem
>>> (i.e., the interpreter does not throw an IllegalAccessError if (2) does
>>> not hold). I'd prefer to file a separate bug for that problem.
>> I'm confused. How do you observe the difference between -Xint & -Xcomp
>> mode then?
>
> Without the patch applied:
> * With -Xint, the VM  does not throw an exception IllegalAccessError (as
> required by the VM spec). Instead, the VM executes the putstatic to the
> static final field as any other putstatic.
> * -Xcomp generates optimized code that folds a constant and results in a
> NullPointerException being thrown.

It's a different bug then. Compilers should not constant fold final 
field loads (from the same class) in initializers.

> The behavior with -Xint is the behavior expected and accepted by Jython.
> The problem was there since at least JDK 6, but was never observed "in
> the wild". The reason is that the method triggering the problem is not
> hot enough to be compiled (and so it is only interpreted).

I vaguely remember JVMS allowed changing final fields in the same class 
from any code, but then that part was tightened (in 5?).

I'd prefer to avoid discrepancy in behavior between different execution 
modes. Either disable problematic constant folding in compilers for now 
or fix compilers and interpreter at once.

> More details about the failure are here [3].
>
> With the patch applied, the program always completes as it is expected
> by Jython, no matter if we use -Xint, -Xcomp, or -Xmixed (because we
> always interpret methods non-conforming with the VM spec).
>
> Here is the updated webrev:
> http://cr.openjdk.java.net/~zmajo/8157181/webrev.01/
Bailing out the whole compilation in C2 is even less appropriate. It 
should generate an uncommon trap for such accesses instead (see 
Parse::do_field_access).

Best regards,
Vladimir Ivanov

[1] 
http://hg.openjdk.java.net/jdk9/hs-comp/hotspot/file/07a5eceac654/src/share/vm/c1/c1_GraphBuilder.cpp#l1595