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

[Vladimir Ivanov]

>> Otherwise, it's a bug in C1 and patching is broken for unlinked field
>> accesses.
>
> I did what you suggested and patching works fine. The putstatic (and the
> corresponding StoreField from above) is treated as a unlinked (as
> expected).
>
> However, linking does not generate the appropriate exception. The reason
> is that LinkResolver::resolve_field() checks only if the holder of the
> field is the same as the holder of the method, but not if the method is
> an initializer. So execution continues to bytecode @29 (that is constant
> folded by the compiler) and the original failure still appears.

It's contradicts JVMS, which requires an IAE to be thrown.
I think major release is the right place to fix it.

>>
>> 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.
>
> I assume by "problematic constant folding" you mean "constant fold final
> field loads (from the same class) in initializers" from above.

Yes, because JVMS allows multiple updates of final fields from 
initializers and compilers should respect that.

> My impression is that the compilers can be safe only by completely
> disabling folding of final fields. The reason is that, currently, any
> method of a class 'C' declaring a static final field 'f' can change the
> field 'f' after class 'C' has been initialized. So even if we are
> compiling a method 'm' of a class 'X' that is accessing field 'C::f',
> 'f' can change after 'm' has been compiled.
>
> So folding final fields must be disabled for all compilations. Or am I
> missing something?
Yes, you are right. The only way to completely avoid discrepancy is to 
disable constant folding of final fields.

So, I'm in favor of a generic fix to align the behavior with JVMS.

Best regards,
Vladimir Ivanov

>
> I agree with you, alternatively we can propose a more generic fix (fix
> the interpreter and the compilers as well). The fix would most likely
> affect field resolution in LinkResolver::resolve_field() around here:
> http://hg.openjdk.java.net/jdk9/hs-comp/hotspot/file/07a5eceac654/src/share/vm/interpreter/linkResolver.cpp#l910
>
>
> Changing resolve_field() to throw an IllegalAccessError for
> inappropriate field access will propagate the exception to the
> interpreter. Changing ciField::will_link() will most likely kill (some)
> compilations (if, e.g., fields are linked through ciField::will_link()).
>
> I'd prefer to take the second option (changing field resolution), but
> there might be some disadvantages related to option I might be overseeing.




>
> Thank you!
>
> Best regards,
>
>
> Zoltan
>
>
>>
>>> 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
>>
>