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

[Zoltán Majó]

Hi Vladimir,


thank you for the detailed feedback. Please see comments below.

On 05/18/2016 02:17 PM, Vladimir Ivanov wrote:
>>> 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?

No, I did not and that was the reason why the access to offset -1 was 
generated.

>
> 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.

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 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.

I assume by "problematic constant folding" you mean "constant fold final 
field loads (from the same class) in initializers" from above.

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?

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