RFR JDK-8149644 Integrate VarHandles

[Vladimir Ivanov]

Thanks for the clarifications. JDK part looks good.

Best regards,
Vladimir Ivanov

On 2/26/16 10:01 PM, Paul Sandoz wrote:
> Hi Vladimir,
>
> Thanks for the reviews.
>
>> On 26 Feb 2016, at 17:31, Vladimir Ivanov <vladimir.x.ivanov at oracle.com> wrote:
>>
>>> Hotspot:
>>>    http://cr.openjdk.java.net/~psandoz/jdk9/JDK-8149644-varhandles-integration/hotspot/webrev/index.html
>> Looks good.
>>
>>> JDK:
>>>    http://cr.openjdk.java.net/~psandoz/jdk9/JDK-8149644-varhandles-integration/jdk/webrev/index.html
>> src/java.base/share/classes/java/lang/invoke/MethodHandles.java:
>> +        public VarHandle unreflectVarHandle(Field f) throws IllegalAccessException {
>> +            MemberName getField = new MemberName(f, false);
>> +            MemberName putField = new MemberName(f, true);
>> +            return getFieldVarHandleNoSecurityManager(getField.getReferenceKind(), putField.getReferenceKind(),
>> + f.getDeclaringClass(), getField, putField);
>> +        }
>>
>> unreflectField has the following check:
>>             Lookup lookup = f.isAccessible() ? IMPL_LOOKUP : this;
>>             return lookup.getDirectFieldNoSecurityManager(field.getReferenceKind(), f.getDeclaringClass(), field);
>>         }
>>
>> Why don't you do the same?
>>
>
> So as not to widen the places where the reflection accessibility bit can be leveraged (calls to setAccessible), and thus not support more things that can stomp on final fields.
>
> The documentation on unreflectVarHandle states:
>
> "Access checking is performed immediately on behalf of the lookup class, regardless of value of the field's accessible flag.”
>
> (There is a typo there i need to fix, “of value” -> “of the value”.)
>
>
> There is also a comment in the following code that is not quite correct when i updated the implementation with the accessibility restriction:
>
> private VarHandle getFieldVarHandleCommon(byte getRefKind, byte putRefKind,
>                                            Class<?> refc, MemberName getField, MemberName putField,
>                                            boolean checkSecurity) throws IllegalAccessException {
>      assert getField.isStatic() == putField.isStatic();
>      assert getField.isGetter() && putField.isSetter();
>      assert MethodHandleNatives.refKindIsStatic(getRefKind) == MethodHandleNatives.refKindIsStatic(putRefKind);
>      assert MethodHandleNatives.refKindIsGetter(getRefKind) && MethodHandleNatives.refKindIsSetter(putRefKind);
>
>      checkField(getRefKind, refc, getField);
>      if (checkSecurity)
>          checkSecurityManager(refc, getField);
>
>      if (!putField.isFinal()) {
>          // A VarHandle will only support updates final fields if allowed
>          // modes is TRUSTED.  In such cases the following checks are
>          // no-ops and therefore there is no need to invoke if the field
>          // is marked final
>          checkField(putRefKind, refc, putField);
>          if (checkSecurity)
>              checkSecurityManager(refc, putField);
>      }
>
> I will update to:
>
> // A VarHandle does not support updates to final fields, any
> // such VarHandle to a final field will be read-only and
> // therefore the following write-based accessibility checks are
> // only required for non-final fields
>
> Paul.
>
>> Otherwise, looks good.
>>
>> Best regards,
>> Vladimir Ivanov
>