RFR JDK-8149644 Integrate VarHandles

[Paul Sandoz]

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