[9] RFR (S): 8160527: Check for final instance field updates can be omitted
Coleen Phillimore
coleen.phillimore at oracle.com
Thu Jun 30 19:31:55 UTC 2016
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
>>>
>>
>
More information about the hotspot-runtime-dev
mailing list