Re: RFR: 8282221: x86 intrinsics for divideUnsigned and remainderUnsigned methods in java.lang.Integer and java.lang.Long

[Jatin Bhateja]

On Tue, 22 Feb 2022 09:24:47 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.

src/hotspot/cpu/x86/x86_64.ad line 8602:

> 8600: __ jmp(done);
> 8601: __ bind(neg_divisor_fastpath); 
> 8602: // Fastpath for divisor < 0: 

Move in macro assembly routine.

src/hotspot/cpu/x86/x86_64.ad line 8633:

> 8631: __ jmp(done);
> 8632: __ bind(neg_divisor_fastpath);
> 8633: // Fastpath for divisor < 0: 

Move in macro assembly rountine.

src/hotspot/cpu/x86/x86_64.ad line 8722:

> 8720: __ shrl(rax, 31); // quotient
> 8721: __ sarl(tmp, 31);
> 8722: __ andl(tmp, divisor);

Move in macro assembly routine.

src/hotspot/cpu/x86/x86_64.ad line 8763:

> 8761: __ andnq(rax, rax, rdx);
> 8762: __ movq(tmp, rax);
> 8763: __ shrq(rax, 63); // quotient

Move in macro assembly routine.

src/hotspot/cpu/x86/x86_64.ad line 8902:

> 8900: __ subl(tmp_rax, divisor);
> 8901: __ andnl(tmp_rax, tmp_rax, rdx);
> 8902: __ sarl(tmp_rax, 31);

Please move this into a macro assembly routine.

src/hotspot/cpu/x86/x86_64.ad line 8932:

> 8930: // Fastpath when divisor < 0: 
> 8931: // remainder = dividend - (((dividend & ~(dividend - divisor)) >> 
> (Long.SIZE - 1)) & divisor)
> 8932: // See Hacker's Delight (2nd ed), section 9.3 which is implemented 
> in java.lang.Long.remainderUnsigned()

Please move it into a macro assembly routine.

src/hotspot/share/opto/compile.cpp line 3499:

> 3497:   Node* d = n->find_similar(Op_UDivI);
> 3498:   if (d) {
> 3499: // Replace them with a fused unsigned divmod if supported

Can you explain a bit here, why can't this transformation be handled earlier ?

src/hotspot/share/opto/divnode.cpp line 1350:

> 1348: return NULL;
> 1349:   }
> 1350: 

Please remove Value and Ideal routines if no explicit transforms are being done.

src/hotspot/share/opto/divnode.cpp line 1362:

> 1360:   }
> 1361: 
> 1362: 
> //=

You can remove Ideal routine is not transformation is being done.

test/micro/org/openjdk/bench/java/lang/IntegerDivMod.java line 76:

> 74: return quotients;
> 75: }
> 76: 

Return seems redundant here.

test/micro/org/openjdk/bench/java/lang/IntegerDivMod.java line 83:

> 81: }
> 82: return remainders;
> 83: }

Return seems redundant here.

test/micro/org/openjdk/bench/java/lang/LongDivMod.java line 75:

> 73: }
> 74: return quotients;
> 75: }

Do we need to return quotients, since it's a field  being explicitly modified.

test/micro/org/openjdk/bench/java/lang/LongDivMod.java line 82:

> 80: remainders[i] = Long.remainderUnsigned(dividends[i], 
> divisors[i]);
> 81: }
> 82: return remainders;

Same as above

-

PR: https://git.openjdk.java.net/jdk/pull/7572

Re: RFR: 8275731: CDS archived enums objects are recreated at runtime [v6]

[Ioi Lam]

