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
