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

[Zoltán Majó]

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.

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

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.

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.

>
> Why have a global flag if it's an error?

David has pointed out the reasons in his reply.

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