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

[Vladimir Ivanov]

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


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();
}


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

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