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

[Vladimir Kozlov]

How about only fixing C1 to take into account the store (or not constant fold in such case) because it was allowed before JVMS-7?

And file a separate issue for JVMS-7 conformance. It looks like it may have huge impact on performance and we are already very late in JDK 9 development to make such changes in rush.

Zoltan, what C2 does in such case?
Tom, what Graal does in such case?

The case is:

https://bugs.openjdk.java.net/browse/JDK-8157181?focusedCommentId=13942824&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-13942824

"While compiling bytecode @29, the C1 compiler assumes that the value of the _pyx0.self field has already been set in the class initializer <clinit> method (as the class is initialized). The C1 
compiler also assumes that the field will not change (as it is static final). Therefore, the C1 compiler omits reading the field _pyx0.self and passes the current value of the field ('null') to the 
org/python/core/Py.newCode() method called at bytecode @37. (The correct value would be 'this'.)"

Thanks,
Vladimir

On 6/2/16 10:51 AM, Tom Rodriguez wrote:
>>>> Therefore, in the current webrev, I've disabled folding of accesses to
>>>> all non-stable fields.
>>> Please do not do that, it will be a huge regression in term of perf at least for the language runtimes i maintain
>>> (and i suppose for Nashorn, JRuby or Groovy), Here are the perf assertions i use routinely,
>>> static final method handle is considered as constant, final fields (even not static) of VM anonymous class is considered as truly final.
>>>
>>
>> I fully understand your concerns. And thank you for sharing your asserts!
>
> The folding within the compiler should be handled differently since the concerns are different and removing it will likely cause invokeynamic performance to collapse.  The compiler is only folding
> final instance fields that are part of a chain of constants, so the main concern is whether an object has been published before it’s been fully constructed or if someone is going to use reflection to
> modify a final field after the object has been published.  The trust_final_non_static_fields logic in ciField.cpp is calling out classes which we control that shouldn’t violate those rules and that
> are important for performance.  So I think it should be left as is.  It really doesn’t relate to the issue at hand I think.
>
> Also you’ve removed folding of static final fields which would be a very bad thing to do.
>
> ciField has some caching logic for _known_to_link_with_get/set and if you are adding a ciMethod* argument you also need to cache the method that was checked last time as well.
>
> tom
>
>>
>> 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.
>>
>> Thank you and best regards,
>>
>>
>> Zoltan
>>
>>
>>
>>>> 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
>