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

[Zoltán Majó]

Hi Vladimir,


On 05/18/2016 03:47 PM, Vladimir Ivanov wrote:
> [...]
>
> So, I'm in favor of a generic fix to align the behavior with JVMS.

OK, so I have implemented a generic fix that adds the checks required by 
the JVMS.

For bytecodes modifying final fields, the VM checks
- that the accessing method is <clinit> (for static fields)
- that the accessing method is <init> (for instance fields)
and thrown an IllegalAccessError if any of the checks fails.

Here is the new webrev:
http://cr.openjdk.java.net/~zmajo/8157181/webrev.02/

I did the following testing:
- JPRT
- pre-PIT RBT run (in progress)
- the reproducer.

The reproducer now behaves as expected (an IllegalAccessError is 
thrown). Also, the pre-PIT RBT run has shown no new failures so far.

But there is a problem with JPRT: A JCK test, putstatic01801m, fails.

The reason for the failure is that the the test generates a method that 
contains a putstatic to a static final field (i.e., the bytecodes 
generated by the test do not conform to the JVMS). (Even the test 
mentions that the Java source equivalent to the generated bytecodes is 
compilable only if the "final" keyword is removed from the declaration 
of the static field.)

So (if we decide to push the current fix) the issue with the test needs 
to be fixed first. Do you know how we could proceed with that?

Thank you and best regards,


Zoltan

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