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

[Zoltán Majó]

Hi Rémi,


On 06/02/2016 06:30 PM, Remi Forax wrote:
> [...]
>
> ----- Mail original -----
>> De: "Zoltán Majó" <zoltan.majo at oracle.com>
>> À: hotspot-compiler-dev at openjdk.java.net
>> Envoyé: Jeudi 2 Juin 2016 17:20:37
>> Objet: Re: [9] RFR (S): 8157181: Compilers accept modification of final	fields outside initializer methods
>>
>> Hi,
>>
>>
>> On 06/02/2016 11:29 AM, Zoltán Majó wrote:
>>> [...]
>>>
>>> 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?
>> After a (private) discussion with Igor Ignatyev, I've filed a bug with
>> the JCK team to address the issue with the putstatic01801m test.
>>
>> In the bug report, David Holmes suggest the following:
>>
>> "If this is a long standing non-compliance with the JVMS then its impact
>> can not be considered high. The longer the VM and spec do not match the
>> more likely we adjust the spec to match the VM behaviour. When that is
>> not feasible then the usual approach is to enforce the correct rules as
>> of the current classfile version, otherwise we risk introducing
>> compatibility issues. Even if we make the correct behaviour the default,
>> we will usually provide a flag to enable the old behaviour for
>> compatibility purposes. Only if this is a security concern would we
>> force the change to the new behaviour. "
>>
>> So, to follow David's suggestions, I did the following modifications to
>> the previous webrev:
>> - the check for the caller method in LinkResolver::resolve_field() is
>> performed only for classes with _major_version >= 52;
>> - I added a new diagnostic flag, CheckFinalFieldModifications to disable
>> the new checks (please feel free to suggest a better flag name).
>>
>> That should solve the problem with the JCK test. However, if we follow
>> David's suggestions, we can't guarantee for all classes that final
>> fields won't change after initialization. So the compilers can generate
>> incorrect code when compiling accessing to fields of those classes.
>>
>> 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!

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