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

Zoltán Majó zoltan.majo at oracle.com
Fri Jun 10 06:38:38 UTC 2016 

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.

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-compiler-dev mailing list