[9] RFR (S): 8157181: Compilers accept modification of final fields outside initializer methods

[Zoltán Majó]

Hi Coleen,


thank you for the feedback! Please see some detailed answers inline.

On 06/13/2016 02:23 PM, Coleen Phillimore wrote:
>
> Hi Zoltan,
>
> On 6/11/16 6:13 AM, Zoltán Majó wrote:
>> Hi Coleen,
>>
>>
>> thank you for the feedback!
>>
>> On 06/10/2016 09:43 PM, Coleen Phillimore wrote:
>>>
>>> Hi,
>>>
>>> I'm late to the thread, which is good because it's better that this 
>>> check is done in the rewriter, than in InstanceKlass.
>>
>> I'm glad you are OK with that.
>
> Yes, I think this is better.  We only want one bytecode iteration 
> during linking.  At one point, we talked about not having the rewriter 
> but that'll never happen.
>>
>>>
>>> But I'm fuzzy on why this wasn't checked in the verifier, and what 
>>> the purpose of the has_initialized_final_update (which means a non 
>>> <init> method has updated a final field, which is a verify error, 
>>> right?
>>
>> I think it is a linking exception that is supposed to be thrown 
>> during field resolution. Please see some details below (I use 
>> putfield as an example).
>>
>> For putfield "Linking Exceptions", the JVMS-6 [1] states:
>>
>> "During resolution of the symbolic reference to the field, any of the 
>> exceptions pertaining to field resolution documented in Section 
>> 5.4.3.2 can be thrown.
>> [...]
>> Otherwise, if the field is final, it must be declared in the current 
>> class. Otherwise, an IllegalAccessError is thrown."
>>
>> With JVMS-7 [2], the second sentence has changed the following way:
>>
>> "Otherwise, if the field is final, it must be declared in the current 
>> class, and the instruction must occur in an instance initialization 
>> method (<init>) of the current class. Otherwise, an 
>> IllegalAccessError is thrown."
>>
>> HotSpot currently enforces JVMS-6 (i.e.,that the final field accessed 
>> by the putfield must be declared in the current class). That 
>> condition is checked in LinkResolver::resolve_field() in 
>> linkResolver.cpp.
>
> Thank you for the clarification.  It looks correct.

Thank you!

>>
>> The current patch proposes to check *also* that the instruction 
>> occurs in the <init> method, as required by JVMS-7. I've added that 
>> check also to LinkResolver::resolve_field() (I had the feeling that 
>> the two checks belong together). The new check is, however, enforced 
>> only for class file versions >=JDK-9 (as David pointed out earlier).
>>
>> Regarding has_initialized_final_update: Some compiler optimizations 
>> assume that final fields are modified only in initializer methods. 
>> That assumption does always not hold (e.g., for class file versions 
>> <JDK-9). So the compiler must be made aware of fields that are 
>> updated outside initializers; for such fields 
>> has_initialized_final_update() returns true.
>>
>> The analysis to detect "invalid" updates to final fields must be 
>> performed before any of a class's methods is compiled. So the 
>> analysis is performed at class loading and initialization.
>
> Ok.  I didn't understand how it affected the compilers but that's 
> beyond the scope of my review anyway.
>
>>
>> The current patch proposes to do the analysis in the rewriter. That 
>> way we don't introduce an extra iteration over all bytecodes that are 
>> loaded (because we piggyback on the iteration performed by the 
>> rewriter). So we reduce the risk of potential performance degradations.
>>
>>>
>>> Also, have you run the JCK tests?
>>
>> Yes, I've run all all JCK tests executed in JPRT (the runThese8 
>> target). I also did a complete pre-PIT RBT (with -Xmixed and -Xcomp) 
>> run. No failures have occurred.
>>
>
> I'll send you a link to how to run the jck9 jck tests.  They take a 
> bit longer to run.  They may run with the pre-PIT RBT though, so you 
> might be ok.   Please check that this is the case.

Thank you for sending me the instructions! To be an the safe side, I've 
run all JCK tests (the way you've suggested), all pass. I've attached to 
the bug description a file containing the test output and some details 
on the setup I used to run the tests.

>
>>>
>>> Why have a global flag if it's an error?
>>
>> David has pointed out the reasons in his reply.
>>
>
> Ok.  Makes sense.  I wonder if I should have done that with 
> https://bugs.openjdk.java.net/browse/JDK-8145148

I'm not sure.

For the current bug, it seems that we've have agreed to not have a 
global flag after all. For JDK-8145148, we would probably would have to 
think of the possible (customer) impact of the introduced changes by and 
decide then if such a flag is necessary. Also, the fix for JDK-8145148 
was not backported to earlier releases, so maybe it's fine to have 
stricter checks introduced with a new major release.

Thank you!

Best regards,


Zoltan

>
>
> Thanks,
> Coleen
>> Thank you!
>>
>> Best regards,
>>
>>
>> Zoltan
>>
>> [1] 
>> https://docs.oracle.com/javase/specs/jvms/se6/html/Instructions2.doc11.html
>>
>> [2] 
>> https://docs.oracle.com/javase/specs/jvms/se7/html/jvms-6.html#jvms-6.5.putfield
>>>
>>> Thanks,
>>> Coleen
>>>
>>>
>>>
>>> On 6/10/16 1:40 PM, Zoltán Majó wrote:
>>>> Hi Vladimir,
>>>>
>>>>
>>>> On 06/10/2016 06:05 PM, Vladimir Ivanov wrote:
>>>>>
>>>>>> Please note an additional small change in rewriter.cpp:
>>>>>>
>>>>>> +if (!reverse) {
>>>>>> // Check if any final field of the class given as parameter is 
>>>>>> modified
>>>>>> // outside of initializer methods of the class. Fields that are 
>>>>>> modified
>>>>>>    ...
>>>>>> +}
>>>>>>
>>>>>> It's sufficient to check fields during rewriting (i.e., we're coming
>>>>>> from Rewriter::rewrite_bytecodes()). We do not need to do the 
>>>>>> checks if
>>>>>> the class has already been rewritten but we're reversing changes 
>>>>>> due to
>>>>>> some failure that appeared during rewriting (in that case 
>>>>>> scan_method()
>>>>>> is called from Rewriter::restore_bytecodes()).
>>>>>
>>>>> Ok.
>>>>>
>>>>>> Here is the updated webrev:
>>>>>> http://cr.openjdk.java.net/~zmajo/8157181/webrev.11/
>>>>>
>>>>> Looks good.
>>>>>
>>>>> src/share/vm/runtime/globals.hpp
>>>>>
>>>>> +          "for classfile version >= 51")
>>>>>
>>>>> s/51/53/ (no need to send new webrev).
>>>>
>>>> OK, I've corrected the code in-situ (in webrev.11). I also have the 
>>>> JPRT results now, all tests pass.
>>>>
>>>> Thank you!
>>>>
>>>> Best regards,
>>>>
>>>>
>>>> Zoltan
>>>>
>>>>
>>>>
>>>>>
>>>>> Best regards,
>>>>> Vladimir Ivanov
>>>>
>>>
>>
>