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

[Zoltán Majó]

Forgot to add hotspot-compiler-dev, sorry.

Best regards,


Zoltan

-------- Forwarded Message --------
Subject: 	Re: [9] RFR (S): 8157181: Compilers accept modification of 
final fields outside initializer methods
Date: 	Sat, 11 Jun 2016 12:13:12 +0200
From: 	Zoltán Majó <zoltan.majo at oracle.com>
To: 	Coleen Phillimore <coleen.phillimore at oracle.com>, 
hotspot-dev at openjdk.java.net



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