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

[Zoltán Majó]

Hi Coleen,


thank you for the feedback! Please see below.

On 07/01/2016 02:39 PM, Coleen Phillimore wrote:
> [...]
>>
>> 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.

Thank you!

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

I have not found that test, unfortunately. If you point me to the test, 
I'd be glad to run it.

Alternatively, I can run all hotspot tests on all supported platforms 
with -Xmixed and -Xcomp. That testbase has a good potential for 
triggering problems.

Thank you!

Best regards,


Zoltan


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