[9] RFR (S): 8160527: Check for final instance field updates can be omitted
Zoltán Majó
zoltan.majo at oracle.com
Fri Jul 1 11:52:46 UTC 2016
Hi Coleen,
thank you for the feedback!Please see details below.
On 06/30/2016 10:15 PM, Coleen Phillimore wrote:
>
> Hi, Looking at this further:
>
> http://cr.openjdk.java.net/~zmajo/8160527/webrev.01/src/share/vm/interpreter/interpreterRuntime.cpp.udiff.html
>
>
> *+ bool final_instance_update = info.field_holder()->major_version()
> >= 53 &&*
> *+ info.has_initialized_final_update() &&*
> *+ (bytecode == Bytecodes::_putfield || bytecode ==
> Bytecodes::_nofast_putfield);*
> *+ *
> *+ *
>
> I think you can test info.access_flags().is_final() and not use the
> new has_initialized_final_update() flag.
>
> If the current_class of the function with the putfield is not the same
> as the class owning the field, an error would be thrown in
> linkResolver.cpp and the bytecode would never be marked as resolved in
> the constant pool of the current_class (only the class owning the
> field). If the field is_final, it'll only be initialized in the
> <init> function so no point marking it resolved in the cpCache of the
> resolved_class anyway.
I agree,but based on your second suggestion (below), it seems we don't
need the variable 'final_instance_update' anymore. Please see below.
>
> It seems like this is_put part of this 'if' statement:
>
> if (!uninitialized_static) {
> get_code = ((is_static) ? Bytecodes::_getstatic :
> Bytecodes::_getfield);
> if (is_put || !info.access_flags().is_final()) {
> put_code = ((is_static) ? Bytecodes::_putstatic :
> Bytecodes::_putfield);
> }
> }
>
> Should be moved up to:
>
> // We need to delay resolving put instructions on final fields
> // until we actually invoke one. This is required so we throw
> // exceptions at the correct place. If we do not resolve completely
> // in the current pass, leaving the put_code set to zero will
> // cause the next put instruction to reresolve.
> Bytecodes::Code put_code = (Bytecodes::Code)0;
> if (is_put && !info.access_flags().is_final()) {
> put_code = ((is_static) ? Bytecodes::_putstatic :
> Bytecodes::_putfield);
> }
>
> Where it'll match the comment.
Yes, that is a good idea.
But in the original code 'put_code' is set only if !uninitialized_static
is true.
595 if (!uninitialized_static) {
596 get_code = ((is_static) ? Bytecodes::_getstatic : Bytecodes::_getfield);
597 if (is_put || !info.access_flags().is_final()) {
598 put_code = ((is_static) ? Bytecodes::_putstatic : Bytecodes::_putfield);
599 }
600 }
Therefore, the conditionyou've suggested
if (is_put && !info.access_flags().is_final()) {
should include !uninitialized_static as well
if (is_put && !info.access_flags().is_final() &&
!uninitialized_static) {
The comment
// We also need to delay resolving getstatic instructions until the
// class is intitialized. This is required so that access to the static
should be updated as well to
// We also need to delay resolving getstatic and *putstatic*
instructions until the
// class is intitialized. This is required so that access to the static
as we delay putstatic instructions as well.
I restructured the code (incl. comments) accordingly. Here is the
updated webrev:
http://cr.openjdk.java.net/~zmajo/8160527/webrev.02/
JPRT testing passes.
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
>>>>>
>>>>>
>>>>
>>>
>>
>
More information about the hotspot-runtime-dev
mailing list