Re: RFR: 8302189: Mark assertion failures noreturn [v2]
On Tue, 7 Mar 2023 05:58:57 GMT, Kim Barrett wrote:
>> No I'm comparing the syntax used elsewhere:
>>
>> DebuggingContext debugging{};
>>
>> with that used here:
>>
>> DebuggingContext debugging;
>>
>> why the braces in most cases but not here?
>
> "syntax used elsewhere" are declarations of local variables initialized with
> _value initialization_ (which is _default initialization_ for this type).
> "that used here" is a member variable declaration in the Command class, where
> the initialization occurs elsewhere (in the constructor of the Command class
> [*]). They are entirely different syntactic entities. Does that help? Or
> am I still not understanding your question?
>
> [*] With C++11, data members can have default initializers, but that's not an
> approved feature for HotSpot.
Sorry I was fixated on the weirdness (to me) of the {} initialization. I missed
the fact these were different kinds of declarations.
-
PR: https://git.openjdk.org/jdk/pull/12845
Re: RFR: 8302189: Mark assertion failures noreturn [v2]
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 Tue, 7 Mar 2023 01:25:47 GMT, David Holmes wrote:
>> I believe David is asking about why these particular member names were
>> changed
>
> No I'm comparing the syntax used elsewhere:
>
> DebuggingContext debugging{};
>
> with that used here:
>
> DebuggingContext debugging;
>
> why the braces in most cases but not here?
"syntax used elsewhere" are declarations of local variables initialized with
_value initialization_ (which is _default initialization_ for this type).
"that used here" is a member variable declaration in the Command class, where
the initialization occurs elsewhere (in the constructor of the Command class
[*]). They are entirely different syntactic entities. Does that help? Or am
I still not understanding your question?
[*] With C++11, data members can have default initializers, but that's not an
approved feature for HotSpot.
-
PR: https://git.openjdk.org/jdk/pull/12845
Re: RFR: 8302189: Mark assertion failures noreturn [v2]
On Sat, 4 Mar 2023 11:23:58 GMT, Kim Barrett wrote: >> Also 8302799: Refactor Debugging variable usage for noreturn crash reporting > > Kim Barrett has updated the pull request incrementally with one additional > commit since the last revision: > > make Debugging::_enabled a nesting counter Okay other than the one outstanding minor syntax query, I have nothing further to add. Thanks. - Marked as reviewed by dholmes (Reviewer). PR: https://git.openjdk.org/jdk/pull/12845
Re: RFR: JDK-8302801: Remove fdlibm C sources [v4]
On Sun, 5 Mar 2023 17:10:08 GMT, Joe Darcy wrote: > PS Successful Mach 5 job of default builds and tier 1 tests with this make > line present. PPS And for extra measure as suggested by David Holmes, a tier 1 through 5 build job was also successful. - PR: https://git.openjdk.org/jdk/pull/12821
Re: RFR: 8302189: Mark assertion failures noreturn [v2]
On Sat, 4 Mar 2023 11:16:10 GMT, Kim Barrett wrote:
>> src/hotspot/share/utilities/debug.cpp line 85:
>>
>>> 83: if (is_enabled()) {
>>> 84: fatal("Multiple Debugging contexts");
>>> 85: }
>>
>> This seems too restrictive as you could hit different DebuggingContexts in
>> different threads. ??
>
> This facility is only intended for use by manually invoked commands while the
> program is stopped in a debugger. Multi-threaded use is not an issue (and was
> not supported previously either). I don't think there are any nested uses
> either, but I've now run across a couple of places where nesting could be
> useful. So I'm changing the state from a simple bool to a nesting counter.
Okay I hadn't realized the process was basically "suspended" when this was
activated - as I said I never use this stuff.
-
PR: https://git.openjdk.org/jdk/pull/12845
Re: RFR: 8302189: Mark assertion failures noreturn [v2]
On Sun, 5 Mar 2023 15:36:30 GMT, Julian Waters wrote:
>> I don't understand the question? These are member variable declarations.
>
> I believe David is asking about why these particular member names were changed
No I'm comparing the syntax used elsewhere:
DebuggingContext debugging{};
with that used here:
DebuggingContext debugging;
why the braces in most cases but not here?
-
PR: https://git.openjdk.org/jdk/pull/12845
Re: RFR: JDK-8301998: Update HarfBuzz to 7.0.1 [v3]
On Tue, 7 Mar 2023 00:36:18 GMT, Harshitha Onkar wrote: >> HarfBuzz library updated from v4.4.1 to v7.0.1 >> >> - harfbuzz.md file updated >> - Added -DHB_NO_VISIBILITY to HARFBUZZ_CFLAGS in Awt2DLibraries.gmk to avoid >> build issues on linux and macos targets. >> - GPOS.hh has been moved from Layout to Layout/GPOS to match upstream >> changes. > > Harshitha Onkar has updated the pull request incrementally with one > additional commit since the last revision: > > deleted files With the latest update, following files have been deleted: Newly added files that have been deleted. 1. src/java.desktop/share/native/libharfbuzz/hb-icu.cc 2. src/java.desktop/share/native/libharfbuzz/hb-icu.h 3. src/java.desktop/share/native/libharfbuzz/hb-ms-feature-ranges.hh 4. src/java.desktop/share/native/libharfbuzz/hb-subset-repacker.cc 5. src/java.desktop/share/native/libharfbuzz/hb-subset-repacker.h 6. src/java.desktop/share/native/libharfbuzz/hb-buffer-deserialize-text.hh Following files have been moved to OT/Color in upstream and the duplicates are deleted. 1. src/java.desktop/share/native/libharfbuzz/hb-ot-color-cbdt-table.hh 2. src/java.desktop/share/native/libharfbuzz/hb-ot-color-colr-table.hh 3. src/java.desktop/share/native/libharfbuzz/hb-ot-color-colrv1-closure.hh 4. src/java.desktop/share/native/libharfbuzz/hb-ot-color-cpal-table.hh 5. src/java.desktop/share/native/libharfbuzz/hb-ot-color-sbix-table.hh 6. src/java.desktop/share/native/libharfbuzz/hb-ot-color-svg-table.hh - PR: https://git.openjdk.org/jdk/pull/12864
Re: RFR: JDK-8301998: Update HarfBuzz to 7.0.1 [v3]
> HarfBuzz library updated from v4.4.1 to v7.0.1 > > - harfbuzz.md file updated > - Added -DHB_NO_VISIBILITY to HARFBUZZ_CFLAGS in Awt2DLibraries.gmk to avoid > build issues on linux and macos targets. > - GPOS.hh has been moved from Layout to Layout/GPOS to match upstream changes. Harshitha Onkar has updated the pull request incrementally with one additional commit since the last revision: deleted files - Changes: - all: https://git.openjdk.org/jdk/pull/12864/files - new: https://git.openjdk.org/jdk/pull/12864/files/6db2d186..0341f8f4 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=12864=02 - incr: https://webrevs.openjdk.org/?repo=jdk=12864=01-02 Stats: 5083 lines in 12 files changed: 0 ins; 5083 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/12864.diff Fetch: git fetch https://git.openjdk.org/jdk pull/12864/head:pull/12864 PR: https://git.openjdk.org/jdk/pull/12864
Re: RFR: 8303480: Miscellaneous fixes to mostly invisible doc comments [v2]
On Mon, 6 Mar 2023 20:22:48 GMT, Pavel Rappo wrote: >> Please review this superficial documentation cleanup that was triggered by >> unrelated analysis of doc comments in JDK API. >> >> The only effect that this multi-area PR has on the JDK API Documentation >> (i.e. the observable effect on the generated HTML pages) can be summarized >> as follows: >> >> >> diff -ur >> build/macosx-aarch64/images/docs-before/api/serialized-form.html >> build/macosx-aarch64/images/docs-after/api/serialized-form.html >> --- build/macosx-aarch64/images/docs-before/api/serialized-form.html >> 2023-03-02 11:47:44 >> +++ build/macosx-aarch64/images/docs-after/api/serialized-form.html >> 2023-03-02 11:48:45 >> @@ -17084,7 +17084,7 @@ >> throws > href="java.base/java/io/IOException.html" title="class in >> java.io">IOException, >> ClassNotFoundException >> readObject is called to restore the >> state of the >> - (@code BasicPermission} from a stream. >> + BasicPermission from a stream. >> >> Parameters: >> s - the ObjectInputStream from which data >> is read >> >> Notes >> - >> >> * I'm not an expert in any of the affected areas, except for jdk.javadoc, >> and I was merely after misused tags. Because of that, I would appreciate >> reviews from experts in other areas. >> * I discovered many more issues than I included in this PR. The excluded >> issues seem to occur in infrequently updated third-party code (e.g. >> javax.xml), which I assume we shouldn't touch unless necessary. >> * I will update copyright years after (and if) the fix had been approved, as >> required. > > Pavel Rappo 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 two additional > commits since the last revision: > > - Merge branch 'master' into 8303480 > - Initial commit Marked as reviewed by rriggs (Reviewer). - PR: https://git.openjdk.org/jdk/pull/12826
Re: RFR: 8303480: Miscellaneous fixes to mostly invisible doc comments [v2]
On Mon, 6 Mar 2023 20:22:48 GMT, Pavel Rappo wrote: >> Please review this superficial documentation cleanup that was triggered by >> unrelated analysis of doc comments in JDK API. >> >> The only effect that this multi-area PR has on the JDK API Documentation >> (i.e. the observable effect on the generated HTML pages) can be summarized >> as follows: >> >> >> diff -ur >> build/macosx-aarch64/images/docs-before/api/serialized-form.html >> build/macosx-aarch64/images/docs-after/api/serialized-form.html >> --- build/macosx-aarch64/images/docs-before/api/serialized-form.html >> 2023-03-02 11:47:44 >> +++ build/macosx-aarch64/images/docs-after/api/serialized-form.html >> 2023-03-02 11:48:45 >> @@ -17084,7 +17084,7 @@ >> throws > href="java.base/java/io/IOException.html" title="class in >> java.io">IOException, >> ClassNotFoundException >> readObject is called to restore the >> state of the >> - (@code BasicPermission} from a stream. >> + BasicPermission from a stream. >> >> Parameters: >> s - the ObjectInputStream from which data >> is read >> >> Notes >> - >> >> * I'm not an expert in any of the affected areas, except for jdk.javadoc, >> and I was merely after misused tags. Because of that, I would appreciate >> reviews from experts in other areas. >> * I discovered many more issues than I included in this PR. The excluded >> issues seem to occur in infrequently updated third-party code (e.g. >> javax.xml), which I assume we shouldn't touch unless necessary. >> * I will update copyright years after (and if) the fix had been approved, as >> required. > > Pavel Rappo 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 two additional > commits since the last revision: > > - Merge branch 'master' into 8303480 > - Initial commit Marked as reviewed by lancea (Reviewer). - PR: https://git.openjdk.org/jdk/pull/12826
Re: RFR: 8303480: Miscellaneous fixes to mostly invisible doc comments [v2]
On Mon, 6 Mar 2023 20:22:48 GMT, Pavel Rappo wrote: >> Please review this superficial documentation cleanup that was triggered by >> unrelated analysis of doc comments in JDK API. >> >> The only effect that this multi-area PR has on the JDK API Documentation >> (i.e. the observable effect on the generated HTML pages) can be summarized >> as follows: >> >> >> diff -ur >> build/macosx-aarch64/images/docs-before/api/serialized-form.html >> build/macosx-aarch64/images/docs-after/api/serialized-form.html >> --- build/macosx-aarch64/images/docs-before/api/serialized-form.html >> 2023-03-02 11:47:44 >> +++ build/macosx-aarch64/images/docs-after/api/serialized-form.html >> 2023-03-02 11:48:45 >> @@ -17084,7 +17084,7 @@ >> throws > href="java.base/java/io/IOException.html" title="class in >> java.io">IOException, >> ClassNotFoundException >> readObject is called to restore the >> state of the >> - (@code BasicPermission} from a stream. >> + BasicPermission from a stream. >> >> Parameters: >> s - the ObjectInputStream from which data >> is read >> >> Notes >> - >> >> * I'm not an expert in any of the affected areas, except for jdk.javadoc, >> and I was merely after misused tags. Because of that, I would appreciate >> reviews from experts in other areas. >> * I discovered many more issues than I included in this PR. The excluded >> issues seem to occur in infrequently updated third-party code (e.g. >> javax.xml), which I assume we shouldn't touch unless necessary. >> * I will update copyright years after (and if) the fix had been approved, as >> required. > > Pavel Rappo 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 two additional > commits since the last revision: > > - Merge branch 'master' into 8303480 > - Initial commit Marked as reviewed by jjg (Reviewer). - PR: https://git.openjdk.org/jdk/pull/12826
RFR: 8303691: Fedora based devkit build should load more packages from archive location
When building Fedora based Linux devkits, rpm packages are downloaded from locations at the Fedora project. The latest/active versions reside under https://dl.fedoraproject.org while older, archived versions live at https://archives.fedoraproject.org. It seems more releases have been archived in the meanwhile, so e.g. a build based on Fedora 27, which is currently marked as default, would fail. I suggest to add a variable `LATEST_ARCHIVED_OS_VERSION` in the make file that denotes the Fedora version up to which a download would be attempted from archives. For later releases, the download is done from dl. - Commit messages: - While touching the file, fix another typo - JDK-8303691 Changes: https://git.openjdk.org/jdk/pull/12888/files Webrev: https://webrevs.openjdk.org/?repo=jdk=12888=00 Issue: https://bugs.openjdk.org/browse/JDK-8303691 Stats: 9 lines in 1 file changed: 3 ins; 2 del; 4 mod Patch: https://git.openjdk.org/jdk/pull/12888.diff Fetch: git fetch https://git.openjdk.org/jdk pull/12888/head:pull/12888 PR: https://git.openjdk.org/jdk/pull/12888
Re: RFR: 8303480: Miscellaneous fixes to mostly invisible doc comments [v2]
On Fri, 3 Mar 2023 11:31:04 GMT, Alexey Ivanov wrote: >> Yes, iff means if-and-only-if and is used for extra precision in formal >> logic, mathematics. As @pavelrappo points out it's a relatively common >> occurrence in the OpenJDK sources, though perhaps not in the public >> javadocs. Perhaps a bit pretentious, but mostly a terse way to say "return >> true if the BSM method type exactly matches X, otherwise false". >> >> The broken link stems from the fact that the method I was targeting (a way >> to use condy for lambda proxy singletons rather than a >> `MethodHandle.constant`) was never integrated. We'll look at either getting >> that done (@briangoetz suggested the time might be ready for it) or remove >> this currently pointless static bootstrap specialization test. > >> Yes, iff means if-and-only-if and is used for extra precision in formal >> logic, mathematics. > > I've never come across it before. With your explanations, it makes perfect > sense. I would recommend (separately) changing `iff` to the expanded form `if and only if` - PR: https://git.openjdk.org/jdk/pull/12826
Re: RFR: 8303480: Miscellaneous fixes to mostly invisible doc comments [v2]
> Please review this superficial documentation cleanup that was triggered by > unrelated analysis of doc comments in JDK API. > > The only effect that this multi-area PR has on the JDK API Documentation > (i.e. the observable effect on the generated HTML pages) can be summarized as > follows: > > > diff -ur build/macosx-aarch64/images/docs-before/api/serialized-form.html > build/macosx-aarch64/images/docs-after/api/serialized-form.html > --- build/macosx-aarch64/images/docs-before/api/serialized-form.html > 2023-03-02 11:47:44 > +++ build/macosx-aarch64/images/docs-after/api/serialized-form.html > 2023-03-02 11:48:45 > @@ -17084,7 +17084,7 @@ > throws href="java.base/java/io/IOException.html" title="class in > java.io">IOException, > ClassNotFoundException > readObject is called to restore the > state of the > - (@code BasicPermission} from a stream. > + BasicPermission from a stream. > > Parameters: > s - the ObjectInputStream from which data > is read > > Notes > - > > * I'm not an expert in any of the affected areas, except for jdk.javadoc, and > I was merely after misused tags. Because of that, I would appreciate reviews > from experts in other areas. > * I discovered many more issues than I included in this PR. The excluded > issues seem to occur in infrequently updated third-party code (e.g. > javax.xml), which I assume we shouldn't touch unless necessary. > * I will update copyright years after (and if) the fix had been approved, as > required. Pavel Rappo 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 two additional commits since the last revision: - Merge branch 'master' into 8303480 - Initial commit - Changes: - all: https://git.openjdk.org/jdk/pull/12826/files - new: https://git.openjdk.org/jdk/pull/12826/files/d2f4a553..87166408 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=12826=01 - incr: https://webrevs.openjdk.org/?repo=jdk=12826=00-01 Stats: 13433 lines in 415 files changed: 9003 ins; 2610 del; 1820 mod Patch: https://git.openjdk.org/jdk/pull/12826.diff Fetch: git fetch https://git.openjdk.org/jdk pull/12826/head:pull/12826 PR: https://git.openjdk.org/jdk/pull/12826
Re: RFR: 8294982: Implementation of Classfile API [v36]
On Mon, 6 Mar 2023 17:34:35 GMT, Adam Sotona wrote: >> 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)`? 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". - PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v36]
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 [v36]
On Mon, 6 Mar 2023 17:02:46 GMT, Adam Sotona wrote:
>> 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.
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.
-
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 [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]
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 [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: 8295884: Implement IDE support for Eclipse [v38]
> Eclipse is a popular and very well-known IDE in the world of Java > development, utilized widely in many contexts, by beginners and experienced > teams alike. Although a relatively lightweight IDE, it features surprisingly > powerful indexing and code analysis capabilities, as well as useful tools, > among which are make integration. While the tools it provides are not always > as sophisticated as other IDEs (IntelliJ IDEA will likely come to mind as one > such competitor), the simplicity of using it, as well as the reliability of > this rugged IDE makes up greatly for the slightly less advanced tooling. > Eclipse requires very little starting infrastructure in the workspace for all > these features and indexing support as well, which makes it a good candidate > for developing on the JDK. > > This enhancement adds 4 extra targets to the make system for generating a > basic Eclipse Workspace that provides almost full indexing support for the > JDK, with varying levels as desired, from a minimalistic option only > including the Java Virtual Machine's source code, to generating a workspace > with both Java and C/C++ natures included, which allows for using Eclipse's > unique ability to quickly swap between Java and C/C++ mode to work on both > native and Java sources at the same time. Cross Compiling support is > available, and in its entirety the change touches very little of the existing > make system, barring its own Makefile within the ide subdirectory. > > Indexing capabilities utilizing the enhancement: > src="https://user-images.githubusercontent.com/32636402/197784819-67ec7de4-7e27-4f33-b738-59b75a9e4403.PNG;> > src="https://user-images.githubusercontent.com/32636402/197784843-df8621a8-7b0a-42da-86f4-3afb92de0f71.PNG;> Julian Waters has updated the pull request incrementally with one additional commit since the last revision: Wording - Changes: - all: https://git.openjdk.org/jdk/pull/10853/files - new: https://git.openjdk.org/jdk/pull/10853/files/85df2b7b..13c0b9b9 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=10853=37 - incr: https://webrevs.openjdk.org/?repo=jdk=10853=36-37 Stats: 10 lines in 5 files changed: 4 ins; 0 del; 6 mod Patch: https://git.openjdk.org/jdk/pull/10853.diff Fetch: git fetch https://git.openjdk.org/jdk pull/10853/head:pull/10853 PR: https://git.openjdk.org/jdk/pull/10853
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]
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 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 [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
