Re: RFR: 8324632: Update Zlib Data Compression Library to Version 1.3.1
On Mon, 29 Jan 2024 16:57:00 GMT, Lance Andersen wrote: > Hi all, > > Please review this PR which updates zlib from 1.3 to 1.3.1 in OpenJDK > > The [Zlib Data Compression Library](https://github.com/madler/zlib) has > released Zlib 1.3.1 on January 24, 2024. > > There are a [small number of > updates](https://github.com/madler/zlib/compare/v1.3.1..v1.3) between 1.3 and > 1.3.1 > > Mach5 tiers1-3 have run clean Marked as reviewed by alanb (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/17619#pullrequestreview-1850429347
Re: RFR: 8321274: Rename ZipEntry.extraAttributes to ZipEntry.externalAttributes
On Mon, 4 Dec 2023 15:34:34 GMT, Eirik Bjørsnøs wrote: > Please consider this PR which suggests we rename `ZipEntry.extraAttributes` > to `ZipEntry.externalAttributes`. > > This field was introduced in > [JDK-8218021](https://bugs.openjdk.org/browse/JDK-8218021), originally under > the name `ZipEntry.posixPerms`. > [JDK-8250968](https://bugs.openjdk.org/browse/JDK-8250968) later renamed the > field to `ZipEntry.extraAttributes` and extended its semantics to hold the > full two-byte value of the `external file attributes` field, as defined by > `APPNOTE.TXT` > > The name `extraAttributes` is misleading. It has nothing to do with the > `extra field` (an unrelated structure defined in `APPNOTE.TXT`), although the > name indicates it does. > > To prevent confusion and make life easier for future maintainers, I suggest > we rename this field to `ZipEntry.externalAttributes` and update related > methods, parameters and comments accordingly. > > While this change is a straightforward renaming, reviewers should consider > whether it carries its weight, especially considering it might complicate > future backports. > > As a note to reviewers, this PR includes the following intended updates: > > - Rename `ZipEntry.extraAttributes` and any references to this field to > `ZipEntry.externalAttributes` > - Update `JavaUtilZipFileAccess` to similarly rename methods to > `setExternalAttributes` and `getExternalAttributes` > - Rename the field `JarSigner.extraAttrsDetected` to > `JarSigner.externalAttrsDetected` > - Rename a local variable in `JarSigner.writeEntry` to `externalAttrs` > - Rename `s.s.t.jarsigner.Main.extraAttrsDetected` to `externalAttrsDetected` > - Rename resource string key names in `s.s.t.jarsigner.Resources` from > `extra.attributes.detected` to `external.attributes.detected` > - Rename method `SymlinkTest.verifyExtraAttrs` to `verifyExternalAttrs`, also > updated two references to 'extra attributes' in this method > - Updated copyright in all affected files > > If the resource file changes should be dropped and instead handled via > `msgdop` updates, let me know and I can revert the non-default files. > > I did a search across the code base to find 'extraAttrs', 'extra.attr' after > these updates, and found nothing related to zip/jar. The `zip` and `jar` > tests run clean: > > > make test TEST="test/jdk/java/util/jar" > make test TEST="test/jdk/java/util/zip" Hello Eirik, I think this is a reasonable change. I haven't had a chance to review some of these PRs due to some other priority tasks. I have these PRs on my TODO list. So if you want to pursue this further, please go ahead and reopen this and I'll review this in the coming days. - PR Comment: https://git.openjdk.org/jdk/pull/16952#issuecomment-1916110895
Withdrawn: 8321274: Rename ZipEntry.extraAttributes to ZipEntry.externalAttributes
On Mon, 4 Dec 2023 15:34:34 GMT, Eirik Bjørsnøs wrote: > Please consider this PR which suggests we rename `ZipEntry.extraAttributes` > to `ZipEntry.externalAttributes`. > > This field was introduced in > [JDK-8218021](https://bugs.openjdk.org/browse/JDK-8218021), originally under > the name `ZipEntry.posixPerms`. > [JDK-8250968](https://bugs.openjdk.org/browse/JDK-8250968) later renamed the > field to `ZipEntry.extraAttributes` and extended its semantics to hold the > full two-byte value of the `external file attributes` field, as defined by > `APPNOTE.TXT` > > The name `extraAttributes` is misleading. It has nothing to do with the > `extra field` (an unrelated structure defined in `APPNOTE.TXT`), although the > name indicates it does. > > To prevent confusion and make life easier for future maintainers, I suggest > we rename this field to `ZipEntry.externalAttributes` and update related > methods, parameters and comments accordingly. > > While this change is a straightforward renaming, reviewers should consider > whether it carries its weight, especially considering it might complicate > future backports. > > As a note to reviewers, this PR includes the following intended updates: > > - Rename `ZipEntry.extraAttributes` and any references to this field to > `ZipEntry.externalAttributes` > - Update `JavaUtilZipFileAccess` to similarly rename methods to > `setExternalAttributes` and `getExternalAttributes` > - Rename the field `JarSigner.extraAttrsDetected` to > `JarSigner.externalAttrsDetected` > - Rename a local variable in `JarSigner.writeEntry` to `externalAttrs` > - Rename `s.s.t.jarsigner.Main.extraAttrsDetected` to `externalAttrsDetected` > - Rename resource string key names in `s.s.t.jarsigner.Resources` from > `extra.attributes.detected` to `external.attributes.detected` > - Rename method `SymlinkTest.verifyExtraAttrs` to `verifyExternalAttrs`, also > updated two references to 'extra attributes' in this method > - Updated copyright in all affected files > > If the resource file changes should be dropped and instead handled via > `msgdop` updates, let me know and I can revert the non-default files. > > I did a search across the code base to find 'extraAttrs', 'extra.attr' after > these updates, and found nothing related to zip/jar. The `zip` and `jar` > tests run clean: > > > make test TEST="test/jdk/java/util/jar" > make test TEST="test/jdk/java/util/zip" This pull request has been closed without being integrated. - PR: https://git.openjdk.org/jdk/pull/16952
Re: RFR: 8200559: Java agents doing instrumentation need a means to define auxiliary classes [v2]
On Mon, 29 Jan 2024 14:31:10 GMT, Andrew Dinn wrote: > > Bytecode transformation should not be rocket science, but it progressively > > is developing in that direction. > > Hmm? Bytecode transformation of the JDK runtime implementation is a lot more > complicated than your comments seem to acknowledge That is, because I was not talking about JDK runtime transformation but about what the AspectJ weaving agent does: transformation of application classes during class-loading. I am aware of the fact, that it is also possible to retransform already loaded classes, as a special case also bootstrap ones from the JRE. Of course, this is more complicated than the simple case. But my point was, that even the simple case is not simple, if I need to define classes in an arbitrary class loader - not because technically it is difficult, but simply because the JRE API to do so is more and more sealed off with each new Java release. This is also what I mean, when I said, that developers are not treated as adults but "protected" by well-meaning, but ill-doing helicopter parents. > here's the important thing, _it always has been_. No, byte code transformation is not complicated per se. Getting the transformed classes where they need to be is complicated, but artificially so. > You need to remember that instrumenting JDK runtime code involves rebuilding > the engine that you are driving while you are in mid-flight. No, I do not need to remember, because I am aware of that fact. It is just off-topic here with regard to what I asked about. But that other use case, which I have experimented with in another context (test mocking and stubbing) in the past, is an intriguing one, too. I am not underestimating anything there, but for AspectJ it is simply out of scope. Should I ever decide to add the capability to weave aspects into JRE classes, of course that will up the complexity by a notch or two. - PR Comment: https://git.openjdk.org/jdk/pull/3546#issuecomment-1915860036
Re: RFR: 8315487: Security Providers Filter [v6]
> In addition to the goals, scope, motivation, specification and requirement > notes in [JDK-8315487](https://bugs.openjdk.org/browse/JDK-8315487), we would > like to describe the most relevant decisions taken during the implementation > of this enhancement. These notes are organized by feature, may encompass more > than one file or code segment, and are aimed to provide a high-level view of > this PR. > > ## ProvidersFilter > > ### Filter construction (parser) > > The providers filter is constructed from a string value, taken from either a > system or a security property with name "jdk.security.providers.filter". This > process occurs at sun.security.jca.ProvidersFilter class —simply referred as > ProvidersFilter onward— static initialization. Thus, changes to the filter's > overridable property are not effective afterwards and no assumptions should > be made regarding when this class gets initialized. > > The filter's string value is processed with a custom parser of order 'n', > being 'n' the number of characters. The parser, represented by the > ProvidersFilter.Parser class, can be characterized as a Deterministic Finite > Automaton (DFA). The ProvidersFilter.Parser::parse method is the starting > point to get characters from the filter's string value and generate state > transitions in the parser's internal state-machine. See > ProvidersFilter.Parser::nextState for more details about the parser's states > and both valid and invalid transitions. The ParsingState enum defines valid > parser states and Transition the reasons to move between states. If a filter > string cannot be parsed, a ProvidersFilter.ParserException exception is > thrown, and turned into an unchecked IllegalArgumentException in the > ProvidersFilter.Filter constructor. > > While we analyzed —and even tried, at early stages of the development— the > use of regular expressions for filter parsing, we discarded the approach in > order to get maximum performance, support a more advanced syntax and have > flexibility for further extensions in the future. > > ### Filter (structure and behavior) > > A filter is represented by the ProvidersFilter.Filter class. It consists of > an ordered list of rules, returned by the parser, that represents filter > patterns from left to right (see the filter syntax for reference). At the end > of this list, a match-all and deny rule is added for default behavior. When a > service is evaluated against the filter, each filter rule is checked in the > ProvidersFilter.Filter::apply method. The rule makes an allow or deny > decision if the ser... Martin Balao has updated the pull request incrementally with one additional commit since the last revision: Support for cipher transformations and JEP alignment of the java.security documentation. Co-authored-by: Francisco Ferrari Bihurriet Co-authored-by: Martin Balao - Changes: - all: https://git.openjdk.org/jdk/pull/15539/files - new: https://git.openjdk.org/jdk/pull/15539/files/35516004..f015ba87 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=15539&range=05 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=15539&range=04-05 Stats: 555 lines in 9 files changed: 387 ins; 44 del; 124 mod Patch: https://git.openjdk.org/jdk/pull/15539.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/15539/head:pull/15539 PR: https://git.openjdk.org/jdk/pull/15539
Re: RFR: 8303374: Implement JEP 455: Primitive Types in Patterns, instanceof, and switch (Preview) [v56]
> This is the proposed patch for Primitive types in patterns, instanceof, and > switch (Preview). > > Draft spec here: https://cr.openjdk.org/~abimpoudis/instanceof/latest/ Aggelos Biboudis has updated the pull request incrementally with one additional commit since the last revision: Address review by Jan - Changes: - all: https://git.openjdk.org/jdk/pull/15638/files - new: https://git.openjdk.org/jdk/pull/15638/files/e466cfba..8c27c5c0 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=15638&range=55 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=15638&range=54-55 Stats: 165 lines in 12 files changed: 78 ins; 68 del; 19 mod Patch: https://git.openjdk.org/jdk/pull/15638.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/15638/head:pull/15638 PR: https://git.openjdk.org/jdk/pull/15638
RFR: 8324858: [vectorapi] Bounds checking issues when accessing memory segments
The implementation of method `VectorSpecies::fromMemorySegment`, in
`AbstractSpecies::fromMemorySegment`, neglects to perform bounds checks on the
offset argument when the method is compiled by C2 (bounds checks are performed
when interpreted and by C1).
This is an oversight and explicit bounds checks are required, as is already
case for the other load and store memory access methods (including storing to
memory memory segments).
The workaround is to call the static method `{T}Vector::fromMemorySegment`.
The fix is for the implementation(s) of `VectorSpecies::fromMemorySegment` to
do the same and call `{T}Vector::fromMemorySegment`, following the same pattern
for implementations of `VectorSpecies::fromArray`.
The tests have been conservatively updated to call the species access method
were possible in the knowledge that calls the vector access method (the tests
were intended to test out of bounds access when compiled by C2).
Thinking ahead its tempting to remove the species access methods, simplifying
functionality that is duplicated.
-
Commit messages:
- Merge branch 'master' into v-load-segment-bounds-checks
- 8324858: [vectorapi] Bounds checking issues when accessing memory segments
Changes: https://git.openjdk.org/jdk/pull/17621/files
Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=17621&range=00
Issue: https://bugs.openjdk.org/browse/JDK-8324858
Stats: 165 lines in 39 files changed: 56 ins; 8 del; 101 mod
Patch: https://git.openjdk.org/jdk/pull/17621.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/17621/head:pull/17621
PR: https://git.openjdk.org/jdk/pull/17621
Re: RFR: 8303374: Implement JEP 455: Primitive Types in Patterns, instanceof, and switch (Preview) [v55]
On Fri, 26 Jan 2024 18:02:58 GMT, Aggelos Biboudis wrote: >> This is the proposed patch for Primitive types in patterns, instanceof, and >> switch (Preview). >> >> Draft spec here: https://cr.openjdk.org/~abimpoudis/instanceof/latest/ > > Aggelos Biboudis has updated the pull request incrementally with one > additional commit since the last revision: > > Remove redundant null check and introduce a const boolean for > unconditionally exact pairs lgtm - Marked as reviewed by vromero (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/15638#pullrequestreview-1849695360
Re: RFR: JDK-8263261 Extend String::translateEscapes to support unicode escapes [v14]
> Currently String::translateEscapes does not support unicode escapes, reported
> as a IllegalArgumentException("Invalid escape sequence: ...").
> String::translateEscapes should translate unicode escape sequences to provide
> full coverage,
Jim Laskey has updated the pull request incrementally with two additional
commits since the last revision:
- Update test/jdk/java/lang/String/TranslateEscapes.java
Co-authored-by: ExE Boss <3889017+exe-b...@users.noreply.github.com>
- Update src/java.base/share/classes/java/lang/String.java
Co-authored-by: ExE Boss <3889017+exe-b...@users.noreply.github.com>
-
Changes:
- all: https://git.openjdk.org/jdk/pull/17491/files
- new: https://git.openjdk.org/jdk/pull/17491/files/74707a66..61a3abab
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk&pr=17491&range=13
- incr: https://webrevs.openjdk.org/?repo=jdk&pr=17491&range=12-13
Stats: 8 lines in 2 files changed: 0 ins; 7 del; 1 mod
Patch: https://git.openjdk.org/jdk/pull/17491.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/17491/head:pull/17491
PR: https://git.openjdk.org/jdk/pull/17491
Re: RFR: JDK-8263261 Extend String::translateEscapes to support unicode escapes [v13]
On Fri, 26 Jan 2024 17:36:52 GMT, Jim Laskey wrote:
>> Currently String::translateEscapes does not support unicode escapes,
>> reported as a IllegalArgumentException("Invalid escape sequence: ...").
>> String::translateEscapes should translate unicode escape sequences to
>> provide full coverage,
>
> Jim Laskey has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Requested changes
src/java.base/share/classes/java/lang/String.java line 4229:
> 4227: * {@code \u005Cu}
> 4228: * Unicode escape
> 4229: * single UTF-16 code unit equivalent {@code
> U+}multiple 'u' are support per jls 3.3
Suggestion:
* single UTF-16 code unit equivalent {@code U+}multiple 'u'
are supported per JLS 3.3
test/jdk/java/lang/String/TranslateEscapes.java line 120:
> 118: }
> 119: }
> 120:
This method is unused:
Suggestion:
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17491#discussion_r1470081472
PR Review Comment: https://git.openjdk.org/jdk/pull/17491#discussion_r1470082839
Re: RFR: 8323601: Improve LayoutPath.PathElement::toString [v2]
On Mon, 29 Jan 2024 17:22:07 GMT, Per Minborg wrote:
>> Correct. Additional logic is needed to form a correct C syntax. It would be
>> possible to provide a method that does this.
>
> We could add such a method under another issue
> (https://bugs.openjdk.org/browse/JDK-8323746) because it will be much easier
> to implement it once the contemplated changes for that issue are in.
After some more thinking, I think the toString should just mimic the expression
used to create the path.
e.g.
groupElement("foo")
or
sequenceElement()
sequenceElement(2)
sequenceElement(2, 3)
etc.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17417#discussion_r1469972616
Re: RFR: JDK-8263261 Extend String::translateEscapes to support unicode escapes [v13]
On Fri, 26 Jan 2024 17:36:52 GMT, Jim Laskey wrote:
>> Currently String::translateEscapes does not support unicode escapes,
>> reported as a IllegalArgumentException("Invalid escape sequence: ...").
>> String::translateEscapes should translate unicode escape sequences to
>> provide full coverage,
>
> Jim Laskey has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Requested changes
Could I get some reviews for the CSR?
https://bugs.openjdk.org/browse/JDK-8263262
-
PR Comment: https://git.openjdk.org/jdk/pull/17491#issuecomment-1915245775
Re: RFR: 8323621: JDK build should exclude snippet class in java.lang.foreign [v2]
On Mon, 29 Jan 2024 14:03:42 GMT, Per Minborg wrote: >> This PR proposes to remove the snippet files in >> `java/lang/foreign/snippet-files` from the build. > > Per Minborg has updated the pull request incrementally with one additional > commit since the last revision: > > Correct path to excluded directory Marked as reviewed by mcimadamore (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/17403#pullrequestreview-1849345313
Re: RFR: JDK-8263261 Extend String::translateEscapes to support unicode escapes [v13]
On Fri, 26 Jan 2024 17:36:52 GMT, Jim Laskey wrote:
>> Currently String::translateEscapes does not support unicode escapes,
>> reported as a IllegalArgumentException("Invalid escape sequence: ...").
>> String::translateEscapes should translate unicode escape sequences to
>> provide full coverage,
>
> Jim Laskey has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Requested changes
Marked as reviewed by naoto (Reviewer).
-
PR Review: https://git.openjdk.org/jdk/pull/17491#pullrequestreview-1849333765
Re: RFR: 8324632: Update Zlib Data Compression Library to Version 1.3.1
On Mon, 29 Jan 2024 17:15:25 GMT, Alan Bateman wrote: > @LanceAndersen Can you confirm that there are no changes to the 1.3..1.3.1 > diffs? @AlanBateman, yes that is correct, there are no OpenJDK specific changes, it is a straight port of the zlib 1.3.1 changes to our implementation - PR Comment: https://git.openjdk.org/jdk/pull/17619#issuecomment-1915235240
Re: RFR: 8323601: Improve LayoutPath.PathElement::toString [v3]
> This PR proposes to add an improved Improve > `LayoutPath.PathElement::toString` method simplifying debugging. > > Opinions and suggestions for `static PathElement sequenceElement(long start, > long step)` are welcome. Per Minborg 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 four additional commits since the last revision: - Rename local variable - Merge branch 'master' into layout-path-tostring - Rework PathElement:toString - Add toString to PathElement - Changes: - all: https://git.openjdk.org/jdk/pull/17417/files - new: https://git.openjdk.org/jdk/pull/17417/files/83cf10c5..269523b8 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=17417&range=02 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=17417&range=01-02 Stats: 21865 lines in 781 files changed: 12172 ins; 6978 del; 2715 mod Patch: https://git.openjdk.org/jdk/pull/17417.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17417/head:pull/17417 PR: https://git.openjdk.org/jdk/pull/17417
Re: RFR: 8323601: Improve LayoutPath.PathElement::toString [v2]
On Tue, 16 Jan 2024 09:10:04 GMT, Per Minborg wrote: >> src/java.base/share/classes/java/lang/foreign/MemoryLayout.java line 958: >> >>> 956: return new >>> LayoutPath.PathElementImpl(PathKind.DEREF_ELEMENT, >>> 957: LayoutPath::derefElement, >>> 958: "*"); >> >> It seems that this would result in paths like `a.b*` rather than the `*a.b`, >> which is correct C syntax. > > Correct. Additional logic is needed to form a correct C syntax. It would be > possible to provide a method that does this. We could add such a method under another issue (https://bugs.openjdk.org/browse/JDK-8323746) because it will be much easier to implement it once the contemplated changes for that issue are in. - PR Review Comment: https://git.openjdk.org/jdk/pull/17417#discussion_r1469948399
Re: RFR: 8324632: Update Zlib Data Compression Library to Version 1.3.1
On Mon, 29 Jan 2024 16:57:00 GMT, Lance Andersen wrote: > Hi all, > > Please review this PR which updates zlib from 1.3 to 1.3.1 in OpenJDK > > The [Zlib Data Compression Library](https://github.com/madler/zlib) has > released Zlib 1.3.1 on January 24, 2024. > > There are a [small number of > updates](https://github.com/madler/zlib/compare/v1.3.1..v1.3) between 1.3 and > 1.3.1 > > Mach5 tiers1-3 have run clean I've spot-checked these files against the referenced diffs between v1.3.1 and v1.3 and confirm that they are as expected. - Marked as reviewed by iris (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/17619#pullrequestreview-1849300812
Re: RFR: 8324632: Update Zlib Data Compression Library to Version 1.3.1
On Mon, 29 Jan 2024 16:57:00 GMT, Lance Andersen wrote: > Hi all, > > Please review this PR which updates zlib from 1.3 to 1.3.1 in OpenJDK > > The [Zlib Data Compression Library](https://github.com/madler/zlib) has > released Zlib 1.3.1 on January 24, 2024. > > There are a [small number of > updates](https://github.com/madler/zlib/compare/v1.3.1..v1.3) between 1.3 and > 1.3.1 > > Mach5 tiers1-3 have run clean @LanceAndersen Can you confirm that there are no changes to the 1.3..1.3.1 diffs? - PR Comment: https://git.openjdk.org/jdk/pull/17619#issuecomment-1915201608
Integrated: 8324681: Replace NULL with nullptr in HotSpot jtreg test native code files
On Fri, 26 Jan 2024 16:40:32 GMT, Coleen Phillimore wrote: > This mechanically replaces NULL with nullptr in hpp/cpp native files in test > native code. This didn't attempt to change NULL in comments to say null > because nullptr is generally the right thing for the comment to say. It does > attempt to change NULL to "null" rather than "nullptr" in strings. Any > changes for "nullptr" to "null" in comments can be changed in a future RFE in > a smaller patch. I didn't see any when it was scrolling by to make my script > more complicated. > > Ran tier1-4 testing. This pull request has now been integrated. Changeset: a6bdee48 Author:Coleen Phillimore URL: https://git.openjdk.org/jdk/commit/a6bdee48f39993128d8095d40ab417f0102af0f4 Stats: 8218 lines in 750 files changed: 0 ins; 7 del; 8211 mod 8324681: Replace NULL with nullptr in HotSpot jtreg test native code files Reviewed-by: kevinw, kbarrett, dholmes - PR: https://git.openjdk.org/jdk/pull/17593
Re: RFR: 8324681: Replace NULL with nullptr in HotSpot jtreg test native code files [v5]
On Mon, 29 Jan 2024 13:47:10 GMT, Coleen Phillimore wrote: >> This mechanically replaces NULL with nullptr in hpp/cpp native files in test >> native code. This didn't attempt to change NULL in comments to say null >> because nullptr is generally the right thing for the comment to say. It >> does attempt to change NULL to "null" rather than "nullptr" in strings. Any >> changes for "nullptr" to "null" in comments can be changed in a future RFE >> in a smaller patch. I didn't see any when it was scrolling by to make my >> script more complicated. >> >> Ran tier1-4 testing. > > Coleen Phillimore has updated the pull request incrementally with two > additional commits since the last revision: > > - Fix some casts unnecessary with nullptr > - Fix copyrights macos-aarch64 build failure in GHA appears unrelated, internal testing passed. - PR Comment: https://git.openjdk.org/jdk/pull/17593#issuecomment-1915186403
RFR: 8324632: Update Zlib Data Compression Library to Version 1.3.1
Hi all, Please review this PR which updates zlib from 1.3 to 1.3.1 in OpenJDK The [Zlib Data Compression Library](https://github.com/madler/zlib) has released Zlib 1.3.1 on January 24, 2024. There are a [small number of updates](https://github.com/madler/zlib/compare/v1.3.1..v1.3) between 1.3 and 1.3.1 Mach5 tiers1-3 have run clean - Commit messages: - update zlib to zlib 1.3.1 Changes: https://git.openjdk.org/jdk/pull/17619/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=17619&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8324632 Stats: 164 lines in 14 files changed: 82 ins; 35 del; 47 mod Patch: https://git.openjdk.org/jdk/pull/17619.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17619/head:pull/17619 PR: https://git.openjdk.org/jdk/pull/17619
Re: RFR: 8303374: Implement JEP 455: Primitive Types in Patterns, instanceof, and switch (Preview) [v55]
On Fri, 26 Jan 2024 18:02:58 GMT, Aggelos Biboudis
wrote:
>> This is the proposed patch for Primitive types in patterns, instanceof, and
>> switch (Preview).
>>
>> Draft spec here: https://cr.openjdk.org/~abimpoudis/instanceof/latest/
>
> Aggelos Biboudis has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Remove redundant null check and introduce a const boolean for
> unconditionally exact pairs
javac changes look sensible to me - some minor comments inline.
src/jdk.compiler/share/classes/com/sun/tools/javac/code/Types.java line 5040:
> 5038: * @param target Target primitive or reference type
> 5039: */
> 5040: public boolean checkUnconditionallyExact(Type source, Type target) {
Maybe something like `isUnconditionallyExact`?
src/jdk.compiler/share/classes/com/sun/tools/javac/code/Types.java line 5060:
> 5058: * @param targetType Target type
> 5059: */
> 5060: public boolean checkUnconditionallyExactPrimitives(Type
> selectorType, Type targetType) {
Maybe something like `isUnconditionallyExactPrimitives`?
src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Attr.java line 1799:
> 1797: log.error(label.pos(),
> Errors.UnconditionalPatternAndDefault);
> 1798: } else if (booleanSwitch &&
> constants.containsAll(Set.of(0, 1))) {
> 1799: log.error(label.pos(),
> Errors.UnconditionalPatternAndDefault); // TODO improve error
Maybe file a follow-up to improve the error?
src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Lower.java line 3079:
> 3077:
> 3078: // Resolve the exactness method
> 3079: Symbol ecsym = rs.resolveQualifiedMethod(null,
Minor: better use `rs.resolveInternalMethod` or `this.lookupMethod`, so that
the compilation fails more obviously if the method cannot be found.
test/langtools/tools/javac/diags/examples/NotApplicableTypes.java line 21:
> 19: * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
> 20: * or visit www.oracle.com if you need additional information or have any
> 21: * questions.
The key does not exist any, per my understanding. I would suggest to simply
delete the file.
test/langtools/tools/javac/diags/examples/SelectorTypeNotAllowed.java line 24:
> 22: */
> 23:
> 24: // key: compiler.err.preview.feature.disabled.plural
The key does not exist any, per my understanding. I would suggest to simply
delete the file.
-
Marked as reviewed by jlahoda (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/15638#pullrequestreview-1848712425
PR Review Comment: https://git.openjdk.org/jdk/pull/15638#discussion_r1469805646
PR Review Comment: https://git.openjdk.org/jdk/pull/15638#discussion_r1469806136
PR Review Comment: https://git.openjdk.org/jdk/pull/15638#discussion_r1469614384
PR Review Comment: https://git.openjdk.org/jdk/pull/15638#discussion_r1469811642
PR Review Comment: https://git.openjdk.org/jdk/pull/15638#discussion_r1469838360
PR Review Comment: https://git.openjdk.org/jdk/pull/15638#discussion_r1469838529
Re: RFR: JDK-8263261 Extend String::translateEscapes to support unicode escapes [v13]
On Fri, 26 Jan 2024 17:36:52 GMT, Jim Laskey wrote:
>> Currently String::translateEscapes does not support unicode escapes,
>> reported as a IllegalArgumentException("Invalid escape sequence: ...").
>> String::translateEscapes should translate unicode escape sequences to
>> provide full coverage,
>
> Jim Laskey has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Requested changes
Marked as reviewed by rriggs (Reviewer).
-
PR Review: https://git.openjdk.org/jdk/pull/17491#pullrequestreview-1848993761
Re: RFR: 8200559: Java agents doing instrumentation need a means to define auxiliary classes [v2]
On Sat, 27 Jan 2024 05:11:28 GMT, Alexander Kriegisch wrote: > Bytecode transformation should not be rocket science, but it progressively is > developing in that direction. Hmm? Bytecode transformation of the JDK runtime implementation is a lot more complicated than your comments seem to acknowledge and, here's the important thing, *it always has been*. You need to remember that instrumenting JDK runtime code involves rebuilding the engine that you are driving while you are in mid-flight. If you think there are few-to-none hidden gotchas involved in doing that then I suggest you are significantly underestimating the opportunity for things to go wrong -- not just when it comes to instrumenting some specific release of OpenJDK but also when it comes to keeping instrumentation working across legacy and future releases, with all the variety of moving parts that the (necessary) development of the platform requires. The same observation explains why project Jigsaw was needed. The danger of clients using internal JDK runtime APIs -- especially the core runtime APIs -- is much more subtle than many of the programmers who have routinely relied on it recognize. The biggest threat is that public runtime APIs are often implemented via calls to multiple internal APIs -- which may themselves involve multiple entries and re-entries to the JVM. It has always been (and always will be) the case that an isolated call from a client to an internal API can leave the JDK runtime and/or the JVM in an incoherent state because correct use of that internal API requires a correct sequence of invocations with matched inputs and outputs. It is easy even for OpenJDK developers to fails to get this right, especially when calls involve entry to the JVM. The possibility for a programmer who is not very familiar with the JDK runtime code and the JVM code to get it wrong are significant. Worse, the problems may not manifest immediately or in all cases so the danger can be unapparent until disaster strikes. - PR Comment: https://git.openjdk.org/jdk/pull/3546#issuecomment-1914812297
Re: RFR: 8200559: Java agents doing instrumentation need a means to define auxiliary classes [v2]
On Fri, 16 Apr 2021 20:30:15 GMT, Rafael Winterhalter wrote: >> To allow agents the definition of auxiliary classes, an API is needed to >> allow this. Currently, this is often achieved by using `sun.misc.Unsafe` or >> `jdk.internal.misc.Unsafe` ever since the `defineClass` method was removed >> from `sun.misc.Unsafe`. > > Rafael Winterhalter has refreshed the contents of this pull request, and > previous commits have been removed. Incremental views are not available. The > pull request now contains one commit: > > 8200559: Java agents doing instrumentation need a means to define auxiliary > classes There's plenty of them in Byte Buddy and I have seen a bunch in other agents. - PR Comment: https://git.openjdk.org/jdk/pull/3546#issuecomment-1914804192
Re: RFR: 8324681: Replace NULL with nullptr in HotSpot jtreg test native code files [v5]
On Mon, 29 Jan 2024 13:47:10 GMT, Coleen Phillimore wrote: >> This mechanically replaces NULL with nullptr in hpp/cpp native files in test >> native code. This didn't attempt to change NULL in comments to say null >> because nullptr is generally the right thing for the comment to say. It >> does attempt to change NULL to "null" rather than "nullptr" in strings. Any >> changes for "nullptr" to "null" in comments can be changed in a future RFE >> in a smaller patch. I didn't see any when it was scrolling by to make my >> script more complicated. >> >> Ran tier1-4 testing. > > Coleen Phillimore has updated the pull request incrementally with two > additional commits since the last revision: > > - Fix some casts unnecessary with nullptr > - Fix copyrights Thanks Kevin, Kim and David for wading through this. If there are other changes we can address them separately preserving your eyeballs. My copyright script was broken so I fixed it. I'll wait for GHA to make sure I didn't break anything before integrating. - PR Comment: https://git.openjdk.org/jdk/pull/17593#issuecomment-1914798074
Re: RFR: 8200559: Java agents doing instrumentation need a means to define auxiliary classes [v2]
On Sun, 28 Jan 2024 22:33:01 GMT, Rafael Winterhalter
wrote:
> What stops people from supplying a fake instance? Wouldn't you need to "test
> run" the instance first?
Not necessarily. When the generated API implementation relies on the
capabilities of class `Instrumentation` -- such as opening modules -- to
implement the invoked operation the obvious answer is that a fake instance just
won't work.
However, if you want the implementation to validate an incoming call you can
easily arrange for that. For example, provide a method on the agent class that
says yes to its own instance and no for any other instances e.g.
class AgentClass {
private static Instrumentation myInst = null;
void premain(Instrumentation inst) {
myInst = inst;
. . .
}
static boolean validate(Instrumentation inst) {
return myInst != null && inst == myInst;
}
. . .
}
Method validate can be used to ensure API calls only proceed when invoked by
the agent or code that the agent trusts.
-
PR Comment: https://git.openjdk.org/jdk/pull/3546#issuecomment-1914771074
Re: RFR: 8200559: Java agents doing instrumentation need a means to define auxiliary classes [v2]
On Mon, 29 Jan 2024 14:09:40 GMT, Andrew Dinn wrote: > What stops people from supplying a fake instance? Wouldn't you need to "test > run" the instance first? In passing, Instrumentation was a candidate to be sealed at one point as the only implementations should be in the java.instrument module. I haven't seen any delegating or other implementations but they might exist so we would need to be careful with compatibility. - PR Comment: https://git.openjdk.org/jdk/pull/3546#issuecomment-1914776078
Re: RFR: 8323621: JDK build should exclude snippet class in java.lang.foreign [v2]
> This PR proposes to remove the snippet files in > `java/lang/foreign/snippet-files` from the build. Per Minborg has updated the pull request incrementally with one additional commit since the last revision: Correct path to excluded directory - Changes: - all: https://git.openjdk.org/jdk/pull/17403/files - new: https://git.openjdk.org/jdk/pull/17403/files/de3b00a7..e00b123d Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=17403&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=17403&range=00-01 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/17403.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17403/head:pull/17403 PR: https://git.openjdk.org/jdk/pull/17403
Re: RFR: 8323621: JDK build should exclude snippet class in java.lang.foreign [v2]
On Mon, 15 Jan 2024 13:27:25 GMT, Magnus Ihse Bursie wrote: >> If possible, we should simply exclude all files in directories that have `-` >> (minus sign) in their name; this is the intentional design to prevent javac >> from compiling those classes as package names cannot include `-`. > > I agree that this piecemeal approach is not good. I think there is a JBS > enhancement request to filter all "snippet-files" and "javadoc-files" > everywhere. But maybe we can make it broader? Filtering on just `-` makes me > a bit nervous though; it seems like it could unintentionally break at some > point. But maybe filter out all `-files`? It would be good if snippets were compiled (so that the syntax and correctness are ensured) but not included in shipped artifacts. - PR Review Comment: https://git.openjdk.org/jdk/pull/17403#discussion_r1469634458
Re: RFR: 8324681: Replace NULL with nullptr in HotSpot jtreg test native code files [v5]
> This mechanically replaces NULL with nullptr in hpp/cpp native files in test > native code. This didn't attempt to change NULL in comments to say null > because nullptr is generally the right thing for the comment to say. It does > attempt to change NULL to "null" rather than "nullptr" in strings. Any > changes for "nullptr" to "null" in comments can be changed in a future RFE in > a smaller patch. I didn't see any when it was scrolling by to make my script > more complicated. > > Ran tier1-4 testing. Coleen Phillimore has updated the pull request incrementally with two additional commits since the last revision: - Fix some casts unnecessary with nullptr - Fix copyrights - Changes: - all: https://git.openjdk.org/jdk/pull/17593/files - new: https://git.openjdk.org/jdk/pull/17593/files/6eb051ed..6ac8aa85 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=17593&range=04 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=17593&range=03-04 Stats: 32 lines in 27 files changed: 0 ins; 0 del; 32 mod Patch: https://git.openjdk.org/jdk/pull/17593.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17593/head:pull/17593 PR: https://git.openjdk.org/jdk/pull/17593
Re: RFR: 8323159: Consider adding some text re. memory zeroing in Arena::allocate [v5]
On Sun, 28 Jan 2024 00:57:24 GMT, ExE Boss wrote: >> Per Minborg has updated the pull request incrementally with two additional >> commits since the last revision: >> >> - Update copyright year >> - Add test for zero-out > > test/jdk/java/foreign/TestScope.java line 150: > >> 148: } >> 149: >> 150: private static final MemorySegment ZEROED_MEMORY = >> MemorySegment.ofArray(new byte[8102]); > > The nearest power of two is 8192 (213): > Suggestion: > > private static final MemorySegment ZEROED_MEMORY = > MemorySegment.ofArray(new byte[8192]); Good catch. This was a typo. However, the test works as intended and the PR is already integrated. - PR Review Comment: https://git.openjdk.org/jdk/pull/17308#discussion_r1469525320
