Re: RFR: 8267421: j.l.constant.DirectMethodHandleDesc.Kind.valueOf(int) implementation doesn't conform to the spec regarding REF_invokeInterface handling [v2]
> Please review this PR which is just syncing the implementation of > DirectMethodHandleDesc.Kind.valueOf(int) with its spec. My reading of the > method's spec is that if the value of the `refKind` parameter is: > MethodHandleInfo.REF_invokeInterface, then > DirectMethodHandleDesc.Kind.valueOf(int, boolean) should be called with a > value of `true` for its second argument, currently it is invoked with `false` > regardless of the value of the `refKind` parameter > > TIA Vicente Romero has updated the pull request incrementally with one additional commit since the last revision: addressing review changes - Changes: - all: https://git.openjdk.java.net/jdk/pull/4416/files - new: https://git.openjdk.java.net/jdk/pull/4416/files/ea47769d..a5e1c8e5 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=4416&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=4416&range=00-01 Stats: 19 lines in 2 files changed: 16 ins; 0 del; 3 mod Patch: https://git.openjdk.java.net/jdk/pull/4416.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4416/head:pull/4416 PR: https://git.openjdk.java.net/jdk/pull/4416
Re: RFR: 8267421: j.l.constant.DirectMethodHandleDesc.Kind.valueOf(int) implementation doesn't conform to the spec regarding REF_invokeInterface handling [v2]
On Wed, 9 Jun 2021 17:22:30 GMT, Mandy Chung wrote: >> Vicente Romero has updated the pull request incrementally with one >> additional commit since the last revision: >> >> addressing review changes > > test/jdk/java/lang/constant/MethodHandleDescTest.java line 362: > >> 360: public void testKind() { >> 361: for (Kind k : Kind.values()) { >> 362: assertEquals(Kind.valueOf(k.refKind), >> Kind.valueOf(k.refKind, k.refKind == MethodHandleInfo.REF_invokeInterface)); > > Looks like the test does not verify the cases specified by `valueOf(int > refKind, boolean isInterface)`. > i.e. For most values of refKind, there is an exact match regardless of the > value of isInterface except `REF_invokeStatic` and `REF_invokeSpecial`. > > Do you mind adding those cases? @mlchung I have updated the PR with another commit, thanks for your comments - PR: https://git.openjdk.java.net/jdk/pull/4416
Re: RFR: 8267421: j.l.constant.DirectMethodHandleDesc.Kind.valueOf(int) implementation doesn't conform to the spec regarding REF_invokeInterface handling [v2]
On Thu, 10 Jun 2021 23:26:21 GMT, Vicente Romero wrote: >> Please review this PR which is just syncing the implementation of >> DirectMethodHandleDesc.Kind.valueOf(int) with its spec. My reading of the >> method's spec is that if the value of the `refKind` parameter is: >> MethodHandleInfo.REF_invokeInterface, then >> DirectMethodHandleDesc.Kind.valueOf(int, boolean) should be called with a >> value of `true` for its second argument, currently it is invoked with >> `false` regardless of the value of the `refKind` parameter >> >> TIA > > Vicente Romero has updated the pull request incrementally with one additional > commit since the last revision: > > addressing review changes src/java.base/share/classes/java/lang/constant/DirectMethodHandleDesc.java line 143: > 141: } > 142: if (kind.refKind == refKind && > 143: (refKind != REF_invokeStatic || refKind != > REF_invokeSpecial || kind.isInterface == isInterface)){ It reads to me that the static initializer tries to set up the table such that `valueOf(refKind, isInterface)` should find the proper kind to return except this: // There is not a specific Kind for interfaces if (kind == VIRTUAL) kind = INTERFACE_VIRTUAL; This changes the entry for `REF_invokeVirtual` to kind `INTERFACE_VIRTUAL`. Do you know why?If this entry is set to VIRTUAL, then each refKind has two entries in the table corresponding to the correct result for `valueOf`. - PR: https://git.openjdk.java.net/jdk/pull/4416
Re: RFR: 8267421: j.l.constant.DirectMethodHandleDesc.Kind.valueOf(int) implementation doesn't conform to the spec regarding REF_invokeInterface handling [v2]
On Thu, 10 Jun 2021 23:23:23 GMT, Vicente Romero wrote: >> test/jdk/java/lang/constant/MethodHandleDescTest.java line 362: >> >>> 360: public void testKind() { >>> 361: for (Kind k : Kind.values()) { >>> 362: assertEquals(Kind.valueOf(k.refKind), >>> Kind.valueOf(k.refKind, k.refKind == MethodHandleInfo.REF_invokeInterface)); >> >> Looks like the test does not verify the cases specified by `valueOf(int >> refKind, boolean isInterface)`. >> i.e. For most values of refKind, there is an exact match regardless of the >> value of isInterface except `REF_invokeStatic` and `REF_invokeSpecial`. >> >> Do you mind adding those cases? > > @mlchung I have updated the PR with another commit, thanks for your comments It may be better to create a new JBS issue to fix this bug as it may take more time to investigate. - PR: https://git.openjdk.java.net/jdk/pull/4416
Re: RFR: 8267421: j.l.constant.DirectMethodHandleDesc.Kind.valueOf(int) implementation doesn't conform to the spec regarding REF_invokeInterface handling [v2]
On Thu, 10 Jun 2021 23:26:21 GMT, Vicente Romero wrote: >> Please review this PR which is just syncing the implementation of >> DirectMethodHandleDesc.Kind.valueOf(int) with its spec. My reading of the >> method's spec is that if the value of the `refKind` parameter is: >> MethodHandleInfo.REF_invokeInterface, then >> DirectMethodHandleDesc.Kind.valueOf(int, boolean) should be called with a >> value of `true` for its second argument, currently it is invoked with >> `false` regardless of the value of the `refKind` parameter >> >> TIA > > Vicente Romero has updated the pull request incrementally with one additional > commit since the last revision: > > addressing review changes I will be withdrawing this one to open a new [PR-29](https://github.com/openjdk/jdk17/pull/29) intended to jdk17, please let's follow the discussion there - PR: https://git.openjdk.java.net/jdk/pull/4416
Re: [jdk17] RFR: 8267421: j.l.constant.DirectMethodHandleDesc.Kind.valueOf(int) implementation doesn't conform to the spec regarding REF_invokeInterface handling [v2]
> This PR is a copy of [PR-4416](https://github.com/openjdk/jdk/pull/4416) > which was intended to openjdk/jdk. > > Please review this PR which is syncing the implementation of > `DirectMethodHandleDesc.Kind.valueOf(int)` and > `DirectMethodHandleDesc.Kind.valueOf(int, boolean)` with its spec. My reading > of the method's spec is that if the value of the refKind parameter is: > MethodHandleInfo.REF_invokeInterface, then > DirectMethodHandleDesc.Kind.valueOf(int, boolean) should be called with a > value of true for its second argument, currently it is invoked with false > regardless of the value of the refKind parameter > > TIA Vicente Romero has updated the pull request incrementally with one additional commit since the last revision: updating after review comments - Changes: - all: https://git.openjdk.java.net/jdk17/pull/29/files - new: https://git.openjdk.java.net/jdk17/pull/29/files/b6b3f87c..8ed4cdb3 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk17&pr=29&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk17&pr=29&range=00-01 Stats: 12 lines in 1 file changed: 0 ins; 9 del; 3 mod Patch: https://git.openjdk.java.net/jdk17/pull/29.diff Fetch: git fetch https://git.openjdk.java.net/jdk17 pull/29/head:pull/29 PR: https://git.openjdk.java.net/jdk17/pull/29
Re: [jdk17] RFR: 8267421: j.l.constant.DirectMethodHandleDesc.Kind.valueOf(int) implementation doesn't conform to the spec regarding REF_invokeInterface handling [v2]
On Fri, 11 Jun 2021 18:17:10 GMT, Vicente Romero wrote: >> This PR is a copy of [PR-4416](https://github.com/openjdk/jdk/pull/4416) >> which was intended to openjdk/jdk. >> >> Please review this PR which is syncing the implementation of >> `DirectMethodHandleDesc.Kind.valueOf(int)` and >> `DirectMethodHandleDesc.Kind.valueOf(int, boolean)` with its spec. My >> reading of the method's spec is that if the value of the refKind parameter >> is: MethodHandleInfo.REF_invokeInterface, then >> DirectMethodHandleDesc.Kind.valueOf(int, boolean) should be called with a >> value of true for its second argument, currently it is invoked with false >> regardless of the value of the refKind parameter >> >> TIA > > Vicente Romero has updated the pull request incrementally with one additional > commit since the last revision: > > updating after review comments @mlchung I have uploaded a new commit, thanks for your comments - PR: https://git.openjdk.java.net/jdk17/pull/29
Re: [jdk17] RFR: 8267421: j.l.constant.DirectMethodHandleDesc.Kind.valueOf(int) implementation doesn't conform to the spec regarding REF_invokeInterface handling [v2]
On Fri, 11 Jun 2021 18:17:10 GMT, Vicente Romero wrote: >> This PR is a copy of [PR-4416](https://github.com/openjdk/jdk/pull/4416) >> which was intended to openjdk/jdk. >> >> Please review this PR which is syncing the implementation of >> `DirectMethodHandleDesc.Kind.valueOf(int)` and >> `DirectMethodHandleDesc.Kind.valueOf(int, boolean)` with its spec. My >> reading of the method's spec is that if the value of the refKind parameter >> is: MethodHandleInfo.REF_invokeInterface, then >> DirectMethodHandleDesc.Kind.valueOf(int, boolean) should be called with a >> value of true for its second argument, currently it is invoked with false >> regardless of the value of the refKind parameter >> >> TIA > > Vicente Romero has updated the pull request incrementally with one additional > commit since the last revision: > > updating after review comments Marked as reviewed by mchung (Reviewer). src/java.base/share/classes/java/lang/constant/DirectMethodHandleDesc.java line 138: > 136: int i = tableIndex(refKind, isInterface); > 137: if (i >= 2 && i < TABLE.length) { > 138: return TABLE[i]; This fix looks good. The resulting `TABLE` has two entries per `refKind` for `isInterface` = `true` and `false`. test/jdk/java/lang/constant/MethodHandleDescTest.java line 379: > 377: for (int refKind : isInterfaceIgnored) { > 378: assertEquals(Kind.valueOf(refKind, false), > Kind.valueOf(refKind, true)); > 379: } Can you also add the test cases to verify `Kind::valueOf` with `REF_invokeStatic` and `REF_invokeSpecial`? - PR: https://git.openjdk.java.net/jdk17/pull/29
Re: [jdk17] RFR: 8267421: j.l.constant.DirectMethodHandleDesc.Kind.valueOf(int) implementation doesn't conform to the spec regarding REF_invokeInterface handling [v2]
On Tue, 22 Jun 2021 00:45:36 GMT, Mandy Chung wrote: >> Vicente Romero has updated the pull request incrementally with one >> additional commit since the last revision: >> >> updating after review comments > > test/jdk/java/lang/constant/MethodHandleDescTest.java line 379: > >> 377: for (int refKind : isInterfaceIgnored) { >> 378: assertEquals(Kind.valueOf(refKind, false), >> Kind.valueOf(refKind, true)); >> 379: } > > Can you also add the test cases to verify `Kind::valueOf` with > `REF_invokeStatic` and `REF_invokeSpecial`? please see them below, thanks - PR: https://git.openjdk.java.net/jdk17/pull/29