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

[Zoltán Majó]

Hi Rémi,


On 06/02/2016 06:55 PM, forax at univ-mlv.fr wrote:
> [...]
> I fully understand your concerns. And thank you for sharing your asserts!
>
> Let's see what others think about how to go about this problem. So far
> the following options were explored
> - bail out compilation of non-compliant methods;
> - enforce JVMS conformance for all classes and keep constant folding
> enabled;
> - enforce JVMS conformance only for classes with _major_version >=52 and
> completely disable constant folding.
>
> I'm not sure which option is the best. Also, there might be other
> options as well.
> what about bailing out compilation of non-compliant methods if major <= 52 or if a backward compat flag is set and enforcing the JVMS spec for major >= 53 ?

In the latest webrev (webrev.06) compilation is not bailed out for 
non-compliant methods, only constant folding is disabled. The JVMS is 
enforced for version >= 51 (the exact version is subject to discussion).

Please see the latest webrev (webrev.06) in my reply to Vladimir K.

Thank you and best regards,


Zoltán

>> Thank you and best regards,
>>
>>
>> Zoltan
> Rémi
>
>>>> We could relax that criteria, e.g., by disabling folding only for those
>>>> field that were declared in a class with _major_version < 52. But it
>>>> would be great if you could give me some feedback before I look into
>>>> that. Thank you very much in advance!
>>>>
>>>> Updated webrev:
>>>> http://cr.openjdk.java.net/~zmajo/8157181/webrev.05/
>>>>
>>>> Testing: JPRT in progress.
>>>>
>>>> Best regards,
>>>>
>>>>
>>>> Zoltan
>>> Rémi
>>>
>>>>> 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
>>>>>>>>
>>>>>>>>