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

Zoltán Majó zoltan.majo at oracle.com
Fri Jun 10 08:36:18 UTC 2016 

Hi David,


On 06/10/2016 09:30 AM, David Holmes wrote:
>
> On 10/06/2016 4:38 PM, Zoltán Majó wrote:
>> Hi David,
>>
>>
>> On 06/10/2016 02:59 AM, David Holmes wrote:
>>> 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.
>>
>> Thank you for the feedback!
>>
>> I have changed the checked version to JAVA_9 (as you initially
>> suggested). Here is the updated webrev:
>>
>> http://cr.openjdk.java.net/~zmajo/8157181/webrev.08/
>>
>> I hope we don't need CCC approval if we apply the correction for the
>> current class version only.
>>
>> I can file a new bug to investigate if the checks can be tightened up to
>> pre-JAVA_9 -- that could form the discussion with the CCC.
>
> That sounds fine - thanks.

Thank you!

I've filed JDK-8159215 [1] for tightening up the checks for Java 7 
and/or Java 8.

>
> Note I have not reviewed the actual code.

I've noted that. Thank you for all the suggestions/feedback!

Best regards,


Zoltan

[1] https://bugs.openjdk.java.net/browse/JDK-8159215

>
> Cheers,
> David
>
>> Thank you!
>>
>> Best regards,
>>
>>
>> Zoltan
>>
>>>
>>>
>>> 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 
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>
>>>>
>>

More information about the hotspot-dev mailing list