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

[Coleen Phillimore]

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.

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.

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