> **Background:**
> 
> In the Java Language, Enums can be tested for equality, so the constants in 
> an Enum type must be unique. Javac compiles an enum declaration like this:
> 
> 
> public enum Day {  SUNDAY, MONDAY ... } 
> 
> 
> to
> 
> 
> public class Day extends java.lang.Enum {
> public static final SUNDAY = new Day("SUNDAY");
> public static final MONDAY = new Day("MONDAY"); ...
> }
> 
> 
> With CDS archived heap objects, `Day::` is executed twice: once 
> during `java -Xshare:dump`, and once during normal JVM execution. If the 
> archived heap objects references one of the Enum constants created at dump 
> time, we will violate the uniqueness requirements of the Enum constants at 
> runtime. See the test case in the description of 
> [JDK-8275731](https://bugs.openjdk.java.net/browse/JDK-8275731)
> 
> **Fix:**
> 
> During -Xshare:dump, if we discovered that an Enum constant of type X is 
> archived, we archive all constants of type X. At Runtime, type X will skip 
> the normal execution of `X::`. Instead, we run 
> `HeapShared::initialize_enum_klass()` to retrieve all the constants of X that 
> were saved at dump time.
> 
> This is safe as we know that `X::` has no observable side effect -- 
> it only creates the constants of type X, as well as the synthetic value 
> `X::$VALUES`, which cannot be observed until X is fully initialized.
> 
> **Verification:**
> 
> To avoid future problems, I added a new tool, CDSHeapVerifier, to look for 
> similar problems where the archived heap objects reference a static field 
> that may be recreated at runtime. There are some manual steps involved, but I 
> analyzed the potential problems found by the tool are they are all safe 
> (after the current bug is fixed). See cdsHeapVerifier.cpp for gory details. 
> An example trace of this tool can be found at 
> https://bugs.openjdk.java.net/secure/attachment/97242/enum_warning.txt
> 
> **Testing:**
> 
> Passed Oracle CI tiers 1-4. WIll run tier 5 as well.

Ioi Lam has updated the pull request incrementally with one additional commit 
since the last revision:

  fixed whitespace

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/6653/files
  - new: https://git.openjdk.java.net/jdk/pull/6653/files/4764075e..c6e9be1d

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=6653=05
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=6653=04-05

  Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod
  Patch: https://git.openjdk.java.net/jdk/pull/6653.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/6653/head:pull/6653

PR: https://git.openjdk.java.net/jdk/pull/6653

Re: RFR: 8275731: CDS archived enums objects are recreated at runtime [v5]

[Ioi Lam]

> **Background:**
> 
> In the Java Language, Enums can be tested for equality, so the constants in 
> an Enum type must be unique. Javac compiles an enum declaration like this:
> 
> 
> public enum Day {  SUNDAY, MONDAY ... } 
> 
> 
> to
> 
> 
> public class Day extends java.lang.Enum {
> public static final SUNDAY = new Day("SUNDAY");
> public static final MONDAY = new Day("MONDAY"); ...
> }
> 
> 
> With CDS archived heap objects, `Day::` is executed twice: once 
> during `java -Xshare:dump`, and once during normal JVM execution. If the 
> archived heap objects references one of the Enum constants created at dump 
> time, we will violate the uniqueness requirements of the Enum constants at 
> runtime. See the test case in the description of 
> [JDK-8275731](https://bugs.openjdk.java.net/browse/JDK-8275731)
> 
> **Fix:**
> 
> During -Xshare:dump, if we discovered that an Enum constant of type X is 
> archived, we archive all constants of type X. At Runtime, type X will skip 
> the normal execution of `X::`. Instead, we run 
> `HeapShared::initialize_enum_klass()` to retrieve all the constants of X that 
> were saved at dump time.
> 
> This is safe as we know that `X::` has no observable side effect -- 
> it only creates the constants of type X, as well as the synthetic value 
> `X::$VALUES`, which cannot be observed until X is fully initialized.
> 
> **Verification:**
> 
> To avoid future problems, I added a new tool, CDSHeapVerifier, to look for 
> similar problems where the archived heap objects reference a static field 
> that may be recreated at runtime. There are some manual steps involved, but I 
> analyzed the potential problems found by the tool are they are all safe 
> (after the current bug is fixed). See cdsHeapVerifier.cpp for gory details. 
> An example trace of this tool can be found at 
> https://bugs.openjdk.java.net/secure/attachment/97242/enum_warning.txt
> 
> **Testing:**
> 
> Passed Oracle CI tiers 1-4. WIll run tier 5 as well.

Ioi Lam has updated the pull request with a new target base due to a merge or a 
rebase. The pull request now contains seven commits:

 - Fixed comments per @calvinccheung review
 - Merge branch 'master' into 8275731-heapshared-enum
 - Use InstanceKlass::do_local_static_fields for some field iterations
 - Merge branch 'master' into 8275731-heapshared-enum
 - added exclusions needed by "java -Xshare:dump -ea -esa"
 - Comments from @calvinccheung off-line
 - 8275731: CDS archived enums objects are recreated at runtime

-

Changes: https://git.openjdk.java.net/jdk/pull/6653/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk=6653=04
  Stats: 850 lines in 16 files changed: 807 ins; 2 del; 41 mod
  Patch: https://git.openjdk.java.net/jdk/pull/6653.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/6653/head:pull/6653

PR: https://git.openjdk.java.net/jdk/pull/6653

Re: RFR: JDK-8266670: Better modeling of access flags in core reflection [v12]

[Joe Darcy]

> This is an early review of changes to better model JVM access flags, that is 
> "modifiers" like public, protected, etc. but explicitly at a VM level.
> 
> Language level modifiers and JVM level access flags are closely related, but 
> distinct. There are concepts that overlap in the two domains (public, 
> private, etc.), others that only have a language-level modifier (sealed), and 
> still others that only have an access flag (synthetic).
> 
> The existing java.lang.reflect.Modifier class is inadequate to model these 
> subtleties. For example, the bit positions used by access flags on different 
> kinds of elements overlap (such as "volatile" for fields and "bridge" for 
> methods. Just having a raw integer does not provide sufficient context to 
> decode the corresponding language-level string. Methods like 
> Modifier.methodModifiers() were introduced to cope with this situation.
> 
> With additional modifiers and flags on the horizon with projects like 
> Valhalla, addressing the existent modeling deficiency now ahead of time is 
> reasonable before further strain is introduced.
> 
> This PR in its current form is meant to give the overall shape of the API. It 
> is missing implementations to map from, say, method modifiers to access 
> flags, taking into account overlaps in bit positions.
> 
> The CSR https://bugs.openjdk.java.net/browse/JDK-8281660 will be filled in 
> once the API is further along.

Joe Darcy has updated the pull request incrementally with one additional commit 
since the last revision:

  Initial support for accessFlags methods

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/7445/files
  - new: https://git.openjdk.java.net/jdk/pull/7445/files/bb4a3013..e8f5b7d7

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=7445=11
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7445=10-11

  Stats: 53 lines in 5 files changed: 34 ins; 1 del; 18 mod
  Patch: https://git.openjdk.java.net/jdk/pull/7445.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/7445/head:pull/7445

PR: https://git.openjdk.java.net/jdk/pull/7445

Re: RFR: 8279508: Auto-vectorize Math.round API [v6]

[Sandhya Viswanathan]

On Thu, 17 Feb 2022 17:43:43 GMT, Jatin Bhateja  wrote:

>> 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: Fixing for windows failure.

src/hotspot/cpu/x86/c2_MacroAssembler_x86.cpp line 4146:

> 4144:   vaddpd(xtmp1, src , xtmp1, vec_enc);
> 4145:   vrndscalepd(dst, xtmp1, 0x4, vec_enc);
> 4146:   evcvtpd2qq(dst, dst, vec_enc);

Why do we need vrndscalepd in between, could we not directly use cvtpd2qq after 
vaddpd?

-

PR: https://git.openjdk.java.net/jdk/pull/7094

Re: RFR: 8282023: PropertiesStoreTest and StoreReproducibilityTest jtreg failures due to en_CA locale [v2]

[Jaikiran Pai]

On Tue, 22 Feb 2022 17:31:14 GMT, Naoto Sato  wrote:

>> Jaikiran Pai has updated the pull request incrementally with two additional 
>> commits since the last revision:
>> 
>>  - implement review comments
>>  - copyright years
>
> test/jdk/java/util/Properties/PropertiesStoreTest.java line 50:
> 
>> 48:  * @test
>> 49:  * @summary tests the order in which the Properties.store() method 
>> writes out the properties
>> 50:  * @bug 8231640
> 
> Add the bug id, also `@modules jdk.localedata` needs to be added.

I have added this to the updated PR. But just to understand why this is needed, 
I had a look at the jtreg tag specification doc, which states:
>
> a test will not be run if the system being tested does not contain all of the 
> specified modules

So with this `@modules` added, it will no longer run tests where the 
`jdk.localedata` isn't present. Given that this `PropertiesStoreTest` tests a 
few other things other than the locale insensitive parsing of the date comment, 
do you think adding this `@modules` would now skip some of the other testing in 
this test unnecessarily? Furthermore, since the `provideLocales()` data 
provider method in this test has necessary checks (via `getAvailableLocales()`) 
to only use the non-guaranteed locales if they are available, do you think this 
`@modules` is still needed?

-

PR: https://git.openjdk.java.net/jdk/pull/7558

Re: RFR: 8282023: PropertiesStoreTest and StoreReproducibilityTest jtreg failures due to en_CA locale [v2]

[Jaikiran Pai]

> 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.  
> 

Re: RFR: 8282023: PropertiesStoreTest and StoreReproducibilityTest jtreg failures due to en_CA locale

[Jaikiran Pai]

On Mon, 21 Feb 2022 14:09:50 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 `DateTimeFormatter` instance. This PR 
> thus changes the tests to 

Re: RFR: 8279598: Provide adapter from RandomGenerator to Random [v8]

[Yasser Bazzi]

> 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 one additional 
commit since the last revision:

  check from master branch

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/7001/files
  - new: https://git.openjdk.java.net/jdk/pull/7001/files/399aa222..65687cde

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=7001=07
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7001=06-07

  Stats: 10 lines in 1 file changed: 0 ins; 9 del; 1 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

Re: RFR: 8279598: Provide adapter from RandomGenerator to Random [v7]

[Yasser Bazzi]

> 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 one additional 
commit since the last revision:

  Remove changes from RandomGenerator

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/7001/files
  - new: https://git.openjdk.java.net/jdk/pull/7001/files/5dc0e1ea..399aa222

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=7001=06
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7001=05-06

  Stats: 18 lines in 1 file changed: 10 ins; 0 del; 8 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

Re: RFR: JDK-8266670: Better modeling of access flags in core reflection [v11]

[Joe Darcy]

On Wed, 23 Feb 2022 00:40:44 GMT, Joe Darcy  wrote:

>> This is an early review of changes to better model JVM access flags, that is 
>> "modifiers" like public, protected, etc. but explicitly at a VM level.
>> 
>> Language level modifiers and JVM level access flags are closely related, but 
>> distinct. There are concepts that overlap in the two domains (public, 
>> private, etc.), others that only have a language-level modifier (sealed), 
>> and still others that only have an access flag (synthetic).
>> 
>> The existing java.lang.reflect.Modifier class is inadequate to model these 
>> subtleties. For example, the bit positions used by access flags on different 
>> kinds of elements overlap (such as "volatile" for fields and "bridge" for 
>> methods. Just having a raw integer does not provide sufficient context to 
>> decode the corresponding language-level string. Methods like 
>> Modifier.methodModifiers() were introduced to cope with this situation.
>> 
>> With additional modifiers and flags on the horizon with projects like 
>> Valhalla, addressing the existent modeling deficiency now ahead of time is 
>> reasonable before further strain is introduced.
>> 
>> This PR in its current form is meant to give the overall shape of the API. 
>> It is missing implementations to map from, say, method modifiers to access 
>> flags, taking into account overlaps in bit positions.
>> 
>> The CSR https://bugs.openjdk.java.net/browse/JDK-8281660 will be filled in 
>> once the API is further along.
>
> Joe Darcy has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Add mask to access flag functionality.

I've pushed an initial version of functionality to map from an integer mask to 
a set of access flags based on location.

I suspect this will be refined and changed a bit before the code goes back. 
Potential changes I see now:

* an ANY_CLASS location allowing the union of the flags for CLASS and 
INNER_CLASS (or a method providing equivalent functionality)
*  simple maskToAccessFlags(int mask) method that worked if all the bit 
positions were unambiguous.

@asotona , how does this API look for your use cases?

I'll start wiring up the various accessFlag methods on java.lang.Class, 
java.lang.reflect.* etc.

-

PR: https://git.openjdk.java.net/jdk/pull/7445

Re: RFR: JDK-8266670: Better modeling of access flags in core reflection [v11]

[Joe Darcy]

> This is an early review of changes to better model JVM access flags, that is 
> "modifiers" like public, protected, etc. but explicitly at a VM level.
> 
> Language level modifiers and JVM level access flags are closely related, but 
> distinct. There are concepts that overlap in the two domains (public, 
> private, etc.), others that only have a language-level modifier (sealed), and 
> still others that only have an access flag (synthetic).
> 
> The existing java.lang.reflect.Modifier class is inadequate to model these 
> subtleties. For example, the bit positions used by access flags on different 
> kinds of elements overlap (such as "volatile" for fields and "bridge" for 
> methods. Just having a raw integer does not provide sufficient context to 
> decode the corresponding language-level string. Methods like 
> Modifier.methodModifiers() were introduced to cope with this situation.
> 
> With additional modifiers and flags on the horizon with projects like 
> Valhalla, addressing the existent modeling deficiency now ahead of time is 
> reasonable before further strain is introduced.
> 
> This PR in its current form is meant to give the overall shape of the API. It 
> is missing implementations to map from, say, method modifiers to access 
> flags, taking into account overlaps in bit positions.
> 
> The CSR https://bugs.openjdk.java.net/browse/JDK-8281660 will be filled in 
> once the API is further along.

Joe Darcy has updated the pull request incrementally with one additional commit 
since the last revision:

  Add mask to access flag functionality.

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/7445/files
  - new: https://git.openjdk.java.net/jdk/pull/7445/files/46d4d7ea..bb4a3013

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=7445=10
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7445=09-10

  Stats: 80 lines in 2 files changed: 78 ins; 0 del; 2 mod
  Patch: https://git.openjdk.java.net/jdk/pull/7445.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/7445/head:pull/7445

PR: https://git.openjdk.java.net/jdk/pull/7445

Re: RFR: 8279598: Provide adapter from RandomGenerator to Random [v6]

[liach]

On Sun, 20 Feb 2022 06:39:26 GMT, liach  wrote:

>> Yasser Bazzi has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   remove missed whitespace
>
> src/java.base/share/classes/java/util/Random.java line 95:
> 
>> 93: private static class RandomWrapper extends Random implements 
>> RandomGenerator {
>> 94: private final RandomGenerator generator;
>> 95: private final boolean initialized;
> 
> Can we create a private or package-private constructor for `Random` for this 
> subclass' constructor to call? The no-arg constructor has some unnecessary 
> calculations, and the `long` constructor calls `setSeed`. A special `Random` 
> constructor for this subclass allows removing this special logic (by not 
> calling `setSeed` at all) and avoid redundant seed initialization.

More specifically, in Random class, declare

// special constructor for RandomWrapper to call
private Random(Void unused) {
this.seed = new AtomicLong();
}


So the randomwrapper can be (with `initialized` field removed):

private RandomWrapper(RandomGenerator randomToWrap) {
super(null);
this.generator = randomToWrap;
}

@Override
public void setSeed(long seed) {
throw new UnsupportedOperationException();
}


Currently, the RandomWrapper constructor calls the no-arg super constructor 
implicitly, which has some overheads for unnecessary calculations.

-

PR: https://git.openjdk.java.net/jdk/pull/7001

Re: RFR: 8282239 [testbug, AIX] ProcessBuilder/Basic.java fails with incorrect LIBPATH [v2]

[Tyler Steele]

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

Replace simple iterations of Map.entrySet with Map.forEach calls

[-]

Hello,
In the patch for 8281631 ,
xenoamess intentionally avoided
 repeatedly
calling Map::size, for it may not be constant-timed due to being
concurrent. This alerted me that the for loops on Map::entrySet may be
similarly not thread-safe.

I believe that we should migrate for loop on Map::entrySet to Map::forEach
calls; concurrent map callees usually have dedicated logic

to mitigate such thread-safety issues. Synchronized map

callees are benefitted too.

Another lesser benefit is reduced object allocation for iteration. While
the most popular implementations (HashMap, LinkedHashMap, WeakHashMap,
TreeMap) don't need to allocate entries for iteration, many other (EnumMap,
IdentityHashMap, concurrent maps) do, and iterating those maps with forEach
is less costly. (Note that 8170826
 takes care of
implementing proper forEach in EnumMap)

A JBS issue has already been submitted at
https://bugs.openjdk.java.net/browse/JDK-8282178, and I have prepared a
patch. This patch modifies the putAll implementations of a few JDK maps to
let the callee feed entries through Map::forEach to be put into the maps.
Two places of Map::entrySet calls in Collectors has also been updated.

For most other existing usages in the JDK, I don't think they will benefit
from such a migration: They mostly iterate on the entryset of the popular
implementations that already host entries and are single-threaded, and I
don't think it's in our best interest to touch those use cases.

So, here are my questions:
1. Is such a concept of replacing Map::entrySet calls with Map::forEach
calls reasonable, or is there any fundamental flaw?
If the concept sounds good, I will submit a patch, so we can answer
2. Is the changes implemented correct for this concept, i.e. targeting code
where users supply callee maps?

Best regards

Re: RFR: 8282239 [testbug, AIX] ProcessBuilder/Basic.java fails with incorrect LIBPATH [v2]

[Roger Riggs]

On Tue, 22 Feb 2022 22:04:38 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.
>
> Tyler Steele has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Updates copyright year & adds bugid

> /label add noreg-self

fyi, the noreg-* labels apply to the bug report, not the PR.  (and yes, they 
are informative when reviewing).

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.

-

PR: https://git.openjdk.java.net/jdk/pull/7581

Re: RFR: 8282219: jdk/java/lang/ProcessBuilder/Basic.java fails on AIX [v2]

[Tyler Steele]

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: 8281962: Avoid unnecessary native calls in InflaterInputStream [v2]

[Lance Andersen]

On Thu, 17 Feb 2022 16:02:42 GMT, Volker Simonis  wrote:

>> Currently, `InflaterInputStream::read()` first does a native call to the 
>> underlying zlib `inflate()` function and only afterwards checks if the 
>> inflater requires input (i.e. `Inflater::needsInput()`) or has finished 
>> inflating (`Inflater::finished()`). This leads to an unnecessary native call 
>> to `inflate()` when `InflaterInputStream::read()` is invoked for the very 
>> first time because `Inflater::fill()` hasn't been called and another 
>> unnecessary native call to detect the end of the stream. For small 
>> streams/files which completely fit into the output buffer passed to 
>> `InflaterInputStream::read()` we can therefore save two out of three native 
>> calls. The attached micro benchmark shows that this results in a 5%-10% 
>> performance improvement for zip files of sizes between 4096 to 256 bytes 
>> (notice that in common jars like Tomcat, Spring-Boot, Maven, Jackson, etc. 
>> about 60-80% of the classes are smaller than 4096 bytes).
>> 
>> 
>> before JDK-8281962
>> --
>> Benchmark (size)  Mode  Cnt  Score   
>> Error  Units
>> InflaterInputStreams.inflaterInputStreamRead 256  avgt5  2.571 ± 
>> 0.120  us/op
>> InflaterInputStreams.inflaterInputStreamRead 512  avgt5  2.861 ± 
>> 0.064  us/op
>> InflaterInputStreams.inflaterInputStreamRead4096  avgt5  5.110 ± 
>> 0.278  us/op
>> 
>> after JDK-8281962
>> -
>> Benchmark (size)  Mode  Cnt  Score   
>> Error  Units
>> InflaterInputStreams.inflaterInputStreamRead 256  avgt5  2.332 ± 
>> 0.081  us/op
>> InflaterInputStreams.inflaterInputStreamRead 512  avgt5  2.691 ± 
>> 0.293  us/op
>> InflaterInputStreams.inflaterInputStreamRead4096  avgt5  4.812 ± 
>> 1.038  us/op
>> 
>> 
>> Tested with the JTreg zip/jar/zipfs and the JCK zip/jar tests.
>> 
>> As a side effect, this change also fixes an issue with alternative zlib 
>> implementations like zlib-Chromium or zlib-Cloudflare which pad the inflated 
>> bytes with a specif byte pattern at the end of the output for debugging 
>> purpose. This breaks code patterns like the following:
>> 
>> 
>> int readcount = 0;
>> while ((bytesRead = inflaterInputStream.read(data, 0, bufferSize)) != -1) {
>> outputStream.write(data, 0, bytesRead);
>> readCount++;
>> }
>> if (readCount == 1) {
>> return data; //  < first bytes might be overwritten
>> }
>> return outputStream.toByteArray();
>> 
>> 
>> Even if the whole data fits into the `data` array during the first call to 
>> `inflaterInputStream.read()`, we still need a second call to 
>> `inflaterInputStream.read()` in order to detect the end of the inflater 
>> stream. If this second call calls into the native `inflate()` function of 
>> Cloudflare/Chromium's zlib version, this will still write some padding bytes 
>> at the beginning of the `data` array and overwrite the previously read data. 
>> This issue has been reported in Spring [1] and ASM [2] when using these 
>> libraries with Cloudflares zlib version (by setting `LD_LIBRARY_PATH` 
>> accordingly).
>> 
>> [1] https://github.com/spring-projects/spring-framework/issues/27429
>> [2] https://gitlab.ow2.org/asm/asm/-/issues/317955
>
> Volker Simonis has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Changed hardcoded constant to JMH parmater and removed non-ASCII chars from 
> comments

Mach5 runs did not raise any issues, so you should be OK to move forward.

-

Marked as reviewed by lancea (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/7492

Re: RFR: 8280409: JarFile::getInputStream can fail with NPE accessing ze.getName() [v4]

[Lance Andersen]

On Sat, 19 Feb 2022 10:59:56 GMT, Alan Bateman  wrote:

> > Ok, thank you for the feedback. I just pushed a change with additional 
> > comments on the jar creation which hopefully will address your input above.
> 
> It's a bit better but I think it needs a clear step-by-step instructions in a 
> comment before declaration of VALID_ENTRY_NAME to show how the JAR file is 
> created, signed (move the instructions that have been placed on the TestNG 
> setup method), and then converted to the byte array. Further maintainers will 
> thank you.

Just pushed a revised set of comments.  Hopefully this will get us across the 
goal line.

-

PR: https://git.openjdk.java.net/jdk/pull/7348

Re: RFR: 8280409: JarFile::getInputStream can fail with NPE accessing ze.getName() [v8]

[Lance Andersen]

> Hi all,
> 
> Please review the attached patch to address
> 
> - That JarFile::getInputStream did not check for a null ZipEntry passed as a 
> parameter
> - Have Zip/JarFile::getInputStream throw a ZipException in the event that an 
> unexpected exception occurs
> 
> Mach5 tiers1-3 runs are clean as are the TCK java.util.zip and java.util.jar 
> test runs
> 
> Best
> Lance

Lance Andersen has updated the pull request incrementally with one additional 
commit since the last revision:

  Modified and clarified test comments

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/7348/files
  - new: https://git.openjdk.java.net/jdk/pull/7348/files/839d99f7..e540a3a1

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=7348=07
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7348=06-07

  Stats: 64 lines in 1 file changed: 34 ins; 18 del; 12 mod
  Patch: https://git.openjdk.java.net/jdk/pull/7348.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/7348/head:pull/7348

PR: https://git.openjdk.java.net/jdk/pull/7348

Re: RFR: 8282239 [testbug, AIX] ProcessBuilder/Basic.java fails with incorrect LIBPATH [v2]

[Tyler Steele]

> 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

[Tyler Steele]

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

Re: RFR: JDK-8266670: Better modeling of access flags in core reflection [v10]

[Joe Darcy]

> This is an early review of changes to better model JVM access flags, that is 
> "modifiers" like public, protected, etc. but explicitly at a VM level.
> 
> Language level modifiers and JVM level access flags are closely related, but 
> distinct. There are concepts that overlap in the two domains (public, 
> private, etc.), others that only have a language-level modifier (sealed), and 
> still others that only have an access flag (synthetic).
> 
> The existing java.lang.reflect.Modifier class is inadequate to model these 
> subtleties. For example, the bit positions used by access flags on different 
> kinds of elements overlap (such as "volatile" for fields and "bridge" for 
> methods. Just having a raw integer does not provide sufficient context to 
> decode the corresponding language-level string. Methods like 
> Modifier.methodModifiers() were introduced to cope with this situation.
> 
> With additional modifiers and flags on the horizon with projects like 
> Valhalla, addressing the existent modeling deficiency now ahead of time is 
> reasonable before further strain is introduced.
> 
> This PR in its current form is meant to give the overall shape of the API. It 
> is missing implementations to map from, say, method modifiers to access 
> flags, taking into account overlaps in bit positions.
> 
> The CSR https://bugs.openjdk.java.net/browse/JDK-8281660 will be filled in 
> once the API is further along.

Joe Darcy has updated the pull request with a new target base due to a merge or 
a rebase. The incremental webrev excludes the unrelated changes brought in by 
the merge/rebase. The pull request contains 16 additional commits since the 
last revision:

 - Add test for location disjointness
 - Minor cleanup.
 - Merge branch 'master' into JDK-8266670
 - Switch to location enum.
 - Respond to review feedback.
 - Add support for module flags; fix typo.
 - Merge branch 'master' into JDK-8266670
 - Update JVMS references.
 - Merge branch 'master' into JDK-8266670
 - Reorder constants by mask value per review feedback.
 - ... and 6 more: https://git.openjdk.java.net/jdk/compare/024b847b...46d4d7ea

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/7445/files
  - new: https://git.openjdk.java.net/jdk/pull/7445/files/c3f6b0a4..46d4d7ea

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=7445=09
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7445=08-09

  Stats: 8276 lines in 193 files changed: 5054 ins; 1931 del; 1291 mod
  Patch: https://git.openjdk.java.net/jdk/pull/7445.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/7445/head:pull/7445

PR: https://git.openjdk.java.net/jdk/pull/7445

Withdrawn: 8277474: jarsigner does not check if algorithm parameters are disabled

[Hai-May Chao]

On Tue, 22 Feb 2022 20:18:19 GMT, Hai-May Chao  wrote:

> This fixes jarsigner to enforce checking against algorithm constraint 
> properties so when the signature algorithms parameters use disabled or legacy 
> algorithms, it will emit warnings accordingly. If the algorithm used in 
> parameters is disabled, jarsigner treats the jar as unsigned.

This pull request has been closed without being integrated.

-

PR: https://git.openjdk.java.net/jdk/pull/7580

RFR: 8277474: jarsigner does not check if algorithm parameters are disabled

[Hai-May Chao]

This fixes jarsigner to enforce checking against algorithm constraint 
properties so when the signature algorithms parameters use disabled or legacy 
algorithms, it will emit warnings accordingly. If the algorithm used in 
parameters is disabled, jarsigner treats the jar as unsigned.

-

Commit messages:
 - 8277474: jarsigner does not check if algorithm parameters are disabled
 - Testcase updated
 - 8265765: DomainKeyStore may stop enumerating aliases if a constituting 
KeyStore is empty

Changes: https://git.openjdk.java.net/jdk/pull/7580/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk=7580=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8277474
  Stats: 256 lines in 5 files changed: 240 ins; 3 del; 13 mod
  Patch: https://git.openjdk.java.net/jdk/pull/7580.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/7580/head:pull/7580

PR: https://git.openjdk.java.net/jdk/pull/7580

RFR: 8282221: x86 intrinsics for divideUnsigned and remainderUnsigned methods in java.lang.Integer and java.lang.Long

[Vamsi Parasa]

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.

-

Commit messages:
 - fix trailing white space errors
 - fix whitespaces
 - revert comment to original for divmodI
 - Update rax and rdx register usage in x86_64.ad
 - 8282221: x86 intrinsics for divideUnsigned and remainderUnsigned methods in 
java.lang.Integer and java.lang.Long

Changes: https://git.openjdk.java.net/jdk/pull/7572/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk=7572=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8282221
  Stats: 741 lines in 16 files changed: 738 ins; 1 del; 2 mod
  Patch: https://git.openjdk.java.net/jdk/pull/7572.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/7572/head:pull/7572

PR: https://git.openjdk.java.net/jdk/pull/7572

Re: RFR: 8282219: jdk/java/lang/ProcessBuilder/Basic.java fails on AIX [v2]

[Roger Riggs]

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

For my curiosity, how is AIX different from other Linux in that the 
test.nativepath is not/should not be in LIBPATH?

test/jdk/java/lang/ProcessBuilder/Basic.java line 81:

> 79: /* used for AIX only */
> 80: static final String libpath = System.getenv("LIBPATH");
> 81: static final String expectedLibpath;

Can you check the usage of libpath at line 1362. How is this case different 
from the other two at 1878 and 1943?
I would move the change to the `expected` value to move into the block that is 
already testing AIX.is at line 1367.

I would prefer to have a single static with the libpath expected by the 3 
places it is used in the test.

test/jdk/java/lang/ProcessBuilder/Basic.java line 1900:

> 1898: if (AIX.is()) {
> 1899: commandOutput = removeAixExpectedVars(commandOutput);
> 1900: expected = expected + "LIBPATH="+expectedLibpath+",";

Please add spaces around operators +, that is the convention and in this file.

-

PR: https://git.openjdk.java.net/jdk/pull/7574

Re: RFR: 8282023: PropertiesStoreTest and StoreReproducibilityTest jtreg failures due to en_CA locale

[Naoto Sato]

On Mon, 21 Feb 2022 14:09:50 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 `DateTimeFormatter` instance. This PR 
> thus changes the tests to 

Re: RFR: 8282188: Unused static field MathContext.DEFAULT_DIGITS

[Brian Burkhalter]

On Fri, 18 Feb 2022 19:07:15 GMT, Andrey Turbanov  wrote:

> 8282188: Unused static field MathContext.DEFAULT_DIGITS

Marked as reviewed by bpb (Reviewer).

-

PR: https://git.openjdk.java.net/jdk/pull/7538

Re: RFR: 8282219: jdk/java/lang/ProcessBuilder/Basic.java fails on AIX [v2]

[Ichiroh Takiguchi]

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

@RogerRiggs 
I appreciate your good suggestion.
Since `libpath` is just used on AIX and it's `static final String`.
I created `expectedLibpath` for expected result.
About `removeAll` method, it requires regular expression string.
I think if directory name has `.`, unexpected situation may be happened.

-

PR: https://git.openjdk.java.net/jdk/pull/7574

Re: RFR: 8282219: jdk/java/lang/ProcessBuilder/Basic.java fails on AIX [v2]

[Ichiroh Takiguchi]

> 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

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/7574/files
  - new: https://git.openjdk.java.net/jdk/pull/7574/files/9d1faeb7..8b88686d

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=7574=01
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7574=00-01

  Stats: 31 lines in 1 file changed: 16 ins; 14 del; 1 mod
  Patch: https://git.openjdk.java.net/jdk/pull/7574.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/7574/head:pull/7574

PR: https://git.openjdk.java.net/jdk/pull/7574

Re: RFR: 8281525: Enable Zc:strictStrings flag in Visual Studio build [v2]

[Magnus Ihse Bursie]

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

Now the build changes look good. Thanks!

-

Marked as reviewed by ihse (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/7565

Re: RFR: 8209784: Include hsdis in the JDK

[Magnus Ihse Bursie]

On Tue, 22 Feb 2022 16:54:21 GMT, Andrew Haley  wrote:

> Maybe I missed it, but are there instructions somewhere about how to build 
> this, and what should be downloaded?

See https://github.com/openjdk/jdk/blob/master/src/utils/hsdis/README.md

-

PR: https://git.openjdk.java.net/jdk/pull/7578

Integrated: 8261407: ReflectionFactory.checkInitted() is not thread-safe

[liach]

On Sun, 19 Dec 2021 03:21:55 GMT, liach  wrote:

> Upon review of [8261407](https://bugs.openjdk.java.net/browse/JDK-8261407), 
> by design, duplicate initialization of ReflectionFactory should be safe as it 
> performs side-effect-free property read actions, and the suggesting of making 
> the `initted` field volatile cannot prevent concurrent initialization either; 
> however, having `initted == true` published without the other fields' values 
> is a possibility, which this patch addresses.
> 
> This simulates what's done in `CallSite`'s constructor for 
> `ConstantCallSite`. Please feel free to point out the problems with this 
> patch, as I am relatively inexperienced in this field of fences and there are 
> relatively less available documents. (Thanks to 
> https://shipilev.net/blog/2014/on-the-fence-with-dependencies/)

This pull request has now been integrated.

Changeset: 7feabee4
Author:liach 
Committer: Mandy Chung 
URL:   
https://git.openjdk.java.net/jdk/commit/7feabee4265787ea820c1925c0c531933cb0da50
Stats: 125 lines in 1 file changed: 72 ins; 36 del; 17 mod

8261407: ReflectionFactory.checkInitted() is not thread-safe

Co-authored-by: Peter Levart 
Reviewed-by: dholmes, mchung, plevart

-

PR: https://git.openjdk.java.net/jdk/pull/6889

Re: RFR: 8209784: Include hsdis in the JDK

[Andrew Haley]

On Tue, 22 Feb 2022 16:10:22 GMT, Magnus Ihse Bursie  wrote:

> This PR adds a new configure argument, `--enable-hsdis-bundling`. Setting 
> this, together with a valid backend using `--with-hsdis`, will bundle the 
> generated hsdis library with the JDK.

Maybe I missed it, but are there instructions somewhere about how to build 
this, and what should be downloaded?

-

PR: https://git.openjdk.java.net/jdk/pull/7578

Integrated: 8282042: [testbug] FileEncodingTest.java depends on default encoding

[Tyler Steele]

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: 8281525: Enable Zc:strictStrings flag in Visual Studio build [v2]

[Daniel Jeliński]

On Tue, 22 Feb 2022 16:35:09 GMT, Magnus Ihse Bursie  wrote:

>> Done
>
> Did you forget to push the fix?

Push works better when connected... this time it's pushed for real.

-

PR: https://git.openjdk.java.net/jdk/pull/7565

Re: RFR: 8281525: Enable Zc:strictStrings flag in Visual Studio build [v2]

[Magnus Ihse Bursie]

On Tue, 22 Feb 2022 14:13:43 GMT, Daniel Jeliński  wrote:

>> make/autoconf/flags-cflags.m4 line 136:
>> 
>>> 134:   CFLAGS_WARNINGS_ARE_ERRORS="-WX"
>>> 135: 
>>> 136:   WARNINGS_ENABLE_ALL="-W3 -Zc:strictStrings"
>> 
>> This is not strictly a flag to enable warnings. I'd recommend you move it to 
>> line 499 and 500 in the same file, which would align it with the 
>> -Zc:wchar_t- switch. Make sure you add it to both TOOLCHAIN_CFLAGS_JVM and 
>> TOOLCHAIN_CFLAGS_JDK.
>
> Done

Did you forget to push the fix?

-

PR: https://git.openjdk.java.net/jdk/pull/7565

Re: RFR: 8281525: Enable Zc:strictStrings flag in Visual Studio build [v2]

[Daniel Jeliński]

> 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

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/7565/files
  - new: https://git.openjdk.java.net/jdk/pull/7565/files/fe18b41d..0cfbb165

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=7565=01
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7565=00-01

  Stats: 3 lines in 1 file changed: 0 ins; 0 del; 3 mod
  Patch: https://git.openjdk.java.net/jdk/pull/7565.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/7565/head:pull/7565

PR: https://git.openjdk.java.net/jdk/pull/7565

Re: RFR: 8209784: Include hsdis in the JDK

[Erik Joelsson]

On Tue, 22 Feb 2022 16:10:22 GMT, Magnus Ihse Bursie  wrote:

> This PR adds a new configure argument, `--enable-hsdis-bundling`. Setting 
> this, together with a valid backend using `--with-hsdis`, will bundle the 
> generated hsdis library with the JDK.
> 
> The PR also includes a refactoring of the hsdis handling in autoconf, 
> breaking it out to a separate file `lib-hsdis.gmk`, and breaking the 
> functionality up in separate functions per backend.

Marked as reviewed by erikj (Reviewer).

make/Hsdis.gmk line 186:

> 184: $(install-file)
> 185: 
> 186:   $(INSTALLED_HSDIS_IMAGE): $(BUILT_HSDIS_LIB)

This slipped past me earlier, but the install-hsdis target at the top level 
does not have any prereq. I believe it needs to depend on jdk-image for 
installation to the image to work. Otherwise it will just get deleted if the 
jdk-image target is built after.

-

PR: https://git.openjdk.java.net/jdk/pull/7578

Integrated: 8281315: Unicode, (?i) flag and backreference throwing IndexOutOfBounds Exception

[Ian Graves]

On Wed, 16 Feb 2022 18:45:29 GMT, Ian Graves  wrote:

> This is a fix in the buggy way CIBackRef traverses unicode characters that 
> could be variable-length. Originally it followed the approach that BackRef 
> does, but failed to account for unicode characters that could be 2 
> chars-long. The upper bound (groupSize) for the traversing loop is set by the 
> difference between group start and stop indexes. This works for single char 
> characters and it also works for case-sensitive comparisons because 
> byte-by-byte comparisons are acceptable, but it doesn't work for a comparison 
> where some kind of normalization (i.e. case) is required. This fix adjusts 
> the upper bound for the loop that traverses the character when a two-char 
> character is encountered.
> 
> An alternative was to check the length of the group size by scanning the 
> group in advance and converting to code points, but this could potentially 
> result in multiple scans and codepoint conversions of the same matcher group 
> which could be long. The solution that adjusts the loop bounds on the fly 
> avoids this case.

This pull request has now been integrated.

Changeset: 3cb38678
Author:Ian Graves 
URL:   
https://git.openjdk.java.net/jdk/commit/3cb38678aa7f03356421f5a17c1de4156e206d68
Stats: 25 lines in 2 files changed: 21 ins; 0 del; 4 mod

8281315: Unicode, (?i) flag and backreference throwing IndexOutOfBounds 
Exception

Reviewed-by: naoto

-

PR: https://git.openjdk.java.net/jdk/pull/7501

RFR: 8209784: Include hsdis in the JDK

[Magnus Ihse Bursie]

This PR adds a new configure argument, `--enable-hsdis-bundling`. Setting this, 
together with a valid backend using `--with-hsdis`, will bundle the generated 
hsdis library with the JDK.

The PR also includes a refactoring of the hsdis handling in autoconf, breaking 
it out to a separate file `lib-hsdis.gmk`, and breaking the functionality up in 
separate functions per backend.

-

Commit messages:
 - Add bundling of hsdis with java.base
 - Refactor lib-hsdis.gmk by breaking out each backend in a separate function
 - Break out hsdis setup to separate file

Changes: https://git.openjdk.java.net/jdk/pull/7578/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk=7578=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8209784
  Stats: 682 lines in 8 files changed: 390 ins; 281 del; 11 mod
  Patch: https://git.openjdk.java.net/jdk/pull/7578.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/7578/head:pull/7578

PR: https://git.openjdk.java.net/jdk/pull/7578

Re: RFR: 8261407: ReflectionFactory.checkInitted() is not thread-safe [v9]

[liach]

On Sat, 19 Feb 2022 16:06:35 GMT, liach  wrote:

>> Upon review of [8261407](https://bugs.openjdk.java.net/browse/JDK-8261407), 
>> by design, duplicate initialization of ReflectionFactory should be safe as 
>> it performs side-effect-free property read actions, and the suggesting of 
>> making the `initted` field volatile cannot prevent concurrent initialization 
>> either; however, having `initted == true` published without the other 
>> fields' values is a possibility, which this patch addresses.
>> 
>> This simulates what's done in `CallSite`'s constructor for 
>> `ConstantCallSite`. Please feel free to point out the problems with this 
>> patch, as I am relatively inexperienced in this field of fences and there 
>> are relatively less available documents. (Thanks to 
>> https://shipilev.net/blog/2014/on-the-fence-with-dependencies/)
>
> liach has updated the pull request with a new target base due to a merge or a 
> rebase. The incremental webrev excludes the unrelated changes brought in by 
> the merge/rebase. The pull request contains 13 additional commits since the 
> last revision:
> 
>  - Merge branch 'master' into 8261407-reflectionfactory
>  - Whitespace issues
>  - Refine docs, pull members out of the config record
>  - The fast path should always come first. good lesson learned!
>restore config field comments
>  - Try making the config a record and see if it works
>  - Make config a pojo, move loading code into config class
>  - use peter's model of separate data object
>  - Merge branch 'master' into 8261407-reflectionfactory
>  - Include the stable annotation
>  - Merge branch 'master' into 8261407-reflectionfactory
>  - ... and 3 more: 
> https://git.openjdk.java.net/jdk/compare/f0786899...e97ea278

peter or mandy, would you do me a favor and sponsor this patch?

-

PR: https://git.openjdk.java.net/jdk/pull/6889

Integrated: 8276686: Malformed Javadoc inline tags in JDK source in /java/util/regex/Pattern.java

[Ian Graves]

On Thu, 17 Feb 2022 18:02:20 GMT, Ian Graves  wrote:

> Adding a missing period per this doc bug.

This pull request has now been integrated.

Changeset: 41355e2d
Author:Ian Graves 
URL:   
https://git.openjdk.java.net/jdk/commit/41355e2daa43fa8433bf77ed187979c49d453f4a
Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod

8276686: Malformed Javadoc inline tags in JDK source in 
/java/util/regex/Pattern.java

Reviewed-by: iris, bpb, lancea

-

PR: https://git.openjdk.java.net/jdk/pull/7521

Re: RFR: 8282219: jdk/java/lang/ProcessBuilder/Basic.java fails on AIX

[Roger Riggs]

On Tue, 22 Feb 2022 12:17:59 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.

test/jdk/java/lang/ProcessBuilder/Basic.java line 1891:

> 1889: Process p = Runtime.getRuntime().exec(cmdp, envp);
> 1890: String expected = Windows.is() ? 
> "=C:=\\,=ExitValue=3,SystemRoot="+systemRoot+"," : "=C:=\\,";
> 1891: expected = AIX.is() ? expected + 
> "LIBPATH="+removeTestNativepathString(libpath)+",": expected;

Can line 1878 (and the fix) be moved into the `removeAixExpectedVars` method.

1883:if (AIX.is()) {
1884:commandOutput = removeAixExpectedVars(commandOutput);
1885:   }

The `replaceAll` used in that method should be just as effective as the more 
expensive new method.

-

PR: https://git.openjdk.java.net/jdk/pull/7574

Re: RFR: 8281525: Enable Zc:strictStrings flag in Visual Studio build

[Daniel Jeliński]

On Tue, 22 Feb 2022 11:33:52 GMT, Magnus Ihse Bursie  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.
>
> make/autoconf/flags-cflags.m4 line 136:
> 
>> 134:   CFLAGS_WARNINGS_ARE_ERRORS="-WX"
>> 135: 
>> 136:   WARNINGS_ENABLE_ALL="-W3 -Zc:strictStrings"
> 
> This is not strictly a flag to enable warnings. I'd recommend you move it to 
> line 499 and 500 in the same file, which would align it with the -Zc:wchar_t- 
> switch. Make sure you add it to both TOOLCHAIN_CFLAGS_JVM and 
> TOOLCHAIN_CFLAGS_JDK.

Done

-

PR: https://git.openjdk.java.net/jdk/pull/7565

Re: RFR: 8279598: Provide adapter from RandomGenerator to Random [v6]

[Jim Laskey]

On Sun, 20 Feb 2022 03:15:22 GMT, Yasser Bazzi  wrote:

>> 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 one additional 
> commit since the last revision:
> 
>   remove missed whitespace

At this point I think you should just drop all changes to 
src/java.base/share/classes/java/util/random/RandomGenerator.java, since they 
are only cosmetic. Plus you are missing a newline at the end of the file. 
Otherwise, LGTM.

-

Changes requested by jlaskey (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/7001

Re: RFR: 8281525: Enable Zc:strictStrings flag in Visual Studio build

[David Holmes]

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.

Hi Daniel,

Hotspot changes look fine.

Some of the casts in other code look odd to me, but I'll leave that for the 
security-libs folk to comment on.

Thanks,
David

-

Marked as reviewed by dholmes (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/7565

RFR: 8282219: jdk/java/lang/ProcessBuilder/Basic.java fails on AIX

[Ichiroh Takiguchi]

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.

-

Commit messages:
 - 8282219: jdk/java/lang/ProcessBuilder/Basic.java fails on AIX

Changes: https://git.openjdk.java.net/jdk/pull/7574/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk=7574=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8282219
  Stats: 18 lines in 1 file changed: 14 ins; 0 del; 4 mod
  Patch: https://git.openjdk.java.net/jdk/pull/7574.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/7574/head:pull/7574

PR: https://git.openjdk.java.net/jdk/pull/7574

Re: RFR: 8281525: Enable Zc:strictStrings flag in Visual Studio build

[Magnus Ihse Bursie]

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.

make/autoconf/flags-cflags.m4 line 136:

> 134:   CFLAGS_WARNINGS_ARE_ERRORS="-WX"
> 135: 
> 136:   WARNINGS_ENABLE_ALL="-W3 -Zc:strictStrings"

This is not strictly a flag to enable warnings. I'd recommend you move it to 
line 499 and 500 in the same file, which would align it with the -Zc:wchar_t- 
switch. Make sure you add it to both TOOLCHAIN_CFLAGS_JVM and 
TOOLCHAIN_CFLAGS_JDK.

-

PR: https://git.openjdk.java.net/jdk/pull/7565

Re: RFR: 8281962: Avoid unnecessary native calls in InflaterInputStream [v2]

[Volker Simonis]

On Mon, 21 Feb 2022 18:05:08 GMT, Lance Andersen  wrote:

> > > The change looks innocuous so it is probably OK. I would like to kick of 
> > > our Mach5 runs to see if it shakes out any potential issues.
> > 
> > 
> > @LanceAndersen , did you manage to get any Mach5 results? Did you find any 
> > issues?
> 
> Tests are still running so no final results yet (please note that today is 
> also a US holiday)

No problem. Just let me know once the tests have finished.

And thanks @AlanBateman, @cl4es  for the reviews!

-

PR: https://git.openjdk.java.net/jdk/pull/7492