RFR: 8335638: Calling VarHandle.{access-mode} methods reflectively throws wrong exception

[Chen Liang]

On Wed, 3 Jul 2024 19:43:05 GMT, Hannes Greule <hgreule at openjdk.org> wrote:

> Similar to how `MethodHandle#invoke(Exact)` methods are already handled, this change adds special casing for `VarHandle.{access-mode}` methods.
> 
> The exception message is less exact, but I think that's acceptable.

Great work!

src/hotspot/share/prims/methodHandles.cpp line 1372:

> 1370:  */
> 1371: JVM_ENTRY(jobject, VH_UOE(JNIEnv* env, jobject mh, jobjectArray args)) {
> 1372:   THROW_MSG_NULL(vmSymbols::java_lang_UnsupportedOperationException(), "VarHandle access mode method a cannot be invoked reflectively");

Suggestion:

  THROW_MSG_NULL(vmSymbols::java_lang_UnsupportedOperationException(), "VarHandle access mode methods cannot be invoked reflectively");

Looks like a typo to me.

src/hotspot/share/prims/methodHandles.cpp line 1419:

> 1417: static JNINativeMethod VH_methods[] = {
> 1418:   // UnsupportedOperationException throwers
> 1419:   {CC "compareAndExchange",         CC "([" OBJ ")" OBJ,    FN_PTR(VH_UOE)},

I recommend ordering these by the order in `AccessMode`, which is also the declaration order in `VarHandle`; that way, if we add a new access mode, it's easier for us to maintain this list.

src/hotspot/share/prims/methodHandles.cpp line 1457:

> 1455: JVM_ENTRY(void, JVM_RegisterMethodHandleMethods(JNIEnv *env, jclass MHN_class)) {
> 1456:   assert(!MethodHandles::enabled(), "must not be enabled");
> 1457:   assert(vmClasses::MethodHandle_klass() != nullptr, "should be present");

Should we duplicate this assert for `vmClasses::VarHandle_klass()` too?

test/jdk/java/lang/invoke/VarHandles/VarHandleTestReflection.java line 1:

> 1: /*

The copyright header's year needs an update.

test/jdk/java/lang/invoke/VarHandles/VarHandleTestReflection.java line 69:

> 67:         VarHandle v = handle();
> 68: 
> 69:         // Try a reflective invoke using a Method, with an array of 0 arguments

Suggestion:

        // Try a reflective invoke using a Method, with the minimal required argument

test/jdk/java/lang/invoke/VarHandles/VarHandleTestReflection.java line 72:

> 70: 
> 71:         Method vhm = VarHandle.class.getMethod(accessMode.methodName(), Object[].class);
> 72:         Object args = new Object[0];

I recommend naming this `arg`, as this is the single arg to the reflected method. Had you inlined this, you would have called `vhm.invoke(v, (Object) new Object[0]);`

-------------

PR Review: https://git.openjdk.org/jdk/pull/20015#pullrequestreview-2157341254
PR Review Comment: https://git.openjdk.org/jdk/pull/20015#discussion_r1664744641
PR Review Comment: https://git.openjdk.org/jdk/pull/20015#discussion_r1664741601
PR Review Comment: https://git.openjdk.org/jdk/pull/20015#discussion_r1664737631
PR Review Comment: https://git.openjdk.org/jdk/pull/20015#discussion_r1664753008
PR Review Comment: https://git.openjdk.org/jdk/pull/20015#discussion_r1664751627
PR Review Comment: https://git.openjdk.org/jdk/pull/20015#discussion_r1664751688