RFR: 8238549: Add explicit cast to correct implementation type in VarHandle implementation methods

[Paul Sandoz]

Nice work!

I felt guilty hinting at this work so placed my name on the issue with the intent of doing the tedious work, but you beat me to it :-)

AddressVarHandleGenerator
--
265             MethodVisitor mv = cw.visitMethod(ACC_STATIC, methName, methType.toMethodDescriptorString(), null, null);
266             mv.visitAnnotation(Type.getDescriptor(ForceInline.class), true);
267             mv.visitCode();
268 
269             mv.visitVarInsn(ALOAD, 0); // handle impl
270             mv.visitTypeInsn(CHECKCAST, Type.getInternalName(BASE_CLASS));
271             mv.visitVarInsn(ALOAD, 1); // receiver
272 
273             // offset calculation
274             int slot = 2;
275             mv.visitVarInsn(ALOAD, 0); // load recv
276             mv.visitTypeInsn(CHECKCAST, Type.getInternalName(BASE_CLASS));
277             mv.visitFieldInsn(GETFIELD, Type.getInternalName(BASE_CLASS), "offset", "J");
...
291                 mv.visitVarInsn(ALOAD, 0); // load recv
292                 mv.visitTypeInsn(CHECKCAST, implClassName);
293                 mv.visitFieldInsn(GETFIELD, implClassName, "dim" + i, "J");
294                 mv.visitVarInsn(LLOAD, slot);

I cannot recall the rules on whether it is necessary to perform check casts multiple times here given there are no stores, but I guess HS prefers that?

Also we seem to be checking against the impl class within the loop.  If we need to do the checks multiple times can we consistently check against the concrete class?

Paul.


> On Feb 5, 2020, at 5:25 PM, Maurizio Cimadamore <mcimadamore at openjdk.java.net> wrote:
> 
> Hi,
> as promised, I took a look into adding a bit more structure under our var handle implementations - more specifically, I've tweaked the signatures of the various implementation methods to take a plain VarHandle instead of a var handle implementation class.
> 
> This was tedious, but ultimately easy to do. Since, to do this in a complete way I also had to touch regular var handles (e.g. non-memory access ones), it seemed like a good idea to take an existing var handle test (VarHandleTestMethodTypeXYZ) and tweak it so that it tests all combinations of (adpated=yes/no, guards=yes/no). This revealed several issues in the adapter logic:
> 
> * first, varType() and coordinates() triggered a resolution of some of the underlying MH attached to the indirect handle - since this can fail with UOE, these methods could also fail spuriously with UOE which is wrong
> 
> * second, accessModeTypeUncached, in addition to the aforementioned problem, also had an issue in that it didn't make correct use of the direct handle target
> 
> * third, I realized that I did not update the lambda forms for var handle invokers to also call VarHandle::asDirect - this was causing issues when calling an adapted handle through the invoker
> 
> With these changes, all tests pass despite the extra layers of adaptation. So, in the end it turned out to be a fruitful bug hunt (thanks for the hint Paul!).
> 
> Cheers
> Maurizio
> 
> -------------
> 
> Commits:
> - 066559b2: Strengthen var handle checks
> 
> Changes: https://git.openjdk.java.net/panama-foreign/pull/6/files
> Webrev: https://webrevs.openjdk.java.net/panama-foreign/6/webrev.00
>  Issue: https://bugs.openjdk.java.net/browse/JDK-8238549
>  Stats: 440 lines in 18 files changed: 227 ins; 0 del; 213 mod
>  Patch: https://git.openjdk.java.net/panama-foreign/pull/6.diff
>  Fetch: git fetch https://git.openjdk.java.net/panama-foreign pull/6/head:pull/6
> 
> PR: https://git.openjdk.java.net/panama-foreign/pull/6