Re: RFR: 8282219: jdk/java/lang/ProcessBuilder/Basic.java fails on AIX [v4]
On Thu, 24 Feb 2022 18:51:08 GMT, Roger Riggs wrote: >> Ichiroh Takiguchi has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Add null check > > The piece I was missing is that the HotSpot AIX specific code, computes and > sets LIBPATH based on > the path to the java executable. This computed LIBPATH value is set in the > environment unless > it is overridden by an existing LIBPATH in the environment. > > The test cases that failed, do not supply a value for LIBPATH so the launcher > provided value is inserted > and thereafter reported as the environment from the child process. > > Since both of these tests are testing a different condition not related to > LIBPATH, > the presence/accuracy of LIBPATH is not the point of the test. > > The current solution to remove `test.nativepath` works and is ok by me. > > (The alternative used by #7581, to include LIBPATH in the environment when > launching might be slightly more robust to future changes.) > > Thanks for the followup. > Thanks @RogerRiggs and @tstuefe . I appreciate your deeper investigations. > Could you give me some advice on what to do next? Hi @takiguc, My preference would be to spawn `sh -c env` instead of java, since it removes all need for second-guessing what the java child does with the environment (java modifies the environment and also conceivably System.getenv() may lie to us at some point in the future since it seems a convenient way to filter environment variables for java programs). That is if my reasoning was correct in the first place. But I do not have a strong preference and defer to Roger and Tyler. Cheers, Thomas - 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 18:51:08 GMT, Roger Riggs wrote: >> Ichiroh Takiguchi has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Add null check > > The piece I was missing is that the HotSpot AIX specific code, computes and > sets LIBPATH based on > the path to the java executable. This computed LIBPATH value is set in the > environment unless > it is overridden by an existing LIBPATH in the environment. > > The test cases that failed, do not supply a value for LIBPATH so the launcher > provided value is inserted > and thereafter reported as the environment from the child process. > > Since both of these tests are testing a different condition not related to > LIBPATH, > the presence/accuracy of LIBPATH is not the point of the test. > > The current solution to remove `test.nativepath` works and is ok by me. > > (The alternative used by #7581, to include LIBPATH in the environment when > launching might be slightly more robust to future changes.) > > Thanks for the followup. Thanks @RogerRiggs and @tstuefe . I appreciate your deeper investigations. Could you give me some advice on what to do next? - PR: https://git.openjdk.java.net/jdk/pull/7574
Re: RFR: 8279508: Auto-vectorize Math.round API [v9]
> Summary of changes: > - Intrinsify Math.round(float) and Math.round(double) APIs. > - Extend auto-vectorizer to infer vector operations on encountering scalar IR > nodes for above intrinsics. > - Test creation using new IR testing framework. > > Following are the performance number of a JMH micro included with the patch > > Test System: Intel(R) Xeon(R) Platinum 8380 CPU @ 2.30GHz (Icelake Server) > > > Benchmark | TESTSIZE | Baseline AVX3 (ops/ms) | Withopt AVX3 (ops/ms) | Gain > ratio | Baseline AVX2 (ops/ms) | Withopt AVX2 (ops/ms) | Gain ratio > -- | -- | -- | -- | -- | -- | -- | -- > FpRoundingBenchmark.test_round_double | 1024.00 | 504.15 | 2209.54 | 4.38 | > 510.36 | 548.39 | 1.07 > FpRoundingBenchmark.test_round_double | 2048.00 | 293.64 | 1271.98 | 4.33 | > 293.48 | 274.01 | 0.93 > FpRoundingBenchmark.test_round_float | 1024.00 | 825.99 | 4754.66 | 5.76 | > 751.83 | 2274.13 | 3.02 > FpRoundingBenchmark.test_round_float | 2048.00 | 412.22 | 2490.09 | 6.04 | > 388.52 | 1334.18 | 3.43 > > > Kindly review and share your feedback. > > Best Regards, > Jatin Jatin Bhateja has updated the pull request incrementally with one additional commit since the last revision: 8279508: Adding descriptive comments. - Changes: - all: https://git.openjdk.java.net/jdk/pull/7094/files - new: https://git.openjdk.java.net/jdk/pull/7094/files/f7dec3d9..54d4ea36 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=7094=08 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7094=07-08 Stats: 31 lines in 2 files changed: 14 ins; 0 del; 17 mod Patch: https://git.openjdk.java.net/jdk/pull/7094.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7094/head:pull/7094 PR: https://git.openjdk.java.net/jdk/pull/7094
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 Hi Roger, > The piece I was missing is that the HotSpot AIX specific code, computes and > sets LIBPATH based on the path to the java executable. This computed LIBPATH > value is set in the environment unless it is overridden by an existing > LIBPATH in the environment. Ah, thank you for this missing piece! Now I remember. We modify LIBPATH in the java launcher itself. Since the test executable here is also java we call setenv(LIBPATH,..) in the child process. On AIX we do this unconditionally, that may be the difference to other platforms. The point of this test was not to test `System.getenv()` but that Runtime.exec passes the env vector correctly, right? Then a maybe simpler alternative would have been to spawn `sh -c env`, or a simple little C program printing its env vector, instead of java. Less side effects. Test may be a bit quicker too, since child startup would be faster (probably does not save much in the big scheme of things). Thanks, Thomas - PR: https://git.openjdk.java.net/jdk/pull/7574
Re: RFR: 8282023: PropertiesStoreTest and StoreReproducibilityTest jtreg failures due to en_CA locale [v3]
On Fri, 25 Feb 2022 03:51:09 GMT, Jaikiran Pai wrote: >> test/jdk/java/util/Properties/PropertiesStoreTest.java line 112: >> >>> 110: locales.add(Locale.getDefault()); // always test the default >>> locale >>> 111: locales.add(Locale.US); // guaranteed to be present >>> 112: locales.add(Locale.ROOT); // guaranteed to be present >> >> Can we assume the returned `Set` is mutable? `Collectors.toSet()` >> javadoc reads no guarantee for mutability. > > That's a very good point. I've updated the PR to now explicitly use a mutable > `Set` instead of using `Collectors.toSet()` This is ok, although `Collectors.toCollection(HashSet::new)` is a bit concise. - PR: https://git.openjdk.java.net/jdk/pull/7558
Re: RFR: 8282023: PropertiesStoreTest and StoreReproducibilityTest jtreg failures due to en_CA locale [v4]
On Fri, 25 Feb 2022 03:54:32 GMT, Jaikiran Pai wrote: >> Can I please get a review of this test only change which fixes the issue >> noted in https://bugs.openjdk.java.net/browse/JDK-8282023? >> >> As noted in that JBS issue these tests fail when the default locale under >> which those tests are run isn't `en_US`. Both these tests were introduced as >> part of https://bugs.openjdk.java.net/browse/JDK-8231640. At a high level, >> these test do the following: >> - Use Properties.store(...) APIs to write out a properties file. This >> internally ends up writing a date comment via a call to >> `java.util.Date.toString()` method. >> - The test then runs a few checks to make sure that the written out `Date` >> is an expected one. To run these checks it uses the >> `java.time.format.DateTimeFormatter` to parse the date comment out of the >> properties file. >> >> All this works fine when the default locale is (for example) `en_US`. >> However, when the default locale is (for example `en_CA` or even `hi_IN`) >> the tests fail with an exception in the latter step above while parsing the >> date comment using the `DateTimeFormatter` instance. The exception looks >> like: >> >> >> Using locale: he for Properties#store(OutputStream) test >> test PropertiesStoreTest.testStoreOutputStreamDateComment(he): failure >> java.lang.AssertionError: Unexpected date comment: Mon Feb 21 19:10:31 IST >> 2022 >> at org.testng.Assert.fail(Assert.java:87) >> at PropertiesStoreTest.testDateComment(PropertiesStoreTest.java:255) >> at >> PropertiesStoreTest.testStoreOutputStreamDateComment(PropertiesStoreTest.java:223) >> at >> java.base/jdk.internal.reflect.DirectMethodHandleAccessor.invoke(DirectMethodHandleAccessor.java:104) >> at java.base/java.lang.reflect.Method.invoke(Method.java:577) >> at >> org.testng.internal.MethodInvocationHelper.invokeMethod(MethodInvocationHelper.java:132) >> at org.testng.internal.TestInvoker.invokeMethod(TestInvoker.java:599) >> at >> org.testng.internal.TestInvoker.invokeTestMethod(TestInvoker.java:174) >> at org.testng.internal.MethodRunner.runInSequence(MethodRunner.java:46) >> at >> org.testng.internal.TestInvoker$MethodInvocationAgent.invoke(TestInvoker.java:822) >> at >> org.testng.internal.TestInvoker.invokeTestMethods(TestInvoker.java:147) >> at >> org.testng.internal.TestMethodWorker.invokeTestMethods(TestMethodWorker.java:146) >> at org.testng.internal.TestMethodWorker.run(TestMethodWorker.java:128) >> at java.base/java.util.ArrayList.forEach(ArrayList.java:1511) >> at org.testng.TestRunner.privateRun(TestRunner.java:764) >> at org.testng.TestRunner.run(TestRunner.java:585) >> at org.testng.SuiteRunner.runTest(SuiteRunner.java:384) >> at org.testng.SuiteRunner.runSequentially(SuiteRunner.java:378) >> at org.testng.SuiteRunner.privateRun(SuiteRunner.java:337) >> at org.testng.SuiteRunner.run(SuiteRunner.java:286) >> at org.testng.SuiteRunnerWorker.runSuite(SuiteRunnerWorker.java:53) >> at org.testng.SuiteRunnerWorker.run(SuiteRunnerWorker.java:96) >> at org.testng.TestNG.runSuitesSequentially(TestNG.java:1218) >> at org.testng.TestNG.runSuitesLocally(TestNG.java:1140) >> at org.testng.TestNG.runSuites(TestNG.java:1069) >> at org.testng.TestNG.run(TestNG.java:1037) >> at >> com.sun.javatest.regtest.agent.TestNGRunner.main(TestNGRunner.java:94) >> at >> com.sun.javatest.regtest.agent.TestNGRunner.main(TestNGRunner.java:54) >> at >> java.base/jdk.internal.reflect.DirectMethodHandleAccessor.invoke(DirectMethodHandleAccessor.java:104) >> at java.base/java.lang.reflect.Method.invoke(Method.java:577) >> at >> com.sun.javatest.regtest.agent.MainWrapper$MainThread.run(MainWrapper.java:127) >> at java.base/java.lang.Thread.run(Thread.java:828) >> Caused by: java.time.format.DateTimeParseException: Text 'Mon Feb 21 >> 19:10:31 IST 2022' could not be parsed at index 0 >> at >> java.base/java.time.format.DateTimeFormatter.parseResolved0(DateTimeFormatter.java:2106) >> at >> java.base/java.time.format.DateTimeFormatter.parse(DateTimeFormatter.java:1934) >> at PropertiesStoreTest.testDateComment(PropertiesStoreTest.java:253) >> ... 30 more >> >> (in the exception above a locale with language `he` is being used) >> >> The root cause of this failure lies (only) within the tests - the >> `DateTimeFormatter` used in the tests is locale sensitive and uses the >> current (`Locale.getDefault()`) locale by default for parsing the date text. >> This parsing fails because, although `Date.toString()` javadoc states >> nothing about locales, it does mention the exact characters that will be >> used to write out the date comment. In other words, this date comment >> written out is locale insensitive and as such when parsing using >> `DateTimeFormatter` a neutral locale is appropriate on the >>
Re: RFR: 8282023: PropertiesStoreTest and StoreReproducibilityTest jtreg failures due to en_CA locale [v3]
On Thu, 24 Feb 2022 17:15:16 GMT, Naoto Sato wrote: >> Jaikiran Pai has updated the pull request incrementally with four additional >> commits since the last revision: >> >> - use Roger's suggestion of using Stream and Collection based APIs to avoid >> code duplication in the data provider method of the test >> - no need for system.out.println since testng add the chosen params to the >> output logs >> - review comments: >> - upper case static final fields in test >> - use DateTimeFormatter.ofPattern(DATE_FORMAT_PATTERN, Locale.ROOT) >> - remove @modules declaration on the jtreg test > > test/jdk/java/util/Properties/PropertiesStoreTest.java line 112: > >> 110: locales.add(Locale.getDefault()); // always test the default >> locale >> 111: locales.add(Locale.US); // guaranteed to be present >> 112: locales.add(Locale.ROOT); // guaranteed to be present > > Can we assume the returned `Set` is mutable? `Collectors.toSet()` > javadoc reads no guarantee for mutability. That's a very good point. I've updated the PR to now explicitly use a mutable `Set` instead of using `Collectors.toSet()` - PR: https://git.openjdk.java.net/jdk/pull/7558
Re: RFR: 8282023: PropertiesStoreTest and StoreReproducibilityTest jtreg failures due to en_CA locale [v4]
> Can I please get a review of this test only change which fixes the issue > noted in https://bugs.openjdk.java.net/browse/JDK-8282023? > > As noted in that JBS issue these tests fail when the default locale under > which those tests are run isn't `en_US`. Both these tests were introduced as > part of https://bugs.openjdk.java.net/browse/JDK-8231640. At a high level, > these test do the following: > - Use Properties.store(...) APIs to write out a properties file. This > internally ends up writing a date comment via a call to > `java.util.Date.toString()` method. > - The test then runs a few checks to make sure that the written out `Date` is > an expected one. To run these checks it uses the > `java.time.format.DateTimeFormatter` to parse the date comment out of the > properties file. > > All this works fine when the default locale is (for example) `en_US`. > However, when the default locale is (for example `en_CA` or even `hi_IN`) the > tests fail with an exception in the latter step above while parsing the date > comment using the `DateTimeFormatter` instance. The exception looks like: > > > Using locale: he for Properties#store(OutputStream) test > test PropertiesStoreTest.testStoreOutputStreamDateComment(he): failure > java.lang.AssertionError: Unexpected date comment: Mon Feb 21 19:10:31 IST > 2022 > at org.testng.Assert.fail(Assert.java:87) > at PropertiesStoreTest.testDateComment(PropertiesStoreTest.java:255) > at > PropertiesStoreTest.testStoreOutputStreamDateComment(PropertiesStoreTest.java:223) > at > java.base/jdk.internal.reflect.DirectMethodHandleAccessor.invoke(DirectMethodHandleAccessor.java:104) > at java.base/java.lang.reflect.Method.invoke(Method.java:577) > at > org.testng.internal.MethodInvocationHelper.invokeMethod(MethodInvocationHelper.java:132) > at org.testng.internal.TestInvoker.invokeMethod(TestInvoker.java:599) > at > org.testng.internal.TestInvoker.invokeTestMethod(TestInvoker.java:174) > at org.testng.internal.MethodRunner.runInSequence(MethodRunner.java:46) > at > org.testng.internal.TestInvoker$MethodInvocationAgent.invoke(TestInvoker.java:822) > at > org.testng.internal.TestInvoker.invokeTestMethods(TestInvoker.java:147) > at > org.testng.internal.TestMethodWorker.invokeTestMethods(TestMethodWorker.java:146) > at org.testng.internal.TestMethodWorker.run(TestMethodWorker.java:128) > at java.base/java.util.ArrayList.forEach(ArrayList.java:1511) > at org.testng.TestRunner.privateRun(TestRunner.java:764) > at org.testng.TestRunner.run(TestRunner.java:585) > at org.testng.SuiteRunner.runTest(SuiteRunner.java:384) > at org.testng.SuiteRunner.runSequentially(SuiteRunner.java:378) > at org.testng.SuiteRunner.privateRun(SuiteRunner.java:337) > at org.testng.SuiteRunner.run(SuiteRunner.java:286) > at org.testng.SuiteRunnerWorker.runSuite(SuiteRunnerWorker.java:53) > at org.testng.SuiteRunnerWorker.run(SuiteRunnerWorker.java:96) > at org.testng.TestNG.runSuitesSequentially(TestNG.java:1218) > at org.testng.TestNG.runSuitesLocally(TestNG.java:1140) > at org.testng.TestNG.runSuites(TestNG.java:1069) > at org.testng.TestNG.run(TestNG.java:1037) > at > com.sun.javatest.regtest.agent.TestNGRunner.main(TestNGRunner.java:94) > at > com.sun.javatest.regtest.agent.TestNGRunner.main(TestNGRunner.java:54) > at > java.base/jdk.internal.reflect.DirectMethodHandleAccessor.invoke(DirectMethodHandleAccessor.java:104) > at java.base/java.lang.reflect.Method.invoke(Method.java:577) > at > com.sun.javatest.regtest.agent.MainWrapper$MainThread.run(MainWrapper.java:127) > at java.base/java.lang.Thread.run(Thread.java:828) > Caused by: java.time.format.DateTimeParseException: Text 'Mon Feb 21 19:10:31 > IST 2022' could not be parsed at index 0 > at > java.base/java.time.format.DateTimeFormatter.parseResolved0(DateTimeFormatter.java:2106) > at > java.base/java.time.format.DateTimeFormatter.parse(DateTimeFormatter.java:1934) > at PropertiesStoreTest.testDateComment(PropertiesStoreTest.java:253) > ... 30 more > > (in the exception above a locale with language `he` is being used) > > The root cause of this failure lies (only) within the tests - the > `DateTimeFormatter` used in the tests is locale sensitive and uses the > current (`Locale.getDefault()`) locale by default for parsing the date text. > This parsing fails because, although `Date.toString()` javadoc states nothing > about locales, it does mention the exact characters that will be used to > write out the date comment. In other words, this date comment written out is > locale insensitive and as such when parsing using `DateTimeFormatter` a > neutral locale is appropriate on the `DateTimeFormatter` instance. This PR > thus changes the tests to use `Locale.ROOT` while parsing this date comment. >
Withdrawn: 8281093: JDK 11.0.14 violates Attribute-Value Normalization in the XML specification 1.0
On Fri, 25 Feb 2022 00:50:49 GMT, Ravi Reddy wrote: > While normalizing entity with '\r' , we should be checking if the entity is > external before changing the position and offset. This pull request has been closed without being integrated. - PR: https://git.openjdk.java.net/jdk/pull/7617
Re: RFR: 8281093: JDK 11.0.14 violates Attribute-Value Normalization in the XML specification 1.0 [v2]
> While normalizing entity with '\r' , we should be checking if the entity is > external before changing the position and offset. Ravi Reddy has updated the pull request incrementally with one additional commit since the last revision: Updated @LastModified to Feb 2022 - Changes: - all: https://git.openjdk.java.net/jdk/pull/7617/files - new: https://git.openjdk.java.net/jdk/pull/7617/files/f7de9a23..89440bfb Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=7617=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7617=00-01 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/7617.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7617/head:pull/7617 PR: https://git.openjdk.java.net/jdk/pull/7617
Re: RFR: 8281093: JDK 11.0.14 violates Attribute-Value Normalization in the XML specification 1.0
On Fri, 25 Feb 2022 00:50:49 GMT, Ravi Reddy wrote: > While normalizing entity with '\r' , we should be checking if the entity is > external before changing the position and offset. > Please also update the @lastmodified to "Feb 2022" Thanks , I have updated the tag. - PR: https://git.openjdk.java.net/jdk/pull/7617
Re: RFR: 8281093: JDK 11.0.14 violates Attribute-Value Normalization in the XML specification 1.0
On Fri, 25 Feb 2022 00:50:49 GMT, Ravi Reddy wrote: > While normalizing entity with '\r' , we should be checking if the entity is > external before changing the position and offset. Please also update the @LastModified to "Feb 2022" - Marked as reviewed by joehw (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/7617
Re: RFR: 8279995: jpackage --add-launcher option should allow overriding description [v4]
On Thu, 17 Feb 2022 17:53:57 GMT, Alexey Semenyuk wrote:
>> Alexander Matveev has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> 8279995: jpackage --add-launcher option should allow overriding
>> description [v4]
>
> test/jdk/tools/jpackage/helpers/jdk/jpackage/test/WindowsHelper.java line 165:
>
>> 163:
>> 164: return description;
>> 165: }
>
> How about this:
>
> public static String getExecutableDesciption(Path pathToExeFile) {
> Executor exec = Executor.of("powershell",
> "-NoLogo",
> "-NoProfile",
> "-Command",
> "(Get-Item \\""
> + pathToExeFile.toAbsolutePath()
> + "\\").VersionInfo | select FileDescription");
>
> var lineIt = exec.dumpOutput().executeAndGetOutput().iterator();
> while (lineIt.hasNext()) {
> var line = lineIt.next();
> if (line.trim().equals("FileDescription")) {
> // Skip "---" and move to the description value
> lineIt.next();
> var description = lineIt.next().trim();
> if (lineIt.hasNext()) {
> throw new RuntimeException("Unexpected input");
> }
> return description;
> }
> }
>
> throw new RuntimeException(String.format(
> "Failed to get file description of [%s]", pathToExeFile));
> }
>
> Added `Executor.dumpOutput()` call to save the output of powershell command
> in the test log for easier debugging.
> No null initialized `description` variable. No iteration over the list using
> the index. Check for the end of the output.
Fixed. Except I removed if (lineIt.hasNext()) { throw new
RuntimeException("Unexpected input"); }, since there are empty lines after
description in output and this check triggers exception.
-
PR: https://git.openjdk.java.net/jdk/pull/7399
Re: RFR: 8279995: jpackage --add-launcher option should allow overriding description [v6]
> Added ability to override description for additional launchers via > "description" property. Alexander Matveev has updated the pull request incrementally with one additional commit since the last revision: 8279995: jpackage --add-launcher option should allow overriding description [v5] - Changes: - all: https://git.openjdk.java.net/jdk/pull/7399/files - new: https://git.openjdk.java.net/jdk/pull/7399/files/401db249..e4893e57 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=7399=05 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7399=04-05 Stats: 18 lines in 1 file changed: 6 ins; 8 del; 4 mod Patch: https://git.openjdk.java.net/jdk/pull/7399.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7399/head:pull/7399 PR: https://git.openjdk.java.net/jdk/pull/7399
Re: RFR: 8279598: Provide adapter from RandomGenerator to Random [v8]
On Wed, 23 Feb 2022 13:14:07 GMT, Jim Laskey wrote: >> Yasser Bazzi has updated the pull request incrementally with one additional >> commit since the last revision: >> >> check from master branch > > src/java.base/share/classes/java/util/Random.java line 259: > >> 257: >> 258: private static final AtomicLong seedUniquifier >> 259: = new AtomicLong(8682522807148012L); > > This seems (to me) like an odd indentation change. Reverted the wrong indentation changes sorry about that. - PR: https://git.openjdk.java.net/jdk/pull/7001
Re: RFR: 8279598: Provide adapter from RandomGenerator to Random [v9]
> Hi, could i get a review on this implementation proposed by Stuart Marks, i > decided to use the > https://docs.oracle.com/en/java/javase/17/docs/api/java.base/java/util/random/RandomGenerator.html > interface to create the default method `asRandom()` that wraps around the > newer algorithms to be used on classes that do not accept the new interface. > > Some things to note as proposed by the bug report, the protected method > next(int bits) is not overrided and setSeed() method if left blank up to > discussion on what to do with it. > > Small test done on > https://gist.github.com/YShow/da678561419cda8e32fccf3a27a649d4 Yasser Bazzi has updated the pull request incrementally with two additional commits since the last revision: - change wrong indentation - change indentation - Changes: - all: https://git.openjdk.java.net/jdk/pull/7001/files - new: https://git.openjdk.java.net/jdk/pull/7001/files/65687cde..b002205d Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=7001=08 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7001=07-08 Stats: 6 lines in 1 file changed: 0 ins; 0 del; 6 mod Patch: https://git.openjdk.java.net/jdk/pull/7001.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7001/head:pull/7001 PR: https://git.openjdk.java.net/jdk/pull/7001
RFR: 8281093: JDK 11.0.14 violates Attribute-Value Normalization in the XML specification 1.0
While normalizing entity with '\r' , we should be checking if the entity is external before changing the position and offset. - Commit messages: - 8281093: JDK 11.0.14 violates Attribute-Value Normalization in the XML specification 1.0 Changes: https://git.openjdk.java.net/jdk/pull/7617/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=7617=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8281093 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/7617.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7617/head:pull/7617 PR: https://git.openjdk.java.net/jdk/pull/7617
Re: Proposed JEP: Safer Process Launch by ProcessBuilder and Runtime.exec
Hi, on Windows, the mechanism to launch a new program is for the parent to call CreateProcess(). This function accepts, among others parameters, a lpApplicationName string and a lpCommandLine string. There's a *single* lpCommandLine that encodes all the arguments to pass to the new program. The exact encoding of the arguments in the command line is imposed by how the *new* program parses (decodes) the command line to get its constituent parts. There are no fixed rules on how this happens. There are some more or less well documented rules for some runtimes (e.g., the C/C++ runtime) or for some specific programs (e.g., cmd.exe., wscript.exe). In general, however, a program can decode in any way it deems useful. Because the encoding is dictated by the target program's decoding, and because the latter is really arbitrary, there's no safe, universal procedure to encode a list of string arguments to a single command line string. It is only when the decoding rules in the target program are known that encoding in the parent becomes feasible. Thus, it might be more useful on Windows platforms to avoid the API points that expose List or String[] for the arguments to the target program and use the ones that accept a single String, instead. The client of those API points would then have to deal with the encoding specific for that program. This is a better match with the underlying OS mechanism, namely CreateProcess(), which accepts a single, already encoded string. In addition, to assist programmers unfamiliar with specific encodings, widely used specific encoders (e.g., for the C/C++ runtime [1], for cmd.exe, etc.) can be implemented separately. Greetings Raffaello [1] https://mail.openjdk.java.net/pipermail/core-libs-dev/2022-February/086105.html
Re: RFR: 8282008: Incorrect handling of quoted arguments in ProcessBuilder [v3]
Hi,
as far as I know, on Windows every program can obtain the lpCommandLine
argument, used in the call of CreateProcess() from its parent, by
calling GetCommandLine() and parse that string as it sees fit. This is
in stark contrast with how Unix-like systems create and execute
programs, where the system call execve(2) accepts an array of arguments,
not a single command line.
There are no fixed rules on how to parse the command line, as witnessed
by the different strategies implemented in the C/C++ runtime (which
splits the command line according to the rules outlined in [1] to
populate the argv[] array in main()), the cmd.exe shell, the wscript.exe
runtime, etc.
Consequently, there are no fixed rules on how to encode a command line
(specifically, the lpCommandLine argument to CreateProcess()) because it
really is up to the invoked program to parse it, whether explicitly or
implicitly, according to its own, directly or indirectly implemented
rules. Even a C/C++ console program could ignore the result that the
runtime automatically provides in argv[] and parse the command line
directly, as obtained by GetCommandLine().
Without knowing the parsing rules of the target program, it is not
possible to encode a command line correctly for CreateProcess(). I doubt
there's a "common denominator" which would cover most cases encountered
in practice.
The best we can hope is to implement encoders (and decoders) for
specific, widely used runtimes. (BTW, for the C/C++ runtime I prepared
an implementation mentioned in [2].)
Greetings
Raffaello
[1]
https://docs.microsoft.com/en-us/cpp/c-language/parsing-c-command-line-arguments
[2]
https://mail.openjdk.java.net/pipermail/core-libs-dev/2022-February/086105.html
On 2022-02-24 20:18, Olga Mikhaltsova wrote:
On Fri, 18 Feb 2022 17:21:48 GMT, Olga Mikhaltsova
wrote:
This fix made equal processing of strings such as ""C:\\Program Files\\Git\\""
before and after JDK-8250568.
For example, it's needed to execute the following command on Windows:
`C:\Windows\SysWOW64\WScript.exe "MyVB.vbs" "C:\Program Files\Git" "Test"`
it's equal to:
`new ProcessBuilder("C:\\Windows\\SysWOW64\\WScript.exe", "MyVB.vbs", ""C:\\Program
Files\\Git\\"", "Test").start();`
While processing, the 3rd argument ""C:\\Program Files\\Git\\"" treated as
unquoted due to the condition added in JDK-8250568.
private static String unQuote(String str) {
..
if (str.endsWith("\\"")) {
return str;// not properly quoted, treat as unquoted
}
..
}
that leads to the additional surrounding by quotes in
ProcessImpl::createCommandLine(..) because needsEscaping(..) returns true due
to the space inside the string argument.
As a result the native function CreateProcessW
(src/java.base/windows/native/libjava/ProcessImpl_md.c) gets the incorrectly
quoted argument:
pcmd = C:\Windows\SysWOW64\WScript.exe MyVB.vbs ""C:\Program Files\Git"" Test
(jdk.lang.Process.allowAmbiguousCommands = true)
pcmd = "C:\Windows\SysWOW64\WScript.exe" MyVB.vbs ""C:\Program Files\Git\\""
Test
(jdk.lang.Process.allowAmbiguousCommands = false)
Obviously, a string ending with `"\\""` must not be started with `"""` to treat
as unquoted overwise it’s should be treated as properly quoted.
Olga Mikhaltsova has updated the pull request incrementally with one additional
commit since the last revision:
Add a new line to the end of test file for JDK-8282008
Roger, writing a test via echo was not a good idea obviously for this particular case
because of the fact well shown in the doc "4. Everyone Parses Differently",
https://daviddeley.com/autohotkey/parameters/parameters.htm#WINCRULESMSEX. The task is
more complicated than it seems at the first glance. The same command line correctly
parsed in an app written in C/C++ might be incorrectly parsed in a VBS app etc.
The suggestion not to use the path-argument surroundings with `'"'` doesn't fix the issue
in case of VBS. It leads to a resulting path-argument ending with the doubled backslash in a
VBS app according to the rules "10.1 The WSH Command Line Parameter Parsing Rules",
https://daviddeley.com/autohotkey/parameters/parameters.htm#WSH.
Below there are some experiments with an app attached to JDK-8282008:
NO FIX
1. new ProcessBuilder("C:\\Windows\\SysWOW64\\WScript.exe", "MyVB.vbs", ""C:\\Program
Files\\Git\\"", "Test").start();
1.1 allowAmbiguousCommands = false
arg[0] = \C:\Program
arg[1] = Files\Git1\\\
CreateProcessW: pcmd = "C:\Windows\SysWOW64\WScript.exe" MyVB.vbs ""C:\Program
Files\Git1\\"" Test
1.2 allowAmbiguousCommands = true
arg[0] = C:\Program
arg[1] = Files\Git1\
CreateProcessW: pcmd = C:\Windows\SysWOW64\WScript.exe MyVB.vbs ""C:\Program
Files\Git1"" Test
2. new ProcessBuilder("C:\\Windows\\SysWOW64\\WScript.exe", "MyVB.vbs", "C:\\Program
Files\\Git\", "Test").start();
2.1 allowAmbiguousCommands = false
arg[0] = C:\Program Files\Git1\\
Integrated: 8280357: user.home = "?" when running with systemd DynamicUser=true
On Fri, 18 Feb 2022 15:29:34 GMT, Roger Riggs wrote: > In some Linux configurations, the Linux home directory provided by getpwent > is not usable. > The value of the system property `user.home` should fallback to the value of > $HOME > if getpwent.user_home is null or less that 2 characters long. "/" is not a > valid home directory name. > > If $HOME is undefined or empty, the value of the getpwent.user_home is > retained. > > There are more details in the Jira issue: > https://bugs.openjdk.java.net/browse/JDK-8280357 > > The fix has been tested manually on Ubuntu 20.0.4 using the suggested systemd > command line and variations. This pull request has now been integrated. Changeset: bf19fc65 Author:Roger Riggs URL: https://git.openjdk.java.net/jdk/commit/bf19fc65c71cba8cb4383d714fe8993acd01be0a Stats: 10 lines in 1 file changed: 8 ins; 0 del; 2 mod 8280357: user.home = "?" when running with systemd DynamicUser=true Reviewed-by: naoto, alanb - PR: https://git.openjdk.java.net/jdk/pull/7534
Re: RFR: 8282047: Enhance StringDecode/Encode microbenchmarks
On Thu, 17 Feb 2022 15:11:11 GMT, Claes Redestad wrote:
> Splitting out these micro changes from #7231
>
> - Clean up and simplify setup and code
> - Add variants with different inputs with varying lengths and encoding
> weights, but also relevant mixes of each so that we both cover interesting
> corner cases but also verify that performance behave when there's a multitude
> of input shapes. Both simple and mixed variants are interesting diagnostic
> tools.
> - Drop all charsets from the default run configuration except UTF-8.
> Motivation: Defaults should give good coverage but try to keep runtimes at
> bay. Additionally if the charset tested can't encode the higher codepoints
> used in these micros the results might be misleading. If you for example test
> using ISO-8859-1 the UTF-16 bytes in StringDecode.decodeUTF16 will have all
> been replaced by `?`, so the test is effectively the same as testing
> ASCII-only.
Changes requested by bchristi (Reviewer).
test/micro/org/openjdk/bench/java/lang/StringDecode.java line 36:
> 34: @Warmup(iterations = 5, time = 2)
> 35: @Measurement(iterations = 5, time = 3)
> 36: @State(Scope.Thread)
No change to @Fork and @Warmup (as in the Encode test)?
test/micro/org/openjdk/bench/java/lang/StringDecode.java line 49:
> 47: public class StringDecode {
> 48:
> 49: @Param({"US-ASCII", "ISO-8859-1", "UTF-8", "MS932", "ISO-8859-6",
> "ISO-2022-KR"})
What would you think of retaining the previous set of charset names as a
comment -- as a suggestion for someone who wants additional coverage?
test/micro/org/openjdk/bench/java/lang/StringDecode.java line 93:
> 91: public void decodeAsciiLong(Blackhole bh) throws Exception {
> 92: bh.consume(new String(longAsciiString, charset));
> 93: bh.consume(new String(longAsciiString, 0, 1024 + 31, charset));
I imagine the 1024+31 addition gets compiled down, and is not executed during
the test, right?
test/micro/org/openjdk/bench/java/lang/StringEncode.java line 39:
> 37: public class StringEncode {
> 38:
> 39: @Param({"US-ASCII", "ISO-8859-1", "UTF-8", "MS932", "ISO-8859-6"})
Same, re: keeping list as a comment.
-
PR: https://git.openjdk.java.net/jdk/pull/7516
Re: RFR: 8280357: user.home = "?" when running with systemd DynamicUser=true [v2]
On Wed, 23 Feb 2022 22:00:19 GMT, Roger Riggs wrote: >> In some Linux configurations, the Linux home directory provided by getpwent >> is not usable. >> The value of the system property `user.home` should fallback to the value of >> $HOME >> if getpwent.user_home is null or less that 2 characters long. "/" is not a >> valid home directory name. >> >> If $HOME is undefined or empty, the value of the getpwent.user_home is >> retained. >> >> There are more details in the Jira issue: >> https://bugs.openjdk.java.net/browse/JDK-8280357 >> >> The fix has been tested manually on Ubuntu 20.0.4 using the suggested >> systemd command line and variations. > > Roger Riggs has updated the pull request incrementally with one additional > commit since the last revision: > > fallback to '?' for user.home if both the pw_dir and /Users/rriggs are not > valid Marked as reviewed by alanb (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/7534
Re: RFR: 8282008: Incorrect handling of quoted arguments in ProcessBuilder [v3]
On Fri, 18 Feb 2022 17:21:48 GMT, Olga Mikhaltsova
wrote:
>> This fix made equal processing of strings such as ""C:\\Program
>> Files\\Git\\"" before and after JDK-8250568.
>>
>> For example, it's needed to execute the following command on Windows:
>> `C:\Windows\SysWOW64\WScript.exe "MyVB.vbs" "C:\Program Files\Git" "Test"`
>> it's equal to:
>> `new ProcessBuilder("C:\\Windows\\SysWOW64\\WScript.exe", "MyVB.vbs",
>> ""C:\\Program Files\\Git\\"", "Test").start();`
>>
>> While processing, the 3rd argument ""C:\\Program Files\\Git\\"" treated as
>> unquoted due to the condition added in JDK-8250568.
>>
>> private static String unQuote(String str) {
>> ..
>>if (str.endsWith("\\"")) {
>> return str;// not properly quoted, treat as unquoted
>> }
>> ..
>> }
>>
>>
>> that leads to the additional surrounding by quotes in
>> ProcessImpl::createCommandLine(..) because needsEscaping(..) returns true
>> due to the space inside the string argument.
>> As a result the native function CreateProcessW
>> (src/java.base/windows/native/libjava/ProcessImpl_md.c) gets the incorrectly
>> quoted argument:
>>
>> pcmd = C:\Windows\SysWOW64\WScript.exe MyVB.vbs ""C:\Program Files\Git"" Test
>> (jdk.lang.Process.allowAmbiguousCommands = true)
>> pcmd = "C:\Windows\SysWOW64\WScript.exe" MyVB.vbs ""C:\Program Files\Git\\""
>> Test
>> (jdk.lang.Process.allowAmbiguousCommands = false)
>>
>>
>> Obviously, a string ending with `"\\""` must not be started with `"""` to
>> treat as unquoted overwise it’s should be treated as properly quoted.
>
> Olga Mikhaltsova has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Add a new line to the end of test file for JDK-8282008
Roger, writing a test via echo was not a good idea obviously for this
particular case because of the fact well shown in the doc "4. Everyone Parses
Differently",
https://daviddeley.com/autohotkey/parameters/parameters.htm#WINCRULESMSEX. The
task is more complicated than it seems at the first glance. The same command
line correctly parsed in an app written in C/C++ might be incorrectly parsed in
a VBS app etc.
The suggestion not to use the path-argument surroundings with `'"'` doesn't fix
the issue in case of VBS. It leads to a resulting path-argument ending with the
doubled backslash in a VBS app according to the rules "10.1 The WSH Command
Line Parameter Parsing Rules",
https://daviddeley.com/autohotkey/parameters/parameters.htm#WSH.
Below there are some experiments with an app attached to JDK-8282008:
NO FIX
1. new ProcessBuilder("C:\\Windows\\SysWOW64\\WScript.exe", "MyVB.vbs",
""C:\\Program Files\\Git\\"", "Test").start();
1.1 allowAmbiguousCommands = false
arg[0] = \C:\Program
arg[1] = Files\Git1\\\
CreateProcessW: pcmd = "C:\Windows\SysWOW64\WScript.exe" MyVB.vbs ""C:\Program
Files\Git1\\"" Test
1.2 allowAmbiguousCommands = true
arg[0] = C:\Program
arg[1] = Files\Git1\
CreateProcessW: pcmd = C:\Windows\SysWOW64\WScript.exe MyVB.vbs ""C:\Program
Files\Git1"" Test
2. new ProcessBuilder("C:\\Windows\\SysWOW64\\WScript.exe", "MyVB.vbs",
"C:\\Program Files\\Git\", "Test").start();
2.1 allowAmbiguousCommands = false
arg[0] = C:\Program Files\Git1\\
arg[1] = Test
CreateProcessW: pcmd = "C:\Windows\SysWOW64\WScript.exe" MyVB.vbs "C:\Program
Files\Git1\" Test
2.2 allowAmbiguousCommands = true
arg[0] = C:\Program Files\Git1\\
arg[1] = Test
CreateProcessW: pcmd = C:\Windows\SysWOW64\WScript.exe MyVB.vbs "C:\Program
Files\Git1\" Test
FIXED (as in pr)
1. new ProcessBuilder("C:\\Windows\\SysWOW64\\WScript.exe", "MyVB.vbs",
""C:\\Program Files\\Git\\"", "Test").start();
1.1 allowAmbiguousCommands = false
arg[0] = C:\Program Files\Git1\
arg[1] = Test
CreateProcessW: pcmd = "C:\Windows\SysWOW64\WScript.exe" MyVB.vbs "C:\Program
Files\Git1" Test
1.2 allowAmbiguousCommands = true
arg[0] = C:\Program Files\Git1\
arg[1] = Test
CreateProcessW: pcmd = C:\Windows\SysWOW64\WScript.exe MyVB.vbs "C:\Program
Files\Git1" Test
```
Reading the code of unQuote() in terms of logic: "no beginning or ending quote,
or too short => not quoted", - and it seems that if all these conditions are
just opposite (starting and ending quotes and long enough) then a string should
be treated as quoted but it's not. One exception was added and it's strange
that it's applied even in case of paired quotes. Is it truly necessary for the
security fix to skip (=to treat as unquoted) a string starting and ending with
`'"'` in case of a `"\\""` tail?
This proposed fix returns back possibility (as was previously) to use
surrounding with `'"'` as an argument mark-up that allows to pass correctly a
path with a space inside in case of VBS. Roger, would you be so kind to take a
look at this small fix once again, please, and to pay attention to VBS parsing
arguments problem?!
-
PR:
Re: RFR: 8282221: x86 intrinsics for divideUnsigned and remainderUnsigned methods in java.lang.Integer and java.lang.Long [v4]
On Thu, 24 Feb 2022 14:13:47 GMT, Jatin Bhateja wrote: >> Vamsi Parasa has updated the pull request incrementally with one additional >> commit since the last revision: >> >> fix 32bit build issues > > src/hotspot/cpu/x86/c2_MacroAssembler_x86.cpp line 4408: > >> 4406: jmp(done); >> 4407: bind(neg_divisor_fastpath); >> 4408: // Fastpath for divisor < 0: > > How about checking if divisor is +ve or -ve constant and non-constant > dividend in identity routine and setting a flag in IR node, which can be used > to either emit fast / slow path in a new instruction selection pattern. It > will save emitting redundant instructions. Thanks for suggesting the enhancement. This enhancement will be implemented as a part of https://bugs.openjdk.java.net/browse/JDK-8282365 > src/hotspot/share/opto/divnode.cpp line 881: > >> 879: return (phase->type( in(2) )->higher_equal(TypeLong::ONE)) ? in(1) : >> this; >> 880: } >> 881: >> //--Value-- > > Ideal transform to replace unsigned divide by cheaper logical right shift > instruction if divisor is POW will be useful. Thanks for suggesting the enhancement. This enhancement will be implemented as a part of https://bugs.openjdk.java.net/browse/JDK-8282365 > src/hotspot/share/opto/divnode.cpp line 897: > >> 895: >> 896: // Either input is BOTTOM ==> the result is the local BOTTOM >> 897: const Type *bot = bottom_type(); > > Can we add constant folding handling when both dividend and divisor are > constants. Thanks for suggesting the enhancement. This enhancement will be implemented as a part of https://bugs.openjdk.java.net/browse/JDK-8282365 - PR: https://git.openjdk.java.net/jdk/pull/7572
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 The piece I was missing is that the HotSpot AIX specific code, computes and sets LIBPATH based on the path to the java executable. This computed LIBPATH value is set in the environment unless it is overridden by an existing LIBPATH in the environment. The test cases that failed, do not supply a value for LIBPATH so the launcher provided value is inserted and thereafter reported as the environment from the child process. Since both of these tests are testing a different condition not related to LIBPATH, the presence/accuracy of LIBPATH is not the point of the test. The current solution to remove `test.nativepath` works and is ok by me. (The alternative used by #7581, to include LIBPATH in the environment when launching might be slightly more robust to future changes.) Thanks for the followup. - PR: https://git.openjdk.java.net/jdk/pull/7574
Integrated: 8281525: Enable Zc:strictStrings flag in Visual Studio build
On Mon, 21 Feb 2022 19:55:14 GMT, Daniel Jeliński wrote: > Please review this PR that enables > [Zc:strictStrings](https://docs.microsoft.com/en-us/cpp/build/reference/zc-strictstrings-disable-string-literal-type-conversion?view=msvc-170) > compiler flag, which makes assigning a string literal to a non-const pointer > a compile-time error. > > This type of assignment is [disallowed by C++ standard since > C++11](https://en.cppreference.com/w/cpp/language/string_literal). Writing to > a string literal through a non-const pointer [produces a run-time > error](https://docs.microsoft.com/en-us/cpp/cpp/string-and-character-literals-cpp?view=msvc-170#microsoft-specific-1). > > The included code changes are trivial; I added `const` keyword to variable > and parameter declarations where needed, and added explicit casts to > non-const pointers where adding `const` was not feasible. > > I verified that the build passes both with and without `--enable-debug`, both > with VS2017 and VS2019. This pull request has now been integrated. Changeset: 23995f82 Author:Daniel Jeliński Committer: Daniel Fuchs URL: https://git.openjdk.java.net/jdk/commit/23995f822e540d799eb4bbc969229422257fbb08 Stats: 23 lines in 6 files changed: 0 ins; 0 del; 23 mod 8281525: Enable Zc:strictStrings flag in Visual Studio build Reviewed-by: dholmes, ihse - PR: https://git.openjdk.java.net/jdk/pull/7565
Re: RFR: 8281525: Enable Zc:strictStrings flag in Visual Studio build [v2]
On Tue, 22 Feb 2022 16:43:28 GMT, Daniel Jeliński wrote: >> Please review this PR that enables >> [Zc:strictStrings](https://docs.microsoft.com/en-us/cpp/build/reference/zc-strictstrings-disable-string-literal-type-conversion?view=msvc-170) >> compiler flag, which makes assigning a string literal to a non-const >> pointer a compile-time error. >> >> This type of assignment is [disallowed by C++ standard since >> C++11](https://en.cppreference.com/w/cpp/language/string_literal). Writing >> to a string literal through a non-const pointer [produces a run-time >> error](https://docs.microsoft.com/en-us/cpp/cpp/string-and-character-literals-cpp?view=msvc-170#microsoft-specific-1). >> >> The included code changes are trivial; I added `const` keyword to variable >> and parameter declarations where needed, and added explicit casts to >> non-const pointers where adding `const` was not feasible. >> >> I verified that the build passes both with and without `--enable-debug`, >> both with VS2017 and VS2019. > > Daniel Jeliński has updated the pull request incrementally with one > additional commit since the last revision: > > Move strictStrings to toolchain_cflags Great, thanks all for reviewing! - PR: https://git.openjdk.java.net/jdk/pull/7565
Re: RFR: 8281525: Enable Zc:strictStrings flag in Visual Studio build [v2]
On Tue, 22 Feb 2022 16:43:28 GMT, Daniel Jeliński wrote: >> Please review this PR that enables >> [Zc:strictStrings](https://docs.microsoft.com/en-us/cpp/build/reference/zc-strictstrings-disable-string-literal-type-conversion?view=msvc-170) >> compiler flag, which makes assigning a string literal to a non-const >> pointer a compile-time error. >> >> This type of assignment is [disallowed by C++ standard since >> C++11](https://en.cppreference.com/w/cpp/language/string_literal). Writing >> to a string literal through a non-const pointer [produces a run-time >> error](https://docs.microsoft.com/en-us/cpp/cpp/string-and-character-literals-cpp?view=msvc-170#microsoft-specific-1). >> >> The included code changes are trivial; I added `const` keyword to variable >> and parameter declarations where needed, and added explicit casts to >> non-const pointers where adding `const` was not feasible. >> >> I verified that the build passes both with and without `--enable-debug`, >> both with VS2017 and VS2019. > > Daniel Jeliński has updated the pull request incrementally with one > additional commit since the last revision: > > Move strictStrings to toolchain_cflags I'm the original author of `sspi.cpp`. All changes look fine to me. Thanks a lot for the enhancement. - PR: https://git.openjdk.java.net/jdk/pull/7565
Re: Incorrect return value for Java_jdk_internal_loader_NativeLibraries_load() in java.base/share/native/libjava/NativeLibraries.c
This is not an issue and does not affect the runtiime. When it fails to
load a native library, the VM will throw UnsatisfiedLinkError (see
JVM_LoadLibrary). The return value is only important for the JNI case
and calling System::loadLibrary on the system with dynamic linker cache
(Big Sur) since throwExceptionIfFail == false.
I agree that the JNI_TRUE return value for non-JNI case (i.e. raw
library library mechanism) is confusing to the readers. We should fix
it to return handle != 0L to match the specified return value (true only
when succeed).
I will file an issue.
Mandy
On 2/23/22 4:08 PM, Cheng Jin wrote:
Hi there,
The return value from Java_jdk_internal_loader_NativeLibraries_load()
athttps://github.com/openjdk/jdk/blob/master/src/java.base/share/native/libjava/NativeLibraries.c
seems incorrect according to the code logic in there.
Looking at the calling path from loadLibrary() to load() at
src/java.base/share/classes/jdk/internal/loader/NativeLibraries.java as follows:
public NativeLibrary loadLibrary(Class fromClass, String name) {
...
NativeLibrary lib = findFromPaths(LibraryPaths.SYS_PATHS, fromClass,
name); <--
if (lib == null && searchJavaLibraryPath) {
lib = findFromPaths(LibraryPaths.USER_PATHS, fromClass, name);
<
}
return lib;
}
private NativeLibrary findFromPaths(String[] paths, Class fromClass,
String name) {
for (String path : paths) { < keep searching the
library paths till the library is located at the correct lib path
File libfile = new File(path, System.mapLibraryName(name));
NativeLibrary nl = loadLibrary(fromClass, libfile); ?
if (nl != null) {
return nl; <
}
libfile = ClassLoaderHelper.mapAlternativeName(libfile);
if (libfile != null) {
nl = loadLibrary(fromClass, libfile);
if (nl != null) {
return nl;
}
}
}
return null;
}
public NativeLibrary loadLibrary(Class fromClass, File file) {
// Check to see if we're attempting to access a static library
String name = findBuiltinLib(file.getName());
boolean isBuiltin = (name != null);
...
return loadLibrary(fromClass, name, isBuiltin); <
}
private NativeLibrary loadLibrary(Class fromClass, String name, boolean
isBuiltin) {
...
NativeLibraryImpl lib = new NativeLibraryImpl(fromClass, name,
isBuiltin, isJNI);
// load the native library
NativeLibraryContext.push(lib);
try {
if (!lib.open()) { <- call load()
return null;// fail to open the native library
}
...
// register the loaded native library
loadedLibraryNames.add(name);
libraries.put(name, lib);
return lib; <--- return an invalid lib as lib.open()
returns true
} finally {
releaseNativeLibraryLock(name);
}
...
}
/*
* Loads the named native library
*/
boolean open() {
...
return load(this, name, isBuiltin, isJNI,
loadLibraryOnlyIfPresent);
}
private static native boolean load(NativeLibraryImpl impl, String name,
boolean isBuiltin, boolean isJNI, boolean throwExceptionIfFail);
and then native code in java.base/share/native/libjava/NativeLibraries.c
Java_jdk_internal_loader_NativeLibraries_load
(JNIEnv *env, jobject this, jobject lib, jstring name,
jboolean isBuiltin, jboolean isJNI, jboolean throwExceptionIfFail)
{
const char *cname;
jint jniVersion;
jthrowable cause;
void * handle;
jboolean loaded = JNI_FALSE;
...
handle = isBuiltin ? procHandle : JVM_LoadLibrary(cname,
throwExceptionIfFail);
if (isJNI) {
...
}
(*env)->SetLongField(env, lib, handleID, ptr_to_jlong(handle));
loaded = JNI_TRUE; <- always return true when isJNI is false and a
null handle
done:
JNU_ReleaseStringPlatformChars(env, name, cname);
return loaded;
}
Assuming there are multiple library paths existing in the jdk/lib which are
added to sun.boot.library.path and java.library.path
and the expected library (libnative.so) is only placed in jdk/lib. e.g.
lib/libA
lib/libB
lib/libC
lib/libnative.so
when the code in findFromPaths() searches the library paths set in
sun.boot.library.path and java.library.path starting from lib/LibA,
it will end up with an invalid value if load() returns true in the case of a
false isJNI and a null handle returned from JVM_LoadLibrary()
as the library doesn't exist in lib/libA passed to JVM_LoadLibrary().
To ensure findFromPaths()
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: 8282023: PropertiesStoreTest and StoreReproducibilityTest jtreg failures due to en_CA locale [v3]
On Thu, 24 Feb 2022 05:02:47 GMT, Jaikiran Pai wrote: >> Can I please get a review of this test only change which fixes the issue >> noted in https://bugs.openjdk.java.net/browse/JDK-8282023? >> >> As noted in that JBS issue these tests fail when the default locale under >> which those tests are run isn't `en_US`. Both these tests were introduced as >> part of https://bugs.openjdk.java.net/browse/JDK-8231640. At a high level, >> these test do the following: >> - Use Properties.store(...) APIs to write out a properties file. This >> internally ends up writing a date comment via a call to >> `java.util.Date.toString()` method. >> - The test then runs a few checks to make sure that the written out `Date` >> is an expected one. To run these checks it uses the >> `java.time.format.DateTimeFormatter` to parse the date comment out of the >> properties file. >> >> All this works fine when the default locale is (for example) `en_US`. >> However, when the default locale is (for example `en_CA` or even `hi_IN`) >> the tests fail with an exception in the latter step above while parsing the >> date comment using the `DateTimeFormatter` instance. The exception looks >> like: >> >> >> Using locale: he for Properties#store(OutputStream) test >> test PropertiesStoreTest.testStoreOutputStreamDateComment(he): failure >> java.lang.AssertionError: Unexpected date comment: Mon Feb 21 19:10:31 IST >> 2022 >> at org.testng.Assert.fail(Assert.java:87) >> at PropertiesStoreTest.testDateComment(PropertiesStoreTest.java:255) >> at >> PropertiesStoreTest.testStoreOutputStreamDateComment(PropertiesStoreTest.java:223) >> at >> java.base/jdk.internal.reflect.DirectMethodHandleAccessor.invoke(DirectMethodHandleAccessor.java:104) >> at java.base/java.lang.reflect.Method.invoke(Method.java:577) >> at >> org.testng.internal.MethodInvocationHelper.invokeMethod(MethodInvocationHelper.java:132) >> at org.testng.internal.TestInvoker.invokeMethod(TestInvoker.java:599) >> at >> org.testng.internal.TestInvoker.invokeTestMethod(TestInvoker.java:174) >> at org.testng.internal.MethodRunner.runInSequence(MethodRunner.java:46) >> at >> org.testng.internal.TestInvoker$MethodInvocationAgent.invoke(TestInvoker.java:822) >> at >> org.testng.internal.TestInvoker.invokeTestMethods(TestInvoker.java:147) >> at >> org.testng.internal.TestMethodWorker.invokeTestMethods(TestMethodWorker.java:146) >> at org.testng.internal.TestMethodWorker.run(TestMethodWorker.java:128) >> at java.base/java.util.ArrayList.forEach(ArrayList.java:1511) >> at org.testng.TestRunner.privateRun(TestRunner.java:764) >> at org.testng.TestRunner.run(TestRunner.java:585) >> at org.testng.SuiteRunner.runTest(SuiteRunner.java:384) >> at org.testng.SuiteRunner.runSequentially(SuiteRunner.java:378) >> at org.testng.SuiteRunner.privateRun(SuiteRunner.java:337) >> at org.testng.SuiteRunner.run(SuiteRunner.java:286) >> at org.testng.SuiteRunnerWorker.runSuite(SuiteRunnerWorker.java:53) >> at org.testng.SuiteRunnerWorker.run(SuiteRunnerWorker.java:96) >> at org.testng.TestNG.runSuitesSequentially(TestNG.java:1218) >> at org.testng.TestNG.runSuitesLocally(TestNG.java:1140) >> at org.testng.TestNG.runSuites(TestNG.java:1069) >> at org.testng.TestNG.run(TestNG.java:1037) >> at >> com.sun.javatest.regtest.agent.TestNGRunner.main(TestNGRunner.java:94) >> at >> com.sun.javatest.regtest.agent.TestNGRunner.main(TestNGRunner.java:54) >> at >> java.base/jdk.internal.reflect.DirectMethodHandleAccessor.invoke(DirectMethodHandleAccessor.java:104) >> at java.base/java.lang.reflect.Method.invoke(Method.java:577) >> at >> com.sun.javatest.regtest.agent.MainWrapper$MainThread.run(MainWrapper.java:127) >> at java.base/java.lang.Thread.run(Thread.java:828) >> Caused by: java.time.format.DateTimeParseException: Text 'Mon Feb 21 >> 19:10:31 IST 2022' could not be parsed at index 0 >> at >> java.base/java.time.format.DateTimeFormatter.parseResolved0(DateTimeFormatter.java:2106) >> at >> java.base/java.time.format.DateTimeFormatter.parse(DateTimeFormatter.java:1934) >> at PropertiesStoreTest.testDateComment(PropertiesStoreTest.java:253) >> ... 30 more >> >> (in the exception above a locale with language `he` is being used) >> >> The root cause of this failure lies (only) within the tests - the >> `DateTimeFormatter` used in the tests is locale sensitive and uses the >> current (`Locale.getDefault()`) locale by default for parsing the date text. >> This parsing fails because, although `Date.toString()` javadoc states >> nothing about locales, it does mention the exact characters that will be >> used to write out the date comment. In other words, this date comment >> written out is locale insensitive and as such when parsing using >> `DateTimeFormatter` a neutral locale is appropriate on the >>
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: >> Ichiroh Takiguchi has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Add null check > > 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. Thanks @RogerRiggs I could understand why this issue was happened. You said: > As far as I can tell, the test does not modify the environment and originally > passes LIBPATH without modification. I'd like to confirm one thing. My patch did not pass `LIBPATH` environment variable to child process. You mean the testcase should pass parent `LIBPATH` to child process ? If so, it's better to use #7581 . (Please use bugid [8282219](https://bugs.openjdk.java.net/browse/JDK-8282219)) - PR: https://git.openjdk.java.net/jdk/pull/7574
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 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. - 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: 8279508: Auto-vectorize Math.round API [v7]
On Thu, 24 Feb 2022 00:43:27 GMT, Sandhya Viswanathan wrote: > Also curious, how does the performance look with all these changes. Updated new perf numbers. - PR: https://git.openjdk.java.net/jdk/pull/7094
RFR: JDK-8282144 RandomSupport.convertSeedBytesToLongs sign extension overwrites previous bytes
Class: ./java.base/share/classes/jdk/internal/util/random/RandomSupport.java Method: public static long[] convertSeedBytesToLongs(byte[] seed, int n, int z) The method attempts to create an array of longs by consuming the input bytes most significant bit first. New bytes are appended to the existing long using the OR operator on the signed byte. Due to sign extension this will overwrite all the existing bits from 63 to 8 if the next byte is negative. - Commit messages: - JDK-8282144 RandomSupport.convertSeedBytesToLongs sign extension overwrites previous bytes Changes: https://git.openjdk.java.net/jdk/pull/7614/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=7614=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8282144 Stats: 72 lines in 2 files changed: 71 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/7614.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7614/head:pull/7614 PR: https://git.openjdk.java.net/jdk/pull/7614
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 Jtreg constructs the environment before launching the Agent to run the test. https://github.com/openjdk/jtreg/blob/163ae223409f789cb733d32ed8d33686e14a81d9/src/share/classes/com/sun/javatest/regtest/exec/MainAction.java#L415 For AIX, it appends the `test.nativepath` to LIBPATH. https://github.com/openjdk/jtreg/blob/163ae223409f789cb733d32ed8d33686e14a81d9/src/share/classes/com/sun/javatest/regtest/exec/Action.java#L146 As far as I can tell, the test does not modify the environment and originally passes LIBPATH without modification. - PR: https://git.openjdk.java.net/jdk/pull/7574
Re: RFR: 8282221: x86 intrinsics for divideUnsigned and remainderUnsigned methods in java.lang.Integer and java.lang.Long [v4]
On Thu, 24 Feb 2022 02:43:46 GMT, Vamsi Parasa wrote: >> Optimizes the divideUnsigned() and remainderUnsigned() methods in >> java.lang.Integer and java.lang.Long classes using x86 intrinsics. This >> change shows 3x improvement for Integer methods and upto 25% improvement for >> Long. This change also implements the DivMod optimization which fuses >> division and modulus operations if needed. The DivMod optimization shows 3x >> improvement for Integer and ~65% improvement for Long. > > Vamsi Parasa has updated the pull request incrementally with one additional > commit since the last revision: > > fix 32bit build issues src/hotspot/cpu/x86/c2_MacroAssembler_x86.cpp line 4408: > 4406: jmp(done); > 4407: bind(neg_divisor_fastpath); > 4408: // Fastpath for divisor < 0: How about checking if divisor is +ve or -ve constant and non-constant dividend in identity routine and setting a flag in IR node, which can be used to either emit fast / slow path in a new instruction selection pattern. It will save emitting redundant instructions. src/hotspot/share/opto/divnode.cpp line 881: > 879: return (phase->type( in(2) )->higher_equal(TypeLong::ONE)) ? in(1) : > this; > 880: } > 881: > //--Value-- Ideal transform to replace unsigned divide by cheaper logical right shift instruction if divisor is POW will be useful. src/hotspot/share/opto/divnode.cpp line 897: > 895: > 896: // Either input is BOTTOM ==> the result is the local BOTTOM > 897: const Type *bot = bottom_type(); Can we add constant folding handling when both dividend and divisor are constants. - PR: https://git.openjdk.java.net/jdk/pull/7572
Re: RFR: 8282047: Enhance StringDecode/Encode microbenchmarks
On Thu, 17 Feb 2022 15:11:11 GMT, Claes Redestad wrote: > Splitting out these micro changes from #7231 > > - Clean up and simplify setup and code > - Add variants with different inputs with varying lengths and encoding > weights, but also relevant mixes of each so that we both cover interesting > corner cases but also verify that performance behave when there's a multitude > of input shapes. Both simple and mixed variants are interesting diagnostic > tools. > - Drop all charsets from the default run configuration except UTF-8. > Motivation: Defaults should give good coverage but try to keep runtimes at > bay. Additionally if the charset tested can't encode the higher codepoints > used in these micros the results might be misleading. If you for example test > using ISO-8859-1 the UTF-16 bytes in StringDecode.decodeUTF16 will have all > been replaced by `?`, so the test is effectively the same as testing > ASCII-only. Ping? - PR: https://git.openjdk.java.net/jdk/pull/7516
Re: RFR: 8279508: Auto-vectorize Math.round API [v7]
On Thu, 24 Feb 2022 01:43:27 GMT, Sandhya Viswanathan
wrote:
>> Jatin Bhateja has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> 8279508: Review comments resolved.
>
> src/hotspot/cpu/x86/macroAssembler_x86.cpp line 8984:
>
>> 8982: }
>> 8983:
>> 8984: void MacroAssembler::round_double(Register dst, XMMRegister src,
>> Register rtmp, Register rcx) {
>
> Is it possible to implement this using the similar mxcsr change? In any case
> comments will help to review round_double and round_float code.
LDMXCSR has multi-cycle latency and it will degrade the performance of scalar
operation's fast path.
-
PR: https://git.openjdk.java.net/jdk/pull/7094
Re: RFR: 8279508: Auto-vectorize Math.round API [v8]
> Summary of changes: > - Intrinsify Math.round(float) and Math.round(double) APIs. > - Extend auto-vectorizer to infer vector operations on encountering scalar IR > nodes for above intrinsics. > - Test creation using new IR testing framework. > > Following are the performance number of a JMH micro included with the patch > > Test System: Intel(R) Xeon(R) Platinum 8380 CPU @ 2.30GHz (Icelake Server) > > > TESTSIZE | Baseline AVX3 (ops/ms) | Withopt AVX3 (ops/ms) | Gain ratio | > Baseline AVX2 (ops/ms) | Withopt AVX2 (ops/ms) | Gain ratio > -- | -- | -- | -- | -- | -- | -- > 1024.00 | 510.41 | 1811.66 | 3.55 | 510.40 | 502.65 | 0.98 > 2048.00 | 293.52 | 984.37 | 3.35 | 304.96 | 177.88 | 0.58 > 1024.00 | 825.94 | 3387.64 | 4.10 | 750.77 | 1925.15 | 2.56 > 2048.00 | 411.91 | 1942.87 | 4.72 | 412.22 | 1034.13 | 2.51 > > > Kindly review and share your feedback. > > Best Regards, > Jatin Jatin Bhateja has updated the pull request incrementally with one additional commit since the last revision: 8279508: Review comments resolved. - Changes: - all: https://git.openjdk.java.net/jdk/pull/7094/files - new: https://git.openjdk.java.net/jdk/pull/7094/files/6c869c76..f7dec3d9 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=7094=07 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7094=06-07 Stats: 35 lines in 5 files changed: 8 ins; 22 del; 5 mod Patch: https://git.openjdk.java.net/jdk/pull/7094.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7094/head:pull/7094 PR: https://git.openjdk.java.net/jdk/pull/7094
Integrated: 8282188: Unused static field MathContext.DEFAULT_DIGITS
On Fri, 18 Feb 2022 19:07:15 GMT, Andrey Turbanov wrote: > 8282188: Unused static field MathContext.DEFAULT_DIGITS This pull request has now been integrated. Changeset: 3cfffa4f Author:Andrey Turbanov URL: https://git.openjdk.java.net/jdk/commit/3cfffa4f8e5a0fff7f232130125c549f992b533b Stats: 6 lines in 1 file changed: 0 ins; 5 del; 1 mod 8282188: Unused static field MathContext.DEFAULT_DIGITS Reviewed-by: darcy, bpb - PR: https://git.openjdk.java.net/jdk/pull/7538
