Re: RFR: 8285097: Duplicate XML keys in XPATHErrorResources.java and XSLTErrorResources.java [v3]
On Wed, 20 Apr 2022 15:48:54 GMT, Tyler Steele wrote:
>> Shruthi has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Replace the ER_RTF_NOT_SUPPORTED_XRTREEFRAGSELECTWRAPPER key in
>> XPATHErrorResources language files
>
> src/java.xml/share/classes/com/sun/org/apache/xpath/internal/res/XPATHErrorResources.java
> line 599:
>
>> 597:
>> 598: { ER_ASNODEITERATOR_NOT_SUPPORTED_XRTREEFRAGSELECTWRAPPER,
>> 599:"asNodeIterator() not supported by XRTreeFragSelectWrapper"},
>
> For this key, please review places where the old key was used to find places
> where the new key was intended. I believe [this
> line](https://github.com/openjdk/jdk/blob/master/src/java.xml/share/classes/com/sun/org/apache/xpath/internal/objects/XRTreeFragSelectWrapper.java#L155)
> is an example.
[Here](https://github.com/openjdk/jdk/blob/64225e19995e81d2e836ce84befea1a01bb6c860/src/java.xml/share/classes/com/sun/org/apache/xpath/internal/res/XPATHErrorResources_de.java#L595)
is another usage where the other key is intended. I expect you will find
similar references in at least some of the other translation files.
-
PR: https://git.openjdk.java.net/jdk/pull/8318
Re: RFR: 8285097: Duplicate XML keys in XPATHErrorResources.java and XSLTErrorResources.java [v2]
On Mon, 2 May 2022 07:39:39 GMT, Shruthi wrote: >> Shruthi has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Updating last modified tag and XRTreeFragSelectWrapper.java > > `/integrate` LGTM. Nicely done @shruacha1234 - PR: https://git.openjdk.java.net/jdk/pull/8318
Re: RFR: 8285097: Duplicate XML keys in XPATHErrorResources.java and XSLTErrorResources.java [v3]
On Fri, 6 May 2022 14:33:50 GMT, Shruthi wrote:
>> Removing the Duplicate keys present in XSLTErrorResources.java and
>> XPATHErrorResources.java
>>
>> The bug report for the same: https://bugs.openjdk.java.net/browse/JDK-8285097
>
> Shruthi has updated the pull request incrementally with one additional commit
> since the last revision:
>
> Replace the ER_RTF_NOT_SUPPORTED_XRTREEFRAGSELECTWRAPPER key in
> XPATHErrorResources language files
Marked as reviewed by backwater...@github.com (no known OpenJDK username).
src/java.xml/share/classes/com/sun/org/apache/xpath/internal/res/XPATHErrorResources.java
line 599:
> 597:
> 598: { ER_ASNODEITERATOR_NOT_SUPPORTED_XRTREEFRAGSELECTWRAPPER,
> 599:"asNodeIterator() not supported by XRTreeFragSelectWrapper"},
For this key, please review places where the old key was used to find places
where the new key was intended. I believe [this
line](https://github.com/openjdk/jdk/blob/master/src/java.xml/share/classes/com/sun/org/apache/xpath/internal/objects/XRTreeFragSelectWrapper.java#L155)
is an example.
-
PR: https://git.openjdk.java.net/jdk/pull/8318
Integrated: 8286029: Add classpath exemption to globals_vectorApiSupport_***.S.inc
On Mon, 2 May 2022 20:05:36 GMT, Tyler Steele wrote: > Adds missing classpath exception to the header of two GPLv2 files. > > Requested > [here](https://mail.openjdk.java.net/pipermail/jdk-updates-dev/2022-April/013988.html). This pull request has now been integrated. Changeset: 6a1b145a Author:Tyler Steele Committer: Sandhya Viswanathan URL: https://git.openjdk.java.net/jdk/commit/6a1b145a0ab0057037f813f7dd6e71ad5b6f3de2 Stats: 8 lines in 2 files changed: 4 ins; 0 del; 4 mod 8286029: Add classpath exemption to globals_vectorApiSupport_***.S.inc Reviewed-by: sviswanathan - PR: https://git.openjdk.java.net/jdk/pull/8508
Re: RFR: 8286029: Add classpath exemption to globals_vectorApiSupport_***.S.inc [v2]
On Wed, 4 May 2022 17:42:05 GMT, Sandhya Viswanathan wrote: >> Tyler Steele has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Update copyright header year > > Marked as reviewed by sviswanathan (Reviewer). Thank you @sviswa7. > src/jdk.incubator.vector/linux/native/libjsvml/globals_vectorApiSupport_linux.S.inc > line 4: > >> 2: * Copyright (c) 1997, 2021, Oracle and/or its affiliates. All rights >> reserved. >> 3: * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. >> 4: * > > Please update the copyright year to 2022. - PR: https://git.openjdk.java.net/jdk/pull/8508
Re: RFR: 8286029: Add classpath exemption to globals_vectorApiSupport_***.S.inc [v2]
> Adds missing classpath exception to the header of two GPLv2 files. > > Requested > [here](https://mail.openjdk.java.net/pipermail/jdk-updates-dev/2022-April/013988.html). Tyler Steele has updated the pull request incrementally with one additional commit since the last revision: Update copyright header year - Changes: - all: https://git.openjdk.java.net/jdk/pull/8508/files - new: https://git.openjdk.java.net/jdk/pull/8508/files/72f74f0e..cdae1312 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8508=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8508=00-01 Stats: 2 lines in 2 files changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/8508.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8508/head:pull/8508 PR: https://git.openjdk.java.net/jdk/pull/8508
RFR: 8286029: Add classpath exemption to globals_vectorApiSupport_***.S.inc
Adds missing classpath exception to the header of two GPLv2 files. Requested [here](https://mail.openjdk.java.net/pipermail/jdk-updates-dev/2022-April/013988.html). - Commit messages: - Add classpath excemption to copyright header Changes: https://git.openjdk.java.net/jdk/pull/8508/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=8508=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8286029 Stats: 6 lines in 2 files changed: 4 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/8508.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8508/head:pull/8508 PR: https://git.openjdk.java.net/jdk/pull/8508
Re: RFR: 8285097: Duplicate XML keys in XPATHErrorResources.java and XSLTErrorResources.java [v2]
On Fri, 29 Apr 2022 13:31:30 GMT, Shruthi wrote: >> Removing the Duplicate keys present in XSLTErrorResources.java and >> XPATHErrorResources.java >> >> The bug report for the same: https://bugs.openjdk.java.net/browse/JDK-8285097 > > Shruthi has updated the pull request incrementally with one additional commit > since the last revision: > > Updating last modified tag and XRTreeFragSelectWrapper.java It also looks like you have not run the pre-tests yet. Please enable them in GitHub Actions and confirm that they pass. https://wiki.openjdk.java.net/display/SKARA/Testing - PR: https://git.openjdk.java.net/jdk/pull/8318
Re: RFR: 8285097: Duplicate XML keys in XPATHErrorResources.java and XSLTErrorResources.java [v2]
On Mon, 2 May 2022 07:39:39 GMT, Shruthi wrote: >> Shruthi has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Updating last modified tag and XRTreeFragSelectWrapper.java > > `/integrate` @shruacha1234 Please don't integrate without addressing the comments left in my review. There are several files that still use the old key incorrectly. - PR: https://git.openjdk.java.net/jdk/pull/8318
Re: RFR: 8285097: Duplicate XML keys in XPATHErrorResources.java and XSLTErrorResources.java
On Wed, 20 Apr 2022 15:37:13 GMT, Shruthi wrote: > Removing the Duplicate keys present in XSLTErrorResources.java and > XPATHErrorResources.java > > The bug report for the same: https://bugs.openjdk.java.net/browse/JDK-8285097 @shruacha1234, a few notes specific to openJDK PRs: - Please change the issue name to: "8285097: Duplicate XML keys in XPATHErrorResources.java and XSLTErrorResources.java" this will correctly link the issue in bugs.openjdk.net - Please enable github actions. This allows the pre-tests to run. - PR: https://git.openjdk.java.net/jdk/pull/8318
Integrated: 8283287: ClassLoader.c cleanups
On Wed, 16 Mar 2022 21:25:37 GMT, Tyler Steele wrote: > As mentioned in the issue, I'd like to perform the following tidying on > ClassLoader.c > > - Alphabetize includes. > - Replace 'if (ptr == 0)' with 'if (ptr == NULL)'. > - Replace 'return 0' with 'return NULL'. This pull request has now been integrated. Changeset: 3e58a438 Author:Tyler Steele Committer: Thomas Stuefe URL: https://git.openjdk.java.net/jdk/commit/3e58a438e9051d4c976273eea35e36d37d5428c3 Stats: 33 lines in 1 file changed: 8 ins; 5 del; 20 mod 8283287: ClassLoader.c cleanups Reviewed-by: stuefe, alanb, rriggs - PR: https://git.openjdk.java.net/jdk/pull/7846
Re: RFR: 8283287: ClassLoader.c cleanups [v5]
On Fri, 18 Mar 2022 15:40:09 GMT, Tyler Steele wrote: >> As mentioned in the issue, I'd like to perform the following tidying on >> ClassLoader.c >> >> - Alphabetize includes. >> - Replace 'if (ptr == 0)' with 'if (ptr == NULL)'. >> - Replace 'return 0' with 'return NULL'. > > Tyler Steele has updated the pull request incrementally with one additional > commit since the last revision: > > Makes requested readability changes Thanks :-) - PR: https://git.openjdk.java.net/jdk/pull/7846
Re: RFR: 8283287: ClassLoader.c cleanups [v5]
On Fri, 18 Mar 2022 15:40:09 GMT, Tyler Steele wrote: >> As mentioned in the issue, I'd like to perform the following tidying on >> ClassLoader.c >> >> - Alphabetize includes. >> - Replace 'if (ptr == 0)' with 'if (ptr == NULL)'. >> - Replace 'return 0' with 'return NULL'. > > Tyler Steele has updated the pull request incrementally with one additional > commit since the last revision: > > Makes requested readability changes Made some additional readability changes that were requested after #7829 closed. They felt appropriate here since it's already a cleanup PR. All changes complete. Please sponsor when ready. - PR: https://git.openjdk.java.net/jdk/pull/7846
Re: RFR: 8283287: ClassLoader.c cleanups [v5]
> As mentioned in the issue, I'd like to perform the following tidying on > ClassLoader.c > > - Alphabetize includes. > - Replace 'if (ptr == 0)' with 'if (ptr == NULL)'. > - Replace 'return 0' with 'return NULL'. Tyler Steele has updated the pull request incrementally with one additional commit since the last revision: Makes requested readability changes - Changes: - all: https://git.openjdk.java.net/jdk/pull/7846/files - new: https://git.openjdk.java.net/jdk/pull/7846/files/2fd5b368..9a8e6b6f Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=7846=04 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7846=03-04 Stats: 8 lines in 1 file changed: 2 ins; 0 del; 6 mod Patch: https://git.openjdk.java.net/jdk/pull/7846.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7846/head:pull/7846 PR: https://git.openjdk.java.net/jdk/pull/7846
Re: RFR: 8283287: ClassLoader.c cleanups [v4]
On Fri, 18 Mar 2022 15:13:17 GMT, Tyler Steele wrote: >> As mentioned in the issue, I'd like to perform the following tidying on >> ClassLoader.c >> >> - Alphabetize includes. >> - Replace 'if (ptr == 0)' with 'if (ptr == NULL)'. >> - Replace 'return 0' with 'return NULL'. > > Tyler Steele has updated the pull request incrementally with one additional > commit since the last revision: > > Removes offending whitespace Please do not sponsor yet. - PR: https://git.openjdk.java.net/jdk/pull/7846
Re: RFR: 8283225: ClassLoader.c produces incorrect OutOfMemory Exception when length is 0 (aix) [v4]
On Fri, 18 Mar 2022 07:43:21 GMT, Alan Bateman wrote: >> Tyler Steele has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Improve comment >> >> - Reword to avoid double use of malloc(X) >> - Remove bug id > > src/java.base/share/native/libjava/ClassLoader.c line 104: > >> 102: // On AIX malloc(0) returns NULL which looks like an out-of-memory >> condition; so adjust it to malloc(1) >> 103: #ifdef _AIX >> 104: body = (jbyte *)malloc(length == 0 ? 1 : length); > > Can we use identification in the ifdef/else/endif block to make it a bit more > readable. Also can you trim down the comment or split it over two lines to > avoid the really long line (it makes it a bit easier for future side-by-side > reviews). I can split down the comment if you prefer. It might be appropriate to do in the as-yet-unmerged cleanup PR I have open for the same file. I am not sure what you mean by 'use identification'. Can you clarify? - PR: https://git.openjdk.java.net/jdk/pull/7829
Re: RFR: 8283225: ClassLoader.c produces incorrect OutOfMemory Exception when length is 0 (aix) [v4]
On Fri, 18 Mar 2022 15:25:20 GMT, ExE Boss wrote: >> I can split down the comment if you prefer. It might be appropriate to do in >> the as-yet-unmerged cleanup PR I have open for the same file. >> >> I am not sure what you mean by 'use identification'. Can you clarify? > > I think it’s a typo of “indentation”, e.g.: > Suggestion: > > body = (jbyte *)malloc(length == 0 ? 1 : length); Ah, that does make sense. Please request these changes [here](https://github.com/openjdk/jdk/pull/7846) and I am happy to make them. - PR: https://git.openjdk.java.net/jdk/pull/7829
Re: RFR: 8283287: ClassLoader.c cleanups [v4]
> As mentioned in the issue, I'd like to perform the following tidying on > ClassLoader.c > > - Alphabetize includes. > - Replace 'if (ptr == 0)' with 'if (ptr == NULL)'. > - Replace 'return 0' with 'return NULL'. Tyler Steele has updated the pull request incrementally with one additional commit since the last revision: Removes offending whitespace - Changes: - all: https://git.openjdk.java.net/jdk/pull/7846/files - new: https://git.openjdk.java.net/jdk/pull/7846/files/d7e67d4c..2fd5b368 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=7846=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7846=02-03 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/7846.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7846/head:pull/7846 PR: https://git.openjdk.java.net/jdk/pull/7846
Re: RFR: 8283287: ClassLoader.c cleanups [v3]
> As mentioned in the issue, I'd like to perform the following tidying on > ClassLoader.c > > - Alphabetize includes. > - Replace 'if (ptr == 0)' with 'if (ptr == NULL)'. > - Replace 'return 0' with 'return NULL'. Tyler Steele has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains three commits: - Merge branch 'master' into ClassLoaderCleanup - removes unneeded errno.h - Perform minor clean up ClassLoader.c - Alphabatizes includes. - Replaces 'if (ptr == 0)' with 'if (ptr == NULL)'. - Replaces 'return 0' with 'return NULL'. - Changes: https://git.openjdk.java.net/jdk/pull/7846/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=7846=02 Stats: 26 lines in 1 file changed: 6 ins; 5 del; 15 mod Patch: https://git.openjdk.java.net/jdk/pull/7846.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7846/head:pull/7846 PR: https://git.openjdk.java.net/jdk/pull/7846
Integrated: 8283225: ClassLoader.c produces incorrect OutOfMemory Exception when length is 0 (aix)
On Tue, 15 Mar 2022 22:58:48 GMT, Tyler Steele wrote: > As described in the linked issue, NullClassBytesTest fails due an > OutOfMemoryError produced on AIX when the test calls defineClass with a byte > array of size of 0. The native implementation of defineClass then calls > malloc with a size of 0. On AIX malloc(0) returns NULL, while on other > platforms it return a valid address. When NULL is produced by malloc for this > reason, ClassLoader.c incorrectly interprets this as a failure due to a lack > of memory. > > ~~This PR modifies ClassLoader.c to produce an OutOfMemoryError only when > `errno == ENOMEM` and to produce a ClassFormatError with the message > "ClassLoader internal allocation failure" in all other cases (in which malloc > returns NULL).~~ [edit: The above no longer describes the PR's proposed fix. > See discussion below] > > In addition, I performed some minor tidy-up work in ClassLoader.c by changing > instances of `return 0` to `return NULL`, and `if (some_ptr == 0)` to `if > (some_ptr == NULL)`. This was done to improve the clarity of the code in > ClassLoader.c, but didn't feel worthy of opening a separate issue. > > ### Alternatives > > It would be possible to address this failure by modifying the test to accept > the OutOfMemoryError on AIX. I thought it was a better solution to modify > ClassLoader.c to produce an OutOfMemoryError only when the system is actually > out of memory. > > ### Testing > > This change has been tested on AIX and Linux/x86. This pull request has now been integrated. Changeset: cab4ff64 Author:Tyler Steele Committer: Thomas Stuefe URL: https://git.openjdk.java.net/jdk/commit/cab4ff64541393a974ea91e35167668ef0036804 Stats: 11 lines in 1 file changed: 11 ins; 0 del; 0 mod 8283225: ClassLoader.c produces incorrect OutOfMemory Exception when length is 0 (aix) Reviewed-by: stuefe, rriggs, dholmes - PR: https://git.openjdk.java.net/jdk/pull/7829
Re: RFR: 8283225: ClassLoader.c produces incorrect OutOfMemory Exception when length is 0 (aix) [v3]
On Thu, 17 Mar 2022 14:48:09 GMT, Roger Riggs wrote: >> src/java.base/share/native/libjava/ClassLoader.c line 102: >> >>> 100: } >>> 101: >>> 102: // On malloc(0), implementators of malloc(3) have the choice to >>> return either >> >> It is confusing to mix `malloc(0)`, where you are passing an argument zero >> to malloc, with `malloc(3)` which actually means the definition of malloc as >> per the man page in section 3. >> >> Given this is only an issue on AIX the comment can simply say: >> >> `// On AIX malloc(0) returns NULL which looks like an out-of-memory >> condition; so adjust it to malloc(1)`. > > I would omit the bug number reference, they get stale and do not age well, > cluttering up the source. > Git blame can be used to find the origin of the comment if needed. I changed to the streamlined comment and removed the bug id. Thanks for your suggestions. - PR: https://git.openjdk.java.net/jdk/pull/7829
Re: RFR: 8283225: ClassLoader.c produces incorrect OutOfMemory Exception when length is 0 (aix) [v3]
On Thu, 17 Mar 2022 07:46:21 GMT, Thomas Stuefe wrote: >> src/java.base/share/native/libjava/ClassLoader.c line 106: >> >>> 104: // we chose the latter. (see 8283225) >>> 105: #ifdef _AIX >>> 106: body = (jbyte *)malloc(length == 0 ? 1 : length); >> >> Using AIX_ONLY this can be simplified: >> >> `body = (jbyte *)malloc(length AIX_ONLY( == 0 ? 1 : length));` > >> Using AIX_ONLY this can be simplified: >> >> `body = (jbyte *)malloc(length AIX_ONLY( == 0 ? 1 : length));` > > This is jdk, not hotspot. Do we have AIX_ONLY in the JDK? As Thomas mentioned, this does not seem to work here. - PR: https://git.openjdk.java.net/jdk/pull/7829
Re: RFR: 8283225: ClassLoader.c produces incorrect OutOfMemory Exception when length is 0 (aix) [v4]
> As described in the linked issue, NullClassBytesTest fails due an > OutOfMemoryError produced on AIX when the test calls defineClass with a byte > array of size of 0. The native implementation of defineClass then calls > malloc with a size of 0. On AIX malloc(0) returns NULL, while on other > platforms it return a valid address. When NULL is produced by malloc for this > reason, ClassLoader.c incorrectly interprets this as a failure due to a lack > of memory. > > ~~This PR modifies ClassLoader.c to produce an OutOfMemoryError only when > `errno == ENOMEM` and to produce a ClassFormatError with the message > "ClassLoader internal allocation failure" in all other cases (in which malloc > returns NULL).~~ [edit: The above no longer describes the PR's proposed fix. > See discussion below] > > In addition, I performed some minor tidy-up work in ClassLoader.c by changing > instances of `return 0` to `return NULL`, and `if (some_ptr == 0)` to `if > (some_ptr == NULL)`. This was done to improve the clarity of the code in > ClassLoader.c, but didn't feel worthy of opening a separate issue. > > ### Alternatives > > It would be possible to address this failure by modifying the test to accept > the OutOfMemoryError on AIX. I thought it was a better solution to modify > ClassLoader.c to produce an OutOfMemoryError only when the system is actually > out of memory. > > ### Testing > > This change has been tested on AIX and Linux/x86. Tyler Steele has updated the pull request incrementally with one additional commit since the last revision: Improve comment - Reword to avoid double use of malloc(X) - Remove bug id - Changes: - all: https://git.openjdk.java.net/jdk/pull/7829/files - new: https://git.openjdk.java.net/jdk/pull/7829/files/f05c8d85..f0e2800e Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=7829=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7829=02-03 Stats: 6 lines in 1 file changed: 0 ins; 4 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/7829.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7829/head:pull/7829 PR: https://git.openjdk.java.net/jdk/pull/7829
Re: RFR: 8283287: ClassLoader.c cleanups [v2]
> As mentioned in the issue, I'd like to perform the following tidying on > ClassLoader.c > > - Alphabetize includes. > - Replace 'if (ptr == 0)' with 'if (ptr == NULL)'. > - Replace 'return 0' with 'return NULL'. Tyler Steele has updated the pull request incrementally with one additional commit since the last revision: removes unneeded errno.h - Changes: - all: https://git.openjdk.java.net/jdk/pull/7846/files - new: https://git.openjdk.java.net/jdk/pull/7846/files/57a92415..920e76fb Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=7846=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7846=00-01 Stats: 1 line in 1 file changed: 0 ins; 1 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/7846.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7846/head:pull/7846 PR: https://git.openjdk.java.net/jdk/pull/7846
Re: RFR: 8283287: ClassLoader.c cleanups
On Thu, 17 Mar 2022 10:46:35 GMT, Thomas Stuefe wrote: >> As mentioned in the issue, I'd like to perform the following tidying on >> ClassLoader.c >> >> - Alphabetize includes. >> - Replace 'if (ptr == 0)' with 'if (ptr == NULL)'. >> - Replace 'return 0' with 'return NULL'. > > src/java.base/share/native/libjava/ClassLoader.c line 27: > >> 25: >> 26: #include >> 27: #include > > Whats errno.h for? Thanks for catching that. It was for [the other PR](https://github.com/openjdk/jdk/pull/7829). - PR: https://git.openjdk.java.net/jdk/pull/7846
RFR: 8283287: Spring Cleaning for ClassLoader.c
As mentioned in the issue, I'd like to perform the following tidying on ClassLoader.c - Alphabetize includes. - Replace 'if (ptr == 0)' with 'if (ptr == NULL)'. - Replace 'return 0' with 'return NULL'. - Commit messages: - Perform minor clean up ClassLoader.c Changes: https://git.openjdk.java.net/jdk/pull/7846/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=7846=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8283287 Stats: 26 lines in 1 file changed: 7 ins; 5 del; 14 mod Patch: https://git.openjdk.java.net/jdk/pull/7846.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7846/head:pull/7846 PR: https://git.openjdk.java.net/jdk/pull/7846
Re: RFR: 8283225: [AIX] ClassLoader.c produces incorrect OutOfMemory Exception when length is 0 [v2]
On Wed, 16 Mar 2022 21:06:04 GMT, Roger Riggs wrote: >> Hmm, maybe I misunderstand the idea behind this bug tag. Don't we want to >> list the bug ids of any issues the test caught? > > Usually, the bug number implies the test was modified to cover the issue. > Otherwise, we end up with very large bug lists and they lose their meaning. Thanks. That's good to know. - PR: https://git.openjdk.java.net/jdk/pull/7829
Re: RFR: 8283225: [AIX] ClassLoader.c produces incorrect OutOfMemory Exception when length is 0 [v3]
On Wed, 16 Mar 2022 21:10:14 GMT, Tyler Steele wrote: >> As described in the linked issue, NullClassBytesTest fails due an >> OutOfMemoryError produced on AIX when the test calls defineClass with a byte >> array of size of 0. The native implementation of defineClass then calls >> malloc with a size of 0. On AIX malloc(0) returns NULL, while on other >> platforms it return a valid address. When NULL is produced by malloc for >> this reason, ClassLoader.c incorrectly interprets this as a failure due to a >> lack of memory. >> >> ~~This PR modifies ClassLoader.c to produce an OutOfMemoryError only when >> `errno == ENOMEM` and to produce a ClassFormatError with the message >> "ClassLoader internal allocation failure" in all other cases (in which >> malloc returns NULL).~~ [edit: The above no longer describes the PR's >> proposed fix. See discussion below] >> >> In addition, I performed some minor tidy-up work in ClassLoader.c by >> changing instances of `return 0` to `return NULL`, and `if (some_ptr == 0)` >> to `if (some_ptr == NULL)`. This was done to improve the clarity of the code >> in ClassLoader.c, but didn't feel worthy of opening a separate issue. >> >> ### Alternatives >> >> It would be possible to address this failure by modifying the test to accept >> the OutOfMemoryError on AIX. I thought it was a better solution to modify >> ClassLoader.c to produce an OutOfMemoryError only when the system is >> actually out of memory. >> >> ### Testing >> >> This change has been tested on AIX and Linux/x86. > > Tyler Steele has refreshed the contents of this pull request, and previous > commits have been removed. The incremental views will show differences > compared to the previous content of the PR. The pull request contains one new > commit since the last revision: > > Addresses failure in NullClassTest on AIX. > > - Changes malloc(0) call to malloc(1) on AIX. The requested changes have been made, and a better, leaner PR results. In addition to the changes mentioned above, I also incorporated Thomas' suggestion to use `#ifdef _AIX` to ensure the change only happens on AIX where it's needed. I thought I'd skip the call to 'my_malloc' since the change is only required in two places. - PR: https://git.openjdk.java.net/jdk/pull/7829
Re: RFR: 8283225: [AIX] ClassLoader.c produces incorrect OutOfMemory Exception when length is 0 [v3]
> As described in the linked issue, NullClassBytesTest fails due an > OutOfMemoryError produced on AIX when the test calls defineClass with a byte > array of size of 0. The native implementation of defineClass then calls > malloc with a size of 0. On AIX malloc(0) returns NULL, while on other > platforms it return a valid address. When NULL is produced by malloc for this > reason, ClassLoader.c incorrectly interprets this as a failure due to a lack > of memory. > > ~~This PR modifies ClassLoader.c to produce an OutOfMemoryError only when > `errno == ENOMEM` and to produce a ClassFormatError with the message > "ClassLoader internal allocation failure" in all other cases (in which malloc > returns NULL).~~ [edit: The above no longer describes the PR's proposed fix. > See discussion below] > > In addition, I performed some minor tidy-up work in ClassLoader.c by changing > instances of `return 0` to `return NULL`, and `if (some_ptr == 0)` to `if > (some_ptr == NULL)`. This was done to improve the clarity of the code in > ClassLoader.c, but didn't feel worthy of opening a separate issue. > > ### Alternatives > > It would be possible to address this failure by modifying the test to accept > the OutOfMemoryError on AIX. I thought it was a better solution to modify > ClassLoader.c to produce an OutOfMemoryError only when the system is actually > out of memory. > > ### Testing > > This change has been tested on AIX and Linux/x86. Tyler Steele has refreshed the contents of this pull request, and previous commits have been removed. The incremental views will show differences compared to the previous content of the PR. The pull request contains one new commit since the last revision: Addresses failure in NullClassTest on AIX. - Changes malloc(0) call to malloc(1) on AIX. - Changes: - all: https://git.openjdk.java.net/jdk/pull/7829/files - new: https://git.openjdk.java.net/jdk/pull/7829/files/de946b41..f05c8d85 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=7829=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7829=01-02 Stats: 37 lines in 2 files changed: 12 ins; 6 del; 19 mod Patch: https://git.openjdk.java.net/jdk/pull/7829.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7829/head:pull/7829 PR: https://git.openjdk.java.net/jdk/pull/7829
Re: RFR: 8283225: [AIX] ClassLoader.c produces incorrect OutOfMemory Exception when length is 0 [v2]
On Wed, 16 Mar 2022 20:19:08 GMT, Roger Riggs wrote: >> Tyler Steele has updated the pull request incrementally with four additional >> commits since the last revision: >> >> - Fixes type warning. >> - Removes unneeded ClassFormatError from ClassLoader.c >> - Revert "Extract memory error logic to helper procedure" >> >>This reverts commit b631eb0ccd5f3748c2010c864f8ccef0c1da9c42. >> - Avoid calling malloc with size zero. > > test/hotspot/jtreg/runtime/DefineClass/NullClassBytesTest.java line 26: > >> 24: /* >> 25: * @test >> 26: * @bug 8262913 8283225 > > All of the changes can be removed from NullClassBytesTest.java. (copyright > and bug #) Hmm, maybe I misunderstand the idea behind this bug tag. Don't we want to list the bug ids of any issues the test caught? - PR: https://git.openjdk.java.net/jdk/pull/7829
Re: RFR: 8283225: [AIX] ClassLoader.c produces incorrect OutOfMemory Exception when length is 0 [v2]
On Wed, 16 Mar 2022 20:17:50 GMT, Roger Riggs wrote: >> Tyler Steele has updated the pull request incrementally with four additional >> commits since the last revision: >> >> - Fixes type warning. >> - Removes unneeded ClassFormatError from ClassLoader.c >> - Revert "Extract memory error logic to helper procedure" >> >>This reverts commit b631eb0ccd5f3748c2010c864f8ccef0c1da9c42. >> - Avoid calling malloc with size zero. > > src/java.base/share/native/libjava/ClassLoader.c line 106: > >> 104: // NULL or a unique non-NULL pointer. To unify libc behavior across >> our platforms >> 105: // we chose the latter. (see 8283225) >> 106: body = (jbyte *)malloc(length < 1 ? 1 : length); > > This code conflates a length == in the comment with length < 1 in the code. > If the issue is with length == 0, make that be the test. Thanks for your comment. I agree, and will make this change. - PR: https://git.openjdk.java.net/jdk/pull/7829
Re: RFR: 8283225: [AIX] ClassLoader.c produces incorrect OutOfMemory Exception when length is 0
On Wed, 16 Mar 2022 14:38:55 GMT, Thomas Stuefe wrote: >> As described in the linked issue, NullClassBytesTest fails due an >> OutOfMemoryError produced on AIX when the test calls defineClass with a byte >> array of size of 0. The native implementation of defineClass then calls >> malloc with a size of 0. On AIX malloc(0) returns NULL, while on other >> platforms it return a valid address. When NULL is produced by malloc for >> this reason, ClassLoader.c incorrectly interprets this as a failure due to a >> lack of memory. >> >> ~~This PR modifies ClassLoader.c to produce an OutOfMemoryError only when >> `errno == ENOMEM` and to produce a ClassFormatError with the message >> "ClassLoader internal allocation failure" in all other cases (in which >> malloc returns NULL).~~ [edit: The above no longer describes the PR's >> proposed fix. See discussion below] >> >> In addition, I performed some minor tidy-up work in ClassLoader.c by >> changing instances of `return 0` to `return NULL`, and `if (some_ptr == 0)` >> to `if (some_ptr == NULL)`. This was done to improve the clarity of the code >> in ClassLoader.c, but didn't feel worthy of opening a separate issue. >> >> ### Alternatives >> >> It would be possible to address this failure by modifying the test to accept >> the OutOfMemoryError on AIX. I thought it was a better solution to modify >> ClassLoader.c to produce an OutOfMemoryError only when the system is >> actually out of memory. >> >> ### Testing >> >> This change has been tested on AIX and Linux/x86. > > Btw, which malloc call was the problematic exactly? Cannot be the one in > getUTF, since that one already adds len + 1 and never gets called with a zero > length anyway. Thanks @tstuefe! Your suggestion lead to a better change, so I modified the PR. - ClassLoader.c no longer has any reason to throw a ClassFormatError, so that logic is removed. - The test no longer needs to recognize a new error message, so that is changed back as well. - I also alphabetized the header files, because that is the way I am :-) Note: I couldn't find an implementation of MAX2 in a C-friendly 'header.h' file, so I just used the ternary operator in the two places I needed it. - PR: https://git.openjdk.java.net/jdk/pull/7829
Re: RFR: 8283225: [AIX] ClassLoader.c produces incorrect OutOfMemory Exception when length is 0 [v2]
On Wed, 16 Mar 2022 14:37:57 GMT, Thomas Stuefe wrote: > Side note: nothing against changing 0 to NULL, but please in a separate > cleanup patch. I just saw this. I will separate the cleanup into a separate patch. - PR: https://git.openjdk.java.net/jdk/pull/7829
Re: RFR: 8283225: [AIX] ClassLoader.c produces incorrect OutOfMemory Exception when length is 0 [v2]
> As described in the linked issue, NullClassBytesTest fails due an > OutOfMemoryError produced on AIX when the test calls defineClass with a byte > array of size of 0. The native implementation of defineClass then calls > malloc with a size of 0. On AIX malloc(0) returns NULL, while on other > platforms it return a valid address. When NULL is produced by malloc for this > reason, ClassLoader.c incorrectly interprets this as a failure due to a lack > of memory. > > This PR modifies ClassLoader.c to produce an OutOfMemoryError only when > `errno == ENOMEM` and to produce a ClassFormatError with the message > "ClassLoader internal allocation failure" in all other cases (in which malloc > returns NULL). > > In addition, I performed some minor tidy-up work in ClassLoader.c by changing > instances of `return 0` to `return NULL`, and `if (some_ptr == 0)` to `if > (some_ptr == NULL)`. This was done to improve the clarity of the code in > ClassLoader.c, but didn't feel worthy of opening a separate issue. > > ### Alternatives > > It would be possible to address this failure by modifying the test to accept > the OutOfMemoryError on AIX. I thought it was a better solution to modify > ClassLoader.c to produce an OutOfMemoryError only when the system is actually > out of memory. > > ### Testing > > This change has been tested on AIX and Linux/x86. Tyler Steele has updated the pull request incrementally with four additional commits since the last revision: - Fixes type warning. - Removes unneeded ClassFormatError from ClassLoader.c - Revert "Extract memory error logic to helper procedure" This reverts commit b631eb0ccd5f3748c2010c864f8ccef0c1da9c42. - Avoid calling malloc with size zero. - Changes: - all: https://git.openjdk.java.net/jdk/pull/7829/files - new: https://git.openjdk.java.net/jdk/pull/7829/files/b631eb0c..de946b41 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=7829=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7829=00-01 Stats: 41 lines in 2 files changed: 10 ins; 21 del; 10 mod Patch: https://git.openjdk.java.net/jdk/pull/7829.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7829/head:pull/7829 PR: https://git.openjdk.java.net/jdk/pull/7829
Re: RFR: 8283225: [AIX] ClassLoader.c produces incorrect OutOfMemory Exception when length is 0
On Wed, 16 Mar 2022 14:38:55 GMT, Thomas Stuefe wrote: > Btw, which malloc call was the problematic exactly? Cannot be the one in > getUTF, since that one already adds len + 1 and never gets called with a zero > length anyway. Oh, good point. I guess it gets lost in the noise of my other changes. The lines causing the issue was these ones: https://github.com/openjdk/jdk/blob/08cadb4754da0d5e68ee2df45f4098d203d14115/src/java.base/share/native/libjava/ClassLoader.c#L102-L107 - PR: https://git.openjdk.java.net/jdk/pull/7829
Re: RFR: 8283225: [AIX] ClassLoader.c produces incorrect OutOfMemory Exception when length is 0
On Wed, 16 Mar 2022 14:37:57 GMT, Thomas Stuefe wrote: > The way we solve this usually is by homogenizing malloc behavior across all > platforms with `if (len == 0) len=1;` Interesting! That idea didn't occur to me until after I submitted the PR. I'm happy to test that out and see how it works. - PR: https://git.openjdk.java.net/jdk/pull/7829
RFR: 8283225: [AIX] ClassLoader.c produces incorrect OutOfMemory Exception when length is 0
As described in the linked issue, NullClassBytesTest fails due an OutOfMemoryError produced on AIX when the test calls defineClass with a byte array of size of 0. The native implementation of defineClass then calls malloc with a size of 0. On AIX malloc(0) returns NULL, while on other platforms it return a valid address. When NULL is produced by malloc for this reason, ClassLoader.c incorrectly interprets this as a failure due to a lack of memory. This PR modifies ClassLoader.c to produce an OutOfMemoryError only when `errno == ENOMEM` and to produce a ClassFormatError with the message "ClassLoader internal allocation failure" in all other cases (in which malloc returns NULL). In addition, I performed some minor tidy-up work in ClassLoader.c by changing instances of `return 0` to `return NULL`, and `if (some_ptr == 0)` to `if (some_ptr == NULL)`. This was done to improve the clarity of the code in ClassLoader.c, but didn't feel worthy of opening a separate issue. ### Alternatives It would be possible to address this failure by modifying the test to accept the OutOfMemoryError on AIX. I thought it was a better solution to modify ClassLoader.c to produce an OutOfMemoryError only when the system is actually out of memory. ### Testing This change has been tested on AIX and Linux/x86. - Commit messages: - Extract memory error logic to helper procedure - Updates copyright year and adds bug-id to test - Addresses NullClassBytesTest Failure Changes: https://git.openjdk.java.net/jdk/pull/7829/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=7829=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8283225 Stats: 43 lines in 2 files changed: 20 ins; 0 del; 23 mod Patch: https://git.openjdk.java.net/jdk/pull/7829.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7829/head:pull/7829 PR: https://git.openjdk.java.net/jdk/pull/7829
Re: RFR: 8282219: jdk/java/lang/ProcessBuilder/Basic.java fails on AIX [v4]
On Wed, 23 Feb 2022 18:49:22 GMT, Ichiroh Takiguchi wrote: >> Run jtreg:jdk/java/lang/ProcessBuilder/Basic.java on AIX. >> The test was failed by: >> Incorrect handling of envstrings containing NULs >> >> According to my investigation, this issue was happened after following >> change was applied. >> JDK-8272600: (test) Use native "sleep" in Basic.java >> >> test.nativepath value was added into AIX's LIBPATH during running this >> testcase. >> On AIX, test.nativepath value should be removed from LIBPATH value before >> comparing the values. > > Ichiroh Takiguchi has updated the pull request incrementally with one > additional commit since the last revision: > > Add null check I think it's good to keep the test on AIX since we have good solutions to the failure. I added +1 to Roger's suggestion above. Looks like it should work well. - PR: https://git.openjdk.java.net/jdk/pull/7574
Re: RFR: 8282219: jdk/java/lang/ProcessBuilder/Basic.java fails on AIX [v4]
On Thu, 24 Feb 2022 17:01:13 GMT, Roger Riggs wrote: > Javac is compiling the source to a .class file. The contents of the > `java.library.path` do not affect the class file generated. None of the code > of the class is executed during compilation. Yup. Not the best snippet to include. It is set while calling the jvm as well. - PR: https://git.openjdk.java.net/jdk/pull/7574
Re: RFR: 8282219: jdk/java/lang/ProcessBuilder/Basic.java fails on AIX [v4]
On Thu, 24 Feb 2022 07:16:42 GMT, Thomas Stuefe wrote: > If its the former, then the issue is that `libpath` is just outdated when we > get around to use it? In that case, why not just re-aquiring LIBPATH when > building up `expected`? ^This was my thought at first as well :-) but in my investigation, the libpath in the parent didn't change during the run. I think Roger's solution matches more with my understanding of the failure. I noticed that jtreg adds the extra path to the library when invoking javac to compile the test. - PR: https://git.openjdk.java.net/jdk/pull/7574
Re: RFR: 8282219: jdk/java/lang/ProcessBuilder/Basic.java fails on AIX [v3]
On Wed, 23 Feb 2022 09:46:28 GMT, Ichiroh Takiguchi
wrote:
>> Run jtreg:jdk/java/lang/ProcessBuilder/Basic.java on AIX.
>> The test was failed by:
>> Incorrect handling of envstrings containing NULs
>>
>> According to my investigation, this issue was happened after following
>> change was applied.
>> JDK-8272600: (test) Use native "sleep" in Basic.java
>>
>> test.nativepath value was added into AIX's LIBPATH during running this
>> testcase.
>> On AIX, test.nativepath value should be removed from LIBPATH value before
>> comparing the values.
>
> Ichiroh Takiguchi has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Change LIBPATH usage
test/jdk/java/lang/ProcessBuilder/Basic.java line 85:
> 83: if (AIX.is()) {
> 84: String nativepath = System.getProperty("test.nativepath");
> 85: if (nativepath != null) {
I think this null check should be modified. There are two issues at play.
- If nativepath is null it will be caught by the filter below. So it is safe to
pass it past this point in either case (null or non-null).
- If libpathString is null there will be a NullPointerException when split gets
called on it below.
I believe there are at least two reasonable choices here. The first is to
change nativepath -> libpathString in this null check. This option guards
against calling String.split on a null value. Alternatively, you could trust
that since this code doesn't get called unless AIX.is() returns true, that it's
safe to remove this check altogether. This second option is only viable if we
know LIBPATH is always set on AIX.
-
PR: https://git.openjdk.java.net/jdk/pull/7574
Re: RFR: 8282219: jdk/java/lang/ProcessBuilder/Basic.java fails on AIX [v2]
On Wed, 23 Feb 2022 10:14:44 GMT, Ichiroh Takiguchi
wrote:
>> For my curiosity, how is AIX different from other Linux in that the
>> test.nativepath is not/should not be in LIBPATH?
>
> @RogerRiggs
> Many thanks. that's good point.
> Only 1st part has `test.nativepath` because of following code.
>
> if (AIX.is()) {
> pb.environment().put("LIBPATH", libpath);
> }
>
> On current condition, parent (main) process have `test.nativepath` setting
> into LIBPATH environment, but child process does not require
> `test.nativepath` setting.
> So just `libpath` value should not have `test.nativepath` related entry.
> And above code does not require on current condition and this test said "Pass
> Empty environment to child".
> So it should be removed.
>
> #7581 is exactly same issue.
> Please choose the appropriate one.
Hi @takiguc , thanks for your changes. I closed my PR in favour of yours; we
definitely don't need two PRs for this issue :-)
One comment on the approach you took. I considered modifying the static libpath
variable as well, but what really swayed me away from choosing that route is
the Windows tests. On Windows, the situation is analogous to AIX in that a
static systemroot variable is set by querying the parent environment, but it is
explicitly passed to the child process[es] when they are created. This ensures
that the systemroot returned by the child process is the same as the expected
value. Admittedly it's a bit of a nit-pick, but I think having the Windows
version of the test do one thing and AIX version do another make it harder to
understand what is going on. It's for that reason that I took the approach that
I did in my PR.
-
PR: https://git.openjdk.java.net/jdk/pull/7574
Withdrawn: 8282239 [testbug, AIX] ProcessBuilder/Basic.java fails with incorrect LIBPATH
On Tue, 22 Feb 2022 21:22:57 GMT, Tyler Steele wrote: > This test had two failing sections on AIX related to an incorrect expected > value for LIBPATH. The two (previously failing) test sections are below. > > - Test Runtime.exec(...envp...) with envstrings with initial `=' > - Test Runtime.exec(...envp...) with envstrings containing NULs > > This PR modifies the environment passed to the process at ...exec(cmdp, > **envp**) to include the LIBPATH of the parent. With this change, the > expected libpath matches the libpath returned by the process. > > ### Alternatives > > An equivalent change would be to modify the libpath variable used to set the > expected value for the test without explicitly setting the LIBPATH in process > invocation. This would involve removing the libpath for .../jtreg/native that > is added by the test runner by command-line option > `-J-Dtest.nativepath=...images/test/jdk/jtreg/native `. This change would be > reasonable, but I prefer the approach taken in this PR. > > ### Testing > > This test now passes on my test machine running AIX 7.1. This pull request has been closed without being integrated. - PR: https://git.openjdk.java.net/jdk/pull/7581
Re: RFR: 8282239 [testbug, AIX] ProcessBuilder/Basic.java fails with incorrect LIBPATH [v2]
On Tue, 22 Feb 2022 22:52:58 GMT, Roger Riggs wrote: > fyi, the noreg-* labels apply to the bug report, not the PR. (and yes, they > are informative when reviewing). Thanks for pointing that out. I thought I may have been reading some old documentation. > One of [JDK-8282239](https://bugs.openjdk.java.net/browse/JDK-8282239): or > [JDK-8282219](https://bugs.openjdk.java.net/browse/JDK-8282219) should be > closed as a duplicate. I agree, but I have no access to JBS as of yet. Could you take care of that @RogerRiggs? One of the PRs should be closed as well. Shall I close mine? - PR: https://git.openjdk.java.net/jdk/pull/7581
Re: RFR: 8282219: jdk/java/lang/ProcessBuilder/Basic.java fails on AIX [v2]
On Tue, 22 Feb 2022 17:20:25 GMT, Ichiroh Takiguchi wrote: >> Run jtreg:jdk/java/lang/ProcessBuilder/Basic.java on AIX. >> The test was failed by: >> Incorrect handling of envstrings containing NULs >> >> According to my investigation, this issue was happened after following >> change was applied. >> JDK-8272600: (test) Use native "sleep" in Basic.java >> >> test.nativepath value was added into AIX's LIBPATH during running this >> testcase. >> On AIX, test.nativepath value should be removed from LIBPATH value before >> comparing the values. > > Ichiroh Takiguchi has updated the pull request incrementally with one > additional commit since the last revision: > > Use expectedLibpath Looks you and I are thinking similarly. https://github.com/openjdk/jdk/pull/7581 - PR: https://git.openjdk.java.net/jdk/pull/7574
Re: RFR: 8282239 [testbug, AIX] ProcessBuilder/Basic.java fails with incorrect LIBPATH [v2]
> This test had two failing sections on AIX related to an incorrect expected > value for LIBPATH. The two (previously failing) test sections are below. > > - Test Runtime.exec(...envp...) with envstrings with initial `=' > - Test Runtime.exec(...envp...) with envstrings containing NULs > > This PR modifies the environment passed to the process at ...exec(cmdp, > **envp**) to include the LIBPATH of the parent. With this change, the > expected libpath matches the libpath returned by the process. > > ### Alternatives > > An equivalent change would be to modify the libpath variable used to set the > expected value for the test without explicitly setting the LIBPATH in process > invocation. This would involve removing the libpath for .../jtreg/native that > is added by the test runner by command-line option > `-J-Dtest.nativepath=...images/test/jdk/jtreg/native `. This change would be > reasonable, but I prefer the approach taken in this PR. > > ### Testing > > This test now passes on my test machine running AIX 7.1. Tyler Steele has updated the pull request incrementally with one additional commit since the last revision: Updates copyright year & adds bugid - Changes: - all: https://git.openjdk.java.net/jdk/pull/7581/files - new: https://git.openjdk.java.net/jdk/pull/7581/files/5d325951..fd3a84ed Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=7581=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7581=00-01 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/7581.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7581/head:pull/7581 PR: https://git.openjdk.java.net/jdk/pull/7581
RFR: 8282239 [testbug, AIX] ProcessBuilder/Basic.java fails with incorrect LIBPATH
This test had two failing sections on AIX related to an incorrect expected value for LIBPATH. The two (previously failing) test sections are below. - Test Runtime.exec(...envp...) with envstrings with initial `=' - Test Runtime.exec(...envp...) with envstrings containing NULs This PR modifies the environment passed to the process at ...exec(cmdp, **envp**) to include the LIBPATH of the parent. With this change, the expected libpath matches the libpath returned by the process. ### Alternatives An equivalent change would be to modify the libpath variable used to set the expected value for the test without explicitly setting the LIBPATH in process invocation. This would involve removing the libpath for .../jtreg/native that is added by the test runner by command-line option `-J-Dtest.nativepath=...images/test/jdk/jtreg/native `. This change would be reasonable, but I prefer the approach taken in this PR. ### Testing This test now passes on my test machine running AIX 7.1. - Commit messages: - Fixes testbug causing failure in ProcessBuilder/Basic on AIX Changes: https://git.openjdk.java.net/jdk/pull/7581/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=7581=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8282239 Stats: 27 lines in 1 file changed: 1 ins; 8 del; 18 mod Patch: https://git.openjdk.java.net/jdk/pull/7581.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7581/head:pull/7581 PR: https://git.openjdk.java.net/jdk/pull/7581
Integrated: 8282042: [testbug] FileEncodingTest.java depends on default encoding
On Thu, 17 Feb 2022 22:50:37 GMT, Tyler Steele wrote:
> FileEncodingTest expects all non-Windows platforms will have
> `Charset.defaultCharset().name()` set to US-ASCII when file.encoding is set
> to COMPAT. This assumption does not hold for AIX where it is ISO-8859-1.
>
> According to [JEP-400](https://openjdk.java.net/jeps/400), we should expect
> `Charset.defaultCharset().name()` to equal
> `System.getProperty("native.encoding")` whenever the COMPAT flag is set. From
> JEP-400: "... if file.encoding is set to COMPAT on the command line, then the
> run-time value of file.encoding will be the same as the run-time value of
> native.encoding...". So one way to resolve this is to choose the value for
> each system from the native.encoding property.
>
> With these changes however, my test systems continue to fail.
>
> - AIX reports: Default Charset: ISO-8859-1, expected: ISO8859-1
> - Linux/Z reports: Default Charset: US-ASCII, expected: ANSI_X3.4-1968
> - Linux/PowerLE reports: Default Charset: US-ASCII, expected: ANSI_X3.4-1968
>
> Note that the expected value is populated from native.encoding.
>
> This implies more work to be done. It looks to me that some modification to
> java_props_md.c may be needed to ensure that the System properties for
> native.encoding return [canonical
> names](http://www.iana.org/assignments/character-sets).
>
> ---
>
> A tempting alternative is to set the expected value for AIX to "ISO-8859-1"
> in the test explicitly, as was done for the Windows expected encoding prior
> to this proposed change. The main advantage to this alternative is that it is
> quick and easy, but the disadvantages are that it fails to test that COMPAT
> behaves as specified in JEP-400, and the approach does not scale well if it
> happens that other systems require other cases. I wonder if this is the
> reason non-English locals are excluded by the test.
>
> Proceeding with this change and the work implied by the new failures it
> highlights goes beyond the scope of what I thought was a simple testbug. So
> I'm opening this up for some comments before proceeding down the rabbit hole
> of further changes. If there is generally positive support for this direction
> I'm happy to make the modifications necessary to populate native.encoding
> with canonical names. As I am new to OpenJDK, I am especially looking to
> ensure that changing the value returned by native.encoding will not have
> unintended consequences elsewhere in the project.
This pull request has now been integrated.
Changeset: 58e1882f
Author:Tyler Steele
Committer: Naoto Sato
URL:
https://git.openjdk.java.net/jdk/commit/58e1882f3ccc648c5f6d216d37cfd1805889b8d8
Stats: 6 lines in 1 file changed: 2 ins; 0 del; 4 mod
8282042: [testbug] FileEncodingTest.java depends on default encoding
Adds expected encoding "ISO-8859-1" for AIX in FileEncodingTest.java
Reviewed-by: naoto
-
PR: https://git.openjdk.java.net/jdk/pull/7525
Re: RFR: 8282042: [testbug] FileEncodingTest.java depends on default encoding [v7]
On Fri, 18 Feb 2022 20:54:24 GMT, Tyler Steele wrote:
>> FileEncodingTest expects all non-Windows platforms will have
>> `Charset.defaultCharset().name()` set to US-ASCII when file.encoding is set
>> to COMPAT. This assumption does not hold for AIX where it is ISO-8859-1.
>>
>> According to [JEP-400](https://openjdk.java.net/jeps/400), we should expect
>> `Charset.defaultCharset().name()` to equal
>> `System.getProperty("native.encoding")` whenever the COMPAT flag is set.
>> From JEP-400: "... if file.encoding is set to COMPAT on the command line,
>> then the run-time value of file.encoding will be the same as the run-time
>> value of native.encoding...". So one way to resolve this is to choose the
>> value for each system from the native.encoding property.
>>
>> With these changes however, my test systems continue to fail.
>>
>> - AIX reports: Default Charset: ISO-8859-1, expected: ISO8859-1
>> - Linux/Z reports: Default Charset: US-ASCII, expected: ANSI_X3.4-1968
>> - Linux/PowerLE reports: Default Charset: US-ASCII, expected: ANSI_X3.4-1968
>>
>> Note that the expected value is populated from native.encoding.
>>
>> This implies more work to be done. It looks to me that some modification to
>> java_props_md.c may be needed to ensure that the System properties for
>> native.encoding return [canonical
>> names](http://www.iana.org/assignments/character-sets).
>>
>> ---
>>
>> A tempting alternative is to set the expected value for AIX to "ISO-8859-1"
>> in the test explicitly, as was done for the Windows expected encoding prior
>> to this proposed change. The main advantage to this alternative is that it
>> is quick and easy, but the disadvantages are that it fails to test that
>> COMPAT behaves as specified in JEP-400, and the approach does not scale well
>> if it happens that other systems require other cases. I wonder if this is
>> the reason non-English locals are excluded by the test.
>>
>> Proceeding with this change and the work implied by the new failures it
>> highlights goes beyond the scope of what I thought was a simple testbug. So
>> I'm opening this up for some comments before proceeding down the rabbit hole
>> of further changes. If there is generally positive support for this
>> direction I'm happy to make the modifications necessary to populate
>> native.encoding with canonical names. As I am new to OpenJDK, I am
>> especially looking to ensure that changing the value returned by
>> native.encoding will not have unintended consequences elsewhere in the
>> project.
>
> Tyler Steele has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Fixes copyright header error
> /label remove auto
>
> Automatic integration is not appropriate and should not be used except in
> time sensitive cases. It can preempt other reviewers from having a chance to
> review and comment.
Ok. I will avoid using it in the future. I don't have commiter rights, so I was
just looking to signal that I am happy for this change to be integrated when it
has been reviewed. It seemed to fall under the 'benign changes' category of the
`/integrate` command's documentation.
If there is other feedback, I am happy to incorporate it
-
PR: https://git.openjdk.java.net/jdk/pull/7525
Re: RFR: 8282042: [testbug] FileEncodingTest.java depends on default encoding [v6]
On Fri, 18 Feb 2022 20:04:25 GMT, Naoto Sato wrote: >> Tyler Steele has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Makes sm changes requested by Naoto > > test/jdk/java/lang/System/FileEncodingTest.java line 2: > >> 1: /* >> 2: * Copyright (c) 2022, Oracle and/or its affiliates. All rights reserved. > > The year should be `2021, 2022`. (year of inception must be preserved) Oops. Thanks for catching that. - PR: https://git.openjdk.java.net/jdk/pull/7525
Re: RFR: 8282042: [testbug] FileEncodingTest.java depends on default encoding [v7]
> FileEncodingTest expects all non-Windows platforms will have
> `Charset.defaultCharset().name()` set to US-ASCII when file.encoding is set
> to COMPAT. This assumption does not hold for AIX where it is ISO-8859-1.
>
> According to [JEP-400](https://openjdk.java.net/jeps/400), we should expect
> `Charset.defaultCharset().name()` to equal
> `System.getProperty("native.encoding")` whenever the COMPAT flag is set. From
> JEP-400: "... if file.encoding is set to COMPAT on the command line, then the
> run-time value of file.encoding will be the same as the run-time value of
> native.encoding...". So one way to resolve this is to choose the value for
> each system from the native.encoding property.
>
> With these changes however, my test systems continue to fail.
>
> - AIX reports: Default Charset: ISO-8859-1, expected: ISO8859-1
> - Linux/Z reports: Default Charset: US-ASCII, expected: ANSI_X3.4-1968
> - Linux/PowerLE reports: Default Charset: US-ASCII, expected: ANSI_X3.4-1968
>
> Note that the expected value is populated from native.encoding.
>
> This implies more work to be done. It looks to me that some modification to
> java_props_md.c may be needed to ensure that the System properties for
> native.encoding return [canonical
> names](http://www.iana.org/assignments/character-sets).
>
> ---
>
> A tempting alternative is to set the expected value for AIX to "ISO-8859-1"
> in the test explicitly, as was done for the Windows expected encoding prior
> to this proposed change. The main advantage to this alternative is that it is
> quick and easy, but the disadvantages are that it fails to test that COMPAT
> behaves as specified in JEP-400, and the approach does not scale well if it
> happens that other systems require other cases. I wonder if this is the
> reason non-English locals are excluded by the test.
>
> Proceeding with this change and the work implied by the new failures it
> highlights goes beyond the scope of what I thought was a simple testbug. So
> I'm opening this up for some comments before proceeding down the rabbit hole
> of further changes. If there is generally positive support for this direction
> I'm happy to make the modifications necessary to populate native.encoding
> with canonical names. As I am new to OpenJDK, I am especially looking to
> ensure that changing the value returned by native.encoding will not have
> unintended consequences elsewhere in the project.
Tyler Steele has updated the pull request incrementally with one additional
commit since the last revision:
Fixes copyright header error
-
Changes:
- all: https://git.openjdk.java.net/jdk/pull/7525/files
- new: https://git.openjdk.java.net/jdk/pull/7525/files/2a8651b8..ac38c8f3
Webrevs:
- full: https://webrevs.openjdk.java.net/?repo=jdk=7525=06
- incr: https://webrevs.openjdk.java.net/?repo=jdk=7525=05-06
Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod
Patch: https://git.openjdk.java.net/jdk/pull/7525.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/7525/head:pull/7525
PR: https://git.openjdk.java.net/jdk/pull/7525
Re: RFR: 8282042: [testbug] FileEncodingTest.java depends on default encoding [v6]
> FileEncodingTest expects all non-Windows platforms will have
> `Charset.defaultCharset().name()` set to US-ASCII when file.encoding is set
> to COMPAT. This assumption does not hold for AIX where it is ISO-8859-1.
>
> According to [JEP-400](https://openjdk.java.net/jeps/400), we should expect
> `Charset.defaultCharset().name()` to equal
> `System.getProperty("native.encoding")` whenever the COMPAT flag is set. From
> JEP-400: "... if file.encoding is set to COMPAT on the command line, then the
> run-time value of file.encoding will be the same as the run-time value of
> native.encoding...". So one way to resolve this is to choose the value for
> each system from the native.encoding property.
>
> With these changes however, my test systems continue to fail.
>
> - AIX reports: Default Charset: ISO-8859-1, expected: ISO8859-1
> - Linux/Z reports: Default Charset: US-ASCII, expected: ANSI_X3.4-1968
> - Linux/PowerLE reports: Default Charset: US-ASCII, expected: ANSI_X3.4-1968
>
> Note that the expected value is populated from native.encoding.
>
> This implies more work to be done. It looks to me that some modification to
> java_props_md.c may be needed to ensure that the System properties for
> native.encoding return [canonical
> names](http://www.iana.org/assignments/character-sets).
>
> ---
>
> A tempting alternative is to set the expected value for AIX to "ISO-8859-1"
> in the test explicitly, as was done for the Windows expected encoding prior
> to this proposed change. The main advantage to this alternative is that it is
> quick and easy, but the disadvantages are that it fails to test that COMPAT
> behaves as specified in JEP-400, and the approach does not scale well if it
> happens that other systems require other cases. I wonder if this is the
> reason non-English locals are excluded by the test.
>
> Proceeding with this change and the work implied by the new failures it
> highlights goes beyond the scope of what I thought was a simple testbug. So
> I'm opening this up for some comments before proceeding down the rabbit hole
> of further changes. If there is generally positive support for this direction
> I'm happy to make the modifications necessary to populate native.encoding
> with canonical names. As I am new to OpenJDK, I am especially looking to
> ensure that changing the value returned by native.encoding will not have
> unintended consequences elsewhere in the project.
Tyler Steele has updated the pull request incrementally with one additional
commit since the last revision:
Makes sm changes requested by Naoto
-
Changes:
- all: https://git.openjdk.java.net/jdk/pull/7525/files
- new: https://git.openjdk.java.net/jdk/pull/7525/files/56f01452..2a8651b8
Webrevs:
- full: https://webrevs.openjdk.java.net/?repo=jdk=7525=05
- incr: https://webrevs.openjdk.java.net/?repo=jdk=7525=04-05
Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod
Patch: https://git.openjdk.java.net/jdk/pull/7525.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/7525/head:pull/7525
PR: https://git.openjdk.java.net/jdk/pull/7525
Re: RFR: 8282042: [testbug] FileEncodingTest.java depends on default encoding [v5]
On Fri, 18 Feb 2022 19:06:28 GMT, Tyler Steele wrote:
>> FileEncodingTest expects all non-Windows platforms will have
>> `Charset.defaultCharset().name()` set to US-ASCII when file.encoding is set
>> to COMPAT. This assumption does not hold for AIX where it is ISO-8859-1.
>>
>> According to [JEP-400](https://openjdk.java.net/jeps/400), we should expect
>> `Charset.defaultCharset().name()` to equal
>> `System.getProperty("native.encoding")` whenever the COMPAT flag is set.
>> From JEP-400: "... if file.encoding is set to COMPAT on the command line,
>> then the run-time value of file.encoding will be the same as the run-time
>> value of native.encoding...". So one way to resolve this is to choose the
>> value for each system from the native.encoding property.
>>
>> With these changes however, my test systems continue to fail.
>>
>> - AIX reports: Default Charset: ISO-8859-1, expected: ISO8859-1
>> - Linux/Z reports: Default Charset: US-ASCII, expected: ANSI_X3.4-1968
>> - Linux/PowerLE reports: Default Charset: US-ASCII, expected: ANSI_X3.4-1968
>>
>> Note that the expected value is populated from native.encoding.
>>
>> This implies more work to be done. It looks to me that some modification to
>> java_props_md.c may be needed to ensure that the System properties for
>> native.encoding return [canonical
>> names](http://www.iana.org/assignments/character-sets).
>>
>> ---
>>
>> A tempting alternative is to set the expected value for AIX to "ISO-8859-1"
>> in the test explicitly, as was done for the Windows expected encoding prior
>> to this proposed change. The main advantage to this alternative is that it
>> is quick and easy, but the disadvantages are that it fails to test that
>> COMPAT behaves as specified in JEP-400, and the approach does not scale well
>> if it happens that other systems require other cases. I wonder if this is
>> the reason non-English locals are excluded by the test.
>>
>> Proceeding with this change and the work implied by the new failures it
>> highlights goes beyond the scope of what I thought was a simple testbug. So
>> I'm opening this up for some comments before proceeding down the rabbit hole
>> of further changes. If there is generally positive support for this
>> direction I'm happy to make the modifications necessary to populate
>> native.encoding with canonical names. As I am new to OpenJDK, I am
>> especially looking to ensure that changing the value returned by
>> native.encoding will not have unintended consequences elsewhere in the
>> project.
>
> Tyler Steele has refreshed the contents of this pull request, and previous
> commits have been removed. The incremental views will show differences
> compared to the previous content of the PR. The pull request contains one new
> commit since the last revision:
>
> Changes FileEncodingTest to include hardcoded value for AIX
The test now passes on AIX, and Linux/Z. So I believe this change can be merged
once reviewed.
-
PR: https://git.openjdk.java.net/jdk/pull/7525
Re: RFR: 8282042: [testbug] FileEncodingTest.java depends on default encoding [v5]
> FileEncodingTest expects all non-Windows platforms will have
> `Charset.defaultCharset().name()` set to US-ASCII when file.encoding is set
> to COMPAT. This assumption does not hold for AIX where it is ISO-8859-1.
>
> According to [JEP-400](https://openjdk.java.net/jeps/400), we should expect
> `Charset.defaultCharset().name()` to equal
> `System.getProperty("native.encoding")` whenever the COMPAT flag is set. From
> JEP-400: "... if file.encoding is set to COMPAT on the command line, then the
> run-time value of file.encoding will be the same as the run-time value of
> native.encoding...". So one way to resolve this is to choose the value for
> each system from the native.encoding property.
>
> With these changes however, my test systems continue to fail.
>
> - AIX reports: Default Charset: ISO-8859-1, expected: ISO8859-1
> - Linux/Z reports: Default Charset: US-ASCII, expected: ANSI_X3.4-1968
> - Linux/PowerLE reports: Default Charset: US-ASCII, expected: ANSI_X3.4-1968
>
> Note that the expected value is populated from native.encoding.
>
> This implies more work to be done. It looks to me that some modification to
> java_props_md.c may be needed to ensure that the System properties for
> native.encoding return [canonical
> names](http://www.iana.org/assignments/character-sets).
>
> ---
>
> A tempting alternative is to set the expected value for AIX to "ISO-8859-1"
> in the test explicitly, as was done for the Windows expected encoding prior
> to this proposed change. The main advantage to this alternative is that it is
> quick and easy, but the disadvantages are that it fails to test that COMPAT
> behaves as specified in JEP-400, and the approach does not scale well if it
> happens that other systems require other cases. I wonder if this is the
> reason non-English locals are excluded by the test.
>
> Proceeding with this change and the work implied by the new failures it
> highlights goes beyond the scope of what I thought was a simple testbug. So
> I'm opening this up for some comments before proceeding down the rabbit hole
> of further changes. If there is generally positive support for this direction
> I'm happy to make the modifications necessary to populate native.encoding
> with canonical names. As I am new to OpenJDK, I am especially looking to
> ensure that changing the value returned by native.encoding will not have
> unintended consequences elsewhere in the project.
Tyler Steele has refreshed the contents of this pull request, and previous
commits have been removed. The incremental views will show differences compared
to the previous content of the PR. The pull request contains one new commit
since the last revision:
Changes FileEncodingTest to include hardcoded value for AIX
-
Changes:
- all: https://git.openjdk.java.net/jdk/pull/7525/files
- new: https://git.openjdk.java.net/jdk/pull/7525/files/9341ecb7..56f01452
Webrevs:
- full: https://webrevs.openjdk.java.net/?repo=jdk=7525=04
- incr: https://webrevs.openjdk.java.net/?repo=jdk=7525=03-04
Stats: 0 lines in 0 files changed: 0 ins; 0 del; 0 mod
Patch: https://git.openjdk.java.net/jdk/pull/7525.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/7525/head:pull/7525
PR: https://git.openjdk.java.net/jdk/pull/7525
Re: RFR: 8282042: [testbug] FileEncodingTest.java depends on default encoding [v4]
> FileEncodingTest expects all non-Windows platforms will have
> `Charset.defaultCharset().name()` set to US-ASCII when file.encoding is set
> to COMPAT. This assumption does not hold for AIX where it is ISO-8859-1.
>
> According to [JEP-400](https://openjdk.java.net/jeps/400), we should expect
> `Charset.defaultCharset().name()` to equal
> `System.getProperty("native.encoding")` whenever the COMPAT flag is set. From
> JEP-400: "... if file.encoding is set to COMPAT on the command line, then the
> run-time value of file.encoding will be the same as the run-time value of
> native.encoding...". So one way to resolve this is to choose the value for
> each system from the native.encoding property.
>
> With these changes however, my test systems continue to fail.
>
> - AIX reports: Default Charset: ISO-8859-1, expected: ISO8859-1
> - Linux/Z reports: Default Charset: US-ASCII, expected: ANSI_X3.4-1968
> - Linux/PowerLE reports: Default Charset: US-ASCII, expected: ANSI_X3.4-1968
>
> Note that the expected value is populated from native.encoding.
>
> This implies more work to be done. It looks to me that some modification to
> java_props_md.c may be needed to ensure that the System properties for
> native.encoding return [canonical
> names](http://www.iana.org/assignments/character-sets).
>
> ---
>
> A tempting alternative is to set the expected value for AIX to "ISO-8859-1"
> in the test explicitly, as was done for the Windows expected encoding prior
> to this proposed change. The main advantage to this alternative is that it is
> quick and easy, but the disadvantages are that it fails to test that COMPAT
> behaves as specified in JEP-400, and the approach does not scale well if it
> happens that other systems require other cases. I wonder if this is the
> reason non-English locals are excluded by the test.
>
> Proceeding with this change and the work implied by the new failures it
> highlights goes beyond the scope of what I thought was a simple testbug. So
> I'm opening this up for some comments before proceeding down the rabbit hole
> of further changes. If there is generally positive support for this direction
> I'm happy to make the modifications necessary to populate native.encoding
> with canonical names. As I am new to OpenJDK, I am especially looking to
> ensure that changing the value returned by native.encoding will not have
> unintended consequences elsewhere in the project.
Tyler Steele has updated the pull request incrementally with one additional
commit since the last revision:
Minor fixes: Removes unused Booleans, adds varname
-
Changes:
- all: https://git.openjdk.java.net/jdk/pull/7525/files
- new: https://git.openjdk.java.net/jdk/pull/7525/files/ff2918d6..9341ecb7
Webrevs:
- full: https://webrevs.openjdk.java.net/?repo=jdk=7525=03
- incr: https://webrevs.openjdk.java.net/?repo=jdk=7525=02-03
Stats: 3 lines in 1 file changed: 0 ins; 2 del; 1 mod
Patch: https://git.openjdk.java.net/jdk/pull/7525.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/7525/head:pull/7525
PR: https://git.openjdk.java.net/jdk/pull/7525
Re: RFR: 8282042: [testbug] FileEncodingTest.java depends on default encoding [v3]
> FileEncodingTest expects all non-Windows platforms will have
> `Charset.defaultCharset().name()` set to US-ASCII when file.encoding is set
> to COMPAT. This assumption does not hold for AIX where it is ISO-8859-1.
>
> According to [JEP-400](https://openjdk.java.net/jeps/400), we should expect
> `Charset.defaultCharset().name()` to equal
> `System.getProperty("native.encoding")` whenever the COMPAT flag is set. From
> JEP-400: "... if file.encoding is set to COMPAT on the command line, then the
> run-time value of file.encoding will be the same as the run-time value of
> native.encoding...". So one way to resolve this is to choose the value for
> each system from the native.encoding property.
>
> With these changes however, my test systems continue to fail.
>
> - AIX reports: Default Charset: ISO-8859-1, expected: ISO8859-1
> - Linux/Z reports: Default Charset: US-ASCII, expected: ANSI_X3.4-1968
> - Linux/PowerLE reports: Default Charset: US-ASCII, expected: ANSI_X3.4-1968
>
> Note that the expected value is populated from native.encoding.
>
> This implies more work to be done. It looks to me that some modification to
> java_props_md.c may be needed to ensure that the System properties for
> native.encoding return [canonical
> names](http://www.iana.org/assignments/character-sets).
>
> ---
>
> A tempting alternative is to set the expected value for AIX to "ISO-8859-1"
> in the test explicitly, as was done for the Windows expected encoding prior
> to this proposed change. The main advantage to this alternative is that it is
> quick and easy, but the disadvantages are that it fails to test that COMPAT
> behaves as specified in JEP-400, and the approach does not scale well if it
> happens that other systems require other cases. I wonder if this is the
> reason non-English locals are excluded by the test.
>
> Proceeding with this change and the work implied by the new failures it
> highlights goes beyond the scope of what I thought was a simple testbug. So
> I'm opening this up for some comments before proceeding down the rabbit hole
> of further changes. If there is generally positive support for this direction
> I'm happy to make the modifications necessary to populate native.encoding
> with canonical names. As I am new to OpenJDK, I am especially looking to
> ensure that changing the value returned by native.encoding will not have
> unintended consequences elsewhere in the project.
Tyler Steele has updated the pull request incrementally with one additional
commit since the last revision:
Changes FileEncodingTest to include hardcoded value for AIX
-
Changes:
- all: https://git.openjdk.java.net/jdk/pull/7525/files
- new: https://git.openjdk.java.net/jdk/pull/7525/files/83b6e4bc..ff2918d6
Webrevs:
- full: https://webrevs.openjdk.java.net/?repo=jdk=7525=02
- incr: https://webrevs.openjdk.java.net/?repo=jdk=7525=01-02
Stats: 5 lines in 1 file changed: 4 ins; 0 del; 1 mod
Patch: https://git.openjdk.java.net/jdk/pull/7525.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/7525/head:pull/7525
PR: https://git.openjdk.java.net/jdk/pull/7525
Re: RFR: 8282042: [testbug] FileEncodingTest.java depends on default encoding [v2]
> FileEncodingTest expects all non-Windows platforms will have
> `Charset.defaultCharset().name()` set to US-ASCII when file.encoding is set
> to COMPAT. This assumption does not hold for AIX where it is ISO-8859-1.
>
> According to [JEP-400](https://openjdk.java.net/jeps/400), we should expect
> `Charset.defaultCharset().name()` to equal
> `System.getProperty("native.encoding")` whenever the COMPAT flag is set. From
> JEP-400: "... if file.encoding is set to COMPAT on the command line, then the
> run-time value of file.encoding will be the same as the run-time value of
> native.encoding...". So one way to resolve this is to choose the value for
> each system from the native.encoding property.
>
> With these changes however, my test systems continue to fail.
>
> - AIX reports: Default Charset: ISO-8859-1, expected: ISO8859-1
> - Linux/Z reports: Default Charset: US-ASCII, expected: ANSI_X3.4-1968
> - Linux/PowerLE reports: Default Charset: US-ASCII, expected: ANSI_X3.4-1968
>
> Note that the expected value is populated from native.encoding.
>
> This implies more work to be done. It looks to me that some modification to
> java_props_md.c may be needed to ensure that the System properties for
> native.encoding return [canonical
> names](http://www.iana.org/assignments/character-sets).
>
> ---
>
> A tempting alternative is to set the expected value for AIX to "ISO-8859-1"
> in the test explicitly, as was done for the Windows expected encoding prior
> to this proposed change. The main advantage to this alternative is that it is
> quick and easy, but the disadvantages are that it fails to test that COMPAT
> behaves as specified in JEP-400, and the approach does not scale well if it
> happens that other systems require other cases. I wonder if this is the
> reason non-English locals are excluded by the test.
>
> Proceeding with this change and the work implied by the new failures it
> highlights goes beyond the scope of what I thought was a simple testbug. So
> I'm opening this up for some comments before proceeding down the rabbit hole
> of further changes. If there is generally positive support for this direction
> I'm happy to make the modifications necessary to populate native.encoding
> with canonical names. As I am new to OpenJDK, I am especially looking to
> ensure that changing the value returned by native.encoding will not have
> unintended consequences elsewhere in the project.
Tyler Steele has refreshed the contents of this pull request, and previous
commits have been removed. Incremental views are not available.
-
Changes:
- all: https://git.openjdk.java.net/jdk/pull/7525/files
- new: https://git.openjdk.java.net/jdk/pull/7525/files/fd06a608..83b6e4bc
Webrevs:
- full: https://webrevs.openjdk.java.net/?repo=jdk=7525=01
- incr: https://webrevs.openjdk.java.net/?repo=jdk=7525=00-01
Stats: 8 lines in 1 file changed: 4 ins; 0 del; 4 mod
Patch: https://git.openjdk.java.net/jdk/pull/7525.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/7525/head:pull/7525
PR: https://git.openjdk.java.net/jdk/pull/7525
Withdrawn: 8282042: [testbug] FileEncodingTest.java depends on default encoding
On Thu, 17 Feb 2022 22:50:37 GMT, Tyler Steele wrote:
> FileEncodingTest expects all non-Windows platforms will have
> `Charset.defaultCharset().name()` set to US-ASCII when file.encoding is set
> to COMPAT. This assumption does not hold for AIX where it is ISO-8859-1.
>
> According to [JEP-400](https://openjdk.java.net/jeps/400), we should expect
> `Charset.defaultCharset().name()` to equal
> `System.getProperty("native.encoding")` whenever the COMPAT flag is set. From
> JEP-400: "... if file.encoding is set to COMPAT on the command line, then the
> run-time value of file.encoding will be the same as the run-time value of
> native.encoding...". So one way to resolve this is to choose the value for
> each system from the native.encoding property.
>
> With these changes however, my test systems continue to fail.
>
> - AIX reports: Default Charset: ISO-8859-1, expected: ISO8859-1
> - Linux/Z reports: Default Charset: US-ASCII, expected: ANSI_X3.4-1968
> - Linux/PowerLE reports: Default Charset: US-ASCII, expected: ANSI_X3.4-1968
>
> Note that the expected value is populated from native.encoding.
>
> This implies more work to be done. It looks to me that some modification to
> java_props_md.c may be needed to ensure that the System properties for
> native.encoding return [canonical
> names](http://www.iana.org/assignments/character-sets).
>
> ---
>
> A tempting alternative is to set the expected value for AIX to "ISO-8859-1"
> in the test explicitly, as was done for the Windows expected encoding prior
> to this proposed change. The main advantage to this alternative is that it is
> quick and easy, but the disadvantages are that it fails to test that COMPAT
> behaves as specified in JEP-400, and the approach does not scale well if it
> happens that other systems require other cases. I wonder if this is the
> reason non-English locals are excluded by the test.
>
> Proceeding with this change and the work implied by the new failures it
> highlights goes beyond the scope of what I thought was a simple testbug. So
> I'm opening this up for some comments before proceeding down the rabbit hole
> of further changes. If there is generally positive support for this direction
> I'm happy to make the modifications necessary to populate native.encoding
> with canonical names. As I am new to OpenJDK, I am especially looking to
> ensure that changing the value returned by native.encoding will not have
> unintended consequences elsewhere in the project.
This pull request has been closed without being integrated.
-
PR: https://git.openjdk.java.net/jdk/pull/7525
Re: RFR: 8282042: [testbug] FileEncodingTest.java depends on default encoding
On Thu, 17 Feb 2022 22:50:37 GMT, Tyler Steele wrote:
> FileEncodingTest expects all non-Windows platforms will have
> `Charset.defaultCharset().name()` set to US-ASCII when file.encoding is set
> to COMPAT. This assumption does not hold for AIX where it is ISO-8859-1.
>
> According to [JEP-400](https://openjdk.java.net/jeps/400), we should expect
> `Charset.defaultCharset().name()` to equal
> `System.getProperty("native.encoding")` whenever the COMPAT flag is set. From
> JEP-400: "... if file.encoding is set to COMPAT on the command line, then the
> run-time value of file.encoding will be the same as the run-time value of
> native.encoding...". So one way to resolve this is to choose the value for
> each system from the native.encoding property.
>
> With these changes however, my test systems continue to fail.
>
> - AIX reports: Default Charset: ISO-8859-1, expected: ISO8859-1
> - Linux/Z reports: Default Charset: US-ASCII, expected: ANSI_X3.4-1968
> - Linux/PowerLE reports: Default Charset: US-ASCII, expected: ANSI_X3.4-1968
>
> Note that the expected value is populated from native.encoding.
>
> This implies more work to be done. It looks to me that some modification to
> java_props_md.c may be needed to ensure that the System properties for
> native.encoding return [canonical
> names](http://www.iana.org/assignments/character-sets).
>
> ---
>
> A tempting alternative is to set the expected value for AIX to "ISO-8859-1"
> in the test explicitly, as was done for the Windows expected encoding prior
> to this proposed change. The main advantage to this alternative is that it is
> quick and easy, but the disadvantages are that it fails to test that COMPAT
> behaves as specified in JEP-400, and the approach does not scale well if it
> happens that other systems require other cases. I wonder if this is the
> reason non-English locals are excluded by the test.
>
> Proceeding with this change and the work implied by the new failures it
> highlights goes beyond the scope of what I thought was a simple testbug. So
> I'm opening this up for some comments before proceeding down the rabbit hole
> of further changes. If there is generally positive support for this direction
> I'm happy to make the modifications necessary to populate native.encoding
> with canonical names. As I am new to OpenJDK, I am especially looking to
> ensure that changing the value returned by native.encoding will not have
> unintended consequences elsewhere in the project.
Thanks for your feedback Naoto.
I agree it's a little odd to test the way I proposed above, as it introduces
uncertainty as you mentioned, as well as other issues like both native.encoding
and Charsets.defaultCharset() being wrong, but being wrong in the same way. The
main part of testing this way was the quoted line from JEP-400 (of which I
recognize you are an author). Maybe I'm being too literal; in my testing the
encodings match, even if the names are aliases of the ones I expect. In
addition, you have a good point about the purpose of the COMPAT flag being
compatibility. I agree that it's not really appropriate to change the values of
native.encoding to the canonical ones.
I was feeling torn between the proposed option and alternative, and your
feedback definitely sways me towards the alternative. I'll change this PR to
simply add an exception to the test for AIX.
-
PR: https://git.openjdk.java.net/jdk/pull/7525
RFR: 8282042: [testbug] FileEncodingTest.java depends on default encoding
FileEncodingTest expects all non-Windows platforms will have
`Charset.defaultCharset().name()` set to US-ASCII when file.encoding is set to
COMPAT. This assumption does not hold for AIX where it is ISO-8859-1.
According to [JEP-400](https://openjdk.java.net/jeps/400), we should expect
`Charset.defaultCharset().name()` to equal
`System.getProperty("native.encoding")` whenever the COMPAT flag is set. From
JEP-400: "... if file.encoding is set to COMPAT on the command line, then the
run-time value of file.encoding will be the same as the run-time value of
native.encoding...". So one way to resolve this is to choose the value for each
system from the native.encoding property.
With these changes however, my test systems continue to fail.
- AIX reports: Default Charset: ISO-8859-1, expected: ISO8859-1
- Linux/Z reports: Default Charset: US-ASCII, expected: ANSI_X3.4-1968
- Linux/PowerLE reports: Default Charset: US-ASCII, expected: ANSI_X3.4-1968
Note that the expected value is populated from native.encoding.
This implies more work to be done. It looks to me that some modification to
java_props_md.c may be needed to ensure that the System properties for
native.encoding return [canonical
names](http://www.iana.org/assignments/character-sets).
---
A tempting alternative is to set the expected value for AIX to "ISO-8859-1" in
the test explicitly, as was done for the Windows expected encoding prior to
this proposed change. The main advantage to this alternative is that it is
quick and easy, but the disadvantages are that it fails to test that COMPAT
behaves as specified in JEP-400, and the approach does not scale well if it
happens that other systems require other cases. I wonder if this is the reason
non-English locals are excluded by the test.
Proceeding with this change and the work implied by the new failures it
highlights goes beyond the scope of what I thought was a simple testbug. So I'm
opening this up for some comments before proceeding down the rabbit hole of
further changes. If there is generally positive support for this direction I'm
happy to make the modifications necessary to populate native.encoding with
canonical names. As I am new to OpenJDK, I am especially looking to ensure that
changing the value returned by native.encoding will not have unintended
consequences elsewhere in the project.
-
Commit messages:
- Changes FileEncodingTest test to delegate behaviour of
-Dfile.encoding=COMPAT to
Changes: https://git.openjdk.java.net/jdk/pull/7525/files
Webrev: https://webrevs.openjdk.java.net/?repo=jdk=7525=00
Issue: https://bugs.openjdk.java.net/browse/JDK-8282042
Stats: 8 lines in 1 file changed: 0 ins; 4 del; 4 mod
Patch: https://git.openjdk.java.net/jdk/pull/7525.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/7525/head:pull/7525
PR: https://git.openjdk.java.net/jdk/pull/7525
