Re: RFR: 8286781: Replace the deprecated/obsolete gethostbyname and inet_addr calls
On Thu, 2 Mar 2023 22:35:17 GMT, David Holmes wrote: > We can replace `gethostbyname`, which is deprecated on Windows and Linux, > with `getaddrinfo`. This API is available on all supported platforms and so > can be placed in shared code. @djelinski pointed out that `getaddrinfo` can > resolve both IP addresses and host names so the two step approach used in > `networkStream::connect` is not necessary and we can do away with > `os::get_host_by_name()` completely. > > The build is updated to enable winsock deprecation warnings, and now we need > to use `ws2_32.lib` we can drop `wsock32.lib` (as it is basically a subset - > again thanks @djelinski ). > > Testing > - all Oracle builds in tiers 1-5 > - All GHA builds > > The actual code change has to be manually tested because the code is only > used by Ideal Graph Printing to connect to the Ideal Graph Visualizer. I've > manually tested on Windows and Linux and @tobiasholenstein tested macOS. > > Thanks. Looks good. - Marked as reviewed by kbarrett (Reviewer). PR: https://git.openjdk.org/jdk/pull/12842
Re: RFR: 8302189: Mark assertion failures noreturn
On Fri, 3 Mar 2023 04:04:25 GMT, Kim Barrett wrote:
> Also 8302799: Refactor Debugging variable usage for noreturn crash reporting
A few initial comments while I try to digest this.
I don't use these particular debugging mechanism, neither Debugger nor
BREAKPOINT, but it seems potentially problematic to me that the removed
BREAKPOINTs happened after the error was reported, and IIUC the new mechanism
will activate before the error is reported.
Thanks.
make/hotspot/lib/CompileJvm.gmk line 103:
> 101: DISABLED_WARNINGS_xlc := tautological-compare shift-negative-value
> 102:
> 103: DISABLED_WARNINGS_microsoft := 4624 4244 4291 4146 4127 4722
It is annoying that we don't document what these warnings are. :(
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. ??
src/hotspot/share/utilities/debug.cpp line 290:
> 288: private:
> 289: ResourceMark _rm;
> 290: DebuggingContext _debugging;
Why a different initialization syntax here?
-
PR: https://git.openjdk.org/jdk/pull/12845
Re: RFR: 8302189: Mark assertion failures noreturn
On Fri, 3 Mar 2023 04:04:25 GMT, Kim Barrett wrote: > Also 8302799: Refactor Debugging variable usage for noreturn crash reporting make/hotspot/lib/CompileJvm.gmk line 103: > 101: DISABLED_WARNINGS_xlc := tautological-compare shift-negative-value > 102: > 103: DISABLED_WARNINGS_microsoft := 4624 4244 4291 4146 4127 4722 I forgot to mention this in the PR summary. New warning suppression for 4722: 'function' : destructor never returns, potential memory leak We have various destructors calling ShouldNotCallThis, ShouldNotReachHere, or the like, which triggers this warning. It might be possible to deal with some (but not all) of those by deleting the destructor. The warning doesn't seem all that useful to us, so I just suppressed it. - PR: https://git.openjdk.org/jdk/pull/12845
RFR: 8302189: Mark assertion failures noreturn
Also 8302799: Refactor Debugging variable usage for noreturn crash reporting - Commit messages: - new implementation of Debugging - noreturn attributes Changes: https://git.openjdk.org/jdk/pull/12845/files Webrev: https://webrevs.openjdk.org/?repo=jdk=12845=00 Issue: https://bugs.openjdk.org/browse/JDK-8302189 Stats: 168 lines in 8 files changed: 128 ins; 19 del; 21 mod Patch: https://git.openjdk.org/jdk/pull/12845.diff Fetch: git fetch https://git.openjdk.org/jdk pull/12845/head:pull/12845 PR: https://git.openjdk.org/jdk/pull/12845
Re: RFR: JDK-8302801: Remove fdlibm C sources [v3]
On Fri, 3 Mar 2023 01:02:40 GMT, Vladimir Kozlov wrote: > Hotspot changes are okay but I'm a bit confused about what the hotspot code > will now be used for? I'm not 100% positive if the current __kernel_rem_pio2 code would be in use. IIRC, back when we used the fsin/fcos instructions to intrinsify sin/cos on the x87 FPU, to meet Java semantics we needed to do argument reduction into the range supported by fsin/fcos. Perhaps __kernel_rem_pio2 is a hold-over from that time? I believe my recent sin/cos algorithms for instrincs wouldn't need to use these pathways. - PR: https://git.openjdk.org/jdk/pull/12821
Re: RFR: JDK-8302801: Remove fdlibm C sources [v3]
On Fri, 3 Mar 2023 00:38:18 GMT, David Holmes wrote: > Actually this is really my lack of understanding about the current code: why > do we intrinsify `Math` but not `StrictMath`? In brief, the Math methods are allowed implementation flexibility in terms of their algorithm but the StrictMath methods are not. The "interesting" StrictMath methods are required to use the FDLIBM algorithms. (One exception is StrictMath.sqrt. Since sqrt is required to be correctly rounded, there is only one correct answer for any given argument and StrictMath.sqrt can be intrinsified to a hardware sqrt instruction just like Math.sqrt can.) - PR: https://git.openjdk.org/jdk/pull/12821
Re: RFR: 8294982: Implementation of Classfile API [v36]
On Thu, 2 Mar 2023 14:31:06 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
>
> 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/TypeKind.java line 45:
> 43: DoubleType("double", "D", 7),
> 44: /** a reference type */
> 45: ReferenceType("reference type", "A", -1),
Suggestion:
ReferenceType("reference type", "L", -1),
?
Otherwise `TypeKind.fromDescriptor(TypeKind.ReferenceType.descriptor())` will
fail.
-
PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v36]
On Thu, 2 Mar 2023 14:31:06 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
>
> 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/NonterminalCodeBuilder.java
line 46:
> 44: this.terminal = switch (parent) {
> 45: case ChainedCodeBuilder cb -> cb.terminal;
> 46: case BlockCodeBuilderImpl cb -> cb.terminal;
Suggestion:
case NonterminalCodeBuilder cb -> cb.terminal;
?
-
PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: JDK-8302801: Remove fdlibm C sources [v3]
On Fri, 3 Mar 2023 00:31:12 GMT, David Holmes wrote: > Hotspot changes are okay but I'm a bit confused about what the hotspot code > will now be used for? `SharedRuntime::*` runtime math functions are used on platforms where there are no HW instructions or intrinsics (Zero VM). JIT compiled code may also call them in such case (or when intrinsics disabled with flag): https://github.com/openjdk/jdk/blob/master/src/hotspot/share/opto/library_call.cpp#L1815 Consider them as default intrinsics for all platforms (since they are written in C). They are faster than interpreting bytecode. They are also needed for results consistency - the same code is used by Interpreter and JITed code. - PR: https://git.openjdk.org/jdk/pull/12821
Re: RFR: 8294982: Implementation of Classfile API [v36]
On Thu, 2 Mar 2023 14:31:06 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
>
> 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.
-
PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: JDK-8302801: Remove fdlibm C sources [v3]
On Thu, 2 Mar 2023 19:55:39 GMT, Joe Darcy wrote: >> While the review of https://github.com/openjdk/jdk/pull/12800 finishes up, I >> thought I'd get out for the review the next phase of the FDLIBM port: >> removing the FDLIBM C sources from the repo. >> >> A repo with the changes for JDK-8302027 and this PR successful build on the >> default set of platform and successfully run tier 1 tests, which includes >> tests of the math library. >> >> There are a few remaining references to the case-independent string "fdlibm" >> in the make directory and HotSpot sources. HotSpot contains a partial fork >> for FDLIBM (a tine of FDLIBM?) to use for intrinsics. The remaining make >> machinery contains logic to determine what set of gcc options can be used >> for the compile. >> >> The intention of this change is to remove use of FDLIBM for the core >> libraries. > > Joe Darcy has updated the pull request incrementally with one additional > commit since the last revision: > > Respond to review feedback. Actually this is really my lack of understanding about the current code: why do we intrinsify `Math` but not `StrictMath`? - PR: https://git.openjdk.org/jdk/pull/12821
Re: RFR: 8294982: Implementation of Classfile API [v36]
On Thu, 2 Mar 2023 14:31:06 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
>
> 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/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.
-
PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: JDK-8302801: Remove fdlibm C sources [v3]
On Thu, 2 Mar 2023 19:55:39 GMT, Joe Darcy wrote: >> While the review of https://github.com/openjdk/jdk/pull/12800 finishes up, I >> thought I'd get out for the review the next phase of the FDLIBM port: >> removing the FDLIBM C sources from the repo. >> >> A repo with the changes for JDK-8302027 and this PR successful build on the >> default set of platform and successfully run tier 1 tests, which includes >> tests of the math library. >> >> There are a few remaining references to the case-independent string "fdlibm" >> in the make directory and HotSpot sources. HotSpot contains a partial fork >> for FDLIBM (a tine of FDLIBM?) to use for intrinsics. The remaining make >> machinery contains logic to determine what set of gcc options can be used >> for the compile. >> >> The intention of this change is to remove use of FDLIBM for the core >> libraries. > > Joe Darcy has updated the pull request incrementally with one additional > commit since the last revision: > > Respond to review feedback. Hotspot changes are okay but I'm a bit confused about what the hotspot code will now be used for? - PR: https://git.openjdk.org/jdk/pull/12821
Re: RFR: 8294982: Implementation of Classfile API [v36]
On Thu, 2 Mar 2023 14:31:06 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
>
> 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`?
-
PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v36]
On Thu, 2 Mar 2023 14:31:06 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
>
> 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?).
-
PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v36]
On Thu, 2 Mar 2023 14:31:06 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
>
> 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 164:
> 162: * generate the code.
> 163: * @return this builder
> 164: */
Suggestion:
/**
* Apply a transform to the code built by a handler, directing results to
this builder.
*
* @param transform the transform to apply to the code built by the handler
* @param handler the handler that receives a {@linkplain CodeBuilder} to
* build the code.
* @return this builder
*/
-
PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8286781: Replace the deprecated/obsolete gethostbyname and inet_addr calls
On Thu, 2 Mar 2023 22:35:17 GMT, David Holmes wrote: > We can replace `gethostbyname`, which is deprecated on Windows and Linux, > with `getaddrinfo`. This API is available on all supported platforms and so > can be placed in shared code. @djelinski pointed out that `getaddrinfo` can > resolve both IP addresses and host names so the two step approach used in > `networkStream::connect` is not necessary and we can do away with > `os::get_host_by_name()` completely. > > The build is updated to enable winsock deprecation warnings, and now we need > to use `ws2_32.lib` we can drop `wsock32.lib` (as it is basically a subset - > again thanks @djelinski ). > > Testing > - all Oracle builds in tiers 1-5 > - All GHA builds > > The actual code change has to be manually tested because the code is only > used by Ideal Graph Printing to connect to the Ideal Graph Visualizer. I've > manually tested on Windows and Linux and @tobiasholenstein tested macOS. > > Thanks. I forgot to mention I have no way to test on AIX so if someone were able to do that, that would be good. Thanks. I know it builds okay. - PR: https://git.openjdk.org/jdk/pull/12842
Re: RFR: 8294982: Implementation of Classfile API [v36]
On Thu, 2 Mar 2023 14:31:06 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
>
> 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 93:
> 91: * directly; they are passed to handlers by methods such as {@link
> 92: * MethodBuilder#withCode(Consumer)} or to code transforms. The elements
> of a
> 93: * code can be specified abstractly (by passing a {@link CodeElement} to
> {@link
Suggestion:
* code can be specified abstractly, by passing a {@link CodeElement} to {@link
-
PR: https://git.openjdk.org/jdk/pull/10982
RFR: 8286781: Replace the deprecated/obsolete gethostbyname and inet_addr calls
We can replace `gethostbyname`, which is deprecated on Windows and Linux, with `getaddrinfo`. This API is available on all supported platforms and so can be placed in shared code. @djelinski pointed out that `getaddrinfo` can resolve both IP addresses and host names so the two step approach used in `networkStream::connect` is not necessary and we can do away with `os::get_host_by_name()` completely. The build is updated to enable winsock deprecation warnings, and now we need to use `ws2_32.lib` we can drop `wsock32.lib` (as it is basically a subset - again thanks @djelinski ). Testing - all Oracle builds in tiers 1-5 - All GHA builds The actual code change has to be manually tested because the code is only used by Ideal Graph Printing to connect to the Ideal Graph Visualizer. I've manually tested on Windows and Linux and @tobiasholenstein tested macOS. Thanks. - Commit messages: - 8286781: Replace the deprecated/obsolete gethostbyname and inet_addr calls Changes: https://git.openjdk.org/jdk/pull/12842/files Webrev: https://webrevs.openjdk.org/?repo=jdk=12842=00 Issue: https://bugs.openjdk.org/browse/JDK-8286781 Stats: 41 lines in 6 files changed: 13 ins; 17 del; 11 mod Patch: https://git.openjdk.org/jdk/pull/12842.diff Fetch: git fetch https://git.openjdk.org/jdk pull/12842/head:pull/12842 PR: https://git.openjdk.org/jdk/pull/12842
Re: RFR: 8303480: Miscellaneous fixes to mostly invisible doc comments
On Thu, 2 Mar 2023 12:03:44 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. The SA changes (jdk.hotspot.agent) look fine. - Marked as reviewed by cjplummer (Reviewer). PR: https://git.openjdk.org/jdk/pull/12826
Re: RFR: 8294982: Implementation of Classfile API [v36]
On Thu, 2 Mar 2023 14:31:06 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
>
> 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).
-
PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8303480: Miscellaneous fixes to mostly invisible doc comments
On Thu, 2 Mar 2023 12:03:44 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. java.desktop changes are fine - Marked as reviewed by prr (Reviewer). PR: https://git.openjdk.org/jdk/pull/12826
Re: RFR: 8294982: Implementation of Classfile API [v36]
On Thu, 2 Mar 2023 14:31:06 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
>
> 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?)
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?
-
PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v36]
On Thu, 2 Mar 2023 14:31:06 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
>
> 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`?
-
PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v36]
On Thu, 2 Mar 2023 14:31:06 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
>
> 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/ExceptionCatch.java
line 77:
> 75: static ExceptionCatch of(Label handler, Label tryStart, Label tryEnd,
> 76: Optional catchTypeEntry) {
> 77: return new AbstractPseudoInstruction.ExceptionCatchImpl(handler,
> tryStart, tryEnd, catchTypeEntry);
Suggestion:
return new AbstractPseudoInstruction.ExceptionCatchImpl(handler,
tryStart, tryEnd, catchTypeEntry.orElse(null));
Then you only need one constructor for `ExceptionCatchImpl` and you don't need
to disambiguate a call using a cast for a final argument that is `null` .
-
PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v15]
On Wed, 15 Feb 2023 14:12:21 GMT, Adam Sotona wrote: >> src/java.base/share/classes/jdk/internal/classfile/instruction/ConstantInstruction.java >> line 63: >> >>> 61: * aload_0}). >>> 62: */ >>> 63: sealed interface IntrinsicConstantInstruction extends >>> ConstantInstruction >> >> I'm not super sure of the fine-grained distinction here. The constant pool >> variant is interesting (as you can ask for the associated constant entry) - >> but the distinction between intrinsics vs. argument seems rather weak. > > They significantly differ in instruction formats and instruction format > distinction is critical for some use cases. I think this distinction is appropriate for the level of modeling. CodeBuilder::constantInstruction(ConstantDesc value) is very useful in selecting the most appropriate specific constant instruction. - PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v36]
On Thu, 2 Mar 2023 14:31:06 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
>
> 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/ConstantInstruction.java
line 41:
> 39: * Models a constant-load instruction in the {@code code} array of a
> {@code
> 40: * Code} attribute, including "intrinsic constant" instructions (e.g.,
> {@code
> 41: * aload_0}), "argument constant" instructions (e.g., {@code bipush}),
> and "load
Suggestion:
* iconst_0}), "argument constant" instructions (e.g., {@code bipush}), and
"load
Otherwise we can refer to `aconst_null`
src/java.base/share/classes/jdk/internal/classfile/instruction/ConstantInstruction.java
line 60:
> 58: /**
> 59: * Models an "intrinsic constant" instruction (e.g., {@code
> 60: * aload_0}).
Suggestion:
* iconst_0}).
-
PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: JDK-8302801: Remove fdlibm C sources [v3]
On Thu, 2 Mar 2023 19:55:39 GMT, Joe Darcy wrote: >> While the review of https://github.com/openjdk/jdk/pull/12800 finishes up, I >> thought I'd get out for the review the next phase of the FDLIBM port: >> removing the FDLIBM C sources from the repo. >> >> A repo with the changes for JDK-8302027 and this PR successful build on the >> default set of platform and successfully run tier 1 tests, which includes >> tests of the math library. >> >> There are a few remaining references to the case-independent string "fdlibm" >> in the make directory and HotSpot sources. HotSpot contains a partial fork >> for FDLIBM (a tine of FDLIBM?) to use for intrinsics. The remaining make >> machinery contains logic to determine what set of gcc options can be used >> for the compile. >> >> The intention of this change is to remove use of FDLIBM for the core >> libraries. > > Joe Darcy has updated the pull request incrementally with one additional > commit since the last revision: > > Respond to review feedback. Looks fine to me. - Marked as reviewed by bpb (Reviewer). PR: https://git.openjdk.org/jdk/pull/12821
Re: RFR: 8294982: Implementation of Classfile API [v36]
On Thu, 2 Mar 2023 14:31:06 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 > > 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() ? - PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: JDK-8302801: Remove fdlibm C sources [v3]
> While the review of https://github.com/openjdk/jdk/pull/12800 finishes up, I > thought I'd get out for the review the next phase of the FDLIBM port: > removing the FDLIBM C sources from the repo. > > A repo with the changes for JDK-8302027 and this PR successful build on the > default set of platform and successfully run tier 1 tests, which includes > tests of the math library. > > There are a few remaining references to the case-independent string "fdlibm" > in the make directory and HotSpot sources. HotSpot contains a partial fork > for FDLIBM (a tine of FDLIBM?) to use for intrinsics. The remaining make > machinery contains logic to determine what set of gcc options can be used for > the compile. > > The intention of this change is to remove use of FDLIBM for the core > libraries. Joe Darcy has updated the pull request incrementally with one additional commit since the last revision: Respond to review feedback. - Changes: - all: https://git.openjdk.org/jdk/pull/12821/files - new: https://git.openjdk.org/jdk/pull/12821/files/481af9ad..417cb739 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=12821=02 - incr: https://webrevs.openjdk.org/?repo=jdk=12821=01-02 Stats: 5 lines in 1 file changed: 0 ins; 1 del; 4 mod Patch: https://git.openjdk.org/jdk/pull/12821.diff Fetch: git fetch https://git.openjdk.org/jdk pull/12821/head:pull/12821 PR: https://git.openjdk.org/jdk/pull/12821
Re: RFR: 8294982: Implementation of Classfile API [v36]
On Thu, 2 Mar 2023 14:31:06 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 > > 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 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). - PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v36]
On Thu, 2 Mar 2023 14:31:06 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
>
> 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.
-
PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: JDK-8302801: Remove fdlibm C sources [v2]
On Thu, 2 Mar 2023 19:19:20 GMT, Brian Burkhalter wrote:
>> Joe Darcy has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Respond to review feedback and add description of transliteration process.
>
> src/java.base/share/classes/java/lang/StrictMath.java line 45:
>
>> 43: * href="https://www.netlib.org/fdlibm/;>{@code fdlibm}. These
>> 44: * algorithms, which are written in the C programming language, are
>> 45: * then to be understood to be transliterated into Java and executed
>
> This change looks good (did not build and view javadoc).
I checked that the change rendered as desired in a local docs build.
-
PR: https://git.openjdk.org/jdk/pull/12821
Re: RFR: 8294982: Implementation of Classfile API [v36]
On Thu, 2 Mar 2023 14:31:06 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
>
> 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?
-
PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: JDK-8302801: Remove fdlibm C sources [v2]
On Thu, 2 Mar 2023 18:31:54 GMT, Joe Darcy wrote: >> While the review of https://github.com/openjdk/jdk/pull/12800 finishes up, I >> thought I'd get out for the review the next phase of the FDLIBM port: >> removing the FDLIBM C sources from the repo. >> >> A repo with the changes for JDK-8302027 and this PR successful build on the >> default set of platform and successfully run tier 1 tests, which includes >> tests of the math library. >> >> There are a few remaining references to the case-independent string "fdlibm" >> in the make directory and HotSpot sources. HotSpot contains a partial fork >> for FDLIBM (a tine of FDLIBM?) to use for intrinsics. The remaining make >> machinery contains logic to determine what set of gcc options can be used >> for the compile. >> >> The intention of this change is to remove use of FDLIBM for the core >> libraries. > > Joe Darcy has updated the pull request incrementally with one additional > commit since the last revision: > > Respond to review feedback and add description of transliteration process. src/hotspot/share/runtime/sharedRuntimeTrig.cpp line 34: > 32: // StrictMath. Avoiding the indirect call through function > 33: // pointer out to libjava.so in SharedRuntime speeds these routines up > 34: // by roughly 15% on multiple architectures. These functions will no longer be in libjava so maybe the "Avoiding the indirect call ..." sentence should be dropped too. - PR: https://git.openjdk.org/jdk/pull/12821
Re: RFR: JDK-8302801: Remove fdlibm C sources [v2]
On Thu, 2 Mar 2023 18:31:54 GMT, Joe Darcy wrote:
>> While the review of https://github.com/openjdk/jdk/pull/12800 finishes up, I
>> thought I'd get out for the review the next phase of the FDLIBM port:
>> removing the FDLIBM C sources from the repo.
>>
>> A repo with the changes for JDK-8302027 and this PR successful build on the
>> default set of platform and successfully run tier 1 tests, which includes
>> tests of the math library.
>>
>> There are a few remaining references to the case-independent string "fdlibm"
>> in the make directory and HotSpot sources. HotSpot contains a partial fork
>> for FDLIBM (a tine of FDLIBM?) to use for intrinsics. The remaining make
>> machinery contains logic to determine what set of gcc options can be used
>> for the compile.
>>
>> The intention of this change is to remove use of FDLIBM for the core
>> libraries.
>
> Joe Darcy has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Respond to review feedback and add description of transliteration process.
src/java.base/share/classes/java/lang/StrictMath.java line 45:
> 43: * href="https://www.netlib.org/fdlibm/;>{@code fdlibm}. These
> 44: * algorithms, which are written in the C programming language, are
> 45: * then to be understood to be transliterated into Java and executed
This change looks good (did not build and view javadoc).
-
PR: https://git.openjdk.org/jdk/pull/12821
Re: RFR: 8294982: Implementation of Classfile API [v36]
On Thu, 2 Mar 2023 14:31:06 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
>
> 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/snippet-files/PackageSnippets.java
line 157:
> 155: if (!(ce instanceof
> MethodModel mm
> 156: &&
> mm.methodName().stringValue().startsWith("debug")))
> 157: classBuilder.with(ce);
Indentation is confusing (even though its less concise its probably better to
use braces for all the examples):
Suggestion:
classBuilder.with(ce);
-
PR: https://git.openjdk.org/jdk/pull/10982
Integrated: 8303442: Clean up w2k_lsa_auth linker parameters
On Wed, 1 Mar 2023 13:06:35 GMT, Daniel Jeliński wrote: > Please review this PR that upgrades winsock lib from version 1 (wsock32) to > version 2 (ws2_32) and removes unnecessary libs from the build. > > With the winsock upgrade we should be able to avoid loading wsock32.dll at > runtime. Winsock is only needed for `htonl` function, and wsock32.dll > forwards that function to ws2_32 anyway. > > All jgss/krb5 (jdk_security4) tests continue to pass. This pull request has now been integrated. Changeset: 843d632a Author:Daniel Jeliński URL: https://git.openjdk.org/jdk/commit/843d632ad4bd372506dd4d1ea0cf4157cb668fc1 Stats: 3 lines in 1 file changed: 0 ins; 2 del; 1 mod 8303442: Clean up w2k_lsa_auth linker parameters Reviewed-by: erikj - PR: https://git.openjdk.org/jdk/pull/12805
Integrated: 8303039: Utilize `coverageLevels.txt`
On Wed, 1 Mar 2023 19:50:56 GMT, Naoto Sato wrote: > This is a pre-requisite for supporting CLDR v43, where they combine `seeds` > locales with `common` locales > (https://cldr.unicode.org/index/downloads/cldr-43#h.7s25aqdv767e). In order > to have the same coverage level of locales, CLDRConverter tool needs to comb > through the locale files based on the `coverageLevels.txt` file, (and the > ones we already included as of v42). Confirmed the same set of locales is > generated before and after this modification. This pull request has now been integrated. Changeset: 0b635579 Author:Naoto Sato URL: https://git.openjdk.org/jdk/commit/0b6355794101bda9de623016ce88f8abbb314f63 Stats: 319 lines in 4 files changed: 315 ins; 1 del; 3 mod 8303039: Utilize `coverageLevels.txt` Reviewed-by: iris, joehw - PR: https://git.openjdk.org/jdk/pull/12812
Re: RFR: JDK-8302801: Remove fdlibm C sources [v2]
On Thu, 2 Mar 2023 14:05:55 GMT, Erik Joelsson wrote: >> Joe Darcy has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Respond to review feedback and add description of transliteration process. > > make/autoconf/buildjdk-spec.gmk.in line 85: > >> 83: JVM_LIBS := @OPENJDK_BUILD_JVM_LIBS@ >> 84: >> 85: FDLIBM_CFLAGS := @OPENJDK_BUILD_FDLIBM_CFLAGS@ > > If the hotspot build still needs `FDLIBM_CFLAGS`, then this line needs to > stay. Okay; added back. - PR: https://git.openjdk.org/jdk/pull/12821
Re: RFR: JDK-8302801: Remove fdlibm C sources [v2]
> While the review of https://github.com/openjdk/jdk/pull/12800 finishes up, I > thought I'd get out for the review the next phase of the FDLIBM port: > removing the FDLIBM C sources from the repo. > > A repo with the changes for JDK-8302027 and this PR successful build on the > default set of platform and successfully run tier 1 tests, which includes > tests of the math library. > > There are a few remaining references to the case-independent string "fdlibm" > in the make directory and HotSpot sources. HotSpot contains a partial fork > for FDLIBM (a tine of FDLIBM?) to use for intrinsics. The remaining make > machinery contains logic to determine what set of gcc options can be used for > the compile. > > The intention of this change is to remove use of FDLIBM for the core > libraries. Joe Darcy has updated the pull request incrementally with one additional commit since the last revision: Respond to review feedback and add description of transliteration process. - Changes: - all: https://git.openjdk.org/jdk/pull/12821/files - new: https://git.openjdk.org/jdk/pull/12821/files/a1a12347..481af9ad Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=12821=01 - incr: https://webrevs.openjdk.org/?repo=jdk=12821=00-01 Stats: 36 lines in 5 files changed: 22 ins; 5 del; 9 mod Patch: https://git.openjdk.org/jdk/pull/12821.diff Fetch: git fetch https://git.openjdk.org/jdk pull/12821/head:pull/12821 PR: https://git.openjdk.org/jdk/pull/12821
Re: RFR: JDK-8302801: Remove fdlibm C sources
On Thu, 2 Mar 2023 07:12:01 GMT, Alan Bateman wrote: > This is a great milestone to get to! Does the comment at the top of > sharedRuntimeTrig.cpp need updating? Updated several of the comments in the HotSpot sources and added a description of the transliteration process to StrictMath. - PR: https://git.openjdk.org/jdk/pull/12821
Re: RFR: 8303039: Utilize `coverageLevels.txt` [v2]
On Thu, 2 Mar 2023 01:03:20 GMT, Naoto Sato wrote: >> This is a pre-requisite for supporting CLDR v43, where they combine `seeds` >> locales with `common` locales >> (https://cldr.unicode.org/index/downloads/cldr-43#h.7s25aqdv767e). In order >> to have the same coverage level of locales, CLDRConverter tool needs to comb >> through the locale files based on the `coverageLevels.txt` file, (and the >> ones we already included as of v42). Confirmed the same set of locales is >> generated before and after this modification. > > Naoto Sato has updated the pull request incrementally with one additional > commit since the last revision: > > Explicitly filter coverage levels Marked as reviewed by joehw (Reviewer). - PR: https://git.openjdk.org/jdk/pull/12812
Re: RFR: 8294982: Implementation of Classfile API [v13]
On Thu, 2 Mar 2023 14:29:13 GMT, Adam Sotona wrote: >> 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. That seems reasonable (top level `StackMapFrameInfo` with nesting for less important stuff.) - PR: https://git.openjdk.org/jdk/pull/10982
Integrated: JDK-8303476: Add the runtime version in the release file of a JDK image
On Wed, 1 Mar 2023 20:28:14 GMT, Mandy Chung wrote: > `$JAVA_HOME/release` file currently includes `JAVA_VERSION` which is the > version > number plus the pre-release identifier for example "21-ea".The build > number > and additional build information are useful in identifying the information > about a JDK image but not available. This proposes to add a new entry in the > release file: > > JAVA_RUNTIME_VERSION="$(VSTR)" > > where `$(VSTR)` is the string represented by `Runtime::version()`. This pull request has now been integrated. Changeset: 32247c33 Author:Mandy Chung URL: https://git.openjdk.org/jdk/commit/32247c336a189a40f696626a2578c65535ef6376 Stats: 322 lines in 3 files changed: 173 ins; 149 del; 0 mod 8303476: Add the runtime version in the release file of a JDK image Reviewed-by: erikj - PR: https://git.openjdk.org/jdk/pull/12813
Re: RFR: 8303039: Utilize `coverageLevels.txt` [v2]
On Thu, 2 Mar 2023 01:03:20 GMT, Naoto Sato wrote: >> This is a pre-requisite for supporting CLDR v43, where they combine `seeds` >> locales with `common` locales >> (https://cldr.unicode.org/index/downloads/cldr-43#h.7s25aqdv767e). In order >> to have the same coverage level of locales, CLDRConverter tool needs to comb >> through the locale files based on the `coverageLevels.txt` file, (and the >> ones we already included as of v42). Confirmed the same set of locales is >> generated before and after this modification. > > Naoto Sato has updated the pull request incrementally with one additional > commit since the last revision: > > Explicitly filter coverage levels Marked as reviewed by iris (Reviewer). - PR: https://git.openjdk.org/jdk/pull/12812
Re: RFR: 8295884: Implement IDE support for Eclipse [v33]
> 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 with a new target base due to a merge or a rebase. The pull request now contains 42 commits: - Workspace File - Merge remote-tracking branch 'upstream/master' into eclipse - classpaths now use templates - Register classpath and cproject - Merge remote-tracking branch 'upstream/master' into eclipse - Progress - Merge remote-tracking branch 'upstream/master' into eclipse - Formatting Cleanup - Proper Checks - Merge remote-tracking branch 'upstream/master' into eclipse - ... and 32 more: https://git.openjdk.org/jdk/compare/b51ea420...cbd0572f - Changes: https://git.openjdk.org/jdk/pull/10853/files Webrev: https://webrevs.openjdk.org/?repo=jdk=10853=32 Stats: 752 lines in 10 files changed: 750 ins; 0 del; 2 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 [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: JDK-8302801: Remove fdlibm C sources
On Thu, 2 Mar 2023 05:54:52 GMT, Joe Darcy wrote: > While the review of https://github.com/openjdk/jdk/pull/12800 finishes up, I > thought I'd get out for the review the next phase of the FDLIBM port: > removing the FDLIBM C sources from the repo. > > A repo with the changes for JDK-8302027 and this PR successful build on the > default set of platform and successfully run tier 1 tests, which includes > tests of the math library. > > There are a few remaining references to the case-independent string "fdlibm" > in the make directory and HotSpot sources. HotSpot contains a partial fork > for FDLIBM (a tine of FDLIBM?) to use for intrinsics. The remaining make > machinery contains logic to determine what set of gcc options can be used for > the compile. > > The intention of this change is to remove use of FDLIBM for the core > libraries. make/autoconf/buildjdk-spec.gmk.in line 85: > 83: JVM_LIBS := @OPENJDK_BUILD_JVM_LIBS@ > 84: > 85: FDLIBM_CFLAGS := @OPENJDK_BUILD_FDLIBM_CFLAGS@ If the hotspot build still needs `FDLIBM_CFLAGS`, then this line needs to stay. - PR: https://git.openjdk.org/jdk/pull/12821
Re: RFR: 8303480: Miscellaneous fixes to mostly invisible doc comments
On Thu, 2 Mar 2023 12:03:44 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. security related changes look fine. - Marked as reviewed by mullan (Reviewer). PR: https://git.openjdk.org/jdk/pull/12826
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
RFR: 8303480: Miscellaneous fixes to mostly invisible doc comments
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 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. - Commit messages: - Initial commit Changes: https://git.openjdk.org/jdk/pull/12826/files Webrev: https://webrevs.openjdk.org/?repo=jdk=12826=00 Issue: https://bugs.openjdk.org/browse/JDK-8303480 Stats: 75 lines in 39 files changed: 0 ins; 0 del; 75 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 [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 [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 [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 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: 8302659: Modernize Windows native code for NetworkInterface [v5]
On Wed, 1 Mar 2023 18:55:05 GMT, Rich DiCroce wrote: >> Improves performance and correctness, as discussed on the net-dev mailing >> list. > > Rich DiCroce has updated the pull request incrementally with one additional > commit since the last revision: > > Revert flag change in ResolverConfigurationImpl Marked as reviewed by djelinski (Committer). - PR: https://git.openjdk.org/jdk/pull/12593
Integrated: 8303355: The Depend plugin does fully recompile when primitive type changes
On Wed, 1 Mar 2023 10:12:41 GMT, Jan Lahoda wrote: > The OpenJDK build is using a Plugin called Depend to avoid building Java code > unnecessarily. It has two parts, one is checking module APIs (and forces > rebuild of dependent modules if a dependency changes), and second takes > modified files in a module, and attempts to detect whether it is enough to > recompile these files, or if the whole module must be rebuilt. > > There's a bug in the second part, it does not track changes in primitive > types, so e.g. a change of a type of a public field form `int` to `long` does > not cause recompile. > > This patch fixes that by visiting the primitive types, and including them in > the hash computed for the given file. > > There's also another problem - if a module is being recompiled from scratch > due to a change in a file hash, the new/updated file hashes are written > immediately. And if the compilation consequently fails, the state is not > compared to the original hash, but to the hash from the broken compilation, > which may lead to missing compilation of some files. > > This patch moves the code to write the new hashes to the end of compilation, > and only do that when there were no compile-time errors during the > compilation. This pull request has now been integrated. Changeset: dbb562d3 Author:Jan Lahoda URL: https://git.openjdk.org/jdk/commit/dbb562d3b128094cb5bca55237e1331e83526adb Stats: 113 lines in 2 files changed: 77 ins; 23 del; 13 mod 8303355: The Depend plugin does fully recompile when primitive type changes Reviewed-by: erikj, vromero - PR: https://git.openjdk.org/jdk/pull/12801
