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

Zoltán Majó zoltan.majo at oracle.com
Thu Jun 2 16:48:03 UTC 2016

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


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

More information about the hotspot-compiler-dev mailing list