Re: RFR: 8335638: Calling VarHandle.{access-mode} methods reflectively throws wrong exception [v2]
On Thu, 4 Jul 2024 06:22:31 GMT, Hannes Greule 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.
>
> Hannes Greule has updated the pull request incrementally with one additional
> commit since the last revision:
>
> address comments
Thanks for this fix and the discussions. We should be good to go.
-
PR Comment: https://git.openjdk.org/jdk/pull/20015#issuecomment-2263247935
Re: RFR: 8335638: Calling VarHandle.{access-mode} methods reflectively throws wrong exception [v2]
On Fri, 5 Jul 2024 14:23:50 GMT, Jorn Vernee wrote: >> Hannes Greule has updated the pull request incrementally with one additional >> commit since the last revision: >> >> address comments > > I think this needs a CSR, to document the change in behavior. (See e.g. > https://bugs.openjdk.org/browse/JDK-8335554 which is a very similar case) > @JornVernee if you want you can also review > https://bugs.openjdk.org/browse/JDK-8337301 @liach already asked me to take a look. The note looks good. There's no way for me to mark it as reviewed it seems - PR Comment: https://git.openjdk.org/jdk/pull/20015#issuecomment-2263238945
Re: RFR: 8335638: Calling VarHandle.{access-mode} methods reflectively throws wrong exception [v2]
On Thu, 4 Jul 2024 06:22:31 GMT, Hannes Greule 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.
>
> Hannes Greule has updated the pull request incrementally with one additional
> commit since the last revision:
>
> address comments
@SirYwell
Your change (at version e329ceb206d198e35186843391f4a8a25732e64e) is now ready
to be sponsored by a Committer.
-
PR Comment: https://git.openjdk.org/jdk/pull/20015#issuecomment-2262992409
Re: RFR: 8335638: Calling VarHandle.{access-mode} methods reflectively throws wrong exception [v2]
On Thu, 4 Jul 2024 06:22:31 GMT, Hannes Greule 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.
>
> Hannes Greule has updated the pull request incrementally with one additional
> commit since the last revision:
>
> address comments
@JornVernee if you want you can also review
-
PR Comment: https://git.openjdk.org/jdk/pull/20015#issuecomment-2262988654
Re: RFR: 8335638: Calling VarHandle.{access-mode} methods reflectively throws wrong exception [v2]
On Thu, 4 Jul 2024 06:22:31 GMT, Hannes Greule 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.
>
> Hannes Greule has updated the pull request incrementally with one additional
> commit since the last revision:
>
> address comments
I think this needs a CSR, to document the change in behavior. (See e.g.
https://bugs.openjdk.org/browse/JDK-8335554 which is a very similar case)
-
PR Comment: https://git.openjdk.org/jdk/pull/20015#issuecomment-2210971780
Re: RFR: 8335638: Calling VarHandle.{access-mode} methods reflectively throws wrong exception [v2]
On Thu, 4 Jul 2024 06:22:31 GMT, Hannes Greule 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.
>
> Hannes Greule has updated the pull request incrementally with one additional
> commit since the last revision:
>
> address comments
Marked as reviewed by liach (Committer).
-
PR Review: https://git.openjdk.org/jdk/pull/20015#pullrequestreview-2158742384
Re: RFR: 8335638: Calling VarHandle.{access-mode} methods reflectively throws wrong exception [v2]
> 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.
Hannes Greule has updated the pull request incrementally with one additional
commit since the last revision:
address comments
-
Changes:
- all: https://git.openjdk.org/jdk/pull/20015/files
- new: https://git.openjdk.org/jdk/pull/20015/files/fe43b749..e329ceb2
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk&pr=20015&range=01
- incr: https://webrevs.openjdk.org/?repo=jdk&pr=20015&range=00-01
Stats: 43 lines in 2 files changed: 17 ins; 16 del; 10 mod
Patch: https://git.openjdk.org/jdk/pull/20015.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/20015/head:pull/20015
PR: https://git.openjdk.org/jdk/pull/20015
Re: RFR: 8335638: Calling VarHandle.{access-mode} methods reflectively throws wrong exception
On Wed, 3 Jul 2024 19:43:05 GMT, Hannes Greule 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.
src/hotspot/share/prims/methodHandles.cpp line 1371:
> 1369: * invoked directly.
> 1370: */
> 1371: JVM_ENTRY(jobject, VH_UOE(JNIEnv* env, jobject mh, jobjectArray args)) {
Suggestion:
JVM_ENTRY(jobject, VH_UOE(JNIEnv* env, jobject vh, jobjectArray args)) {
-
PR Review Comment: https://git.openjdk.org/jdk/pull/20015#discussion_r1664836522
Re: RFR: 8335638: Calling VarHandle.{access-mode} methods reflectively throws wrong exception
On Wed, 3 Jul 2024 19:43:05 GMT, Hannes Greule 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
RFR: 8335638: Calling VarHandle.{access-mode} methods reflectively throws wrong exception
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.
-
Commit messages:
- add test (and find missing method)
- make reflective calls to signature polymorphic methods in VarHandle throw UOE
Changes: https://git.openjdk.org/jdk/pull/20015/files
Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=20015&range=00
Issue: https://bugs.openjdk.org/browse/JDK-8335638
Stats: 75 lines in 2 files changed: 71 ins; 0 del; 4 mod
Patch: https://git.openjdk.org/jdk/pull/20015.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/20015/head:pull/20015
PR: https://git.openjdk.org/jdk/pull/20015
