[9] RFR (S): 8160527: Check for final instance field updates can be omitted

Coleen Phillimore coleen.phillimore at oracle.com
Fri Jul 1 12:39:51 UTC 2016 


On 7/1/16 7:52 AM, Zoltán Majó wrote:
> Hi Coleen,
>
>
> thank you for the feedback!Please see details below.
>
> On 06/30/2016 10:15 PM, Coleen Phillimore wrote:
>>
>> Hi,  Looking at this further:
>>
>> http://cr.openjdk.java.net/~zmajo/8160527/webrev.01/src/share/vm/interpreter/interpreterRuntime.cpp.udiff.html 
>>
>>
>> *+ bool final_instance_update = info.field_holder()->major_version() 
>> >= 53 &&*
>> *+ info.has_initialized_final_update() &&*
>> *+ (bytecode == Bytecodes::_putfield || bytecode == 
>> Bytecodes::_nofast_putfield);*
>> *+ *
>> *+ *
>>
>> I think you can test info.access_flags().is_final() and not use the 
>> new has_initialized_final_update() flag.
>>
>> If the current_class of the function with the putfield is not the 
>> same as the class owning the field, an error would be thrown in 
>> linkResolver.cpp and the bytecode would never be marked as resolved 
>> in the constant pool of the current_class (only the class owning the 
>> field).  If the field is_final, it'll only be initialized in the 
>> <init> function so no point marking it resolved in the cpCache of the 
>> resolved_class anyway.
>
> I agree,but based on your second suggestion (below), it seems we don't 
> need the variable 'final_instance_update' anymore. Please see below.
>
>>
>> It seems like this is_put part of this 'if' statement:
>>
>>   if (!uninitialized_static) {
>>     get_code = ((is_static) ? Bytecodes::_getstatic : 
>> Bytecodes::_getfield);
>>     if (is_put || !info.access_flags().is_final()) {
>>       put_code = ((is_static) ? Bytecodes::_putstatic : 
>> Bytecodes::_putfield);
>>     }
>>   }
>>
>> Should be moved up to:
>>
>>   // We need to delay resolving put instructions on final fields
>>   // until we actually invoke one. This is required so we throw
>>   // exceptions at the correct place. If we do not resolve completely
>>   // in the current pass, leaving the put_code set to zero will
>>   // cause the next put instruction to reresolve.
>>   Bytecodes::Code put_code = (Bytecodes::Code)0;
>>   if (is_put && !info.access_flags().is_final()) {
>>     put_code = ((is_static) ? Bytecodes::_putstatic : 
>> Bytecodes::_putfield);
>>   }
>>
>> Where it'll match the comment.
>
> Yes, that is a good idea.
>
> But in the original code 'put_code' is set only if 
> !uninitialized_static is true.
>
>  595   if (!uninitialized_static) {
>  596     get_code = ((is_static) ? Bytecodes::_getstatic : 
> Bytecodes::_getfield);
> 597 if (is_put || !info.access_flags().is_final()) {
>  598       put_code = ((is_static) ? Bytecodes::_putstatic : 
> Bytecodes::_putfield);
>  599     }
>  600   }
>
>
> Therefore, the conditionyou've suggested
>
>   if (is_put && !info.access_flags().is_final()) {
>
> should include !uninitialized_static as well
>
>   if (is_put && !info.access_flags().is_final() && 
> !uninitialized_static) {

Okay, I see it now.   uninitialized_static is to delay resolution for 
the nonfinal fields.
>
> The comment
>
>   // We also need to delay resolving getstatic instructions until the
>   // class is intitialized. This is required so that access to the static
> should be updated as well to
>
>   // We also need to delay resolving getstatic and *putstatic* 
> instructions until the
>   // class is intitialized. This is required so that access to the static
>
> as we delay putstatic instructions as well.
>
> I restructured the code (incl. comments) accordingly. Here is the 
> updated webrev:
> http://cr.openjdk.java.net/~zmajo/8160527/webrev.02/

Yes, this looks very good.   It's much clearer because it was confusing 
with all these conditions.


>
> JPRT testing passes.

There's a test for delaying resolution for non-final fields somewhere.  
Did you find that?  I think it's in the compiler jtreg directories.

Thanks,
Coleen
>
> Thank you!
>
> Best regards,
>
>
> Zoltan
>
>
>
>>
>> Coleen
>>
>> On 6/30/16 3:31 PM, Coleen Phillimore wrote:
>>>
>>> Zoltan,
>>>
>>> On 6/30/16 6:03 AM, Zoltán Majó wrote:
>>>> Hi Coleen,
>>>>
>>>>
>>>> thank you for the feedback and for the review! Please see below.
>>>>
>>>> On 06/29/2016 11:01 PM, Coleen Phillimore wrote:
>>>>>
>>>>> Zoltan,
>>>>>
>>>>> http://cr.openjdk.java.net/~zmajo/8160527/webrev.00/src/share/vm/interpreter/interpreterRuntime.cpp.udiff.html 
>>>>>
>>>>>
>>>>> Yes, this matches the general strategy.   You don't have to have 
>>>>> this line
>>>>>
>>>>> *+ (bytecode == Bytecodes::_putfield || bytecode == 
>>>>> Bytecodes::_nofast_putfield);*
>>>>>
>>>>>
>>>>> Because the test below is:
>>>>>
>>>>> *!if (_(_is_put_&& !final_instance_update)_|| 
>>>>> !info.access_flags().is_final()) {*
>>>>
>>>> I think we need the condition
>>>>
>>>>     (bytecode == Bytecodes::_putfield || bytecode == 
>>>> Bytecodes::_nofast_putfield);
>>>>
>>>> for 'final_instance_update'. The reason is that we need to detect 
>>>> invalid updates only to *instance* fields but not to *static* 
>>>> fields; the condition 'is_put' includes Bytecodes::_putstatic as well.
>>>>
>>>>     bool is_put = (bytecode == Bytecodes::_putfield || bytecode == 
>>>> Bytecodes::_nofast_putfield ||
>>>> -->                bytecode == Bytecodes::_putstatic);
>>>>
>>>> The reason do not need to include putstatic is that the current 
>>>> delaying mechanism [1] [2] works fine for static final fields (but 
>>>> it is insufficient for instance final fields).
>>>
>>> ok.
>>>
>>>>
>>>> To check that invalid updates to static fields are properly 
>>>> detected, I've added a new test, TestPutStatic.jasm. The test is 
>>>> analogous to the TestPutField.jasm test that you've seen in the 
>>>> previous webrev (webrev.00); the VM passes the test already in its 
>>>> current state (without the change).
>>>>
>>>> Here is the updated webrev:
>>>> http://cr.openjdk.java.net/~zmajo/8160527/webrev.01/
>>>
>>> This looks good.
>>>>
>>>> I re-tested with JPRT, all tests (incl. the newly added ones) pass.
>>>>
>>>>>
>>>>>
>>>>> Otherwise, looks fine.  I'm sorry I didn't realize this in the 
>>>>> initial code review.
>>>>
>>>> I did not realize either that this problem existed -- good that 
>>>> John has pointed it out.
>>>>
>>>
>>> Thanks,
>>> Coleen
>>>
>>>> Thank you!
>>>>
>>>> Best regards,
>>>>
>>>>
>>>> Zoltan
>>>>
>>>> [1] 
>>>> http://hg.openjdk.java.net/jdk9/hs/hotspot/file/2af3fb9f244f/src/share/vm/interpreter/interpreterRuntime.cpp#l579
>>>> [2] 
>>>> http://hg.openjdk.java.net/jdk9/hs/hotspot/file/2af3fb9f244f/src/share/vm/interpreter/interpreterRuntime.cpp#l586
>>>>
>>>>>
>>>>> Coleen
>>>>>
>>>>>
>>>>> On 6/29/16 11:00 AM, Zoltán Majó wrote:
>>>>>> Hi,
>>>>>>
>>>>>>
>>>>>> please review the patch for 8160527.
>>>>>>
>>>>>> https://bugs.openjdk.java.net/browse/JDK-8160527
>>>>>>
>>>>>> Problem: 8157181 added a check that verifies that final instance 
>>>>>> fields can be updated only by object initializer methods (as 
>>>>>> required by JVMS >=7) [1].
>>>>>>
>>>>>> Unfortunately, the newly added check can be circumvented due to 
>>>>>> constant pool caching: If the instance field update is executed 
>>>>>> in an <init> method first, the instruction updating the field is 
>>>>>> cached in the constant pool cache. Subsequent updates use the 
>>>>>> constant pool cache (and do not call into the VM where the check 
>>>>>> is executed). As a result, any method can update a final instance 
>>>>>> field if that field was first updated in an instance initializer 
>>>>>> (the method must be declared in the same class as the field, 
>>>>>> though).
>>>>>>
>>>>>> John's comment in the bug description provides more detailed 
>>>>>> information on how the above can happen.
>>>>>>
>>>>>>
>>>>>> Solution: Do not cache putfield instructions to final instance 
>>>>>> fields if the verifier has detected that the field is updated in 
>>>>>> other methods than <init> (i.e., has_initialized_field_update() 
>>>>>> returns true for the field). Avoiding caching results in the 
>>>>>> field access being re-resolved until the offending access is 
>>>>>> attempted; then an exception is thrown. Avoiding caching must be 
>>>>>> done very infrequently, as offending code is rare (e.g., such 
>>>>>> code cannot be produced by javac).
>>>>>>
>>>>>> I've also corrected a small mistake in the fix for 8157181 (the 
>>>>>> class name is printed in the error message instead of the 
>>>>>> method's name).
>>>>>>
>>>>>> Webrev:
>>>>>> http://cr.openjdk.java.net/~zmajo/8160527/webrev.00/
>>>>>>
>>>>>> Testing:
>>>>>> - JPRT (testset hotspot, incl. newly added test);
>>>>>> - verified that the newly added test triggers the problem with an 
>>>>>> unmodified VM.
>>>>>>
>>>>>> Thank you!
>>>>>>
>>>>>> Best regards,
>>>>>>
>>>>>>
>>>>>> Zoltan
>>>>>>
>>>>>> [1] 
>>>>>> http://hg.openjdk.java.net/jdk9/hs-comp/hotspot/rev/c558d46c1af2#l11.76 
>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>

More information about the hotspot-runtime-dev mailing list