Re: RFR: 8303684: Lift upcall sharing mechanism to AbstractLinker (mainline)
On Mon, 6 Mar 2023 18:40:47 GMT, Jorn Vernee wrote:
> Port of: https://github.com/openjdk/panama-foreign/pull/791 which lifts the
> sharing mechanism for upcall stubs to AbstractLinker.
>
> This also speeds up upcall stub linking:
src/java.base/share/classes/jdk/internal/foreign/abi/aarch64/windows/WindowsAArch64Linker.java
line 61:
> 59: @Override
> 60: protected UpcallStubFactory arrangeUpcall(MethodType targetType,
> FunctionDescriptor function) {
> 61: return CallArranger.WINDOWS.arrangeUpcall(targetType, function);
Suggestion:
return CallArranger.WINDOWS.arrangeUpcall(targetType, function);
-
PR: https://git.openjdk.org/jdk/pull/12883
Re: RFR: 8303920: Avoid calling out to python in DataDescriptorSignatureMissing test [v6]
> Please review this PR which brings the DataDescriptorSignatureMissing test > back to life. > > This test currently calls out to Python to create a test vector ZIP with a > Data Descriptor without the recommended but optional signature. The Python > dependency has turned out to be very brittle, so the test is currently marked > with `@ignore` > > The PR replaces Python callouts with directly creating the test vector ZIP in > the test itself. We can then remove the `@ignore`tag and run this useful test > automatically. Eirik Bjorsnos has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains eight additional commits since the last revision: - Merge branch 'master' into signature-less-data-descriptor - Add assertNotNulls to catch unexpectedly missing entries - Revert change to Google copyright line, add an Oracle copyright line instead. - Use "Signatureless" consistently - Remove reference to python in the @summary of DataDescriptorSignatureMissing - Update copyright years - Add method comments - Instead of calling out to python, create a ZIP file and remove the data descriptor signature. - Changes: - all: https://git.openjdk.org/jdk/pull/12959/files - new: https://git.openjdk.org/jdk/pull/12959/files/85eab4a3..0fb30670 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=12959&range=05 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=12959&range=04-05 Stats: 8132 lines in 183 files changed: 1014 ins; 1302 del; 5816 mod Patch: https://git.openjdk.org/jdk/pull/12959.diff Fetch: git fetch https://git.openjdk.org/jdk pull/12959/head:pull/12959 PR: https://git.openjdk.org/jdk/pull/12959
Re: RFR: 8206890: jlink --endian XXX generates unusable image if endian-ness does not match architecture [v2]
> Can I please get a review for this change which proposes to fix the issue > reported in https://bugs.openjdk.org/browse/JDK-8206890? > > The `jlink` command allows a `--endian` option to specify the byte order in > the generated image. Before this change, when such a image was being > launched, the code would assume the byte order in the image to be the native > order of the host where the image is being launched. That would result in > failure to launch java, as noted in the linked issue. > > The commit in this PR, changes relevant places to not assume native order and > instead determine the byte order by reading the magic bytes in the image > file's header content. > > A new jtreg test has been added which reproduces the issue and verifies the > fix. Jaikiran Pai has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains 10 additional commits since the last revision: - Infer endianness for the generated image from the target platform - Revert "8206890: jlink --endian XXX generates unusable image if endian-ness does not match architecture" This reverts commit 4020a37849d04d0638941b36c8953884b933461e. - Revert "take into account SecurityManager checks" This reverts commit 1694f3a02cf470ac3aaef5d8cdeb0a40c0c66b12. - Revert "Alan's input - remove "final" and match the current code style" This reverts commit 7a754a1bcd20f02da33a9f5d3d170ead0675d33c. - Revert "fix jcheck issue - convert tab to space" This reverts commit 2422399094862718ed8e0e9d3de77d9396283fa4. - merge latest master branch - fix jcheck issue - convert tab to space - Alan's input - remove "final" and match the current code style - take into account SecurityManager checks - 8206890: jlink --endian XXX generates unusable image if endian-ness does not match architecture - Changes: - all: https://git.openjdk.org/jdk/pull/11943/files - new: https://git.openjdk.org/jdk/pull/11943/files/24223990..d0f0e0ee Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=11943&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=11943&range=00-01 Stats: 384736 lines in 5099 files changed: 212681 ins; 120746 del; 51309 mod Patch: https://git.openjdk.org/jdk/pull/11943.diff Fetch: git fetch https://git.openjdk.org/jdk pull/11943/head:pull/11943 PR: https://git.openjdk.org/jdk/pull/11943
Re: RFR: 8206890: jlink --endian XXX generates unusable image if endian-ness does not match architecture
On Fri, 20 Jan 2023 14:23:13 GMT, Jaikiran Pai wrote: > Right now the ModuleTarget class file attribute is not enough but a short > term fix might be for jlink to have a simple mapping of known ModuleTarget > values to endianness. Overall, it's probably low priority as the most likely > users of this will be environments building for embedded platforms. I've now updated this PR to revert the previous commits and instead follow this suggestion to infer the endianness of the target platform while generating the image. The updated PR now continues to allow users to provide `--endian` option while generating an image through `jlink`. If any value is provided for that option, the image generation will continue to honour that value. However, if no value is provided for that option, then during image generation, we will determine the endianness of the target platform, by using the (OpenJDK specific) `ModuleTarget` attribute of target `java.base` module's `module-info` class. This determination is best-effort and if the value cannot be determined, then we default to the current platform's endianness (like we have been doing so far). This commit hardcodes the mapping between a `ModuleTarget`'s architecture and endianness and this mapping is borrowed from what we already do in the OpenJDK build file https://github.com/openjdk/jdk/blob/master/make/autoconf/platform.m4#L26 No new tests have been added in this change, because I couldn't think of a way to test the cross platform `jlink` execution. Existing tests in `test/jdk/tools/jlink` continue to pass with this change. I've triggered our CI runs to run the tiered tests to make sure nothing regresses. The commit in this PR doesn't deprecate the --endian option and I think we can do that as a separate issue. - PR: https://git.openjdk.org/jdk/pull/11943
Re: RFR: 8304006: jlink should create the jimage file in the native endian for the target platform [v2]
On Sat, 11 Mar 2023 10:40:19 GMT, Jaikiran Pai wrote: >> Can I please get a review for this change which proposes to fix the issue >> reported in https://bugs.openjdk.org/browse/JDK-8206890? >> >> The `jlink` command allows a `--endian` option to specify the byte order in >> the generated image. Before this change, when such a image was being >> launched, the code would assume the byte order in the image to be the native >> order of the host where the image is being launched. That would result in >> failure to launch java, as noted in the linked issue. >> >> The commit in this PR, changes relevant places to not assume native order >> and instead determine the byte order by reading the magic bytes in the image >> file's header content. >> >> A new jtreg test has been added which reproduces the issue and verifies the >> fix. > > Jaikiran Pai has updated the pull request with a new target base due to a > merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains 10 additional > commits since the last revision: > > - Infer endianness for the generated image from the target platform > - Revert "8206890: jlink --endian XXX generates unusable image if > endian-ness does not match architecture" > >This reverts commit 4020a37849d04d0638941b36c8953884b933461e. > - Revert "take into account SecurityManager checks" > >This reverts commit 1694f3a02cf470ac3aaef5d8cdeb0a40c0c66b12. > - Revert "Alan's input - remove "final" and match the current code style" > >This reverts commit 7a754a1bcd20f02da33a9f5d3d170ead0675d33c. > - Revert "fix jcheck issue - convert tab to space" > >This reverts commit 2422399094862718ed8e0e9d3de77d9396283fa4. > - merge latest master branch > - fix jcheck issue - convert tab to space > - Alan's input - remove "final" and match the current code style > - take into account SecurityManager checks > - 8206890: jlink --endian XXX generates unusable image if endian-ness does > not match architecture I've updated the PR title to now point to the new https://bugs.openjdk.org/browse/JDK-8304006 - PR: https://git.openjdk.org/jdk/pull/11943
Re: RFR: 8304006: jlink should create the jimage file in the native endian for the target platform [v3]
> Can I please get a review for this change which proposes to fix the issue > reported in https://bugs.openjdk.org/browse/JDK-8206890? > > The `jlink` command allows a `--endian` option to specify the byte order in > the generated image. Before this change, when such a image was being > launched, the code would assume the byte order in the image to be the native > order of the host where the image is being launched. That would result in > failure to launch java, as noted in the linked issue. > > The commit in this PR, changes relevant places to not assume native order and > instead determine the byte order by reading the magic bytes in the image > file's header content. > > A new jtreg test has been added which reproduces the issue and verifies the > fix. Jaikiran Pai has updated the pull request incrementally with one additional commit since the last revision: undo unintentional copyright year change - Changes: - all: https://git.openjdk.org/jdk/pull/11943/files - new: https://git.openjdk.org/jdk/pull/11943/files/d0f0e0ee..cdfd993e Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=11943&range=02 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=11943&range=01-02 Stats: 2 lines in 2 files changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/11943.diff Fetch: git fetch https://git.openjdk.org/jdk pull/11943/head:pull/11943 PR: https://git.openjdk.org/jdk/pull/11943
Re: RFR: 8304006: jlink should create the jimage file in the native endian for the target platform [v4]
> Can I please get a review for this change which proposes to fix the issue > reported in https://bugs.openjdk.org/browse/JDK-8206890? > > The `jlink` command allows a `--endian` option to specify the byte order in > the generated image. Before this change, when such a image was being > launched, the code would assume the byte order in the image to be the native > order of the host where the image is being launched. That would result in > failure to launch java, as noted in the linked issue. > > The commit in this PR, changes relevant places to not assume native order and > instead determine the byte order by reading the magic bytes in the image > file's header content. > > A new jtreg test has been added which reproduces the issue and verifies the > fix. Jaikiran Pai has updated the pull request incrementally with one additional commit since the last revision: missed ppc64 from the arch mapping - Changes: - all: https://git.openjdk.org/jdk/pull/11943/files - new: https://git.openjdk.org/jdk/pull/11943/files/cdfd993e..e68c2105 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=11943&range=03 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=11943&range=02-03 Stats: 3 lines in 1 file changed: 1 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/11943.diff Fetch: git fetch https://git.openjdk.org/jdk pull/11943/head:pull/11943 PR: https://git.openjdk.org/jdk/pull/11943
Re: RFR: 8303684: Lift upcall sharing mechanism to AbstractLinker (mainline) [v2]
> Port of: https://github.com/openjdk/panama-foreign/pull/791 which lifts the > sharing mechanism for upcall stubs to AbstractLinker. > > This also speeds up upcall stub linking: Jorn Vernee has updated the pull request incrementally with one additional commit since the last revision: Remove spurious space Co-authored-by: Andrey Turbanov - Changes: - all: https://git.openjdk.org/jdk/pull/12883/files - new: https://git.openjdk.org/jdk/pull/12883/files/0355ab19..b359e2a0 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=12883&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=12883&range=00-01 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/12883.diff Fetch: git fetch https://git.openjdk.org/jdk pull/12883/head:pull/12883 PR: https://git.openjdk.org/jdk/pull/12883
Re: RFR: 8294961: Convert java.base/java.lang.reflect.ProxyGenerator to use the Classfile API to generate proxy classes [v6]
On Fri, 10 Mar 2023 08:48:14 GMT, Adam Sotona wrote:
>> java.base java.lang.reflect.ProxyGenerator uses ASM to generate proxy
>> classes and this patch converts it to use Classfile API.
>>
>> Please review.
>>
>> Thank you,
>> Adam
>
> Adam Sotona has updated the pull request with a new target base due to a
> merge or a rebase. The pull request now contains 196 commits:
>
> - Merge branch 'master' into JDK-8294961-proxy
> - Merge branch 'master' into JDK-8294961-proxy
> - Merge branch 'JDK-8294982' into JDK-8294961
> - removed obsolete javadoc from implementation classes
> - minor fix in CodeBuilder and added test cases to LDCTest
> - EntryMap::nextPowerOfTwo delegates to Long:numberOfLeadingZeros
> - fixed CodeBuilder:constantInstruction for -0.0d and -0.0f values and added
> test
> - Merge branch 'master' into JDK-8294982
> - fixed new lines at end of file
> - package jdk.internal.classfile.jdktypes moved to
> jdk.internal.classfile.java.lang.constant
> - ... and 186 more: https://git.openjdk.org/jdk/compare/e26cc526...951b1bc7
src/java.base/share/classes/java/lang/reflect/ProxyGenerator.java line 68:
> 66: CD_InvocationHandler =
> ClassDesc.ofInternalName("java/lang/reflect/InvocationHandler"),
> 67: CD_Method =
> ClassDesc.ofInternalName("java/lang/reflect/Method"),
> 68: CD_MethodHandles$Lookup =
> ClassDesc.ofInternalName("java/lang/invoke/MethodHandles$Lookup"),
Can reuse CD_MethodHandles_Lookup from ConstantDescs:
Suggestion:
src/java.base/share/classes/java/lang/reflect/ProxyGenerator.java line 85:
> 83: MTD_ClassLoader = MethodTypeDesc.of(CD_ClassLoader),
> 84: MTD_MethodHandles$Lookup =
> MethodTypeDesc.of(CD_MethodHandles$Lookup),
> 85: MTD_MethodHandles$Lookup_MethodHandles$Lookup =
> MethodTypeDesc.of(CD_MethodHandles$Lookup, CD_MethodHandles$Lookup),
Suggestion:
MTD_MethodHandles$Lookup =
MethodTypeDesc.of(CD_MethodHandles_Lookup),
MTD_MethodHandles$Lookup_MethodHandles$Lookup =
MethodTypeDesc.of(CD_MethodHandles_Lookup, CD_MethodHandles_Lookup),
src/java.base/share/classes/java/lang/reflect/ProxyGenerator.java line 426:
> 424: */
> 425: localCache.computeIfAbsent(classDesc, cd -> {
> 426: try {
This probably can be a factory method in ClassHierarchyResolver too, by
examining the reflection API via a ClassLoader/Lookup. I'm inclined to submit a
patch for improvement.
Also, why does ProxyGenerator come with a custom ClassHierarchyResolver while
InnerClassLambdaMetafactory in 8294960 #12945 doesn't?
src/java.base/share/classes/java/lang/reflect/ProxyGenerator.java line 602:
> 600: .block(blockBuilder -> blockBuilder
> 601: .aload(cob.parameterSlot(0))
> 602:
> .invokevirtual(CD_MethodHandles$Lookup, "lookupClass", MTD_Class)
Suggestion:
.invokevirtual(CD_MethodHandles_Lookup,
"lookupClass", MTD_Class)
src/java.base/share/classes/java/lang/reflect/ProxyGenerator.java line 606:
> 604: .if_acmpne(blockBuilder.breakLabel())
> 605: .aload(cob.parameterSlot(0))
> 606:
> .invokevirtual(CD_MethodHandles$Lookup, "hasFullPrivilegeAccess", MTD_boolean)
Suggestion:
.invokevirtual(CD_MethodHandles_Lookup,
"hasFullPrivilegeAccess", MTD_boolean)
src/java.base/share/classes/java/lang/reflect/ProxyGenerator.java line 613:
> 611: .dup()
> 612: .aload(cob.parameterSlot(0))
> 613: .invokevirtual(CD_MethodHandles$Lookup,
> "toString", MTD_String)
Suggestion:
.invokevirtual(CD_MethodHandles_Lookup, "toString",
MTD_String)
-
PR: https://git.openjdk.org/jdk/pull/10991
Re: RFR: 8294962: Convert java.base/jdk.internal.module package to use the Classfile API to modify and write module-info.class [v4]
On Fri, 10 Mar 2023 19:30:45 GMT, Mandy Chung wrote:
>> The ModulePackages attribute is optional and an optimization to avoid
>> scanning the module contents to get the full set of packages. Tooling that
>> creates packaged modules (jar and jmod for now) will want the ModulePackages
>> attribute emitted always. So maybe the Classfile.buildModule methods should
>> be looked at again, at least I think the 2-arg and 3-arg methods should emit
>> the ModulePackages unconditionally, the 1-arg buildModule maybe not. This
>> isn't an issue for ModuleInfoWriter of course as it is only used by tests.
>
> `buildModules` is expected to be called with additional packages but instead
> it's called with all packages including all exported and open packages.
>
>
> /**
> * Build a module descriptor into a byte array.
> * @param moduleAttribute the {@code Module} attribute
> * @param packages additional module packages
> * @param handler a handler that receives a {@link ClassBuilder}
> * @return the classfile bytes
> */
> public static byte[] buildModule(ModuleAttribute moduleAttribute,
> List packages,
> Consumer handler) {
>
>
> I checked the implementation that seems to match `@param packages` that
> expects additional module packages that are not exported nor open. If it
> intends to take additional packages, it will need to filter the exported and
> open packages at the callsite.
>
> Or the `packages` parameter lists all packages that will be used to create
> `ModulePackages` attribute. This seems to be easier to understand.
Maybe the variants of Classfile.buildModule need to be looked at again. For the
usage here, buildModule(ModuleAttribute, Consumer) would
be more useful as it would allow all of the additional attributes to be emitted
in the handler rather than having buildModule making the decision on whether to
emit the ModulePackages attribute.
-
PR: https://git.openjdk.org/jdk/pull/11368
Re: RFR: 8303922: build-test-lib target is broken
On Thu, 9 Mar 2023 20:09:46 GMT, Eirik Bjorsnos wrote: > The Make target 'build-test-lib-target' is broken in a few ways: > > - make/test/BuildTestLib.gmk references the directory > $(TEST_LIB_SOURCE_DIR)/sun which does not seem to exist. This can be fixed by > removing the reference. > - Some test-lib sources use preview-features which is not enabled by > make/test/BuildTestLib.gmk. This is fixed by adding a JAVAC_FLAGS with > --enable-preview and also adding 'preview' to DISABLED_WARNINGS > - ASN1Formatter.annotate has a possible lossy conversion from long to int > which can be fixed by adding an explicit cast > > This PR fixes the above issues. Marked as reviewed by jwaters (Committer). Looks ok to me, good work :) - PR: https://git.openjdk.org/jdk/pull/12960
Integrated: 8303922: build-test-lib target is broken
On Thu, 9 Mar 2023 20:09:46 GMT, Eirik Bjorsnos wrote: > The Make target 'build-test-lib-target' is broken in a few ways: > > - make/test/BuildTestLib.gmk references the directory > $(TEST_LIB_SOURCE_DIR)/sun which does not seem to exist. This can be fixed by > removing the reference. > - Some test-lib sources use preview-features which is not enabled by > make/test/BuildTestLib.gmk. This is fixed by adding a JAVAC_FLAGS with > --enable-preview and also adding 'preview' to DISABLED_WARNINGS > - ASN1Formatter.annotate has a possible lossy conversion from long to int > which can be fixed by adding an explicit cast > > This PR fixes the above issues. This pull request has now been integrated. Changeset: c313e1ac Author:Eirik Bjorsnos Committer: Julian Waters URL: https://git.openjdk.org/jdk/commit/c313e1ac7b3305b1c012755de4e94728b17e2505 Stats: 8 lines in 2 files changed: 2 ins; 0 del; 6 mod 8303922: build-test-lib target is broken Reviewed-by: erikj, jwaters - PR: https://git.openjdk.org/jdk/pull/12960
Re: RFR: 8294962: Convert java.base/jdk.internal.module package to use the Classfile API to modify and write module-info.class [v4]
On Fri, 10 Mar 2023 16:46:35 GMT, Glavo wrote: > The Classfile API looks great. I want to ask if you have any plans to > completely replace all uses of `jdk.internal.org.objectweb.asm` and delete > its source code from OpenJDK in a short time? I guess yes. https://bugs.openjdk.org/browse/JDK-8294957 would be a good place to track the progress. As the overview JBS issue mentions, there are other ad-hoc utilities that read class files, such as proprietary libraries. In addition, [asmtools](https://github.com/openjdk/asmtools) which generates some assemblies can be migrated to Classfile API as well in the future. - PR: https://git.openjdk.org/jdk/pull/11368
