RFR: 8238494: Odd behavior when using VarHandle::toMethodHandle on a memory access handle

[Maurizio Cimadamore]

I've decided to push this as is for now - I filed this followup issue:

https://bugs.openjdk.java.net/browse/JDK-8238549

Thanks
Maurizio

On 05/02/2020 00:08, Paul Sandoz wrote:
> On Tue, 4 Feb 2020 23:49:42 GMT, Maurizio Cimadamore <mcimadamore at openjdk.org> wrote:
>
>>> Hi,
>>> this change fixes a bug introduced by the recently pushed VarHandle adaptation support. More specifically, when calling `VarHandle::toMethodHandle` on a memory access handle, some spurious index out of bound exception is generated. This is caused by the fact that when `IndirectVarHandle::toMethodHandle` generates the required method handle, it binds it against `this`, rather than against the leaf direct var handle. Which means we're calling a method handle which expects a certain kind of a VarHandle (a memory access handle) with a different VarHandle which has no associated offset information. In fact, the memory access var handle helper classes will try to read the `offset` field from a class that doesn't have it - resulting in a spurious read.
>>>
>>> I've updated the adapter test to do the final get in the various tests through a method handle obtained by the corresponding adapted var handle.
>>>
>>> Cheers
>>> Maurizio
>>> Is it possible to add a cast or assert somewhere checking that the bound VH is not an instance of IndirectVarhandle?
>> Not sure I understand what you mean. If you mean checking that the argument of a bindTo in VarHandle::toMethodHandle is a direct handle, I'm not sure how we can enforce that, as that's just a plain MH::bindTo call.
>>
>> What could be perhaps more useful, I suppose, would be to tweak the VarHandle generation strategy so that the various methods just take a `VarHandle` and a cast is inserted in the helpers to make sure that we can't pass the 'wrong' handle of type A and treat it like a B. But I think we should do this for _all_ var handle code, even the array/bytebuffer ones - since they also use specific subclasses.
> Yes, that is what i was thinking.  Prior to adapting VHs it was more obviously correct by construction.
> e.g.:
>          @ForceInline
>          static int get(VarHandle _handle, Object holder) {
>              FieldInstanceReadOnly handle = (FieldInstanceReadOnly) _handle;
>              return UNSAFE.getInt(Objects.requireNonNull(handle.receiverType.cast(holder)),
>                                   handle.fieldOffset);
>          }
>
> -------------
>
> PR: https://git.openjdk.java.net/panama-foreign/pull/4