Re: RFR: 8294974: jdk.jshell jdk.jshell.execution.LocalExecutionControl uses ASM to instrument classes [v3]
> 8294974: jdk.jshell jdk.jshell.execution.LocalExecutionControl uses ASM to > instrument classes > This patch converts it to use Classfile API. > > Please review. > Thanks, > 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 194 commits: - Merge branch 'master' into JDK-8294974-jshell - Merge branch 'master' into JDK-8294974-jshell - Merge branch 'master' into JDK-8294974-jshell - Merge branch 'JDK-8294982' into JDK-8294974 - 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 - ... and 184 more: https://git.openjdk.org/jdk/compare/b1d89f30...070be0a8 - Changes: https://git.openjdk.org/jdk/pull/11413/files Webrev: https://webrevs.openjdk.org/?repo=jdk=11413=02 Stats: 55 lines in 2 files changed: 11 ins; 22 del; 22 mod Patch: https://git.openjdk.org/jdk/pull/11413.diff Fetch: git fetch https://git.openjdk.org/jdk pull/11413/head:pull/11413 PR: https://git.openjdk.org/jdk/pull/11413
Re: RFR: 8294966: jdk.jartool sun.tools.jar.FingerPrint uses ASM to parse class jar entries [v4]
> 8294966: jdk.jartool sun.tools.jar.FingerPrint uses ASM to parse class jar > entries > This patch converts it to use Classfile API. > > Please review. > Thanks, > 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 199 commits: - Merge branch 'master' into JDK-8294966-jartool - FingerPrint.maybeNestedClass set final - Merge branch 'master' into JDK-8294966-jartool - Merge branch 'master' into JDK-8294966-jartool - FingerPrint fixes Co-authored-by: Mandy Chung - Update src/jdk.jartool/share/classes/sun/tools/jar/FingerPrint.java Co-authored-by: Mandy Chung - Merge branch 'JDK-8294982' into JDK-8294966 - removed obsolete javadoc from implementation classes - minor fix in CodeBuilder and added test cases to LDCTest - EntryMap::nextPowerOfTwo delegates to Long:numberOfLeadingZeros - ... and 189 more: https://git.openjdk.org/jdk/compare/b1d89f30...240c0bfc - Changes: https://git.openjdk.org/jdk/pull/11694/files Webrev: https://webrevs.openjdk.org/?repo=jdk=11694=03 Stats: 122 lines in 4 files changed: 48 ins; 40 del; 34 mod Patch: https://git.openjdk.org/jdk/pull/11694.diff Fetch: git fetch https://git.openjdk.org/jdk pull/11694/head:pull/11694 PR: https://git.openjdk.org/jdk/pull/11694
Integrated: 8294971: jdk.jlink jdk.tools.jimage.JImageTask is using ASM to verify classes
On Thu, 9 Mar 2023 11:33:13 GMT, Adam Sotona wrote: > jdk.jlink jdk.tools.jimage.JImageTask is using ASM to verify classes > > This patch converts ASM calls to Classfile API. > > Please review. > > Thanks, > Adam This pull request has now been integrated. Changeset: b1d89f30 Author:Adam Sotona URL: https://git.openjdk.org/jdk/commit/b1d89f30663aed28783e839c5690f46a2b382002 Stats: 15 lines in 2 files changed: 10 ins; 2 del; 3 mod 8294971: jdk.jlink jdk.tools.jimage.JImageTask is using ASM to verify classes Reviewed-by: mchung - PR: https://git.openjdk.org/jdk/pull/12943
Re: RFR: 8294959: java.base java.lang.Module uses ASM to load module-info.class
On Fri, 10 Mar 2023 09:16:03 GMT, Eirik Bjorsnos wrote: > This PR seems to have broken `make docs` It has been fixed by https://github.com/openjdk/jdk/pull/12957 - PR: https://git.openjdk.org/jdk/pull/11367
Re: RFR: 8294966: jdk.jartool sun.tools.jar.FingerPrint uses ASM to parse class jar entries [v3]
On Thu, 9 Mar 2023 18:19:17 GMT, Mandy Chung wrote: >> Adam Sotona has updated the pull request incrementally with one additional >> commit since the last revision: >> >> FingerPrint.maybeNestedClass set final > > src/jdk.jartool/share/classes/sun/tools/jar/FingerPrint.java line 252: > >> 250: private final int access; >> 251: private final boolean publicClass; >> 252: private boolean maybeNestedClass; > > should be final since this is no longer updated. fixed, thanks. - PR: https://git.openjdk.org/jdk/pull/11694
Re: RFR: 8294966: jdk.jartool sun.tools.jar.FingerPrint uses ASM to parse class jar entries [v3]
> 8294966: jdk.jartool sun.tools.jar.FingerPrint uses ASM to parse class jar > entries > This patch converts it to use Classfile API. > > Please review. > Thanks, > Adam Adam Sotona has updated the pull request incrementally with one additional commit since the last revision: FingerPrint.maybeNestedClass set final - Changes: - all: https://git.openjdk.org/jdk/pull/11694/files - new: https://git.openjdk.org/jdk/pull/11694/files/f595a905..f2131215 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=11694=02 - incr: https://webrevs.openjdk.org/?repo=jdk=11694=01-02 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/11694.diff Fetch: git fetch https://git.openjdk.org/jdk pull/11694/head:pull/11694 PR: https://git.openjdk.org/jdk/pull/11694
Re: RFR: 8294974: jdk.jshell jdk.jshell.execution.LocalExecutionControl uses ASM to instrument classes [v2]
> 8294974: jdk.jshell jdk.jshell.execution.LocalExecutionControl uses ASM to > instrument classes > This patch converts it to use Classfile API. > > Please review. > Thanks, > 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 193 commits: - Merge branch 'master' into JDK-8294974-jshell - Merge branch 'master' into JDK-8294974-jshell - Merge branch 'JDK-8294982' into JDK-8294974 - 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 183 more: https://git.openjdk.org/jdk/compare/e26cc526...f9fb8998 - Changes: https://git.openjdk.org/jdk/pull/11413/files Webrev: https://webrevs.openjdk.org/?repo=jdk=11413=01 Stats: 54 lines in 2 files changed: 11 ins; 21 del; 22 mod Patch: https://git.openjdk.org/jdk/pull/11413.diff Fetch: git fetch https://git.openjdk.org/jdk pull/11413/head:pull/11413 PR: https://git.openjdk.org/jdk/pull/11413
Re: RFR: 8294961: java.base java.lang.reflect.ProxyGenerator uses ASM to generate proxy classes [v6]
> 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 - Changes: https://git.openjdk.org/jdk/pull/10991/files Webrev: https://webrevs.openjdk.org/?repo=jdk=10991=05 Stats: 465 lines in 2 files changed: 48 ins; 199 del; 218 mod Patch: https://git.openjdk.org/jdk/pull/10991.diff Fetch: git fetch https://git.openjdk.org/jdk pull/10991/head:pull/10991 PR: https://git.openjdk.org/jdk/pull/10991
Re: RFR: 8294962: java.base jdk.internal.module package uses ASM to modify and write module-info.class [v3]
On Thu, 9 Mar 2023 22:05:55 GMT, Uwe Schindler wrote: >> Adam Sotona has updated the pull request with a new target base due to a >> merge or a rebase. The pull request now contains 198 commits: >> >> - ModuleInfoWriter fixes >> - ModuleInfoExtender comment fix >> - Merge branch 'master' into JDK-8294962-internal-module >> - Update >> src/java.base/share/classes/jdk/internal/module/ModuleInfoExtender.java >> >>Co-authored-by: Mandy Chung >> - Merge branch 'master' into JDK-8294962-internal-module >> - fixed jdk.internal.classfile.java.lang.constant package name >> - Merge branch 'JDK-8294982' into JDK-8294962 >> - removed obsolete javadoc from implementation classes >> - minor fix in CodeBuilder and added test cases to LDCTest >> - EntryMap::nextPowerOfTwo delegates to Long:numberOfLeadingZeros >> - ... and 188 more: https://git.openjdk.org/jdk/compare/e26cc526...9e366fbd > > src/java.base/share/classes/jdk/internal/module/ModuleInfoWriter.java line > 140: > >> 138: p.providers() >> 139: .stream() >> 140: .map(pn -> ClassDesc.of(pn)) > > Change to: `.map(ClassDesc::of)` fixed, thanks. - PR: https://git.openjdk.org/jdk/pull/11368
Re: RFR: 8294962: java.base jdk.internal.module package uses ASM to modify and write module-info.class [v3]
> 8294962: java.base jdk.internal.module package uses ASM to modify and write > module-info.class. > This patch converts it to use Classfile API. > > Please review. > Thanks, > 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 198 commits: - ModuleInfoWriter fixes - ModuleInfoExtender comment fix - Merge branch 'master' into JDK-8294962-internal-module - Update src/java.base/share/classes/jdk/internal/module/ModuleInfoExtender.java Co-authored-by: Mandy Chung - Merge branch 'master' into JDK-8294962-internal-module - fixed jdk.internal.classfile.java.lang.constant package name - Merge branch 'JDK-8294982' into JDK-8294962 - removed obsolete javadoc from implementation classes - minor fix in CodeBuilder and added test cases to LDCTest - EntryMap::nextPowerOfTwo delegates to Long:numberOfLeadingZeros - ... and 188 more: https://git.openjdk.org/jdk/compare/e26cc526...9e366fbd - Changes: https://git.openjdk.org/jdk/pull/11368/files Webrev: https://webrevs.openjdk.org/?repo=jdk=11368=02 Stats: 268 lines in 2 files changed: 78 ins; 119 del; 71 mod Patch: https://git.openjdk.org/jdk/pull/11368.diff Fetch: git fetch https://git.openjdk.org/jdk pull/11368/head:pull/11368 PR: https://git.openjdk.org/jdk/pull/11368
Re: RFR: 8294962: java.base jdk.internal.module package uses ASM to modify and write module-info.class [v3]
On Thu, 9 Mar 2023 18:36:05 GMT, Mandy Chung wrote:
>> Adam Sotona has updated the pull request with a new target base due to a
>> merge or a rebase. The pull request now contains 198 commits:
>>
>> - ModuleInfoWriter fixes
>> - ModuleInfoExtender comment fix
>> - Merge branch 'master' into JDK-8294962-internal-module
>> - Update
>> src/java.base/share/classes/jdk/internal/module/ModuleInfoExtender.java
>>
>>Co-authored-by: Mandy Chung
>> - Merge branch 'master' into JDK-8294962-internal-module
>> - fixed jdk.internal.classfile.java.lang.constant package name
>> - Merge branch 'JDK-8294982' into JDK-8294962
>> - removed obsolete javadoc from implementation classes
>> - minor fix in CodeBuilder and added test cases to LDCTest
>> - EntryMap::nextPowerOfTwo delegates to Long:numberOfLeadingZeros
>> - ... and 188 more: https://git.openjdk.org/jdk/compare/e26cc526...9e366fbd
>
> src/java.base/share/classes/jdk/internal/module/ModuleInfoExtender.java line
> 170:
>
>> 168: }
>> 169:
>> 170: // add ModuleTarget, ModuleResolution and ModuleHashes
>> attributes
>
> This adds or replaces if the attribute exists. Perhaps just drop "add" from
> the comment?
fixed, thanks.
> src/java.base/share/classes/jdk/internal/module/ModuleInfoWriter.java line
> 138:
>
>> 136: for (ModuleDescriptor.Provides p : md.provides()) {
>> 137: mb.provides(ClassDesc.of(p.service()),
>> 138: p.providers()
>
> Formatting nit: align `p.providers()` with the first parameter to
> `mb.provides` - a little confusing with current indentation.
fixed, thanks.
-
PR: https://git.openjdk.org/jdk/pull/11368
Re: RFR: 8294962: java.base jdk.internal.module package uses ASM to modify and write module-info.class [v3]
On Thu, 9 Mar 2023 19:41:28 GMT, liach wrote: >> Adam Sotona has updated the pull request with a new target base due to a >> merge or a rebase. The pull request now contains 198 commits: >> >> - ModuleInfoWriter fixes >> - ModuleInfoExtender comment fix >> - Merge branch 'master' into JDK-8294962-internal-module >> - Update >> src/java.base/share/classes/jdk/internal/module/ModuleInfoExtender.java >> >>Co-authored-by: Mandy Chung >> - Merge branch 'master' into JDK-8294962-internal-module >> - fixed jdk.internal.classfile.java.lang.constant package name >> - Merge branch 'JDK-8294982' into JDK-8294962 >> - removed obsolete javadoc from implementation classes >> - minor fix in CodeBuilder and added test cases to LDCTest >> - EntryMap::nextPowerOfTwo delegates to Long:numberOfLeadingZeros >> - ... and 188 more: https://git.openjdk.org/jdk/compare/e26cc526...9e366fbd > > src/java.base/share/classes/jdk/internal/module/ModuleInfoWriter.java line 89: > >> 87: >> 88: int moduleFlags = md.modifiers().stream() >> 89: .map(MODULE_MODS_TO_FLAGS::get) > > The `MODULE_MODS_TO_FLAGS` can be reused in `modifiersToFlags`. Otherwise, > it's useless and can be removed. `modifiersToFlags` method is now directly inlined using `MODULE_MODS_TO_FLAGS` stream mapping Thank you. - PR: https://git.openjdk.org/jdk/pull/11368
Re: RFR: 8294966: jdk.jartool sun.tools.jar.FingerPrint uses ASM to parse class jar entries [v2]
> 8294966: jdk.jartool sun.tools.jar.FingerPrint uses ASM to parse class jar > entries > This patch converts it to use Classfile API. > > Please review. > Thanks, > 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 197 commits: - Merge branch 'master' into JDK-8294966-jartool - Merge branch 'master' into JDK-8294966-jartool - FingerPrint fixes Co-authored-by: Mandy Chung - Update src/jdk.jartool/share/classes/sun/tools/jar/FingerPrint.java Co-authored-by: Mandy Chung - Merge branch 'JDK-8294982' into JDK-8294966 - 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 - ... and 187 more: https://git.openjdk.org/jdk/compare/e26cc526...f595a905 - Changes: https://git.openjdk.org/jdk/pull/11694/files Webrev: https://webrevs.openjdk.org/?repo=jdk=11694=01 Stats: 123 lines in 4 files changed: 49 ins; 40 del; 34 mod Patch: https://git.openjdk.org/jdk/pull/11694.diff Fetch: git fetch https://git.openjdk.org/jdk pull/11694/head:pull/11694 PR: https://git.openjdk.org/jdk/pull/11694
Re: RFR: 8294966: jdk.jartool sun.tools.jar.FingerPrint uses ASM to parse class jar entries [v2]
On Thu, 9 Mar 2023 18:16:49 GMT, Mandy Chung wrote: >> Adam Sotona has updated the pull request with a new target base due to a >> merge or a rebase. The pull request now contains 197 commits: >> >> - Merge branch 'master' into JDK-8294966-jartool >> - Merge branch 'master' into JDK-8294966-jartool >> - FingerPrint fixes >>Co-authored-by: Mandy Chung >> - Update src/jdk.jartool/share/classes/sun/tools/jar/FingerPrint.java >> >>Co-authored-by: Mandy Chung >> - Merge branch 'JDK-8294982' into JDK-8294966 >> - 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 >> - ... and 187 more: https://git.openjdk.org/jdk/compare/e26cc526...f595a905 > > make/modules/jdk.jartool/Java.gmk line 28: > >> 26: DISABLED_WARNINGS_java += missing-explicit-ctor >> 27: JAVAC_FLAGS += -XDstringConcat=inline >> 28: JAVAC_FLAGS += --enable-preview > > This change is no longer needed. It's not a preview API. Unfortunately patterns in switch statements are still in preview. This flag and participation in preview can be removed once they are finalised. - PR: https://git.openjdk.org/jdk/pull/11694
Re: RFR: 8294972: jdk.jlink internal plugins are heavily using ASM [v3]
On Thu, 9 Mar 2023 22:38:32 GMT, Mandy Chung wrote:
>> Adam Sotona has updated the pull request with a new target base due to a
>> merge or a rebase. The pull request now contains 207 commits:
>>
>> - Merge branch 'master' into JDK-8294972-jlink-plugins
>> - fixed SystemModulesPlugin formatting
>> - 8303624: The java.lang.Thread.FieldHolder can be null for JNI attaching
>> threads
>>
>>Reviewed-by: alanb, dcubed
>> - 8302360: Atomic*.compareAndExchange Javadoc unclear
>>
>>Reviewed-by: martin, dholmes
>> - 8302779: HelidonAppTest.java fails with "assert(_cb ==
>> CodeCache::find_blob(pc())) failed: Must be the same" or SIGSEGV
>>
>>Reviewed-by: coleenp, sspitsyn
>> - 8303691: Fedora based devkit build should load more packages from archive
>> location
>>
>>Reviewed-by: mbaesken, erikj
>> - 8303924: ProblemList serviceability/sa/UniqueVtableTest.java on Linux
>>
>>Reviewed-by: dcubed
>> - 8303609: ProblemList serviceability/sa/TestSysProps.java with ZGC
>>
>>Reviewed-by: dcubed
>> - 8289765: JDI EventSet/resume/resume008 failed with "ERROR: suspendCounts
>> don't match for : VirtualThread-unparker"
>>
>>Reviewed-by: sspitsyn, kevinw
>> - 8301074: Replace NULL with nullptr in share/opto/
>>
>>Reviewed-by: kvn, jwilhelm
>> - ... and 197 more: https://git.openjdk.org/jdk/compare/e26cc526...ec1126f7
>
> src/jdk.jlink/share/classes/jdk/tools/jlink/internal/plugins/SystemModulesPlugin.java
> line 619:
>
>> 617: "",
>> 618:
>> MethodTypeDesc.of(CD_void),
>> 619:false);
>
> nit: extra space
>
> sorry this might be from the patch I sent you.
fixed, thanks.
> src/jdk.jlink/share/classes/jdk/tools/jlink/internal/plugins/SystemModulesPlugin.java
> line 1644:
>
>> 1642: CD_SYSTEM_MODULES_MAP,
>> 1643: clb -> {
>> 1644: clb.withFlags(ACC_FINAL+ACC_SUPER);
>
> Nit: consistent with line 578 with a space before and after `+`
fixed, thanks.
-
PR: https://git.openjdk.org/jdk/pull/12944
Re: RFR: 8294972: jdk.jlink internal plugins are heavily using ASM [v3]
> jdk.jlink internal plugins are heavily using ASM > > This patch converts ASM calls to Classfile API. > > Please review. > Thanks, > 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 207 commits: - Merge branch 'master' into JDK-8294972-jlink-plugins - fixed SystemModulesPlugin formatting - 8303624: The java.lang.Thread.FieldHolder can be null for JNI attaching threads Reviewed-by: alanb, dcubed - 8302360: Atomic*.compareAndExchange Javadoc unclear Reviewed-by: martin, dholmes - 8302779: HelidonAppTest.java fails with "assert(_cb == CodeCache::find_blob(pc())) failed: Must be the same" or SIGSEGV Reviewed-by: coleenp, sspitsyn - 8303691: Fedora based devkit build should load more packages from archive location Reviewed-by: mbaesken, erikj - 8303924: ProblemList serviceability/sa/UniqueVtableTest.java on Linux Reviewed-by: dcubed - 8303609: ProblemList serviceability/sa/TestSysProps.java with ZGC Reviewed-by: dcubed - 8289765: JDI EventSet/resume/resume008 failed with "ERROR: suspendCounts don't match for : VirtualThread-unparker" Reviewed-by: sspitsyn, kevinw - 8301074: Replace NULL with nullptr in share/opto/ Reviewed-by: kvn, jwilhelm - ... and 197 more: https://git.openjdk.org/jdk/compare/e26cc526...ec1126f7 - Changes: https://git.openjdk.org/jdk/pull/12944/files Webrev: https://webrevs.openjdk.org/?repo=jdk=12944=02 Stats: 1025 lines in 6 files changed: 215 ins; 300 del; 510 mod Patch: https://git.openjdk.org/jdk/pull/12944.diff Fetch: git fetch https://git.openjdk.org/jdk pull/12944/head:pull/12944 PR: https://git.openjdk.org/jdk/pull/12944
Re: RFR: 8294971: jdk.jlink jdk.tools.jimage.JImageTask is using ASM to verify classes [v2]
> jdk.jlink jdk.tools.jimage.JImageTask is using ASM to verify classes > > This patch converts ASM calls to Classfile API. > > Please review. > > Thanks, > 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 190 commits: - Merge branch 'master' into JDK-8294971-jlink-jimage - Merge branch 'master' into JDK-8294971-jlink-jimage - 8294971: jdk.jlink jdk.tools.jimage.JImageTask is using ASM to verify classes - 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 180 more: https://git.openjdk.org/jdk/compare/e26cc526...37fac1d2 - Changes: https://git.openjdk.org/jdk/pull/12943/files Webrev: https://webrevs.openjdk.org/?repo=jdk=12943=01 Stats: 15 lines in 2 files changed: 10 ins; 2 del; 3 mod Patch: https://git.openjdk.org/jdk/pull/12943.diff Fetch: git fetch https://git.openjdk.org/jdk pull/12943/head:pull/12943 PR: https://git.openjdk.org/jdk/pull/12943
Re: RFR: 8294972: jdk.jlink internal plugins are heavily using ASM [v2]
> jdk.jlink internal plugins are heavily using ASM > > This patch converts ASM calls to Classfile API. > > Please review. > Thanks, > Adam Adam Sotona has updated the pull request incrementally with one additional commit since the last revision: Update src/jdk.jlink/share/classes/jdk/tools/jlink/internal/plugins/IncludeLocalesPlugin.java Co-authored-by: Mandy Chung - Changes: - all: https://git.openjdk.org/jdk/pull/12944/files - new: https://git.openjdk.org/jdk/pull/12944/files/49c5bd59..0c0df14c Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=12944=01 - incr: https://webrevs.openjdk.org/?repo=jdk=12944=00-01 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/12944.diff Fetch: git fetch https://git.openjdk.org/jdk pull/12944/head:pull/12944 PR: https://git.openjdk.org/jdk/pull/12944
Re: RFR: 8294962: java.base jdk.internal.module package uses ASM to modify and write module-info.class [v2]
> 8294962: java.base jdk.internal.module package uses ASM to modify and write > module-info.class. > This patch converts it to use Classfile API. > > Please review. > Thanks, > Adam Adam Sotona has updated the pull request incrementally with one additional commit since the last revision: Update src/java.base/share/classes/jdk/internal/module/ModuleInfoExtender.java Co-authored-by: Mandy Chung - Changes: - all: https://git.openjdk.org/jdk/pull/11368/files - new: https://git.openjdk.org/jdk/pull/11368/files/011a6700..03da6dd3 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=11368=01 - incr: https://webrevs.openjdk.org/?repo=jdk=11368=00-01 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/11368.diff Fetch: git fetch https://git.openjdk.org/jdk pull/11368/head:pull/11368 PR: https://git.openjdk.org/jdk/pull/11368
Re: RFR: 8294971: jdk.jlink jdk.tools.jimage.JImageTask is using ASM to verify classes
The message from this sender included one or more files which could not be scanned for virus detection; do not open these files unless you are certain of the sender's intent. -- On Thu, 9 Mar 2023 11:33:13 GMT, Adam Sotona wrote: > jdk.jlink jdk.tools.jimage.JImageTask is using ASM to verify classes > > This patch converts ASM calls to Classfile API. > > Please review. > > Thanks, > Adam Provided code only reflect original verification by parsing the class content. It is now possible to extend the verification by utilizing Classfile API bytecode verification. However it would require to provide appropriate class hierarchy resolution for stack map table verification. - PR: https://git.openjdk.org/jdk/pull/12943
Integrated: 8294959: java.base java.lang.Module uses ASM to load module-info.class
On Fri, 25 Nov 2022 14:35:22 GMT, Adam Sotona wrote: > java.base java.lang.Module uses ASM to load module-info.class and this patch > converts it to use Classfile API. This pull request has now been integrated. Changeset: 595645c7 Author: Adam Sotona URL: https://git.openjdk.org/jdk/commit/595645c76d09b0c30da7fa7d8435ca960c8e3268 Stats: 54 lines in 1 file changed: 8 ins; 37 del; 9 mod 8294959: java.base java.lang.Module uses ASM to load module-info.class Reviewed-by: mchung - PR: https://git.openjdk.org/jdk/pull/11367
RFR: 8294972: jdk.jlink internal plugins are heavily using ASM
jdk.jlink internal plugins are heavily using ASM This patch converts ASM calls to Classfile API. Please review. Thanks, Adam - Commit messages: - Merge branch 'master' into JDK-8294972-jlink-plugins - renamed variables in SystemModulesPlugin - 8294972: jdk.jlink internal plugins are heavily using ASM - 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 180 more: https://git.openjdk.org/jdk/compare/cdcf5c1e...49c5bd59 Changes: https://git.openjdk.org/jdk/pull/12944/files Webrev: https://webrevs.openjdk.org/?repo=jdk=12944=00 Issue: https://bugs.openjdk.org/browse/JDK-8294972 Stats: 1025 lines in 6 files changed: 215 ins; 300 del; 510 mod Patch: https://git.openjdk.org/jdk/pull/12944.diff Fetch: git fetch https://git.openjdk.org/jdk pull/12944/head:pull/12944 PR: https://git.openjdk.org/jdk/pull/12944
RFR: 8294971: jdk.jlink jdk.tools.jimage.JImageTask is using ASM to verify classes
The message from this sender included one or more files which could not be scanned for virus detection; do not open these files unless you are certain of the sender's intent. -- jdk.jlink jdk.tools.jimage.JImageTask is using ASM to verify classes This patch converts ASM calls to Classfile API. Please review. Thanks, Adam - Commit messages: - Merge branch 'master' into JDK-8294971-jlink-jimage - 8294971: jdk.jlink jdk.tools.jimage.JImageTask is using ASM to verify classes - 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 - fixed CodeRelabeler javadoc - ... and 179 more: https://git.openjdk.org/jdk/compare/cdcf5c1e...68009406 Changes: https://git.openjdk.org/jdk/pull/12943/files Webrev: https://webrevs.openjdk.org/?repo=jdk=12943=00 Issue: https://bugs.openjdk.org/browse/JDK-8294971 Stats: 15 lines in 2 files changed: 10 ins; 2 del; 3 mod Patch: https://git.openjdk.org/jdk/pull/12943.diff Fetch: git fetch https://git.openjdk.org/jdk pull/12943/head:pull/12943 PR: https://git.openjdk.org/jdk/pull/12943
Re: RFR: 8294966: jdk.jartool sun.tools.jar.FingerPrint uses ASM to parse class jar entries
On Tue, 7 Mar 2023 20:47:31 GMT, Mandy Chung wrote:
>> 8294966: jdk.jartool sun.tools.jar.FingerPrint uses ASM to parse class jar
>> entries
>> This patch converts it to use Classfile API.
>>
>> Please review.
>> Thanks,
>> Adam
>
> src/jdk.jartool/share/classes/sun/tools/jar/FingerPrint.java line 42:
>
>> 40: import jdk.internal.classfile.FieldModel;
>> 41: import jdk.internal.classfile.MethodModel;
>> 42: import jdk.internal.classfile.attribute.EnclosingMethodAttribute;
>
> this import is unused.
fixed, thanks.
> src/jdk.jartool/share/classes/sun/tools/jar/FingerPrint.java line 313:
>
>> 311: @Override
>> 312: public void visitEnd() {
>> 313: this.nestedClass = this.outerClassName != null;
>
> `this.nestedClass` is updated after the attributes are visited. Suggest to
> rename `nestedClass` to `maybeNestedClass` and change `isNestedClass` to:
>
> public boolean isNestedClass() {
> return attrs.maybeNestedClass && attrs.outerClassName != null;
> }
I've changed `nestedClass` according to the suggestion, thanks.
-
PR: https://git.openjdk.org/jdk/pull/11694
RFR: 8294966: jdk.jartool sun.tools.jar.FingerPrint uses ASM to parse class jar entries
8294966: jdk.jartool sun.tools.jar.FingerPrint uses ASM to parse class jar entries This patch converts it to use Classfile API. Please review. Thanks, Adam - Commit messages: - Merge branch 'master' into JDK-8294966-jartool - FingerPrint fixes - Update src/jdk.jartool/share/classes/sun/tools/jar/FingerPrint.java - Merge branch 'JDK-8294982' into JDK-8294966 - 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 - ... and 186 more: https://git.openjdk.org/jdk/compare/cdcf5c1e...3a835539 Changes: https://git.openjdk.org/jdk/pull/11694/files Webrev: https://webrevs.openjdk.org/?repo=jdk=11694=00 Issue: https://bugs.openjdk.org/browse/JDK-8294966 Stats: 123 lines in 4 files changed: 49 ins; 40 del; 34 mod Patch: https://git.openjdk.org/jdk/pull/11694.diff Fetch: git fetch https://git.openjdk.org/jdk pull/11694/head:pull/11694 PR: https://git.openjdk.org/jdk/pull/11694
RFR: 8294974: jdk.jshell jdk.jshell.execution.LocalExecutionControl uses ASM to instrument classes
8294974: jdk.jshell jdk.jshell.execution.LocalExecutionControl uses ASM to instrument classes This patch converts it to use Classfile API. Please review. Thanks, Adam - Commit messages: - Merge branch 'master' into JDK-8294974-jshell - Merge branch 'JDK-8294982' into JDK-8294974 - 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 - fixed CodeRelabeler javadoc - ... and 182 more: https://git.openjdk.org/jdk/compare/cdcf5c1e...cfcfb077 Changes: https://git.openjdk.org/jdk/pull/11413/files Webrev: https://webrevs.openjdk.org/?repo=jdk=11413=00 Issue: https://bugs.openjdk.org/browse/JDK-8294974 Stats: 54 lines in 2 files changed: 11 ins; 21 del; 22 mod Patch: https://git.openjdk.org/jdk/pull/11413.diff Fetch: git fetch https://git.openjdk.org/jdk pull/11413/head:pull/11413 PR: https://git.openjdk.org/jdk/pull/11413
Re: RFR: 8294962: java.base jdk.internal.module package uses ASM to modify and write module-info.class
On Wed, 1 Mar 2023 01:19:24 GMT, Mandy Chung wrote: >> 8294962: java.base jdk.internal.module package uses ASM to modify and write >> module-info.class. >> This patch converts it to use Classfile API. >> >> Please review. >> Thanks, >> Adam > > src/java.base/share/classes/jdk/internal/module/ModuleInfoWriter.java line 37: > >> 35: import jdk.internal.classfile.Classfile; >> 36: import jdk.internal.classfile.jdktypes.ModuleDesc; >> 37: import jdk.internal.classfile.jdktypes.PackageDesc; > > I'm a bit surprised of `jdk.internal.classfile.jdktypes` package. ClassFile > API is JDK-specific. Would `jdk.internal.classfile.constant` be more > appropriate? `jdk.internal.classfile.jdktypes` package has been renamed to `jdk.internal.classfile.java.lang.constant` thanks for pointing it out > src/java.base/share/classes/jdk/internal/module/ModuleInfoWriter.java line > 146: > >> 144: >> 145: // packages >> 146: md.packages().stream().map(PackageDesc::of).toList(), > > When writing `module_info.class` via ASM, it only visits the packages if > there are packages that aren't exported or open. > > Does the ClassFile API add `ModulePackages` attribute only if there are > packages that aren't exported or open? Yes, `Classfile::buildModule` contains logic determining if emission of `ModulePackages` is required or not. - PR: https://git.openjdk.org/jdk/pull/11368
Re: RFR: 8294962: java.base jdk.internal.module package uses ASM to modify and write module-info.class
On Fri, 25 Nov 2022 14:43:32 GMT, Alan Bateman wrote:
>> 8294962: java.base jdk.internal.module package uses ASM to modify and write
>> module-info.class.
>> This patch converts it to use Classfile API.
>>
>> Please review.
>> Thanks,
>> Adam
>
> src/java.base/share/classes/jdk/internal/module/ModuleInfoExtender.java line
> 184:
>
>> 182: clb.with(ModuleAttribute.of(ma.moduleName(),
>> ma.moduleFlagsMask(), clb.constantPool().utf8Entry(v.toString()),
>> ma.requires(), ma.exports(), ma.opens(), ma.uses(), ma.provides()));
>> 183: } else {
>> 184: clb.accept(cle);
>
> Can you re-format this so that it's consistent with the existing code? Wildly
> long lines (180+ chars in this case) make it impossible to use side-by-side
> views when looking at changes.
Fixed, I'm sorry I missed it.
-
PR: https://git.openjdk.org/jdk/pull/11368
RFR: 8294962: java.base jdk.internal.module package uses ASM to modify and write module-info.class
8294962: java.base jdk.internal.module package uses ASM to modify and write module-info.class. This patch converts it to use Classfile API. Please review. Thanks, Adam - Commit messages: - Merge branch 'master' into JDK-8294962-internal-module - fixed jdk.internal.classfile.java.lang.constant package name - Merge branch 'JDK-8294982' into JDK-8294962 - 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 184 more: https://git.openjdk.org/jdk/compare/cdcf5c1e...011a6700 Changes: https://git.openjdk.org/jdk/pull/11368/files Webrev: https://webrevs.openjdk.org/?repo=jdk=11368=00 Issue: https://bugs.openjdk.org/browse/JDK-8294962 Stats: 273 lines in 2 files changed: 85 ins; 116 del; 72 mod Patch: https://git.openjdk.org/jdk/pull/11368.diff Fetch: git fetch https://git.openjdk.org/jdk pull/11368/head:pull/11368 PR: https://git.openjdk.org/jdk/pull/11368
Re: RFR: 8294961: java.base java.lang.reflect.ProxyGenerator uses ASM to generate proxy classes [v3]
On Sun, 19 Feb 2023 05:23:20 GMT, liach wrote:
>> Adam Sotona 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 six additional
>> commits since the last revision:
>>
>> - j.l.r.ProxyGenerator improvements
>>Author: Mandy Chung
>> - Merge branch 'JDK-8294982' into JDK-8294961
>> - j.l.r.ProxyGenerator fix - Classfile API moved under
>> jdk.internal.classfile package
>> - Merge branch 'JDK-8294982' into JDK-8294961
>> - Merge branch 'JDK-8294982' into JDK-8294961
>> - 8294961: java.base java.lang.reflect.ProxyGenerator uses ASM to generate
>> proxy classes
>
> src/java.base/share/classes/java/lang/reflect/ProxyGenerator.java line 661:
>
>> 659: */
>> 660: private void generateMethod(ClassBuilder clb, ClassDesc
>> className) {
>> 661: MethodTypeDesc desc = MethodType.methodType(returnType,
>> parameterTypes).describeConstable().orElseThrow();
>
> Can we convert this line to:
>
> MethodTypeDesc desc = MethodTypeDesc.of(toClassDesc(returnType),
> Arrays.stream(parameterTypes).map(ProxyGenerator::toClassDesc).toArray(ClassDesc[]::new));
>
>
> Mainly due to that `MethodType` creation involves a lot of unrelated process
> such as generating lambda forms, and we can remove the `MethodType` import as
> a result.
Isn't the "unrelated process such as generating lambda forms" critical for the
`ProxyGenerator` functionality?
-
PR: https://git.openjdk.org/jdk/pull/10991
RFR: 8294959: java.base java.lang.Module uses ASM to load module-info.class
java.base java.lang.Module uses ASM to load module-info.class and this patch converts it to use Classfile API. - Commit messages: - Merge branch 'master' into JDK-8294959-module - Merge branch 'JDK-8294982' into JDK-8294959 - 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 - fixed CodeRelabeler javadoc - ... and 184 more: https://git.openjdk.org/jdk/compare/cdcf5c1e...61748d8c Changes: https://git.openjdk.org/jdk/pull/11367/files Webrev: https://webrevs.openjdk.org/?repo=jdk=11367=00 Issue: https://bugs.openjdk.org/browse/JDK-8294959 Stats: 54 lines in 1 file changed: 8 ins; 37 del; 9 mod Patch: https://git.openjdk.org/jdk/pull/11367.diff Fetch: git fetch https://git.openjdk.org/jdk pull/11367/head:pull/11367 PR: https://git.openjdk.org/jdk/pull/11367
Re: RFR: 8294961: java.base java.lang.reflect.ProxyGenerator uses ASM to generate proxy classes [v5]
> java.base java.lang.reflect.ProxyGenerator uses ASM to generate proxy classes > and this patch converts it to use Classfile API. > > This pull request suppose to chain on the 8294982: Implementation of > Classfile API https://github.com/openjdk/jdk/pull/10982 > > 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 195 commits: - 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 - fixed CodeRelabeler javadoc - ... and 185 more: https://git.openjdk.org/jdk/compare/cdcf5c1e...48ac16f8 - Changes: https://git.openjdk.org/jdk/pull/10991/files Webrev: https://webrevs.openjdk.org/?repo=jdk=10991=04 Stats: 465 lines in 2 files changed: 48 ins; 199 del; 218 mod Patch: https://git.openjdk.org/jdk/pull/10991.diff Fetch: git fetch https://git.openjdk.org/jdk pull/10991/head:pull/10991 PR: https://git.openjdk.org/jdk/pull/10991
Integrated: 8294982: Implementation of Classfile API
On Fri, 4 Nov 2022 12:38:04 GMT, Adam Sotona wrote: > This is root pull request with Classfile API implementation, tests and > benchmarks initial drop into JDK. > > Following pull requests consolidating JDK class files parsing, generating, > and transforming ([JDK-8294957](https://bugs.openjdk.org/browse/JDK-8294957)) > will chain to this one. > > Classfile API development is tracked at: > https://github.com/openjdk/jdk-sandbox/tree/classfile-api-branch > > Development branch of consolidated JDK class files parsing, generating, and > transforming is at: > https://github.com/openjdk/jdk-sandbox/tree/classfile-api-dev-branch > > Classfile API [JEP](https://bugs.openjdk.org/browse/JDK-8280389) and [online > API > documentation](https://htmlpreview.github.io/?https://raw.githubusercontent.com/openjdk/jdk-sandbox/classfile-api-javadoc-branch/doc/classfile-api/javadoc/java.base/jdk/internal/classfile/package-summary.html) > is also available. > > Please take you time to review this non-trivial JDK addition. > > Thank you, > Adam This pull request has now been integrated. Changeset: 4655b790 Author:Adam Sotona URL: https://git.openjdk.org/jdk/commit/4655b790d0b39b4ddabde78d7b3eed196b1152ed Stats: 52932 lines in 322 files changed: 52928 ins; 0 del; 4 mod 8294982: Implementation of Classfile API Reviewed-by: ihse, psandoz, mcimadamore - PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294961: java.base java.lang.reflect.ProxyGenerator uses ASM to generate proxy classes [v4]
> java.base java.lang.reflect.ProxyGenerator uses ASM to generate proxy classes > and this patch converts it to use Classfile API. > > This pull request suppose to chain on the 8294982: Implementation of > Classfile API https://github.com/openjdk/jdk/pull/10982 > > 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 incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains seven additional commits since the last revision: - Merge branch 'JDK-8294982' into JDK-8294961 - j.l.r.ProxyGenerator improvements Author: Mandy Chung - Merge branch 'JDK-8294982' into JDK-8294961 - j.l.r.ProxyGenerator fix - Classfile API moved under jdk.internal.classfile package - Merge branch 'JDK-8294982' into JDK-8294961 - Merge branch 'JDK-8294982' into JDK-8294961 - 8294961: java.base java.lang.reflect.ProxyGenerator uses ASM to generate proxy classes - Changes: - all: https://git.openjdk.org/jdk/pull/10991/files - new: https://git.openjdk.org/jdk/pull/10991/files/9d671a04..0330c488 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=10991=03 - incr: https://webrevs.openjdk.org/?repo=jdk=10991=02-03 Stats: 116652 lines in 3261 files changed: 55855 ins; 28518 del; 32279 mod Patch: https://git.openjdk.org/jdk/pull/10991.diff Fetch: git fetch https://git.openjdk.org/jdk pull/10991/head:pull/10991 PR: https://git.openjdk.org/jdk/pull/10991
Re: RFR: 8294982: Implementation of Classfile API [v49]
On Tue, 7 Mar 2023 15:48:58 GMT, Maurizio Cimadamore wrote: >> This is common practise across the whole implementation. Do you suggest to >> remove all similar javadoc from all implementation classes? > > Well, if the javadoc simply states the name of the class it doesn't seem very > useful. But I leave that decision to you. I've removed obsolete javadoc from implementation classes. Thanks. - PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v58]
> This is root pull request with Classfile API implementation, tests and > benchmarks initial drop into JDK. > > Following pull requests consolidating JDK class files parsing, generating, > and transforming ([JDK-8294957](https://bugs.openjdk.org/browse/JDK-8294957)) > will chain to this one. > > Classfile API development is tracked at: > https://github.com/openjdk/jdk-sandbox/tree/classfile-api-branch > > Development branch of consolidated JDK class files parsing, generating, and > transforming is at: > https://github.com/openjdk/jdk-sandbox/tree/classfile-api-dev-branch > > Classfile API [JEP](https://bugs.openjdk.org/browse/JDK-8280389) and [online > API > documentation](https://htmlpreview.github.io/?https://raw.githubusercontent.com/openjdk/jdk-sandbox/classfile-api-javadoc-branch/doc/classfile-api/javadoc/java.base/jdk/internal/classfile/package-summary.html) > is also available. > > Please take you time to review this non-trivial JDK addition. > > Thank you, > Adam Adam Sotona has updated the pull request incrementally with one additional commit since the last revision: removed obsolete javadoc from implementation classes - Changes: - all: https://git.openjdk.org/jdk/pull/10982/files - new: https://git.openjdk.org/jdk/pull/10982/files/cd4a01cd..385cb264 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=10982=57 - incr: https://webrevs.openjdk.org/?repo=jdk=10982=56-57 Stats: 129 lines in 42 files changed: 0 ins; 128 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/10982.diff Fetch: git fetch https://git.openjdk.org/jdk pull/10982/head:pull/10982 PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v57]
> This is root pull request with Classfile API implementation, tests and > benchmarks initial drop into JDK. > > Following pull requests consolidating JDK class files parsing, generating, > and transforming ([JDK-8294957](https://bugs.openjdk.org/browse/JDK-8294957)) > will chain to this one. > > Classfile API development is tracked at: > https://github.com/openjdk/jdk-sandbox/tree/classfile-api-branch > > Development branch of consolidated JDK class files parsing, generating, and > transforming is at: > https://github.com/openjdk/jdk-sandbox/tree/classfile-api-dev-branch > > Classfile API [JEP](https://bugs.openjdk.org/browse/JDK-8280389) and [online > API > documentation](https://htmlpreview.github.io/?https://raw.githubusercontent.com/openjdk/jdk-sandbox/classfile-api-javadoc-branch/doc/classfile-api/javadoc/java.base/jdk/internal/classfile/package-summary.html) > is also available. > > Please take you time to review this non-trivial JDK addition. > > Thank you, > Adam Adam Sotona has updated the pull request incrementally with one additional commit since the last revision: minor fix in CodeBuilder and added test cases to LDCTest - Changes: - all: https://git.openjdk.org/jdk/pull/10982/files - new: https://git.openjdk.org/jdk/pull/10982/files/b49aae8b..cd4a01cd Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=10982=56 - incr: https://webrevs.openjdk.org/?repo=jdk=10982=55-56 Stats: 8 lines in 2 files changed: 5 ins; 0 del; 3 mod Patch: https://git.openjdk.org/jdk/pull/10982.diff Fetch: git fetch https://git.openjdk.org/jdk pull/10982/head:pull/10982 PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v43]
On Tue, 7 Mar 2023 16:06:17 GMT, Paul Sandoz wrote: >> `Long:numberOfLeadingZeros` implementation uses more method calls, >> conditional statements and binary operations. I would prefer to stick with >> the current implementation for its speed and simplicity. > > `numberOfLeadingZeros` is an intrinsic, so it will compile down to a machine > instruction. Up to you. Oh, I've changed it, thanks. - PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v56]
> This is root pull request with Classfile API implementation, tests and > benchmarks initial drop into JDK. > > Following pull requests consolidating JDK class files parsing, generating, > and transforming ([JDK-8294957](https://bugs.openjdk.org/browse/JDK-8294957)) > will chain to this one. > > Classfile API development is tracked at: > https://github.com/openjdk/jdk-sandbox/tree/classfile-api-branch > > Development branch of consolidated JDK class files parsing, generating, and > transforming is at: > https://github.com/openjdk/jdk-sandbox/tree/classfile-api-dev-branch > > Classfile API [JEP](https://bugs.openjdk.org/browse/JDK-8280389) and [online > API > documentation](https://htmlpreview.github.io/?https://raw.githubusercontent.com/openjdk/jdk-sandbox/classfile-api-javadoc-branch/doc/classfile-api/javadoc/java.base/jdk/internal/classfile/package-summary.html) > is also available. > > Please take you time to review this non-trivial JDK addition. > > Thank you, > Adam Adam Sotona has updated the pull request incrementally with two additional commits since the last revision: - EntryMap::nextPowerOfTwo delegates to Long:numberOfLeadingZeros - fixed CodeBuilder:constantInstruction for -0.0d and -0.0f values and added test - Changes: - all: https://git.openjdk.org/jdk/pull/10982/files - new: https://git.openjdk.org/jdk/pull/10982/files/4680572a..b49aae8b Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=10982=55 - incr: https://webrevs.openjdk.org/?repo=jdk=10982=54-55 Stats: 21 lines in 3 files changed: 9 ins; 6 del; 6 mod Patch: https://git.openjdk.org/jdk/pull/10982.diff Fetch: git fetch https://git.openjdk.org/jdk pull/10982/head:pull/10982 PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v55]
> This is root pull request with Classfile API implementation, tests and > benchmarks initial drop into JDK. > > Following pull requests consolidating JDK class files parsing, generating, > and transforming ([JDK-8294957](https://bugs.openjdk.org/browse/JDK-8294957)) > will chain to this one. > > Classfile API development is tracked at: > https://github.com/openjdk/jdk-sandbox/tree/classfile-api-branch > > Development branch of consolidated JDK class files parsing, generating, and > transforming is at: > https://github.com/openjdk/jdk-sandbox/tree/classfile-api-dev-branch > > Classfile API [JEP](https://bugs.openjdk.org/browse/JDK-8280389) and [online > API > documentation](https://htmlpreview.github.io/?https://raw.githubusercontent.com/openjdk/jdk-sandbox/classfile-api-javadoc-branch/doc/classfile-api/javadoc/java.base/jdk/internal/classfile/package-summary.html) > is also available. > > Please take you time to review this non-trivial JDK addition. > > 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 183 commits: - 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 - fixed CodeRelabeler javadoc - Shared `toString` formats for bound and unbound instructions - generic implementation of ResolvedTransform - snippets and tests synced with jdk.jfr class instrumentation source code - simplified initialization of terminal builder in chained builders - simplified CodeImpl.SINGLETON_INSTRUCTIONS initialization - fixed handling of array descriptors by Util::toInternalName - ... and 173 more: https://git.openjdk.org/jdk/compare/45a616a8...4680572a - Changes: https://git.openjdk.org/jdk/pull/10982/files Webrev: https://webrevs.openjdk.org/?repo=jdk=10982=54 Stats: 53052 lines in 322 files changed: 53048 ins; 0 del; 4 mod Patch: https://git.openjdk.org/jdk/pull/10982.diff Fetch: git fetch https://git.openjdk.org/jdk pull/10982/head:pull/10982 PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v54]
> This is root pull request with Classfile API implementation, tests and > benchmarks initial drop into JDK. > > Following pull requests consolidating JDK class files parsing, generating, > and transforming ([JDK-8294957](https://bugs.openjdk.org/browse/JDK-8294957)) > will chain to this one. > > Classfile API development is tracked at: > https://github.com/openjdk/jdk-sandbox/tree/classfile-api-branch > > Development branch of consolidated JDK class files parsing, generating, and > transforming is at: > https://github.com/openjdk/jdk-sandbox/tree/classfile-api-dev-branch > > Classfile API [JEP](https://bugs.openjdk.org/browse/JDK-8280389) and [online > API > documentation](https://htmlpreview.github.io/?https://raw.githubusercontent.com/openjdk/jdk-sandbox/classfile-api-javadoc-branch/doc/classfile-api/javadoc/java.base/jdk/internal/classfile/package-summary.html) > is also available. > > Please take you time to review this non-trivial JDK addition. > > Thank you, > Adam Adam Sotona has updated the pull request incrementally with one additional commit since the last revision: fixed new lines at end of file - Changes: - all: https://git.openjdk.org/jdk/pull/10982/files - new: https://git.openjdk.org/jdk/pull/10982/files/a994c572..f14287d0 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=10982=53 - incr: https://webrevs.openjdk.org/?repo=jdk=10982=52-53 Stats: 13 lines in 13 files changed: 0 ins; 0 del; 13 mod Patch: https://git.openjdk.org/jdk/pull/10982.diff Fetch: git fetch https://git.openjdk.org/jdk pull/10982/head:pull/10982 PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v49]
On Tue, 7 Mar 2023 15:48:22 GMT, Maurizio Cimadamore wrote: >> Here I'm also not sure I understand, the long line has bee wrapped. > > I mean here (and in the other) there seems to be a missing newline at the end > of the file OK, I'll fix it, thanks. - PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v36]
On Tue, 7 Mar 2023 08:35:34 GMT, Adam Sotona wrote: >> I am unsure how you might use `BlockCodeBuilder`. If the current signature >> is not changed then `transformFromHandler` seems reasonable (since its the >> handler that pushes elements into its given builder). >> >> The other `transform` is implicitly "transform model". > > I'm sorry my answer was inaccurate, what I'm proposing is > `transformBlock(Consumer, CodeTransform)` or > `transformedBlock(Consumer, CodeTransform)`. > `BlockCodeBuilder` is just an extension of `CodeBuilder` with ability to > break block execution. > Block is a sub-unit of code, so I expect it would be a logical extension of > the actual `block(Consumer`. > I'll move this discussion to the mailing list to make common decision about > API change. > Thanks. This topic is now tracked as RFE - PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v53]
> This is root pull request with Classfile API implementation, tests and > benchmarks initial drop into JDK. > > Following pull requests consolidating JDK class files parsing, generating, > and transforming ([JDK-8294957](https://bugs.openjdk.org/browse/JDK-8294957)) > will chain to this one. > > Classfile API development is tracked at: > https://github.com/openjdk/jdk-sandbox/tree/classfile-api-branch > > Development branch of consolidated JDK class files parsing, generating, and > transforming is at: > https://github.com/openjdk/jdk-sandbox/tree/classfile-api-dev-branch > > Classfile API [JEP](https://bugs.openjdk.org/browse/JDK-8280389) and [online > API > documentation](https://htmlpreview.github.io/?https://raw.githubusercontent.com/openjdk/jdk-sandbox/classfile-api-javadoc-branch/doc/classfile-api/javadoc/java.base/jdk/internal/classfile/package-summary.html) > is also available. > > Please take you time to review this non-trivial JDK addition. > > Thank you, > Adam Adam Sotona has updated the pull request incrementally with one additional commit since the last revision: package jdk.internal.classfile.jdktypes moved to jdk.internal.classfile.java.lang.constant - Changes: - all: https://git.openjdk.org/jdk/pull/10982/files - new: https://git.openjdk.org/jdk/pull/10982/files/7a2b5cbe..a994c572 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=10982=52 - incr: https://webrevs.openjdk.org/?repo=jdk=10982=51-52 Stats: 35 lines in 26 files changed: 0 ins; 0 del; 35 mod Patch: https://git.openjdk.org/jdk/pull/10982.diff Fetch: git fetch https://git.openjdk.org/jdk pull/10982/head:pull/10982 PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v52]
> This is root pull request with Classfile API implementation, tests and > benchmarks initial drop into JDK. > > Following pull requests consolidating JDK class files parsing, generating, > and transforming ([JDK-8294957](https://bugs.openjdk.org/browse/JDK-8294957)) > will chain to this one. > > Classfile API development is tracked at: > https://github.com/openjdk/jdk-sandbox/tree/classfile-api-branch > > Development branch of consolidated JDK class files parsing, generating, and > transforming is at: > https://github.com/openjdk/jdk-sandbox/tree/classfile-api-dev-branch > > Classfile API [JEP](https://bugs.openjdk.org/browse/JDK-8280389) and [online > API > documentation](https://htmlpreview.github.io/?https://raw.githubusercontent.com/openjdk/jdk-sandbox/classfile-api-javadoc-branch/doc/classfile-api/javadoc/java.base/jdk/internal/classfile/package-summary.html) > is also available. > > Please take you time to review this non-trivial JDK addition. > > Thank you, > Adam Adam Sotona has updated the pull request incrementally with one additional commit since the last revision: fixed CodeRelabeler javadoc - Changes: - all: https://git.openjdk.org/jdk/pull/10982/files - new: https://git.openjdk.org/jdk/pull/10982/files/2b1bd7f8..7a2b5cbe Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=10982=51 - incr: https://webrevs.openjdk.org/?repo=jdk=10982=50-51 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/10982.diff Fetch: git fetch https://git.openjdk.org/jdk pull/10982/head:pull/10982 PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v49]
On Tue, 7 Mar 2023 11:14:17 GMT, Maurizio Cimadamore
wrote:
>> Adam Sotona has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> snippets and tests synced with jdk.jfr class instrumentation source code
>
> src/java.base/share/classes/jdk/internal/classfile/components/CodeRelabeler.java
> line 44:
>
>> 42:
>> 43: /**
>> 44: * CodeRelabeler is a {@link jdk.internal.classfile.CodeTransform}
>> replacing all occurrences
>
> Nit - perhaps just use `A code relabeler is a ...` (instead of using the
> class name, which then would require another `{@code ... }`.
fixed, thanks.
> src/java.base/share/classes/jdk/internal/classfile/components/package-info.java
> line 114:
>
>> 112: * {@snippet lang="java" class="PackageSnippets"
>> region="classInstrumentation"}
>> 113: */
>> 114: package jdk.internal.classfile.components;
>
> watch out for newline
I'm not quite sure I understand what is wrong with this javadoc fragment.
Thanks.
> src/java.base/share/classes/jdk/internal/classfile/impl/EntryMap.java line
> 194:
>
>> 192: return (int)s;
>> 193: }
>> 194: }
>
> newline!
Here I'm also not sure I understand, the long line has bee wrapped.
> src/java.base/share/classes/jdk/internal/classfile/impl/TargetInfoImpl.java
> line 34:
>
>> 32:
>> 33: /**
>> 34: * TargetInfoImpl
>
> Does this javadoc add any value? Since this is implementation, we could also
> remove it.
This is common practise across the whole implementation. Do you suggest to
remove all similar javadoc from all implementation classes?
-
PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v36]
On Fri, 3 Mar 2023 15:51:25 GMT, Adam Sotona wrote:
>> src/java.base/share/classes/jdk/internal/classfile/instruction/NewObjectInstruction.java
>> line 38:
>>
>>> 36: * of a {@link CodeModel}.
>>> 37: */
>>> 38: public sealed interface NewObjectInstruction extends Instruction
>>
>> Should we add a helper method on `CodeBuilder` that does the new + dup +
>> invoke special dance?
>
> That is great RFE for `CodeBuilder`, thanks.
collected and tracked in the mailing list
-
PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v15]
On Wed, 15 Feb 2023 14:24:18 GMT, Adam Sotona wrote:
>> src/java.base/share/classes/jdk/internal/classfile/instruction/CharacterRange.java
>> line 47:
>>
>>> 45: * {@return the start of the instruction range}
>>> 46: */
>>> 47: Label startScope();
>>
>> I noticed that this pattern of start/endScope is here, but also in
>> ExceptionCatch, LocalVariable and LocalVariableType. Consider using a common
>> interface for "instructions that are associated with a range".
>
> That is interesting RFE, thanks.
collected and tracked in the mailing list
-
PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v43]
On Fri, 3 Mar 2023 22:48:23 GMT, Paul Sandoz wrote:
>> Adam Sotona has updated the pull request incrementally with three additional
>> commits since the last revision:
>>
>> - fixed AccessFlags javadoc
>> - TransformImpl.FakeXyzTransform renamed to UnresolvedXyzTransform
>> - removed obsolete generic parameter from AbstractDirectBuilder
>
> src/java.base/share/classes/jdk/internal/classfile/impl/EntryMap.java line
> 176:
>
>> 174: }
>> 175:
>> 176: public static long nextPowerOfTwo( long x ) {
>
> If you like you can change the implementation to be:
>
> x = -1 >>> Long.numberOfLeadingZeros(x - 1);
> return x + 1;
>
> See `ConcurrentHashMap`.
`Long:numberOfLeadingZeros` implementation uses more method calls, conditional
statements and binary operations. I would prefer to stick with the current
implementation for its speed and simplicity.
-
PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v12]
On Mon, 6 Feb 2023 14:25:54 GMT, Adam Sotona wrote:
>> src/java.base/share/classes/jdk/internal/classfile/CodeBuilder.java line
>> 1371:
>>
>>> 1369: }
>>> 1370:
>>> 1371: default CodeBuilder tableswitch(Label defaultTarget,
>>> List cases) {
>>
>> `switch` seems the one instruction for which an high-level variant (like
>> `trying`) could be useful, as generating code for that can be quite painful.
>
> Nice RFE, thanks.
collected and tracked in the mailing list
-
PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v36]
On Thu, 2 Mar 2023 22:05:24 GMT, Paul Sandoz wrote:
>> Adam Sotona has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> StackMapFrameInfo extracted to top level from StackMapTableAttribute
>
> src/java.base/share/classes/jdk/internal/classfile/impl/AbstractInstruction.java
> line 156:
>
>> 154: @Override
>> 155: public String toString() {
>> 156: return String.format("Store[OP=%s, slot=%d]",
>> this.opcode(), slot());
>
> A suggestion. I believe the `toString` output for bound and unbound
> instructions should be identical and it can composed solely from methods on
> the public instruction interface. There are some differences e.g. `Increment`
> and `Inc` for the unbound and bound increment instruction respectively.
>
> If those assumptions are correct i recommend placing a static `toString`
> method on all unbound instructions that also have bound instructions,
> accepting the public instruction interface as an argument. Then the
> overridden `Object::toString` method defers to those. Thereby there is no
> duplication.
> (Alas we cannot override `toString` on the instruction interface itself).
resolved using common static format Strings
-
PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v51]
> This is root pull request with Classfile API implementation, tests and > benchmarks initial drop into JDK. > > Following pull requests consolidating JDK class files parsing, generating, > and transforming ([JDK-8294957](https://bugs.openjdk.org/browse/JDK-8294957)) > will chain to this one. > > Classfile API development is tracked at: > https://github.com/openjdk/jdk-sandbox/tree/classfile-api-branch > > Development branch of consolidated JDK class files parsing, generating, and > transforming is at: > https://github.com/openjdk/jdk-sandbox/tree/classfile-api-dev-branch > > Classfile API [JEP](https://bugs.openjdk.org/browse/JDK-8280389) and [online > API > documentation](https://htmlpreview.github.io/?https://raw.githubusercontent.com/openjdk/jdk-sandbox/classfile-api-javadoc-branch/doc/classfile-api/javadoc/java.base/jdk/internal/classfile/package-summary.html) > is also available. > > Please take you time to review this non-trivial JDK addition. > > Thank you, > Adam Adam Sotona has updated the pull request incrementally with one additional commit since the last revision: Shared `toString` formats for bound and unbound instructions - Changes: - all: https://git.openjdk.org/jdk/pull/10982/files - new: https://git.openjdk.org/jdk/pull/10982/files/a87d0096..2b1bd7f8 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=10982=50 - incr: https://webrevs.openjdk.org/?repo=jdk=10982=49-50 Stats: 59 lines in 1 file changed: 23 ins; 0 del; 36 mod Patch: https://git.openjdk.org/jdk/pull/10982.diff Fetch: git fetch https://git.openjdk.org/jdk pull/10982/head:pull/10982 PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v50]
> This is root pull request with Classfile API implementation, tests and > benchmarks initial drop into JDK. > > Following pull requests consolidating JDK class files parsing, generating, > and transforming ([JDK-8294957](https://bugs.openjdk.org/browse/JDK-8294957)) > will chain to this one. > > Classfile API development is tracked at: > https://github.com/openjdk/jdk-sandbox/tree/classfile-api-branch > > Development branch of consolidated JDK class files parsing, generating, and > transforming is at: > https://github.com/openjdk/jdk-sandbox/tree/classfile-api-dev-branch > > Classfile API [JEP](https://bugs.openjdk.org/browse/JDK-8280389) and [online > API > documentation](https://htmlpreview.github.io/?https://raw.githubusercontent.com/openjdk/jdk-sandbox/classfile-api-javadoc-branch/doc/classfile-api/javadoc/java.base/jdk/internal/classfile/package-summary.html) > is also available. > > Please take you time to review this non-trivial JDK addition. > > Thank you, > Adam Adam Sotona has updated the pull request incrementally with one additional commit since the last revision: generic implementation of ResolvedTransform - Changes: - all: https://git.openjdk.org/jdk/pull/10982/files - new: https://git.openjdk.org/jdk/pull/10982/files/6df1297e..a87d0096 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=10982=49 - incr: https://webrevs.openjdk.org/?repo=jdk=10982=48-49 Stats: 46 lines in 5 files changed: 1 ins; 18 del; 27 mod Patch: https://git.openjdk.org/jdk/pull/10982.diff Fetch: git fetch https://git.openjdk.org/jdk pull/10982/head:pull/10982 PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v36]
On Thu, 2 Mar 2023 23:49:24 GMT, Paul Sandoz wrote:
>> Adam Sotona has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> StackMapFrameInfo extracted to top level from StackMapTableAttribute
>
> src/java.base/share/classes/jdk/internal/classfile/CodeTransform.java line 91:
>
>> 89: @Override
>> 90: default ResolvedTransform resolve(CodeBuilder builder) {
>> 91: return new TransformImpl.CodeTransformImpl(e -> accept(builder,
>> e),
>
> Make `CodeTransformImpl` generic in the class file element, rename to say
> `ResolvedTransformImpl` and reuse for the other `XxxTransform`?
Good idea, I'll fix it, thanks.
-
PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v36]
On Fri, 3 Mar 2023 16:33:49 GMT, Adam Sotona wrote:
>> src/java.base/share/classes/jdk/internal/classfile/impl/TransformImpl.java
>> line 63:
>>
>>> 61: private static final Runnable NOTHING = () -> { };
>>> 62:
>>> 63: interface FakeClassTransform extends ClassTransform {
>>
>> Rename to `UnresolvedXxxTransform`? I think that is a better name, since it
>> could appear in stack traces. Like with `XxxTransformImpl` it may be
>> possible to share across all implementations by mixing in?
>
> Renamed to `UnresolvedXyzTransform`, thanks.
> I'll consider conversion to generic form in a next step.
Unfortunately application of common generic `UnresolvedTransform` does
not work here.
-
PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v36]
On Mon, 6 Mar 2023 17:45:27 GMT, Paul Sandoz wrote: >> I see what you mean. `transforming` was selected to represent the >> continuation of the code building process, similar to `trying` and >> `catching`. While `transform` may imply that the actual code builder gets >> transformed into something else. For example `MethodModel::transformCode` >> takes `CodeModel`, applies `CodeTransform` and after that the code of the >> actual method is finished. >> What about `transformBlock(BlockCodeBuilder, CodeTransform)`? > > I am unsure how you might use `BlockCodeBuilder`. If the current signature is > not changed then `transformFromHandler` seems reasonable (since its the > handler that pushes elements into its given builder). > > The other `transform` is implicitly "transform model". I'm sorry my answer was inaccurate, what I'm proposing is `transformBlock(Consumer, CodeTransform)` or `transformedBlock(Consumer, CodeTransform)`. `BlockCodeBuilder` is just an extension of `CodeBuilder` with ability to break block execution. Block is a sub-unit of code, so I expect it would be a logical extension of the actual `block(Consumer`. I'll move this discussion to the mailing list to make common decision about API change. Thanks. - PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v36]
The message from this sender included one or more files which could not be scanned for virus detection; do not open these files unless you are certain of the sender's intent. -- On Mon, 6 Mar 2023 17:15:25 GMT, Paul Sandoz wrote: >> The `CodeBuilder::transforming` solves a bit different use cases than all >> the other transform. >> It is designed to be able to use code transformations on a code building >> handler within a single pass. >> Main reason is support of features like `StackTracker` in a form of code >> transformation. `StackTracker` (or any other similar tool requiring to >> monitor or affect code building) is passed as a transformation of a code >> fragment, while it can immediately serve as a source of information >> necessary to generate follow-up bytecode of the same method (in the same >> pass). >> Example of such use case is here: >> https://github.com/openjdk/jdk/blob/0e43af667ba6c6bda61461c260688bc46d3f3474/src/java.base/share/classes/jdk/internal/classfile/components/CodeStackTracker.java#L49 >> >> These code generation/transformation cases must be handled in a single pass >> and `CodeBuilder::transforming` method has no similar peer in any other >> (method, field or class) builder, because it is not necessary. > > The use-case seems fine to me (and that it only makes sense for building > code). I still think it's a "transform", but with a different source. Subtly > changing the name makes it seem different and fundamentally it is not AFAICT. > If there is a separate name I think it should reflect the difference in > source input to the transformation, rather than differentiate via the present > participle. I see what you mean. `transforming` was selected to represent the continuation of the code building process, similar to `trying` and `catching`. While `transform` may imply that the actual code builder gets transformed into something else. For example `MethodModel::transformCode` takes `CodeModel`, applies `CodeTransform` and after that the code of the actual method is finished. What about `transformBlock(BlockCodeBuilder, CodeTransform)`? - PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v36]
On Thu, 2 Mar 2023 19:56:08 GMT, Paul Sandoz wrote: >> Adam Sotona has updated the pull request incrementally with one additional >> commit since the last revision: >> >> StackMapFrameInfo extracted to top level from StackMapTableAttribute > > src/java.base/share/classes/jdk/internal/classfile/components/snippet-files/PackageSnippets.java > line 149: > >> 147: var instrumentorCodeMap = instrumentor.methods().stream() >> 148: >> .filter(instrumentedMethodsFilter) >> 149: >> .collect(Collectors.toMap(mm -> mm.methodName().stringValue() + >> mm.methodType().stringValue(), mm -> mm.code().orElse(null))); > > Should we be filtering out abstract methods before the `collect` and/or > replace with: > > mm -> mm.code().orElseThrow() > > ? Abstract methods should not be selected for instrumentation, so `orElseThrow()` is a good idea. Fixed, thanks. > src/java.base/share/classes/jdk/internal/classfile/components/snippet-files/PackageSnippets.java > line 171: > >> 169: >> 170: //store stacked method >> parameters into locals >> 171: var storeStack = new >> LinkedList(); > > FWIW you can use an ArrayDeque + push + forEach, in the spirit of > highlighting Deque over LinkedList for stack-based usage (since this is an > example with visibility). fixed, thanks. - PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v49]
> This is root pull request with Classfile API implementation, tests and > benchmarks initial drop into JDK. > > Following pull requests consolidating JDK class files parsing, generating, > and transforming ([JDK-8294957](https://bugs.openjdk.org/browse/JDK-8294957)) > will chain to this one. > > Classfile API development is tracked at: > https://github.com/openjdk/jdk-sandbox/tree/classfile-api-branch > > Development branch of consolidated JDK class files parsing, generating, and > transforming is at: > https://github.com/openjdk/jdk-sandbox/tree/classfile-api-dev-branch > > Classfile API [JEP](https://bugs.openjdk.org/browse/JDK-8280389) and [online > API > documentation](https://htmlpreview.github.io/?https://raw.githubusercontent.com/openjdk/jdk-sandbox/classfile-api-javadoc-branch/doc/classfile-api/javadoc/java.base/jdk/internal/classfile/package-summary.html) > is also available. > > Please take you time to review this non-trivial JDK addition. > > Thank you, > Adam Adam Sotona has updated the pull request incrementally with one additional commit since the last revision: snippets and tests synced with jdk.jfr class instrumentation source code - Changes: - all: https://git.openjdk.org/jdk/pull/10982/files - new: https://git.openjdk.org/jdk/pull/10982/files/0e43af66..6df1297e Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=10982=48 - incr: https://webrevs.openjdk.org/?repo=jdk=10982=47-48 Stats: 13 lines in 2 files changed: 1 ins; 3 del; 9 mod Patch: https://git.openjdk.org/jdk/pull/10982.diff Fetch: git fetch https://git.openjdk.org/jdk pull/10982/head:pull/10982 PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v36]
On Thu, 2 Mar 2023 23:28:23 GMT, Paul Sandoz wrote:
>> Adam Sotona has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> StackMapFrameInfo extracted to top level from StackMapTableAttribute
>
> src/java.base/share/classes/jdk/internal/classfile/CodeBuilder.java line 165:
>
>> 163: * @return this builder
>> 164: */
>> 165: default CodeBuilder transforming(CodeTransform transform,
>> Consumer handler) {
>
> The functionality of this method, `transforming`, and
> `ClassfileBuilder::transform`, are in effect equivalent in their
> transforming: adding the results of transformed code to the builder. They
> differ in the source of code elements.
>
> The latter's behaviour can be implemented using the former, with a consumer
> that passes all elements of a code model to the builder e.g. `builder ->
> model.forEach(builder::with)`.
>
> The difference in naming initially confused me. To me this suggests the
> method names should be the same? (perhaps with the transformer being
> consistently the last argument?).
The `CodeBuilder::transforming` solves a bit different use cases than all the
other transform.
It is designed to be able to use code transformations on a code building
handler within a single pass.
Main reason is support of features like `StackTracker` in a form of code
transformation. `StackTracker` (or any other similar tool requiring to monitor
or affect code building) is passed as a transformation of a code fragment,
while it can immediately serve as a source of information necessary to generate
follow-up bytecode of the same method (in the same pass).
Example of such use case is here:
https://github.com/openjdk/jdk/blob/0e43af667ba6c6bda61461c260688bc46d3f3474/src/java.base/share/classes/jdk/internal/classfile/components/CodeStackTracker.java#L49
These code generation/transformation cases must be handled in a single pass and
`CodeBuilder::transforming` method has no similar peer in any other (method,
field or class) builder, because it is not necessary.
-
PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v20]
On Wed, 1 Mar 2023 10:05:27 GMT, Adam Sotona wrote: >> src/java.base/share/classes/jdk/internal/classfile/impl/ChainedFieldBuilder.java >> line 48: >> >>> 46: this.consumer = consumer; >>> 47: FieldBuilder b = downstream; >>> 48: while (b instanceof ChainedFieldBuilder cb) >> >> I'm probably missing something - but if `b` is another chained builder, >> instead of using a loop, can't we just skip to its `terminal` field? Also, >> the `downstream` field seems unused. (same considerations apply for >> `ChainedMethodBuilder` and `ChainedClassBuilder`). >> >> I note that `NonTerminalCodeBuilder` seems to be already doing what I >> suggest here (e.g. skip to `terminal`). > > I would have to test possible side-effects of the proposed shortcut to give > you answer. > Thanks for pointing it out. Proposed shortcut seems to be OK, I've fixed it in `ChainedFieldBuilder`, `ChainedMethodBuilder` and `ChainedClassBuilder`. And removed unused `ChainedFieldBuilder.downstream` field. - PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v48]
> This is root pull request with Classfile API implementation, tests and > benchmarks initial drop into JDK. > > Following pull requests consolidating JDK class files parsing, generating, > and transforming ([JDK-8294957](https://bugs.openjdk.org/browse/JDK-8294957)) > will chain to this one. > > Classfile API development is tracked at: > https://github.com/openjdk/jdk-sandbox/tree/classfile-api-branch > > Development branch of consolidated JDK class files parsing, generating, and > transforming is at: > https://github.com/openjdk/jdk-sandbox/tree/classfile-api-dev-branch > > Classfile API [JEP](https://bugs.openjdk.org/browse/JDK-8280389) and [online > API > documentation](https://htmlpreview.github.io/?https://raw.githubusercontent.com/openjdk/jdk-sandbox/classfile-api-javadoc-branch/doc/classfile-api/javadoc/java.base/jdk/internal/classfile/package-summary.html) > is also available. > > Please take you time to review this non-trivial JDK addition. > > Thank you, > Adam Adam Sotona has updated the pull request incrementally with one additional commit since the last revision: simplified initialization of terminal builder in chained builders - Changes: - all: https://git.openjdk.org/jdk/pull/10982/files - new: https://git.openjdk.org/jdk/pull/10982/files/46fffe40..0e43af66 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=10982=47 - incr: https://webrevs.openjdk.org/?repo=jdk=10982=46-47 Stats: 14 lines in 3 files changed: 0 ins; 2 del; 12 mod Patch: https://git.openjdk.org/jdk/pull/10982.diff Fetch: git fetch https://git.openjdk.org/jdk pull/10982/head:pull/10982 PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v43]
On Fri, 3 Mar 2023 22:35:48 GMT, Paul Sandoz wrote:
>> Adam Sotona has updated the pull request incrementally with three additional
>> commits since the last revision:
>>
>> - fixed AccessFlags javadoc
>> - TransformImpl.FakeXyzTransform renamed to UnresolvedXyzTransform
>> - removed obsolete generic parameter from AbstractDirectBuilder
>
> src/java.base/share/classes/jdk/internal/classfile/impl/CodeImpl.java line 52:
>
>> 50: static final Instruction[] SINGLETON_INSTRUCTIONS = new
>> Instruction[256];
>> 51:
>> 52: static {
>
> Can we loop through all `Opcode` values filter for `sizeIfFixed == 1` and
> switch on the kind? If so that would avoid the need for explicit lists and
> simplify the code.
Yes, that is good idea.
Fixed, thanks.
-
PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v47]
> This is root pull request with Classfile API implementation, tests and > benchmarks initial drop into JDK. > > Following pull requests consolidating JDK class files parsing, generating, > and transforming ([JDK-8294957](https://bugs.openjdk.org/browse/JDK-8294957)) > will chain to this one. > > Classfile API development is tracked at: > https://github.com/openjdk/jdk-sandbox/tree/classfile-api-branch > > Development branch of consolidated JDK class files parsing, generating, and > transforming is at: > https://github.com/openjdk/jdk-sandbox/tree/classfile-api-dev-branch > > Classfile API [JEP](https://bugs.openjdk.org/browse/JDK-8280389) and [online > API > documentation](https://htmlpreview.github.io/?https://raw.githubusercontent.com/openjdk/jdk-sandbox/classfile-api-javadoc-branch/doc/classfile-api/javadoc/java.base/jdk/internal/classfile/package-summary.html) > is also available. > > Please take you time to review this non-trivial JDK addition. > > Thank you, > Adam Adam Sotona has updated the pull request incrementally with two additional commits since the last revision: - simplified CodeImpl.SINGLETON_INSTRUCTIONS initialization - fixed handling of array descriptors by Util::toInternalName - Changes: - all: https://git.openjdk.org/jdk/pull/10982/files - new: https://git.openjdk.org/jdk/pull/10982/files/61ff1c7c..46fffe40 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=10982=46 - incr: https://webrevs.openjdk.org/?repo=jdk=10982=45-46 Stats: 60 lines in 7 files changed: 0 ins; 34 del; 26 mod Patch: https://git.openjdk.org/jdk/pull/10982.diff Fetch: git fetch https://git.openjdk.org/jdk pull/10982/head:pull/10982 PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v43]
The message from this sender included one or more files
which could not be scanned for virus detection; do not
open these files unless you are certain of the sender's intent.
--
On Fri, 3 Mar 2023 23:12:17 GMT, Paul Sandoz wrote:
>> Adam Sotona has updated the pull request incrementally with three additional
>> commits since the last revision:
>>
>> - fixed AccessFlags javadoc
>> - TransformImpl.FakeXyzTransform renamed to UnresolvedXyzTransform
>> - removed obsolete generic parameter from AbstractDirectBuilder
>
> src/java.base/share/classes/jdk/internal/classfile/impl/Util.java line 161:
>
>> 159: var desc = cd.descriptorString();
>> 160: return switch (desc.charAt(0)) {
>> 161: case '[' -> desc;
>
> Is this correct? Arrays don't have an internal name.
It is a workaround to get CP class entry name string from descriptor, so the
`toInternalName` is not exact.
I'll handle the array descriptors separately and let this method to handle real
class internal names only.
-
PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v43]
On Fri, 3 Mar 2023 21:44:24 GMT, Paul Sandoz wrote:
>> Adam Sotona has updated the pull request incrementally with three additional
>> commits since the last revision:
>>
>> - fixed AccessFlags javadoc
>> - TransformImpl.FakeXyzTransform renamed to UnresolvedXyzTransform
>> - removed obsolete generic parameter from AbstractDirectBuilder
>
> src/java.base/share/classes/jdk/internal/classfile/impl/AbstractInstruction.java
> line 683:
>
>> 681: public UnboundInstruction(Opcode op, int size) {
>> 682: super(op, size);
>> 683: }
>
> Unused?
fixed, thanks.
> src/java.base/share/classes/jdk/internal/classfile/impl/AnnotationReader.java
> line 99:
>
>> 97: }
>> 98:
>> 99: public static List>
>> readParameterAnnotations(ClassReader classReader, int p, boolean isVisible) {
>
> Parameter `isVisible` is unused, but method is called with true/false values.
fixed, thanks.
> src/java.base/share/classes/jdk/internal/classfile/impl/AttributeHolder.java
> line 76:
>
>> 74: }
>> 75:
>> 76: List> attributes() {
>
> Unused
fixed, thanks.
> src/java.base/share/classes/jdk/internal/classfile/impl/BufferedMethodBuilder.java
> line 54:
>
>> 52: private final List elements = new ArrayList<>();
>> 53: private final SplitConstantPool constantPool;
>> 54: private final ClassEntry thisClass;
>
> Unused. Can be removed as can the associated constructor parameter.
fixed, thanks.
-
PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v46]
> This is root pull request with Classfile API implementation, tests and > benchmarks initial drop into JDK. > > Following pull requests consolidating JDK class files parsing, generating, > and transforming ([JDK-8294957](https://bugs.openjdk.org/browse/JDK-8294957)) > will chain to this one. > > Classfile API development is tracked at: > https://github.com/openjdk/jdk-sandbox/tree/classfile-api-branch > > Development branch of consolidated JDK class files parsing, generating, and > transforming is at: > https://github.com/openjdk/jdk-sandbox/tree/classfile-api-dev-branch > > Classfile API [JEP](https://bugs.openjdk.org/browse/JDK-8280389) and [online > API > documentation](https://htmlpreview.github.io/?https://raw.githubusercontent.com/openjdk/jdk-sandbox/classfile-api-javadoc-branch/doc/classfile-api/javadoc/java.base/jdk/internal/classfile/package-summary.html) > is also available. > > Please take you time to review this non-trivial JDK addition. > > Thank you, > Adam Adam Sotona has updated the pull request incrementally with one additional commit since the last revision: removed unused methods, fields and parameters added missing overrides fixed pointless operations and possible null pointer dereferences - Changes: - all: https://git.openjdk.org/jdk/pull/10982/files - new: https://git.openjdk.org/jdk/pull/10982/files/a40842c4..61ff1c7c Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=10982=45 - incr: https://webrevs.openjdk.org/?repo=jdk=10982=44-45 Stats: 297 lines in 35 files changed: 58 ins; 121 del; 118 mod Patch: https://git.openjdk.org/jdk/pull/10982.diff Fetch: git fetch https://git.openjdk.org/jdk pull/10982/head:pull/10982 PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v43]
On Fri, 3 Mar 2023 21:56:39 GMT, Paul Sandoz wrote: >> Adam Sotona has updated the pull request incrementally with three additional >> commits since the last revision: >> >> - fixed AccessFlags javadoc >> - TransformImpl.FakeXyzTransform renamed to UnresolvedXyzTransform >> - removed obsolete generic parameter from AbstractDirectBuilder > > src/java.base/share/classes/jdk/internal/classfile/impl/AttributeHolder.java > line 73: > >> 71: if (a.attributeMapper() == am) >> 72: iterator.remove(); >> 73: } > > Suggestion: > > attributes.removeIf(a -> a.attributeMappter() == am); > > But presumably use an inner class instead. I can understand because of that > if you want to keep the existing code instead. It seems to be OK to use lambda here (not on critical JDK bootstrap path), thanks. - PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v45]
The message from this sender included one or more files which could not be scanned for virus detection; do not open these files unless you are certain of the sender's intent. -- > This is root pull request with Classfile API implementation, tests and > benchmarks initial drop into JDK. > > Following pull requests consolidating JDK class files parsing, generating, > and transforming ([JDK-8294957](https://bugs.openjdk.org/browse/JDK-8294957)) > will chain to this one. > > Classfile API development is tracked at: > https://github.com/openjdk/jdk-sandbox/tree/classfile-api-branch > > Development branch of consolidated JDK class files parsing, generating, and > transforming is at: > https://github.com/openjdk/jdk-sandbox/tree/classfile-api-dev-branch > > Classfile API [JEP](https://bugs.openjdk.org/browse/JDK-8280389) and [online > API > documentation](https://htmlpreview.github.io/?https://raw.githubusercontent.com/openjdk/jdk-sandbox/classfile-api-javadoc-branch/doc/classfile-api/javadoc/java.base/jdk/internal/classfile/package-summary.html) > is also available. > > Please take you time to review this non-trivial JDK addition. > > Thank you, > Adam Adam Sotona has updated the pull request incrementally with one additional commit since the last revision: Update src/java.base/share/classes/jdk/internal/classfile/impl/AttributeHolder.java Co-authored-by: Paul Sandoz - Changes: - all: https://git.openjdk.org/jdk/pull/10982/files - new: https://git.openjdk.org/jdk/pull/10982/files/b44f47ba..a40842c4 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=10982=44 - incr: https://webrevs.openjdk.org/?repo=jdk=10982=43-44 Stats: 6 lines in 1 file changed: 0 ins; 5 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/10982.diff Fetch: git fetch https://git.openjdk.org/jdk pull/10982/head:pull/10982 PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v44]
> This is root pull request with Classfile API implementation, tests and > benchmarks initial drop into JDK. > > Following pull requests consolidating JDK class files parsing, generating, > and transforming ([JDK-8294957](https://bugs.openjdk.org/browse/JDK-8294957)) > will chain to this one. > > Classfile API development is tracked at: > https://github.com/openjdk/jdk-sandbox/tree/classfile-api-branch > > Development branch of consolidated JDK class files parsing, generating, and > transforming is at: > https://github.com/openjdk/jdk-sandbox/tree/classfile-api-dev-branch > > Classfile API [JEP](https://bugs.openjdk.org/browse/JDK-8280389) and [online > API > documentation](https://htmlpreview.github.io/?https://raw.githubusercontent.com/openjdk/jdk-sandbox/classfile-api-javadoc-branch/doc/classfile-api/javadoc/java.base/jdk/internal/classfile/package-summary.html) > is also available. > > Please take you time to review this non-trivial JDK addition. > > Thank you, > Adam Adam Sotona has updated the pull request incrementally with four additional commits since the last revision: - Update src/java.base/share/classes/jdk/internal/classfile/impl/Util.java Co-authored-by: Paul Sandoz - Update src/java.base/share/classes/jdk/internal/classfile/impl/ClassPrinterImpl.java Co-authored-by: Paul Sandoz - Update src/java.base/share/classes/jdk/internal/classfile/impl/BootstrapMethodEntryImpl.java Co-authored-by: Paul Sandoz - Update src/java.base/share/classes/jdk/internal/classfile/impl/BootstrapMethodEntryImpl.java Co-authored-by: Paul Sandoz - Changes: - all: https://git.openjdk.org/jdk/pull/10982/files - new: https://git.openjdk.org/jdk/pull/10982/files/173dc1e7..b44f47ba Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=10982=43 - incr: https://webrevs.openjdk.org/?repo=jdk=10982=42-43 Stats: 19 lines in 3 files changed: 0 ins; 12 del; 7 mod Patch: https://git.openjdk.org/jdk/pull/10982.diff Fetch: git fetch https://git.openjdk.org/jdk pull/10982/head:pull/10982 PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v43]
> This is root pull request with Classfile API implementation, tests and > benchmarks initial drop into JDK. > > Following pull requests consolidating JDK class files parsing, generating, > and transforming ([JDK-8294957](https://bugs.openjdk.org/browse/JDK-8294957)) > will chain to this one. > > Classfile API development is tracked at: > https://github.com/openjdk/jdk-sandbox/tree/classfile-api-branch > > Development branch of consolidated JDK class files parsing, generating, and > transforming is at: > https://github.com/openjdk/jdk-sandbox/tree/classfile-api-dev-branch > > Classfile API [JEP](https://bugs.openjdk.org/browse/JDK-8280389) and [online > API > documentation](https://htmlpreview.github.io/?https://raw.githubusercontent.com/openjdk/jdk-sandbox/classfile-api-javadoc-branch/doc/classfile-api/javadoc/java.base/jdk/internal/classfile/package-summary.html) > is also available. > > Please take you time to review this non-trivial JDK addition. > > Thank you, > Adam Adam Sotona has updated the pull request incrementally with three additional commits since the last revision: - fixed AccessFlags javadoc - TransformImpl.FakeXyzTransform renamed to UnresolvedXyzTransform - removed obsolete generic parameter from AbstractDirectBuilder - Changes: - all: https://git.openjdk.org/jdk/pull/10982/files - new: https://git.openjdk.org/jdk/pull/10982/files/8561d134..173dc1e7 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=10982=42 - incr: https://webrevs.openjdk.org/?repo=jdk=10982=41-42 Stats: 24 lines in 7 files changed: 0 ins; 0 del; 24 mod Patch: https://git.openjdk.org/jdk/pull/10982.diff Fetch: git fetch https://git.openjdk.org/jdk pull/10982/head:pull/10982 PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v36]
On Fri, 3 Mar 2023 00:57:35 GMT, Paul Sandoz wrote:
>> Adam Sotona has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> StackMapFrameInfo extracted to top level from StackMapTableAttribute
>
> src/java.base/share/classes/jdk/internal/classfile/impl/AbstractDirectBuilder.java
> line 34:
>
>> 32: * AbstractDirectBuilder
>> 33: */
>> 34: public class AbstractDirectBuilder {
>
> Type variable `B` is unused.
fixed, thanks.
> src/java.base/share/classes/jdk/internal/classfile/impl/TransformImpl.java
> line 63:
>
>> 61: private static final Runnable NOTHING = () -> { };
>> 62:
>> 63: interface FakeClassTransform extends ClassTransform {
>
> Rename to `UnresolvedXxxTransform`? I think that is a better name, since it
> could appear in stack traces. Like with `XxxTransformImpl` it may be possible
> to share across all implementations by mixing in?
Renamed to `UnresolvedXyzTransform`, thanks.
I'll consider conversion to generic form in a next step.
-
PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v41]
On Fri, 3 Mar 2023 14:14:55 GMT, Jaikiran Pai wrote:
>> Adam Sotona has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Signature.TypeArg does not inherit from Signature
>
> src/java.base/share/classes/jdk/internal/classfile/AccessFlags.java line 54:
>
>> 52: * {@return whether the specified flag is present} The specified
>> flag
>> 53: * should be a valid flag for the classfile location associated with
>> this
>> 54: * element.
>
> Hello Adam, the way this is worded, it feels like if the flag isn't valid
> then this method would raise an exception. Looking at the implementation,
> that doesn't look like the case. Should it be reworded to say it returns
> false in such cases?
>
> On a related note, since this JEP is for introducing this API for internal
> use only, would you prefer if the javadoc text isn't reviewed to this level
> of detail?
I'll fix it, thanks for the review.
Manageable amount of javadoc comments is OK, preferably with directly proposed
patches :)
-
PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v36]
On Thu, 2 Mar 2023 20:54:39 GMT, Paul Sandoz wrote:
>> Adam Sotona has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> StackMapFrameInfo extracted to top level from StackMapTableAttribute
>
> src/java.base/share/classes/jdk/internal/classfile/instruction/LoadInstruction.java
> line 66:
>
>> 64: * @param slot the local varaible slot to load from
>> 65: */
>> 66: static LoadInstruction of(Opcode op, int slot) {
>
> Should you validate that `slot` is compatible with `op`? (same for the
> StoreInstruction?)
There are three aspects of this kind of validation:
1. similar to `arrayType` there was an architectural decision to less validate
so to perform faster
2. static instruction factory does not have the information necessary to
validate local variable slot, it could theoretically be validated in the
`CodeBuilder`
3. incompatible slot would most probably prevent stack map generation and such
code will fail in the later phase, or at least when verification is explicitly
called on the generated code
-
PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v36]
On Thu, 2 Mar 2023 20:37:37 GMT, Paul Sandoz wrote:
>> Adam Sotona has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> StackMapFrameInfo extracted to top level from StackMapTableAttribute
>
> src/java.base/share/classes/jdk/internal/classfile/instruction/NewMultiArrayInstruction.java
> line 60:
>
>> 58: static NewMultiArrayInstruction of(ClassEntry arrayTypeEntry,
>> 59:int dimensions) {
>> 60: return new
>> AbstractInstruction.UnboundNewMultidimensionalArrayInstruction(arrayTypeEntry,
>> dimensions);
>
> Should we validate that the dimensionality of `arrayType` is greater than or
> equal to `dimensions`?
Architectural decision is to do not provide much of validation in favour of
performance, however it might be re-visited in cases like this. Please raise
the validation topic at classfile-api-dev at openjdk.org, thanks.
> src/java.base/share/classes/jdk/internal/classfile/instruction/NewObjectInstruction.java
> line 38:
>
>> 36: * of a {@link CodeModel}.
>> 37: */
>> 38: public sealed interface NewObjectInstruction extends Instruction
>
> Should we add a helper method on `CodeBuilder` that does the new + dup +
> invoke special dance?
That is great RFE for `CodeBuilder`, thanks.
-
PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v36]
On Fri, 3 Mar 2023 14:53:19 GMT, Adam Sotona wrote:
>> src/java.base/share/classes/jdk/internal/classfile/instruction/CharacterRange.java
>> line 42:
>>
>>> 40: * the setting of the {@link Classfile.Option#processDebug(boolean)}
>>> option.
>>> 41: */
>>> 42: public sealed interface CharacterRange extends PseudoInstruction
>>
>> This and some other instructions with unbounded implementations do not have
>> static `of` factory methods. Is that intended?
>
> It seems to be incomplete. I'll add factory methods to `CharacterRange`,
> `LineNumber`, `LocalVariable` and `LocalVariableType`.
> Thanks for pointing it out.
Fixed, thanks.
-
PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v36]
On Thu, 2 Mar 2023 19:31:50 GMT, Paul Sandoz wrote:
>> Adam Sotona has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> StackMapFrameInfo extracted to top level from StackMapTableAttribute
>
> src/java.base/share/classes/jdk/internal/classfile/components/CodeRelabeler.java
> line 98:
>
>> 96: @Override
>> 97: public void accept(CodeBuilder cob, CodeElement coe) {
>> 98: switch (coe) {
>
> Missing case for`CharacterRange` instruction? I am not sure it makes any
> sense, if so perhaps add a comment as to why.
Fixed, thanks.
-
PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v42]
> This is root pull request with Classfile API implementation, tests and > benchmarks initial drop into JDK. > > Following pull requests consolidating JDK class files parsing, generating, > and transforming ([JDK-8294957](https://bugs.openjdk.org/browse/JDK-8294957)) > will chain to this one. > > Classfile API development is tracked at: > https://github.com/openjdk/jdk-sandbox/tree/classfile-api-branch > > Development branch of consolidated JDK class files parsing, generating, and > transforming is at: > https://github.com/openjdk/jdk-sandbox/tree/classfile-api-dev-branch > > Classfile API [JEP](https://bugs.openjdk.org/browse/JDK-8280389) and [online > API > documentation](https://htmlpreview.github.io/?https://raw.githubusercontent.com/openjdk/jdk-sandbox/classfile-api-javadoc-branch/doc/classfile-api/javadoc/java.base/jdk/internal/classfile/package-summary.html) > is also available. > > Please take you time to review this non-trivial JDK addition. > > Thank you, > Adam Adam Sotona has updated the pull request incrementally with two additional commits since the last revision: - factory methods to CharacterRange, LineNumber, LocalVariable and LocalVariableType - CodeRelabeler fix - Changes: - all: https://git.openjdk.org/jdk/pull/10982/files - new: https://git.openjdk.org/jdk/pull/10982/files/b03f15c1..8561d134 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=10982=41 - incr: https://webrevs.openjdk.org/?repo=jdk=10982=40-41 Stats: 107 lines in 6 files changed: 100 ins; 3 del; 4 mod Patch: https://git.openjdk.org/jdk/pull/10982.diff Fetch: git fetch https://git.openjdk.org/jdk pull/10982/head:pull/10982 PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v36]
On Thu, 2 Mar 2023 19:27:58 GMT, Paul Sandoz wrote:
>> Adam Sotona has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> StackMapFrameInfo extracted to top level from StackMapTableAttribute
>
> src/java.base/share/classes/jdk/internal/classfile/instruction/CharacterRange.java
> line 42:
>
>> 40: * the setting of the {@link Classfile.Option#processDebug(boolean)}
>> option.
>> 41: */
>> 42: public sealed interface CharacterRange extends PseudoInstruction
>
> This and some other instructions with unbounded implementations do not have
> static `of` factory methods. Is that intended?
It seems to be incomplete. I'll add factory methods to `CharacterRange`,
`LineNumber`, `LocalVariable` and `LocalVariableType`.
Thanks for pointing it out.
-
PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v41]
> This is root pull request with Classfile API implementation, tests and > benchmarks initial drop into JDK. > > Following pull requests consolidating JDK class files parsing, generating, > and transforming ([JDK-8294957](https://bugs.openjdk.org/browse/JDK-8294957)) > will chain to this one. > > Classfile API development is tracked at: > https://github.com/openjdk/jdk-sandbox/tree/classfile-api-branch > > Development branch of consolidated JDK class files parsing, generating, and > transforming is at: > https://github.com/openjdk/jdk-sandbox/tree/classfile-api-dev-branch > > Classfile API [JEP](https://bugs.openjdk.org/browse/JDK-8280389) and [online > API > documentation](https://htmlpreview.github.io/?https://raw.githubusercontent.com/openjdk/jdk-sandbox/classfile-api-javadoc-branch/doc/classfile-api/javadoc/java.base/jdk/internal/classfile/package-summary.html) > is also available. > > Please take you time to review this non-trivial JDK addition. > > Thank you, > Adam Adam Sotona has updated the pull request incrementally with one additional commit since the last revision: Signature.TypeArg does not inherit from Signature - Changes: - all: https://git.openjdk.org/jdk/pull/10982/files - new: https://git.openjdk.org/jdk/pull/10982/files/79bfd4c5..b03f15c1 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=10982=40 - incr: https://webrevs.openjdk.org/?repo=jdk=10982=39-40 Stats: 78 lines in 5 files changed: 37 ins; 8 del; 33 mod Patch: https://git.openjdk.org/jdk/pull/10982.diff Fetch: git fetch https://git.openjdk.org/jdk pull/10982/head:pull/10982 PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v40]
> This is root pull request with Classfile API implementation, tests and > benchmarks initial drop into JDK. > > Following pull requests consolidating JDK class files parsing, generating, > and transforming ([JDK-8294957](https://bugs.openjdk.org/browse/JDK-8294957)) > will chain to this one. > > Classfile API development is tracked at: > https://github.com/openjdk/jdk-sandbox/tree/classfile-api-branch > > Development branch of consolidated JDK class files parsing, generating, and > transforming is at: > https://github.com/openjdk/jdk-sandbox/tree/classfile-api-dev-branch > > Classfile API [JEP](https://bugs.openjdk.org/browse/JDK-8280389) and [online > API > documentation](https://htmlpreview.github.io/?https://raw.githubusercontent.com/openjdk/jdk-sandbox/classfile-api-javadoc-branch/doc/classfile-api/javadoc/java.base/jdk/internal/classfile/package-summary.html) > is also available. > > Please take you time to review this non-trivial JDK addition. > > Thank you, > Adam Adam Sotona has updated the pull request incrementally with one additional commit since the last revision: Update src/java.base/share/classes/jdk/internal/classfile/TypeKind.java Co-authored-by: Paul Sandoz - Changes: - all: https://git.openjdk.org/jdk/pull/10982/files - new: https://git.openjdk.org/jdk/pull/10982/files/e5fd5764..79bfd4c5 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=10982=39 - incr: https://webrevs.openjdk.org/?repo=jdk=10982=38-39 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/10982.diff Fetch: git fetch https://git.openjdk.org/jdk pull/10982/head:pull/10982 PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v39]
> This is root pull request with Classfile API implementation, tests and > benchmarks initial drop into JDK. > > Following pull requests consolidating JDK class files parsing, generating, > and transforming ([JDK-8294957](https://bugs.openjdk.org/browse/JDK-8294957)) > will chain to this one. > > Classfile API development is tracked at: > https://github.com/openjdk/jdk-sandbox/tree/classfile-api-branch > > Development branch of consolidated JDK class files parsing, generating, and > transforming is at: > https://github.com/openjdk/jdk-sandbox/tree/classfile-api-dev-branch > > Classfile API [JEP](https://bugs.openjdk.org/browse/JDK-8280389) and [online > API > documentation](https://htmlpreview.github.io/?https://raw.githubusercontent.com/openjdk/jdk-sandbox/classfile-api-javadoc-branch/doc/classfile-api/javadoc/java.base/jdk/internal/classfile/package-summary.html) > is also available. > > Please take you time to review this non-trivial JDK addition. > > Thank you, > Adam Adam Sotona has updated the pull request incrementally with one additional commit since the last revision: Update src/java.base/share/classes/jdk/internal/classfile/impl/NonterminalCodeBuilder.java Co-authored-by: Paul Sandoz - Changes: - all: https://git.openjdk.org/jdk/pull/10982/files - new: https://git.openjdk.org/jdk/pull/10982/files/0bd5281f..e5fd5764 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=10982=38 - incr: https://webrevs.openjdk.org/?repo=jdk=10982=37-38 Stats: 2 lines in 1 file changed: 0 ins; 1 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/10982.diff Fetch: git fetch https://git.openjdk.org/jdk pull/10982/head:pull/10982 PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v37]
> This is root pull request with Classfile API implementation, tests and > benchmarks initial drop into JDK. > > Following pull requests consolidating JDK class files parsing, generating, > and transforming ([JDK-8294957](https://bugs.openjdk.org/browse/JDK-8294957)) > will chain to this one. > > Classfile API development is tracked at: > https://github.com/openjdk/jdk-sandbox/tree/classfile-api-branch > > Development branch of consolidated JDK class files parsing, generating, and > transforming is at: > https://github.com/openjdk/jdk-sandbox/tree/classfile-api-dev-branch > > Classfile API [JEP](https://bugs.openjdk.org/browse/JDK-8280389) and [online > API > documentation](https://htmlpreview.github.io/?https://raw.githubusercontent.com/openjdk/jdk-sandbox/classfile-api-javadoc-branch/doc/classfile-api/javadoc/java.base/jdk/internal/classfile/package-summary.html) > is also available. > > Please take you time to review this non-trivial JDK addition. > > Thank you, > Adam Adam Sotona has updated the pull request incrementally with three additional commits since the last revision: - Update src/java.base/share/classes/jdk/internal/classfile/instruction/ConstantInstruction.java Co-authored-by: Paul Sandoz - Update src/java.base/share/classes/jdk/internal/classfile/instruction/ConstantInstruction.java Co-authored-by: Paul Sandoz - Update src/java.base/share/classes/jdk/internal/classfile/snippet-files/PackageSnippets.java Co-authored-by: Paul Sandoz - Changes: - all: https://git.openjdk.org/jdk/pull/10982/files - new: https://git.openjdk.org/jdk/pull/10982/files/074dd304..02bd6dcb Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=10982=36 - incr: https://webrevs.openjdk.org/?repo=jdk=10982=35-36 Stats: 3 lines in 2 files changed: 0 ins; 0 del; 3 mod Patch: https://git.openjdk.org/jdk/pull/10982.diff Fetch: git fetch https://git.openjdk.org/jdk pull/10982/head:pull/10982 PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v38]
> This is root pull request with Classfile API implementation, tests and > benchmarks initial drop into JDK. > > Following pull requests consolidating JDK class files parsing, generating, > and transforming ([JDK-8294957](https://bugs.openjdk.org/browse/JDK-8294957)) > will chain to this one. > > Classfile API development is tracked at: > https://github.com/openjdk/jdk-sandbox/tree/classfile-api-branch > > Development branch of consolidated JDK class files parsing, generating, and > transforming is at: > https://github.com/openjdk/jdk-sandbox/tree/classfile-api-dev-branch > > Classfile API [JEP](https://bugs.openjdk.org/browse/JDK-8280389) and [online > API > documentation](https://htmlpreview.github.io/?https://raw.githubusercontent.com/openjdk/jdk-sandbox/classfile-api-javadoc-branch/doc/classfile-api/javadoc/java.base/jdk/internal/classfile/package-summary.html) > is also available. > > Please take you time to review this non-trivial JDK addition. > > Thank you, > Adam Adam Sotona has updated the pull request incrementally with three additional commits since the last revision: - Update src/java.base/share/classes/jdk/internal/classfile/CodeBuilder.java Co-authored-by: Paul Sandoz - Update src/java.base/share/classes/jdk/internal/classfile/CodeBuilder.java Co-authored-by: Paul Sandoz - Update src/java.base/share/classes/jdk/internal/classfile/instruction/ExceptionCatch.java Co-authored-by: Paul Sandoz - Changes: - all: https://git.openjdk.org/jdk/pull/10982/files - new: https://git.openjdk.org/jdk/pull/10982/files/02bd6dcb..0bd5281f Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=10982=37 - incr: https://webrevs.openjdk.org/?repo=jdk=10982=36-37 Stats: 6 lines in 2 files changed: 0 ins; 0 del; 6 mod Patch: https://git.openjdk.org/jdk/pull/10982.diff Fetch: git fetch https://git.openjdk.org/jdk pull/10982/head:pull/10982 PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v13]
On Wed, 1 Mar 2023 23:57:52 GMT, Paul Sandoz wrote: >> Every case has been considered individually, evaluated on use cases and pros >> and cons have been weighted. Unified approach across the whole API would be >> nice, however not so simple and not the highest priority. > > I had the same observation as Maurizio. I've extracted `StackMapFrameInfo` to the top level and moved `VerificationTypeInfo`, however it is still nested. Let me know if you think we should really avoid all nested or if info inside info is OK. Thanks. - PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v36]
> This is root pull request with Classfile API implementation, tests and > benchmarks initial drop into JDK. > > Following pull requests consolidating JDK class files parsing, generating, > and transforming ([JDK-8294957](https://bugs.openjdk.org/browse/JDK-8294957)) > will chain to this one. > > Classfile API development is tracked at: > https://github.com/openjdk/jdk-sandbox/tree/classfile-api-branch > > Development branch of consolidated JDK class files parsing, generating, and > transforming is at: > https://github.com/openjdk/jdk-sandbox/tree/classfile-api-dev-branch > > Classfile API [JEP](https://bugs.openjdk.org/browse/JDK-8280389) and [online > API > documentation](https://htmlpreview.github.io/?https://raw.githubusercontent.com/openjdk/jdk-sandbox/classfile-api-javadoc-branch/doc/classfile-api/javadoc/java.base/jdk/internal/classfile/package-summary.html) > is also available. > > Please take you time to review this non-trivial JDK addition. > > Thank you, > Adam Adam Sotona has updated the pull request incrementally with one additional commit since the last revision: StackMapFrameInfo extracted to top level from StackMapTableAttribute - Changes: - all: https://git.openjdk.org/jdk/pull/10982/files - new: https://git.openjdk.org/jdk/pull/10982/files/13d78c96..074dd304 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=10982=35 - incr: https://webrevs.openjdk.org/?repo=jdk=10982=34-35 Stats: 225 lines in 6 files changed: 123 ins; 93 del; 9 mod Patch: https://git.openjdk.org/jdk/pull/10982.diff Fetch: git fetch https://git.openjdk.org/jdk pull/10982/head:pull/10982 PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v35]
> This is root pull request with Classfile API implementation, tests and > benchmarks initial drop into JDK. > > Following pull requests consolidating JDK class files parsing, generating, > and transforming ([JDK-8294957](https://bugs.openjdk.org/browse/JDK-8294957)) > will chain to this one. > > Classfile API development is tracked at: > https://github.com/openjdk/jdk-sandbox/tree/classfile-api-branch > > Development branch of consolidated JDK class files parsing, generating, and > transforming is at: > https://github.com/openjdk/jdk-sandbox/tree/classfile-api-dev-branch > > Classfile API [JEP](https://bugs.openjdk.org/browse/JDK-8280389) and [online > API > documentation](https://htmlpreview.github.io/?https://raw.githubusercontent.com/openjdk/jdk-sandbox/classfile-api-javadoc-branch/doc/classfile-api/javadoc/java.base/jdk/internal/classfile/package-summary.html) > is also available. > > Please take you time to review this non-trivial JDK addition. > > Thank you, > Adam Adam Sotona has updated the pull request incrementally with two additional commits since the last revision: - added comment to CodeAttribute::labelToBci - ClassReader::readXyzEntry methods throw IllegalArgumentException instead of ClassCastException - Changes: - all: https://git.openjdk.org/jdk/pull/10982/files - new: https://git.openjdk.org/jdk/pull/10982/files/79ce1698..13d78c96 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=10982=34 - incr: https://webrevs.openjdk.org/?repo=jdk=10982=33-34 Stats: 22 lines in 3 files changed: 8 ins; 2 del; 12 mod Patch: https://git.openjdk.org/jdk/pull/10982.diff Fetch: git fetch https://git.openjdk.org/jdk pull/10982/head:pull/10982 PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v31]
On Wed, 1 Mar 2023 23:07:38 GMT, Paul Sandoz wrote:
>> Adam Sotona has updated the pull request incrementally with four additional
>> commits since the last revision:
>>
>> - renamed all remaining ConcreteXyzEntry to XyzEntryImpl
>> - abstract implementations of RefEntry, RefsEntry and NamedEntry renamed to
>> AbstractRefEntry, AbstractRefsEntry and AbstractNamedEntry
>> - renamed ConcreteBootstrapMethodEntry to BootstrapMethodEntryImpl
>> - ConcreteEntry renamed to AbstractPoolEntry
>
> src/java.base/share/classes/jdk/internal/classfile/ClassReader.java line 144:
>
>> 142: * constant pool size, or zero
>> 143: * @throws ClassCastException if the index does not correspond to
>> 144: * a module entry
>
> Throwing `ClassCastException` is certainly convenient from an implementation
> perspective, but I think it is better to throw `IllegalArgumentException`.
Fixed in all `ClassReader::readXyzEntry` methods, thanks.
> src/java.base/share/classes/jdk/internal/classfile/impl/ClassReaderImpl.java
> line 90:
>
>> 88: private BootstrapMethodsAttribute bootstrapMethodsAttribute;
>> 89:
>> 90: @SuppressWarnings("unchecked")
>
> Is this needed?
not needed, removed, thanks
> src/java.base/share/classes/jdk/internal/classfile/impl/ClassReaderImpl.java
> line 115:
>
>> 113:
>> 114: // 4
>> 115: case TAG_CONSTANTDYNAMIC, TAG_FIELDREF, TAG_FLOAT,
>> TAG_INTEGER, TAG_INTERFACEMETHODREF, TAG_INVOKEDYNAMIC, TAG_METHODREF,
>> TAG_NAMEANDTYPE -> p += 4;
>
> Break the line
fixed, thanks.
> src/java.base/share/classes/jdk/internal/classfile/impl/ClassReaderImpl.java
> line 132:
>
>> 130: this.cp = new PoolEntry[constantPoolCount];
>> 131:
>> 132: p = metadataStart;
>
> Redundant assignment (see line 127).
fixed, thanks.
-
PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v31]
On Wed, 1 Mar 2023 23:43:55 GMT, Paul Sandoz wrote:
>> Adam Sotona has updated the pull request incrementally with four additional
>> commits since the last revision:
>>
>> - renamed all remaining ConcreteXyzEntry to XyzEntryImpl
>> - abstract implementations of RefEntry, RefsEntry and NamedEntry renamed to
>> AbstractRefEntry, AbstractRefsEntry and AbstractNamedEntry
>> - renamed ConcreteBootstrapMethodEntry to BootstrapMethodEntryImpl
>> - ConcreteEntry renamed to AbstractPoolEntry
>
> src/java.base/share/classes/jdk/internal/classfile/attribute/CodeAttribute.java
> line 56:
>
>> 54: * @param label a marker for a position within this {@code
>> CodeAttribute}
>> 55: * @return position of the {@code Label} in the {@code codeArray}
>> 56: */
>
> Suggestion:
>
> /**
> * {@return the position of the {@code Label} in the {@code codeArray}}
> * @param label a marker for a position within this {@code CodeAttribute}
> */
>
> Throws IAE if the label is not positioned in the code array?
All the dependent code expects -1 when the Label is not positioned in the code
array.
Throwing IAE would require significant refactoring and may have performance
effects.
I'll add a javadoc comment meanwhile.
-
PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v34]
> This is root pull request with Classfile API implementation, tests and > benchmarks initial drop into JDK. > > Following pull requests consolidating JDK class files parsing, generating, > and transforming ([JDK-8294957](https://bugs.openjdk.org/browse/JDK-8294957)) > will chain to this one. > > Classfile API development is tracked at: > https://github.com/openjdk/jdk-sandbox/tree/classfile-api-branch > > Development branch of consolidated JDK class files parsing, generating, and > transforming is at: > https://github.com/openjdk/jdk-sandbox/tree/classfile-api-dev-branch > > Classfile API [JEP](https://bugs.openjdk.org/browse/JDK-8280389) and [online > API > documentation](https://htmlpreview.github.io/?https://raw.githubusercontent.com/openjdk/jdk-sandbox/classfile-api-javadoc-branch/doc/classfile-api/javadoc/java.base/jdk/internal/classfile/package-summary.html) > is also available. > > Please take you time to review this non-trivial JDK addition. > > Thank you, > Adam Adam Sotona has updated the pull request incrementally with two additional commits since the last revision: - Update src/java.base/share/classes/jdk/internal/classfile/attribute/CodeAttribute.java Co-authored-by: Paul Sandoz - Update src/java.base/share/classes/jdk/internal/classfile/ClassModel.java Co-authored-by: Paul Sandoz - Changes: - all: https://git.openjdk.org/jdk/pull/10982/files - new: https://git.openjdk.org/jdk/pull/10982/files/e674bada..79ce1698 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=10982=33 - incr: https://webrevs.openjdk.org/?repo=jdk=10982=32-33 Stats: 3 lines in 2 files changed: 0 ins; 1 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/10982.diff Fetch: git fetch https://git.openjdk.org/jdk pull/10982/head:pull/10982 PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v33]
> This is root pull request with Classfile API implementation, tests and > benchmarks initial drop into JDK. > > Following pull requests consolidating JDK class files parsing, generating, > and transforming ([JDK-8294957](https://bugs.openjdk.org/browse/JDK-8294957)) > will chain to this one. > > Classfile API development is tracked at: > https://github.com/openjdk/jdk-sandbox/tree/classfile-api-branch > > Development branch of consolidated JDK class files parsing, generating, and > transforming is at: > https://github.com/openjdk/jdk-sandbox/tree/classfile-api-dev-branch > > Classfile API [JEP](https://bugs.openjdk.org/browse/JDK-8280389) and [online > API > documentation](https://htmlpreview.github.io/?https://raw.githubusercontent.com/openjdk/jdk-sandbox/classfile-api-javadoc-branch/doc/classfile-api/javadoc/java.base/jdk/internal/classfile/package-summary.html) > is also available. > > Please take you time to review this non-trivial JDK addition. > > Thank you, > Adam Adam Sotona has updated the pull request incrementally with three additional commits since the last revision: - Update src/java.base/share/classes/jdk/internal/classfile/ClassReader.java Co-authored-by: Paul Sandoz - Update src/java.base/share/classes/jdk/internal/classfile/impl/TemporaryConstantPool.java Co-authored-by: Paul Sandoz - SplitConstantPool fixes - Changes: - all: https://git.openjdk.org/jdk/pull/10982/files - new: https://git.openjdk.org/jdk/pull/10982/files/70ec5ec7..e674bada Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=10982=32 - incr: https://webrevs.openjdk.org/?repo=jdk=10982=31-32 Stats: 20 lines in 3 files changed: 10 ins; 2 del; 8 mod Patch: https://git.openjdk.org/jdk/pull/10982.diff Fetch: git fetch https://git.openjdk.org/jdk pull/10982/head:pull/10982 PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v31]
On Wed, 1 Mar 2023 22:38:32 GMT, Paul Sandoz wrote:
>> Adam Sotona has updated the pull request incrementally with four additional
>> commits since the last revision:
>>
>> - renamed all remaining ConcreteXyzEntry to XyzEntryImpl
>> - abstract implementations of RefEntry, RefsEntry and NamedEntry renamed to
>> AbstractRefEntry, AbstractRefsEntry and AbstractNamedEntry
>> - renamed ConcreteBootstrapMethodEntry to BootstrapMethodEntryImpl
>> - ConcreteEntry renamed to AbstractPoolEntry
>
> src/java.base/share/classes/jdk/internal/classfile/impl/SplitConstantPool.java
> line 167:
>
>> 165: buf.patchInt(pos + 2, 4, attrLen - 6);
>> 166: buf.patchInt(pos + 6, 2, bsmSize);
>> 167: return true;
>
> The if and else branch return true, factor out at the end of the method?
fixed, thanks.
> src/java.base/share/classes/jdk/internal/classfile/impl/SplitConstantPool.java
> line 339:
>
>> 337: }
>> 338:
>> 339: private AbstractPoolEntry.Utf8EntryImpl tryFindUtf8(int hash,
>> String target) {
>
> Unused type variable `T`
fixed, thanks.
> src/java.base/share/classes/jdk/internal/classfile/impl/SplitConstantPool.java
> line 488:
>
>> 486: return methodHandleEntry(refKind, reference);
>> 487: }
>> 488: return internalAdd(new
>> AbstractPoolEntry.MethodHandleEntryImpl(this, size, hash, refKind,
>> (AbstractPoolEntry.AbstractMemberRefEntry) reference), hash);
>
> Break the long line (same for two following methods).
fixed, thanks.
-
PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v31]
On Wed, 1 Mar 2023 21:36:41 GMT, Paul Sandoz wrote:
>> Adam Sotona has updated the pull request incrementally with four additional
>> commits since the last revision:
>>
>> - renamed all remaining ConcreteXyzEntry to XyzEntryImpl
>> - abstract implementations of RefEntry, RefsEntry and NamedEntry renamed to
>> AbstractRefEntry, AbstractRefsEntry and AbstractNamedEntry
>> - renamed ConcreteBootstrapMethodEntry to BootstrapMethodEntryImpl
>> - ConcreteEntry renamed to AbstractPoolEntry
>
> src/java.base/share/classes/jdk/internal/classfile/constantpool/ClassEntry.java
> line 48:
>
>> 46: default ConstantDesc constantValue() {
>> 47: return asSymbol();
>> 48: }
>
> It seems possible to use this pattern consistently for
> `ConstantDynamicEntry`, `MethodHandleEntry`, and `MethodTypeEntry`?
>
> At first i thought why not make `asSymbol` a covariant override of
> `constantValue`, but its not the same thing, since there are constant values
> (subtypes of `ConstantValueEntry`) that are not symbols.
Yes, I've moved the default `constantValue` delegation to `asSymbol` from
implementations up to the `ConstantDynamicEntry`, `MethodHandleEntry`, and
`MethodTypeEntry`.
Thanks.
> src/java.base/share/classes/jdk/internal/classfile/constantpool/ClassEntry.java
> line 71:
>
>> 69: * @return the combined {@link List}
>> 70: */
>> 71: static List adding(List base,
>> List additions) {
>
> This and all the other following static methods seem more like implementation
> details rather than part of the public API?
I've searched over all use cases we have and found no direct use of these API
methods, so I've removed them.
We may later re-open discussion about possible API to combine and deduplicate
lists of entries and symbols, however this isolated solution really does not
fit and does not serve any purpose.
> src/java.base/share/classes/jdk/internal/classfile/constantpool/ConstantPool.java
> line 49:
>
>> 47: import static jdk.internal.classfile.Classfile.TAG_STRING;
>> 48: import static jdk.internal.classfile.Classfile.TAG_UTF8;
>> 49:
>
> Unused imports, perhaps sweep through all classes (i think it can be done
> over multiple packages with IntelliJ).
This and hopefully all other unused imports have been removed.
Thanks.
> src/java.base/share/classes/jdk/internal/classfile/constantpool/ConstantPoolBuilder.java
> line 222:
>
>> 220: * @param typeEntry the member field or method descriptor
>> 221: */
>> 222: NameAndTypeEntry natEntry(Utf8Entry nameEntry, Utf8Entry typeEntry);
>
> I would be inclined to rename more literally as `nameAndTypeEntry`, which is
> consistent with the naming convention in all other cases in this interface
> AFAICT (edit: almost i also see `bsm`) . (FWIW i keep reading nat as "not a
> type"!)
Good point, renamed.
Thanks.
> src/java.base/share/classes/jdk/internal/classfile/impl/AbstractPoolEntry.java
> line 376:
>
>> 374: } else if (o instanceof Utf8Entry u) {
>> 375: return equalsString(u.stringValue());
>> 376: }
>
> Dead branch? since there is only one concrete implementation of `Utf8Entry`.
fixed, thanks.
> src/java.base/share/classes/jdk/internal/classfile/snippet-files/PackageSnippets.java
> line 111:
>
>> 109: Set dependencies = cm.elementStream()
>> 110: .filter(ce -> ce instanceof
>> MethodModel)
>> 111: .flatMap(ce -> ((MethodModel)
>> ce).elementStream())
>
> You could potentially collapse this into a single `flatMap` and avoid the
> cast:
>
> .flatMap(ce -> ce instanceof MethodMethod mm ? mm.elementStream() :
> Stream.empty())
fixed, thanks.
-
PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v32]
> This is root pull request with Classfile API implementation, tests and > benchmarks initial drop into JDK. > > Following pull requests consolidating JDK class files parsing, generating, > and transforming ([JDK-8294957](https://bugs.openjdk.org/browse/JDK-8294957)) > will chain to this one. > > Classfile API development is tracked at: > https://github.com/openjdk/jdk-sandbox/tree/classfile-api-branch > > Development branch of consolidated JDK class files parsing, generating, and > transforming is at: > https://github.com/openjdk/jdk-sandbox/tree/classfile-api-dev-branch > > Classfile API [JEP](https://bugs.openjdk.org/browse/JDK-8280389) and [online > API > documentation](https://htmlpreview.github.io/?https://raw.githubusercontent.com/openjdk/jdk-sandbox/classfile-api-javadoc-branch/doc/classfile-api/javadoc/java.base/jdk/internal/classfile/package-summary.html) > is also available. > > Please take you time to review this non-trivial JDK addition. > > Thank you, > Adam Adam Sotona has updated the pull request incrementally with five additional commits since the last revision: - ConstantPoolBuilder::natEntry renamed to nameAndTypeEntry - removed static implementation methods from ClassEntry - removed unused imports - fixed PackageSnipets - default constantValue delegating to asSymbol pulled from implementations to ConstantDynamicEntry, MethodHandleEntry and MethodTypeEntry - Changes: - all: https://git.openjdk.org/jdk/pull/10982/files - new: https://git.openjdk.org/jdk/pull/10982/files/1e95e508..70ec5ec7 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=10982=31 - incr: https://webrevs.openjdk.org/?repo=jdk=10982=30-31 Stats: 363 lines in 47 files changed: 17 ins; 307 del; 39 mod Patch: https://git.openjdk.org/jdk/pull/10982.diff Fetch: git fetch https://git.openjdk.org/jdk pull/10982/head:pull/10982 PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v31]
> This is root pull request with Classfile API implementation, tests and > benchmarks initial drop into JDK. > > Following pull requests consolidating JDK class files parsing, generating, > and transforming ([JDK-8294957](https://bugs.openjdk.org/browse/JDK-8294957)) > will chain to this one. > > Classfile API development is tracked at: > https://github.com/openjdk/jdk-sandbox/tree/classfile-api-branch > > Development branch of consolidated JDK class files parsing, generating, and > transforming is at: > https://github.com/openjdk/jdk-sandbox/tree/classfile-api-dev-branch > > Classfile API [JEP](https://bugs.openjdk.org/browse/JDK-8280389) and [online > API > documentation](https://htmlpreview.github.io/?https://raw.githubusercontent.com/openjdk/jdk-sandbox/classfile-api-javadoc-branch/doc/classfile-api/javadoc/java.base/jdk/internal/classfile/package-summary.html) > is also available. > > Please take you time to review this non-trivial JDK addition. > > Thank you, > Adam Adam Sotona has updated the pull request incrementally with four additional commits since the last revision: - renamed all remaining ConcreteXyzEntry to XyzEntryImpl - abstract implementations of RefEntry, RefsEntry and NamedEntry renamed to AbstractRefEntry, AbstractRefsEntry and AbstractNamedEntry - renamed ConcreteBootstrapMethodEntry to BootstrapMethodEntryImpl - ConcreteEntry renamed to AbstractPoolEntry - Changes: - all: https://git.openjdk.org/jdk/pull/10982/files - new: https://git.openjdk.org/jdk/pull/10982/files/212bb04e..1e95e508 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=10982=30 - incr: https://webrevs.openjdk.org/?repo=jdk=10982=29-30 Stats: 2758 lines in 32 files changed: 1283 ins; 1285 del; 190 mod Patch: https://git.openjdk.org/jdk/pull/10982.diff Fetch: git fetch https://git.openjdk.org/jdk pull/10982/head:pull/10982 PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v20]
On Thu, 16 Feb 2023 14:19:12 GMT, Maurizio Cimadamore wrote: >> Adam Sotona has updated the pull request incrementally with one additional >> commit since the last revision: >> >> added 4-byte Unicode text to Utf8EntryTest > > src/java.base/share/classes/jdk/internal/classfile/impl/ChainedFieldBuilder.java > line 48: > >> 46: this.consumer = consumer; >> 47: FieldBuilder b = downstream; >> 48: while (b instanceof ChainedFieldBuilder cb) > > I'm probably missing something - but if `b` is another chained builder, > instead of using a loop, can't we just skip to its `terminal` field? Also, > the `downstream` field seems unused. (same considerations apply for > `ChainedMethodBuilder` and `ChainedClassBuilder`). > > I note that `NonTerminalCodeBuilder` seems to be already doing what I suggest > here (e.g. skip to `terminal`). I would have to test possible side-effects of the proposed shortcut to give you answer. Thanks for pointing it out. - PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v20]
On Thu, 16 Feb 2023 14:58:21 GMT, Maurizio Cimadamore
wrote:
>> Adam Sotona has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> added 4-byte Unicode text to Utf8EntryTest
>
> src/java.base/share/classes/jdk/internal/classfile/impl/TransformImpl.java
> line 51:
>
>> 49: * TransformImpl
>> 50: */
>> 51: public class TransformImpl {
>
> Understanding check: a transform implementation is made up of two runnable
> (start/end handlers) and a consumer. The consumer can be used e.g. to iterate
> and transform all the contents of a compound element (e.g. all the
> instruction in a code attribute).
>
> User-defined transforms can be "resolved" that is, we can "bind" them to a
> specific builder, which effectively turns them into consumers (and into a
> concrete transform implementation that can be used).
>
> When two transforms T1 and T2 are composed (so that T1 runs before T2), we
> need to implement the `resolve` method of the resulting transform so that:
> * We resolve T2 against the provided builder - this gives us a "downstream"
> consumer;
> * We create a new "chained" builder which wraps the provided builder, and
> whose `with` method calls the downstream consumer;
> * We then resolve T1 against the chained builder, and return the resulting
> consumer
>
> This means that when we call `accept` on the resulting consumer, we will
> transform (using T1) and call `with` (passing the transformed element) on
> the chained builder, which will end up calling `accept` on the downstream
> consumer, which will apply T2 and finally add the resulting element to the
> provided builder.
>
> Correct? (Phew)
Yes, this is how it is designed and implemented.
Actually we have only a few real use cases testing this concept, one is a
prototype of jdk.jfr class instrumentation at:
https://github.com/openjdk/jdk/blob/212bb04e9491badb17c110787727ea19842d1528/test/jdk/jdk/classfile/AdvancedTransformationsTest.java#L293
-
PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v20]
On Thu, 16 Feb 2023 11:34:28 GMT, Maurizio Cimadamore
wrote:
>> Adam Sotona has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> added 4-byte Unicode text to Utf8EntryTest
>
> src/java.base/share/classes/jdk/internal/classfile/components/package-info.java
> line 97:
>
>> 95: * {@snippet lang="java" class="PackageSnippets"
>> region="classInstrumentation"}
>> 96: */
>> 97: package jdk.internal.classfile.components;
>
> watch out for newline
fixed, thanks.
> src/java.base/share/classes/jdk/internal/classfile/impl/TargetInfoImpl.java
> line 34:
>
>> 32:
>> 33: /**
>> 34: *
>
> Empty javadoc?
fixed, thanks.
-
PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v30]
> This is root pull request with Classfile API implementation, tests and > benchmarks initial drop into JDK. > > Following pull requests consolidating JDK class files parsing, generating, > and transforming ([JDK-8294957](https://bugs.openjdk.org/browse/JDK-8294957)) > will chain to this one. > > Classfile API development is tracked at: > https://github.com/openjdk/jdk-sandbox/tree/classfile-api-branch > > Development branch of consolidated JDK class files parsing, generating, and > transforming is at: > https://github.com/openjdk/jdk-sandbox/tree/classfile-api-dev-branch > > Classfile API [JEP](https://bugs.openjdk.org/browse/JDK-8280389) and [online > API > documentation](https://htmlpreview.github.io/?https://raw.githubusercontent.com/openjdk/jdk-sandbox/classfile-api-javadoc-branch/doc/classfile-api/javadoc/java.base/jdk/internal/classfile/package-summary.html) > is also available. > > Please take you time to review this non-trivial JDK addition. > > Thank you, > Adam Adam Sotona has updated the pull request incrementally with one additional commit since the last revision: javadoc fix - Changes: - all: https://git.openjdk.org/jdk/pull/10982/files - new: https://git.openjdk.org/jdk/pull/10982/files/f6c65616..212bb04e Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=10982=29 - incr: https://webrevs.openjdk.org/?repo=jdk=10982=28-29 Stats: 48 lines in 1 file changed: 17 ins; 0 del; 31 mod Patch: https://git.openjdk.org/jdk/pull/10982.diff Fetch: git fetch https://git.openjdk.org/jdk pull/10982/head:pull/10982 PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v29]
> This is root pull request with Classfile API implementation, tests and > benchmarks initial drop into JDK. > > Following pull requests consolidating JDK class files parsing, generating, > and transforming ([JDK-8294957](https://bugs.openjdk.org/browse/JDK-8294957)) > will chain to this one. > > Classfile API development is tracked at: > https://github.com/openjdk/jdk-sandbox/tree/classfile-api-branch > > Development branch of consolidated JDK class files parsing, generating, and > transforming is at: > https://github.com/openjdk/jdk-sandbox/tree/classfile-api-dev-branch > > Classfile API [JEP](https://bugs.openjdk.org/browse/JDK-8280389) and [online > API > documentation](https://htmlpreview.github.io/?https://raw.githubusercontent.com/openjdk/jdk-sandbox/classfile-api-javadoc-branch/doc/classfile-api/javadoc/java.base/jdk/internal/classfile/package-summary.html) > is also available. > > Please take you time to review this non-trivial JDK addition. > > Thank you, > Adam Adam Sotona has updated the pull request incrementally with three additional commits since the last revision: - LabelImpl get/setContextInfo renamed to get/setBCI - removed default constructor from AttributeHolder - InstructionData content moved to CodeImpl - Changes: - all: https://git.openjdk.org/jdk/pull/10982/files - new: https://git.openjdk.org/jdk/pull/10982/files/58c9d2c0..f6c65616 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=10982=28 - incr: https://webrevs.openjdk.org/?repo=jdk=10982=27-28 Stats: 257 lines in 6 files changed: 62 ins; 168 del; 27 mod Patch: https://git.openjdk.org/jdk/pull/10982.diff Fetch: git fetch https://git.openjdk.org/jdk pull/10982/head:pull/10982 PR: https://git.openjdk.org/jdk/pull/10982
