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

[Zoltán Majó]

Hi David,


On 06/11/2016 02:26 AM, David Holmes wrote:
> <adding back hotspot-compiler-dev at openjdk.java.net>
>
> Hi Coleen,
>
> If I may ...
>
> On 11/06/2016 5:43 AM, 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.
>>
>> 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?
>
> Can't comment on the verifier part.
>
> The check is only currently enforced for classfile versions for JDK9+
>
>> Also, have you run the JCK tests?
>>
>> Why have a global flag if it's an error?
>
> We are enforcing a rule that has not been enforced before. To deal 
> with compatibility issues we have provided, as is customary for 
> non-security issues, a flag that allows the enforcement to be turned 
> off, if it causes a problem. (Though that is more likely needed for 
> JDK 7/8 compatibility than for 9).

thank you for clarifying that!

Best regards,


Zoltan

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