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

[Zoltán Majó]

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

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/

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.

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