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

Wed Jun 8 17:52:17 UTC 2016

Hi Vladimir,

thank you for the feedback. Please see comments inline.

On 06/02/2016 12:47 PM, Vladimir Ivanov wrote:
> Zoltan,
>
> (broadening the audience to hotspot-dev@)
>
> I'd like to draw attention from Runtime group to this change.
>> OK, so I have implemented a generic fix that adds the checks required by
>> the JVMS.
>>
>> For bytecodes modifying final fields, the VM checks
>> - that the accessing method is <clinit> (for static fields)
>> - that the accessing method is <init> (for instance fields)
>> and thrown an IllegalAccessError if any of the checks fails.
>>
>> Here is the new webrev:
>> http://cr.openjdk.java.net/~zmajo/8157181/webrev.02/
>
> Regarding tightening linkage checks, your fix doesn't take into 
> account JVMS changes between 6 & 7 (expected behavior differs 
> depending on class file version).
>
> Before JVMS-7, where putfield/putstatic linkage checks were tightened, 
> it was allowed to change final fields from anywhere in the same class.
>
> putfield "Linking Exceptions":
>
>   JVMS-6 [1]:
>     "Otherwise, if the field is final, it must be declared in the 
> current class. Otherwise, an IllegalAccessError is thrown."
>
>   JVMS-7 [2]:
>     "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."

Thanks for drawing attention of the different specification for version 6.

>
>
> src/share/vm/interpreter/linkResolver.cpp:
>
> +         (byte == Bytecodes::_putstatic && fd.is_static() && 
> method_name != vmSymbols::class_initializer_name()) ||
> +         ((byte == Bytecodes::_putfield || byte == 
> Bytecodes::_nofast_putfield) && !fd.is_static() && method_name != 
> vmSymbols::object_initializer_name())
>
>
> Also, as we found earlier, checking method name is not enough:
>
> bool Method::is_static_initializer() const {
>   // For classfiles version 51 or greater, ensure that the clinit 
> method is
>   // static.  Non-static methods with the name "<clinit>" are not static
>   // initializers. (older classfiles exempted for backward compatibility)
>   return name() == vmSymbols::class_initializer_name() &&
>          has_valid_initializer_flags();
> }

Right. I missed that issue, but have addressed it in the latest webrev 
(webrev.06).

>
>
> -  LinkInfo link_info(defc, name, type, KlassHandle(), 
> /*check_access=*/false);
> +  LinkInfo link_info(defc, name, type, KlassHandle(), NULL, 
> /*check_access=*/false);
>
> Use Handle() instead of NULL.

It seems that methodHandle() works better for these cases, but... (see 
below)

>
>
> Also, I'd prefer to see LinkInfo::_current_method and new constructor 
> added:
>
>   LinkInfo(KlassHandle resolved_klass, Symbol* name, Symbol* signature,
>            Handle current_method, bool check_access = true) :
>
> _current_klass can be derived from _current_method, since:
>
> //   current_klass  = sending method holder (i.e., class containing 
> the method
> //                    containing the call being resolved)

... I've added two new constructors. One of them derives _current_method 
from _current_klass, the other uses methodHandle() within the constructor.

Please see the updated webrev (webrev.06) in the reply to Vladimir K.

Best regards,

Zoltan

>
>> I did the following testing:
>> - JPRT
>> - pre-PIT RBT run (in progress)
>> - the reproducer.
>>
>> The reproducer now behaves as expected (an IllegalAccessError is
>> thrown). Also, the pre-PIT RBT run has shown no new failures so far.
>>
>> But there is a problem with JPRT: A JCK test, putstatic01801m, fails.
>>
>> The reason for the failure is that the the test generates a method that
>> contains a putstatic to a static final field (i.e., the bytecodes
>> generated by the test do not conform to the JVMS). (Even the test
>> mentions that the Java source equivalent to the generated bytecodes is
>> compilable only if the "final" keyword is removed from the declaration
>> of the static field.)
>>
>> So (if we decide to push the current fix) the issue with the test needs
>> to be fixed first. Do you know how we could proceed with that?
> File a bug against JCK.
>
> Best regards,
> Vladimir Ivanov
>
> [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
>
> [3] 
> https://docs.oracle.com/javase/specs/jvms/se8/html/jvms-5.html#jvms-5.4.3.2
>
>>
>> Thank you and best regards,
>>
>>
>> Zoltan
>>
>>>
>>> Best regards,
>>> Vladimir Ivanov
>>>
>>>>
>>>> I agree with you, alternatively we can propose a more generic fix (fix
>>>> the interpreter and the compilers as well). The fix would most likely
>>>> affect field resolution in LinkResolver::resolve_field() around here:
>>>> http://hg.openjdk.java.net/jdk9/hs-comp/hotspot/file/07a5eceac654/src/share/vm/interpreter/linkResolver.cpp#l910 
>>>>
>>>>
>>>>
>>>>
>>>> Changing resolve_field() to throw an IllegalAccessError for
>>>> inappropriate field access will propagate the exception to the
>>>> interpreter. Changing ciField::will_link() will most likely kill 
>>>> (some)
>>>> compilations (if, e.g., fields are linked through 
>>>> ciField::will_link()).
>>>>
>>>> I'd prefer to take the second option (changing field resolution), but
>>>> there might be some disadvantages related to option I might be
>>>> overseeing.
>>>
>>>
>>>
>>>
>>>>
>>>> Thank you!
>>>>
>>>> Best regards,
>>>>
>>>>
>>>> Zoltan
>>>>
>>>>
>>>>>
>>>>>> More details about the failure are here [3].
>>>>>>
>>>>>> With the patch applied, the program always completes as it is 
>>>>>> expected
>>>>>> by Jython, no matter if we use -Xint, -Xcomp, or -Xmixed (because we
>>>>>> always interpret methods non-conforming with the VM spec).
>>>>>>
>>>>>> Here is the updated webrev:
>>>>>> http://cr.openjdk.java.net/~zmajo/8157181/webrev.01/
>>>>> Bailing out the whole compilation in C2 is even less appropriate. It
>>>>> should generate an uncommon trap for such accesses instead (see
>>>>> Parse::do_field_access).
>>>>>
>>>>> Best regards,
>>>>> Vladimir Ivanov
>>>>>
>>>>> [1]
>>>>> http://hg.openjdk.java.net/jdk9/hs-comp/hotspot/file/07a5eceac654/src/share/vm/c1/c1_GraphBuilder.cpp#l1595 
>>>>>
>>>>>
>>>>>
>>>>
>>