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

[Zoltán Majó]

Hi Vladimir,


thank you for the feedback!

On 06/10/2016 02:56 PM, Vladimir Ivanov wrote:
> http://cr.openjdk.java.net/~zmajo/8157181/webrev.08
>
> I'm fine with enabling the checks only for 9 for now and handle 7 & 8 
> class files separately.

OK, thanks!

>
>>> src/share/vm/classfile/verifier.cpp:
>>>
>>> + void
>>> Verifier::detect_invalid_final_field_accesses(instanceKlassHandle
>>> klass) {
>>>
>>> The name is misleading: such accesses are legal according to JVMS-6
>>> (and earlier). Maybe something like
>>> detect_initialized_final_field_updates?
>>>
>>> Same for:
>>> +   bool has_invalid_final_access() const    { return
>>> access_flags().has_field_invalid_final_access(); }
>>> +   void set_has_invalid_final_access(const bool value) {
>>> +   JVM_ACC_FIELD_INVALID_FINAL_ACCESS  = 0x00000100,
>>>
>>
>> Yes, that name sounds better and I've updated the code accordingly.
>
> A leftover:
>
> src/share/vm/runtime/fieldDescriptor.hpp:
>
> +   void set_has_invalid_final_access(const bool value) {

Thanks for catching that, corrected.

>
>>> ---
>>> I don't see much benefit in tracking the problematic updates
>>> per-methods and not per-class. So, you could do the checks in
>>> ClassVerifier::verify_field_instructions [1] and mark the whole class
>>> (ClassVerifier::_klass) as containing problematic instructions.
>>
>> The problematic updates are tracked *per-field* (and not *per-method* or
>> * per-class*).
>>
>> Initially, I had a version that tracked problematic instructions on a
>> per-class basis. With that version, once a problematic instruction was
>> detected, the whole class was marked as containing problematic
>> instructions. As a result, constant folding was disabled for accesses to
>> *all* final fields of that class.
>>
>> Tracking problematic updates on a per-field basis seems to be less
>> intrusive. That way we disable folding *only for fields to which
>> problematic accesses are made* (and leave constant folding enabled for
>> accesses to all other fields). So we can fold some accesses even for
>> "misbehaving" classes. I hope that is fine.
>>
>> Regarding doing checks in verify_field_instructions() [2]: It turns out
>> that we have two verifiers, one that is called for
>>
>> major_version >= STACKMAP_ATTRIBUTE_MAJOR_VERSION
>>
>> and a second one that is called for
>>
>> major_version < STACKMAP_ATTRIBUTE_MAJOR_VERSION.
>>
>> ClassVerifier::verify_field_instructions() is called only for class
>> files >= STACKMAP_ATTRIBUTE_MAJOR_VERSION so we need to call
>> detect_initialized_final_field_updates() after verification.
>>
>> But I think it is a good idea to move the check to the linkage phase (as
>> you've suggested above).
>
> While browsing the code, I got another idea: what about piggybacking 
> on Rewriter pass (see Rewriter::scan_method)? It already sets some 
> per-method properties (e.g., Method::has_monitor_bytecodes()).
>
> I'd like to avoid additional pass over the bytecode, because it can 
> cause slow downs during class loading (just a precaution - I haven't 
> measured the impact).

I understand your concern and have moved the detection of initialized 
final field updates to Rewriter::scan_method().

>
>>> ---
>>> src/share/vm/interpreter/linkResolver.cpp:
>>>
>>>         THROW(vmSymbols::java_lang_IllegalAccessError());
>>>
>>> +         THROW(vmSymbols::java_lang_IllegalAccessError());
>>>
>>> Please, add an error message for diagnostic purposes.
>>
>> OK, I did that.
>
> src/share/vm/interpreter/linkResolver.cpp:
>
> +   if (is_put && fd.access_flags().is_final()) {
> +     ResourceMark rm(THREAD);
> +     char msg[200];
>
> There's much more convenient way now - stringStream. It takes care of 
> memory management in resource area for you and provides convenient 
> utility functions:
>
>   ResourceMark rm(THREAD);
>   stringStream ss;
>   ss.printf("...", ...);
>   THROW_MSG(vmSymbols::java_lang_IllegalAccessError(), ss.as_string());

OK, I changed that part.

>
> ---
> +       if ((byte == Bytecodes::_putstatic && fd.is_static() && 
> !m()->is_static_initializer()) ||
> +           ((byte == Bytecodes::_putfield || byte == 
> Bytecodes::_nofast_putfield) && !fd.is_static() && 
> !m->is_object_initializer())) {
>
> Maybe refactor it to make more readable?
>
>   bool is_static_final_update = ...;
>   bool is_instance_final_update = ...;
>   if (is_static_final_update || is_instance_final_update) {

OK, I did that.

>   ...
>
> Otherwise, looks good.

Thank you! Please let me know if you spot anything with the newest changes.

Here is the updated webrev:
http://cr.openjdk.java.net/~zmajo/8157181/webrev.09/

JPRT is running.

Best regards,


Zoltan


>
> Best regards,
> Vladimir Ivanov