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

[Zoltán Majó]

Hi Doug,


On 06/02/2016 12:10 PM, Doug Simon wrote:
> Could you please copy the javadoc for the `method` parameter from jdk.vm.ci.meta.ConstantPool.lookupField to jdk.vm.ci.hotspot.CompilerToVM.resolveFieldInPool.

thank you for the feedback! Here is the updated webrev:

http://cr.openjdk.java.net/~zmajo/8157181/webrev.03/

Best regards,


Zoltan

>
> Otherwise, the changes to the JVMCI code look good.
>
> -Doug
>
>> On 02 Jun 2016, at 11:29, Zoltán Majó <zoltan.majo at oracle.com> wrote:
>>
>> 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
>>>>>