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

[Coleen Phillimore]

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

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


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