[foreign-memaccess] RFR 8237648: Add support for var handle adaptation

[Remi Forax]

Fine for me,
just in MethodHandles.noCheckedExceptions:
  Class<?>[] exceptionTypes = null;
  switch (info.getReferenceKind()) {
is a code smell now that we have a switch expression.

Can be written as:
  Class<?>[] exceptionTypes = switch (info.getReferenceKind()) {
    case ...
    default -> throw WhateverException()
 }
or better list the constants (getfield, getstatic, putfield, putstatic) instead of using "default", so if a new constant is added it will fail to compile.

regards,
Rémi

----- Mail original -----
> De: "Maurizio Cimadamore" <maurizio.cimadamore at oracle.com>
> À: "Paul Sandoz" <paul.sandoz at oracle.com>
> Cc: "panama-dev at openjdk.java.net'" <panama-dev at openjdk.java.net>
> Envoyé: Jeudi 23 Janvier 2020 15:07:00
> Objet: Re: [foreign-memaccess] RFR 8237648: Add support for var handle adaptation

> Here's a new webrev which addresses previous comments, and also adds the
> logic to check whether provided method handle filters are 'throwy' or not.
> 
> http://cr.openjdk.java.net/~mcimadamore/panama/8237648_v2/
> 
> Regarding MethodHandles::throwException, I did few experiments and I
> concluded that it wasn't worth special casing. The MH returned by this
> method is, essentially, MethodHandlesImpl::throwException - which is
> declared to throw Throwable. So, the current logic will detect that, and
> issue an error.
> 
> Refining the logic looking at what actual exception type has been used
> to create the throwy MH is very fragile - that's because of the way
> MethodHandles::throwException, which basically just obtains a MH for
> MethodHandlesImpl::throwException, and _adapts_ that so that the
> parameter type matches that of the exception type selected by the user.
> This means that it will be extremely hard to distinguish between
> system-provided adaptation and user-defined ones.
> 
> If we really really want to close this hole (but I think honestly it
> feels like a corner case), we could rework MethodHandleImpl so that we
> have two named functions - throwException and throwRuntimeException,
> with the right 'throws clauses' so that the logic I wrote will pick that
> up naturally. But, as I said, I don't feel like this is important enough
> to solve - what we have will err on the conservative side, which seems
> like a sound move.
> 
> Cheers
> Maurizio
> 
> On 23/01/2020 10:42, Maurizio Cimadamore wrote:
>> Good point.
>>
>> I'll take it for a spin and report back.
>>
>> Maurizio
>>
>> On 23/01/2020 01:49, Paul Sandoz wrote:
>>> It’s also possible to create a throwing method handle with:
>>>
>>>    public static MethodHandle throwException(Class<?> returnType,
>>> Class<? extends Throwable> exType) {
>>>
>>> But I think that is still amenable to the scanning approach you
>>> suggest (look for the named function throwException and the method
>>> type arg to see if a checked exception).
>>>
>>> It would be nice if any method handle could be structurally
>>> introspected with a public API.  Instead we can traverse the LFs.
>>>
>>> Paul.
>>>
>>>> On Jan 22, 2020, at 3:59 PM, Maurizio Cimadamore
>>>> <maurizio.cimadamore at oracle.com> wrote:
>>>>
>>>> Thinking more about this... it wouldn't be prohibitive to check for
>>>> the validity of the method handles passed to the adapter methods.
>>>>
>>>> After all... the only method handles which can throw checked
>>>> exceptions are the direct ones. And for a direct MH, we can crack it
>>>> and see if it throws.
>>>>
>>>> All the bound method handles cannot introduce _new_ checked
>>>> exceptions - but can of course propagate some of the checked
>>>> exceptions thrown by some of the underlying target MHs.
>>>>
>>>> But, given a bound MH, we can always look a its bound arguments and
>>>> do a recursive scan, to find any MH that is both 'direct' and 'throwy'.
>>>>
>>>> So, it seems there is a path to apply a validation and make sure no
>>>> (checked) exceptions are thrown?
>>>>
>>>> Maurizio
>>>>
>>>> On 22/01/2020 21:16, Maurizio Cimadamore wrote:
>>>>> Thanks for the feedback Paul,
>>>>> what do you think on the topic of exceptions which Remi (correctly)
>>>>> brought up? VarHandle don't throw, but if you combine them with
>>>>> throwy MHs, they can start to throw...
>>>>>
>>>>> Maurizio
>>>>>
>>>>> On 22/01/2020 20:02, Paul Sandoz wrote:
>>>>>> Hi,
>>>>>>
>>>>>> That’s really nice how this slotted into the existing
>>>>>> infrastructure.  Quite a powerful feature.  I wondered when VHs
>>>>>> were being developed whether we could and should do something like
>>>>>> this but a strong enough use-case did not present itself at that
>>>>>> time.
>>>>>>
>>>>>> There is this commented out class in VarHandles called
>>>>>> GuardMethodGenerator that generates the VarHandleGuards source
>>>>>> code.  I try to keep that in sync with the actual code.  I can
>>>>>> update it afterwards.
>>>>>>
>>>>>>
>>>>>> MethodHandles
>>>>>> —
>>>>>>
>>>>>> 4957     public static VarHandle filterValue(VarHandle target,
>>>>>> MethodHandle unbox, MethodHandle box) {
>>>>>>
>>>>>> The terms boxing and unboxing may be misleading in terms of the
>>>>>> corresponding MHs functionality.  I think they are dual mapping
>>>>>> functions from domains S -> T and T -> S, as stated clearly in the
>>>>>> documentation.  Perhaps s/unbox/filterToTarget and
>>>>>> s/filterFromTarget ?
>>>>>>
>>>>>>
>>>>>>
>>>>>> 5104     /**
>>>>>> 5105      * Provides a var handle which adapts the coordinate
>>>>>> values of the target var handle, by re-arranging them
>>>>>> 5106      * so that the new coordinates match the provided
>>>>>> ones.     *
>>>>>>
>>>>>> Rogue “*” at end of line.
>>>>>>
>>>>>>
>>>>>>
>>>>>> 5225     public static VarHandle collectCoordinates(VarHandle
>>>>>> target, int pos, MethodHandle filter) {
>>>>>>
>>>>>> When the filter returns void it drop coordinates.  Is that useful?
>>>>>> Are there cases were some coordinates would be redundant?
>>>>>>
>>>>>>
>>>>>>
>>>>>> 5244     private static List<Class<?>> coordinates(VarHandle
>>>>>> handle) {
>>>>>> 5245         return
>>>>>> handle.accessModeType(VarHandle.AccessMode.GET).parameterList();
>>>>>> 5246     }
>>>>>>
>>>>>>
>>>>>> Usages of this method can be replaced with handle.coordinateTypes()
>>>>>>
>>>>>>
>>>>>>
>>>>>> I suspect to complete the set of combinators a useful addition is
>>>>>> an expanding or spreading combinator e.g. taking a single
>>>>>> coordinate type of say Index(x, y) and substituting with x and y.
>>>>>> However, it's a little tricky with the existing MH functionality
>>>>>> and I believe it requires filtering the composite coordinate into
>>>>>> an array of its component values and then spreading the array
>>>>>> elements to the separate target arguments. I am not confident that
>>>>>> optimizes well, esp. for coordinates of different types.
>>>>>>
>>>>>> Paul.
>>>>>>
>>>>>>
>>>>>>> On Jan 22, 2020, at 6:35 AM, Maurizio Cimadamore
>>>>>>> <maurizio.cimadamore at oracle.com> wrote:
>>>>>>>
>>>>>>> Hi,
>>>>>>> this patch add support for some VarHandle adapters:
>>>>>>>
>>>>>>> http://cr.openjdk.java.net/~mcimadamore/panama/8237648/
>>>>>>>
>>>>>>> More specifically, the following adaptations are supported:
>>>>>>>
>>>>>>> * filterValue
>>>>>>> * insertCoordinates
>>>>>>> * filterCoordinates
>>>>>>> * collectCoordinates
>>>>>>> * permuteCoordinates
>>>>>>>
>>>>>>> This fixes an expressiveness mismatch between VarHandle and
>>>>>>> MethodHandles, as one could adapt MethodHandles, but cannot adapt
>>>>>>> VarHandles (the only way to do that is to map a VarHandle to a
>>>>>>> MethodHandle - using an AccessMode - and then use the MethodHandle).
>>>>>>>
>>>>>>> To achieve this, only minimal surgery to j.l.invoke classes was
>>>>>>> required: the basic idea is to introduce a new class of
>>>>>>> VarHandle, namely IndirectVarHandle, which is merely an aggregate
>>>>>>> of a bunch of method handles, one per supported access modes.
>>>>>>> This means that we have now the ability to take apart an existing
>>>>>>> var handle into its MH components, adapt such components and
>>>>>>> repackage them into a brand new VarHandle.
>>>>>>>
>>>>>>> To save unnecessary bytecode spinning, the component
>>>>>>> MethodHandles of an indirect VarHandle are only created on-demand
>>>>>>> - that is, if you only ever use the GET access mode - the
>>>>>>> machinery will not create the method handles for all the access
>>>>>>> modes which could be in principle be supported by the new var
>>>>>>> handle.
>>>>>>>
>>>>>>> Since adapting a VarHandle might introduce changes in some of the
>>>>>>> coordinate types, I've disabled the VarHandleGuards fastpath for
>>>>>>> indirect handles, as the fast path does an invokeBasic and it
>>>>>>> skips any kind of type adaptation. While invokeBasic will detect
>>>>>>> e.g. a long vs. int mismatch, it does not detect (by design) a
>>>>>>> String vs. Runnable mismatch, so it is preferrable for indirect
>>>>>>> handles to always have an asType() adaptation. If the client is
>>>>>>> doing things "by the books" and using exact types in the
>>>>>>> VarHandle call, the asType() will do nothing and performances
>>>>>>> will not be negatively affected.
>>>>>>>
>>>>>>> I also needed to tweak lambda form generation for var handles a
>>>>>>> bit, to obtain the direct var handle associated with an indirect
>>>>>>> handle before calling the linker method (otherwise we might end
>>>>>>> up the underlying VarHandle routine with the wrong VarHandle type).
>>>>>>>
>>>>>>> I think overall, this implementation strategy proved to be
>>>>>>> flexible and relatively efficient - we can easily add as many
>>>>>>> adapters as we want, since we can build on the already rich
>>>>>>> MethodHandle combinator API.
>>>>>>>
>>>>>>> Cheers
>>>>>>> Maurizio
>>>>>>>