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

[Coleen Phillimore]

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