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

[David Holmes]

Hi Zoltan,

On 9/06/2016 3:52 AM, Zoltán Majó wrote:
> Hi David,
>
>
>
> On 06/02/2016 01:51 PM, David Holmes wrote:
>> [...]
>> The simplest, and common, approach to these kind of issues is to only
>> apply the correction for the current class file version and keep the
>> relaxed rules for all others. That provides the best compatibility
>> story. Of course if security is a concern then that is a different
>> matter - but such concerns can not be discussed here.
>
> thank you for the suggestion!
>
> In the latest webrev (webrev.06), I enable the additional check for
> classfile version >= 51 (JAVA_7_VERSION). With that version, we're
> conforming with the specification. So far no problems have showed up
> with our test base. The new check can be disabled with the
> CheckFinalFieldModifications flag (you've suggested that in your JBS
> comment).
>
> We can also change the checked version to 52 (JAVA_8) or 53 (JAVA_9).

I think this needs to go to CCC so that the compatibility issue can be 
examined. Normally, after long periods of non-conformance, we don't 
"back date" tighter checks - so my initial feeling is that this should 
only be enforced for Java 9 classfiles onwards. But that is not a 
decision for individual engineers to make.

Thanks,
David

> Please see the updated webrev (webrev.06) in my reply to Vladimir K.
>
> Thank you!
>
> Best regards,
>
>
> Zoltan
>
>>
>> David
>> -----
>>
>>> Before JVMS-7, where putfield/putstatic linkage checks were tightened,
>>> it was allowed to change final fields from anywhere in the same class.
>>>
>>> putfield "Linking Exceptions":
>>>
>>>   JVMS-6 [1]:
>>>     "Otherwise, if the field is final, it must be declared in the
>>> current class. Otherwise, an IllegalAccessError is thrown."
>>>
>>>   JVMS-7 [2]:
>>>     "Otherwise, if the field is final, it must be declared in the
>>> current class, and the instruction must occur in an instance
>>> initialization method (<init>) of the current class. Otherwise, an
>>> IllegalAccessError is thrown."
>>>
>>>
>>> src/share/vm/interpreter/linkResolver.cpp:
>>>
>>> +         (byte == Bytecodes::_putstatic && fd.is_static() &&
>>> method_name != vmSymbols::class_initializer_name()) ||
>>> +         ((byte == Bytecodes::_putfield || byte ==
>>> Bytecodes::_nofast_putfield) && !fd.is_static() && method_name !=
>>> vmSymbols::object_initializer_name())
>>>
>>>
>>> Also, as we found earlier, checking method name is not enough:
>>>
>>> bool Method::is_static_initializer() const {
>>>   // For classfiles version 51 or greater, ensure that the clinit
>>> method is
>>>   // static.  Non-static methods with the name "<clinit>" are not static
>>>   // initializers. (older classfiles exempted for backward
>>> compatibility)
>>>   return name() == vmSymbols::class_initializer_name() &&
>>>          has_valid_initializer_flags();
>>> }
>>>
>>>
>>> -  LinkInfo link_info(defc, name, type, KlassHandle(),
>>> /*check_access=*/false);
>>> +  LinkInfo link_info(defc, name, type, KlassHandle(), NULL,
>>> /*check_access=*/false);
>>>
>>> Use Handle() instead of NULL.
>>>
>>> Also, I'd prefer to see LinkInfo::_current_method and new constructor
>>> added:
>>>
>>>   LinkInfo(KlassHandle resolved_klass, Symbol* name, Symbol* signature,
>>>            Handle current_method, bool check_access = true) :
>>>
>>> _current_klass can be derived from _current_method, since:
>>>
>>> //   current_klass  = sending method holder (i.e., class containing the
>>> method
>>> //                    containing the call being resolved)
>>>
>>>> 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?
>>> File a bug against JCK.
>>>
>>> Best regards,
>>> Vladimir Ivanov
>>>
>>> [1]
>>> https://docs.oracle.com/javase/specs/jvms/se6/html/Instructions2.doc11.html
>>>
>>>
>>> [2]
>>> https://docs.oracle.com/javase/specs/jvms/se7/html/jvms-6.html#jvms-6.5.putfield
>>>
>>>
>>>
>>> [3]
>>> https://docs.oracle.com/javase/specs/jvms/se8/html/jvms-5.html#jvms-5.4.3.2
>>>
>>>
>>>>
>>>> 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
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>
>>>>
>