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

[Zoltán Majó]

Hi Coleen,


On 07/01/2016 03:33 PM, Zoltán Majó wrote:
> [...]
>
>>
>>
>>>
>>> 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.

I've run all hotspot tests on all supported platforms with -Xmixed and 
-Xcomp. The change does not trigger any failures.

I plan to push the latest webrev (webrev.02) to hs on Wednesday, July 6 
(unless, of course, there are objections).

Thank you!

Best regards,


Zoltan

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