Re: RFR: 8052260: Reference.isEnqueued() spec does not match the long-standing behavior returning true iff it's in the ref queue [v3]
On Tue, 8 Dec 2020 20:42:52 GMT, Mandy Chung wrote: >> `Reference::isEnqueued` method was never implemented as it was initially >> specified since 1.2. The specification says that it tests if this reference >> object has been enqueued, but the long-standing behavior is to test if this >> reference object is in its associated queue, only reflecting the state at >> the time when this method is executed. The implementation doesn't do what >> the specification promised, which might cause serious bugs if unnoticed. For >> example, an application that relies on this method to release critical >> resources could cause serious performance issues, in particular when this >> method is misused on Reference objects without an associated queue. >> `Reference::refersTo(null)` is the better recommended way to test if a >> Reference object has been cleared. >> >> This proposes to deprecate `Reference::isEnqueued`. Also the spec is >> updated to match the long-standing behavior. >> >> CSR: https://bugs.openjdk.java.net/browse/JDK-8189386 > > Mandy Chung has updated the pull request incrementally with one additional > commit since the last revision: > > improve the deprecation message per feedback Marked as reviewed by alanb (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/1684
Re: RFR: 8234131: Miscellaneous changes imported from jsr166 CVS 2020-12 [v3]
On Tue, 8 Dec 2020 21:15:48 GMT, Martin Buchholz wrote: >> 8234131: Miscellaneous changes imported from jsr166 CVS 2020-12 > > Martin Buchholz has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains one commit: > > JDK-8234131 Marked as reviewed by alanb (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/1647
Integrated: 8257887: java/foreign/TestSegments.java test fails on 32-bit after JDK-8257186
On Tue, 8 Dec 2020 08:54:41 GMT, Aleksey Shipilev wrote: > See here: > https://github.com/mcimadamore/jdk/runs/1460615378 > > $ CONF=linux-x86-server-fastdebug make images run-test > TEST=java/foreign/TestSegments.java > > STDERR: > Invalid maximum heap size: -Xmx4G > The specified size exceeds the maximum representable size. > Error: Could not create the Java Virtual Machine. > Error: A fatal exception has occurred. Program will exit. > > Adding `@requires` in the same form other `java/foreign` tests have it skips > the test on 32-bit platforms. > > Additional testing: > - [x] Affected test on Linux x86_32 (skipped now) > - [x] Affected test on Linux x86_64 (still passes) This pull request has now been integrated. Changeset: 9ce3d806 Author:Aleksey Shipilev URL: https://git.openjdk.java.net/jdk/commit/9ce3d806 Stats: 1 line in 1 file changed: 1 ins; 0 del; 0 mod 8257887: java/foreign/TestSegments.java test fails on 32-bit after JDK-8257186 Reviewed-by: jiefu, adityam, redestad - PR: https://git.openjdk.java.net/jdk/pull/1688
Re: A Bug involving MethodHandles, Nestmates, Reflection and @CallerSensitive
On 09-Dec-20 1:16, Mandy Chung wrote: On 12/8/20 5:07 AM, Johannes Kuhn wrote: Not sure if I read this correctly as "please share some example of code that tries to do that" or "please share code that you write to fix that". So I do both. I was curious to find out what old software attempts to change static final fields via reflection and why they do that. This leads to various unpredictable issues. Looks like you want to get old software to work and have to hack their static final fields!! Just do a quick search for "NoSuchFieldException modifiers". A lot of code does it, my impression is that often someone tried to be clever - just because they can. Setting static final fields does not work [1]. It probably never really did. It usually seems to work - but there is no guarantee that it actually does (like undefined behavior). Field::setXXX will throw IAE on static final fields and non-static fields declared on a record or hidden class too. The example works with Java 8 and 11 - it "tricks" the JIT into constant folding a static final field, so the subsequent change is not reflected. For the nestmate security consideration, the following invariants should already hold: * To call a @CallerSensitive method, the Lookup needs to have full privilege access (Lookup.hasFullPrivilegeAccess()) -> Injected Invokers are only created for full privilege lookups. * The injected invoker is in the same runtime package and has the same protection domain. * It is already possible to obtain a Lookup for the injected invoker by calling MethodHandles.lookup() through a MethodHandle (yikes) [5]. I am concerned with this. I am inclined to say we need to fix JDK-8013527 if we fix this issue. This means, we only have to consider what additional privileges the injected invoker gets if it is also a nestmate of the lookup class. I don't see any problem with that, as you already have a full privilege lookup when the injected invoker is created. - Johannes PS.: JDK-8013527 is mildly related - as the @CallerSensitive methods in java.lang.reflect are "hyper-sensitive". This fix will get JDK-8013527 passed the IAE but the lookup class of the resulting Lookup object is the hidden class which is still incorrect. Mandy A quick history about JDK-8013527 (and JDK-8207834 - which has the same root cause, but I wrote an comment on that): In Java 8, the injected invoker class had a name starting with "java.lang.invoke.". Creating a lookup for a class that starts with that name or binding the caller for such a class is explicitly blocked. In Java 9 the name of the injected invoker has changed - from this point on, the name of the injected invoker did depend on the package of the lookup class / caller. **It is already possible** to obtain a Lookup for the injected invoker by calling MethodHandles.lookup() through a MethodHandle. It doesn't throw an IAE anymore. **Since Java 9.** There are a lot of things to consider when trying to fix JDK-8013527. The first problem is: How do you determine the original class? It doesn't work with just using the nest host - as this might be different from the original class. Using the name might also not work - as the original class could be a hidden class itself. So, the only real solution is to attach that information (a reference to the lookup class) to the injected invoker. Then we have to detect that the target class is an injected invoker - and replace it with the actual lookup class. And this comes with some cost - either the job is done by Reflection.getCallerClass() - which would fix any other problem of that kind - or any hyper-sensitive @CallerSensitive method has to do this on it's own. (Luckily there are only a handful @CallerSensitive methods - and only a few are hyper-sensitive.) Unfortunately, I don't know enough about hotspot to determine if it is possible to change the "owner class" of some code. Also, garbage collection... I don't know enough about those systems. I only look at the stuff from the Java / bytecode side. In the end, I'm not sure if fixing l.lookupClass() == ((Lookup) l.findStatic(MethodHandles.class, "lookup", MethodType.methodType(Lookup.class)).invokeExact()).lookupClass() is worth that effort. - Johannes
Re: RFR: 8257596: Clarify trusted final fields for record classes
On Tue, 8 Dec 2020 22:52:37 GMT, Mandy Chung wrote: > This is a follow-up on JDK-8255342 that removes non-specified JVM checks on > classes with Record attributes. That introduces a regression in > `InstanceKlass::is_record` that returns true on a non-record class which has > `RecordComponents` attribute present. This causes unexpected semantics in > `JVM_IsRecord` and `JVM_GetRecordComponents` and also a regression to trust > final fields for non-record classes. > > I propose to change `InstanceKlass::is_record` to match the JLS semantic of a > record class, i.e. final direct subclass of `java.lang.Record` with the > presence of `RecordComponents` attribute. There is no change to JVM class > file validation. Also I propose clearly define: > - `JVM_IsRecord` returns true if the given class is a record i.e. final > and direct subclass of `java.lang.Record` with `RecordComponents` attribute > - `JVM_GetRecordComponents` returns an `RecordComponents` array if > `RecordComponents` attribute is present; otherwise, returns NULL. This does > not check if it's a record class or not. So it may return non-null on a > non-record class if it has `RecordComponents` attribute. So > `JVM_GetRecordComponents` returns the content of the classfile. Hi Remi, > It's not an issue, the JVM view of what a record is and the JLS view of what > a record is doesn't have to be identical, > only aligned. It's fine for the VM to consider any class that have a record > Attribute is a record. > > We already had this discussion on amber-spec-expert list, > see > https://mail.openjdk.java.net/pipermail/amber-spec-experts/2020-November/002630.html What is the conclusion (sorry it was unclear to me)? Drop TNSFF for records? This issue is to fix the regression introduced by JDK-8255342. I expect someone else will file a new JBS issue and implement what amber-spec-experts decided. > We already know that the JLS definition of a record will have to be changed > for inline record (an inline record is not direct subclass of j.l.Record > because you have the reference projection in the middle). Yes I saw that. I updated [JDK-8251041](https://bugs.openjdk.java.net/browse/JDK-8251041) to follow up. > The real issue is that the JIT optimisation and Field.set() should be > aligned, VarHandle deosn't let you change a final field and Unsafe is unsafe, > so the current problem is that Field.set() relies on the reflection api by > calling Class.isRecord() which is not good because Classs.isRecord() returns > the JLS view of the world not the JVM view of the world. > > As said in the mail chain above, for the JIT optimization, instead of listing > all the new constructs, record, inline, etc, i propose to check the other > way, only allow to set a final field is only allowed for plain old java > class, so the VM should not have a method InstanceKlass::is_record but a > method that return if a class is a plain class or not and this method should > be called by the JIT and by Field.set (through a JVM entry point) > so the fact the optimization will be done or not is not related to what the > JLS think a record is, those are separated concern. I agree the current situation is not ideal which requires to declare all the new constructs explicitly for TNSFF. However, it's a reasonable tradeoff to get the JIT optimization for records in time while waiting for true TNSFF investigation like JMM and other relevant specs. I see this just a stop-gap solution. When the long-term TNSFF is in place, the spec can be updated to drop the explicit list of record, inline, etc. A related issue is [JDK-8233873](https://bugs.openjdk.java.net/browse/JDK-8233873). I'm happy if we can do TNSFF in a different solution. Again this PR intends to fix the regression. Two options: 1. Keep [JDK-8247444](https://bugs.openjdk.java.net/browse/JDK-8247444) and implement as this proposed patch 2. Backout [JDK-8247444](https://bugs.openjdk.java.net/browse/JDK-8247444) I expect Chris and/or others will follow up the decision made by the amber-spec-experts w.r.t. trusting finals in records. I'm okay with either option. - PR: https://git.openjdk.java.net/jdk/pull/1706
Re: RFR: 8247373: ArraysSupport.newLength doc, test, and exception message [v2]
On Tue, 8 Dec 2020 06:14:35 GMT, Stuart Marks wrote: >> This rewrites the doc of ArraysSupport.newLength, adds detail to the >> exception message, and adds a test. In addition to some renaming and a bit >> of refactoring of the actual code, I also made two changes of substance to >> the code: >> >> 1. I fixed a problem with overflow checking. In the original code, if >> oldLength and prefGrowth were both very large (say, Integer.MAX_VALUE), this >> method could return a negative value. It turns out that writing tests helps >> find bugs! >> >> 2. Under the old policy, if oldLength and minGrowth required a length above >> SOFT_MAX_ARRAY_LENGTH but not above Integer.MAX_VALUE, this method would >> return Integer.MAX_VALUE. That doesn't make any sense, because attempting to >> allocate an array of that length will almost certainly cause the Hotspot to >> throw OOME because its implementation limit was exceeded. Instead, if the >> required length is in this range, this method returns that required length. >> >> Separately, I'll work on retrofitting various call sites around the JDK to >> use this method. > > Stuart Marks has updated the pull request incrementally with one additional > commit since the last revision: > > fix typo, clarify asserts disabled, test prefGrowth==0 Marked as reviewed by psandoz (Reviewer). src/java.base/share/classes/jdk/internal/util/ArraysSupport.java line 616: > 614: * greater than the soft maximum but does not exceed > Integer.MAX_VALUE, the minimum > 615: * required length is returned. Otherwise, the minimum required > length exceeds > 616: * Integer.MAX_VALUE, which can never be fulfilled, so this method > throws OutOfMemoryError. I think you can simplify with: Suggestion: * If the preferred length exceeds the soft maximum, we use the minimum growth * amount. The minimum required length is determined by adding the minimum growth * amount to the current length. * If the minimum required length exceeds Integer.MAX_VALUE, then this method * throws OutOfMemoryError. Otherwise, this method returns the soft maximum or * minimum required length, which ever is greater. Then i think it follows that `Math.max` can be used in the implementation. - PR: https://git.openjdk.java.net/jdk/pull/1617
Re: A Bug involving MethodHandles, Nestmates, Reflection and @CallerSensitive
On 12/8/20 5:07 AM, Johannes Kuhn wrote: On 08-Dec-20 5:40, Mandy Chung wrote: Thanks David. I was about to create one. This is indeed a gap in injecting this trampoline class for supporting @CallerSensitive methods. The InjectedInvoker ensures that the InjectedInvoker class has the same class loader as the lookup class. W.r.t. your patch, it seems okay but I have to consider and think through the security implication carefully. You mentioned "Some old software loves to set static final fields through reflection" - can you share some example libraries about this? This is really ugly hack!! Mandy Not sure if I read this correctly as "please share some example of code that tries to do that" or "please share code that you write to fix that". So I do both. I was curious to find out what old software attempts to change static final fields via reflection and why they do that. This leads to various unpredictable issues. Looks like you want to get old software to work and have to hack their static final fields!! Setting static final fields does not work [1]. It probably never really did. It usually seems to work - but there is no guarantee that it actually does (like undefined behavior). Field::setXXX will throw IAE on static final fields and non-static fields declared on a record or hidden class too. JPEXS [2] for example used that for it's configuration. Also some old Minecraft Forge version (a Minecraft mod loader / mod API) depends on this. Example use [3], but they do really ugly things. So, I said I develop agents to get old stuff running on a current Java version. Why? Fun, I guess. I also learn a lot a about what are the main comparability problems with newer Java versions. Pros of writing an agent: * I don't need the source code. * I don't need to setup a build environment with all dependencies, lombok and who knows what else is required. In all, it's faster for me. And I then have a list of problems - and how they can be solved. I did publish my JPESX agent here [4]. But yeah, it's an ugly hack. For the nestmate security consideration, the following invariants should already hold: * To call a @CallerSensitive method, the Lookup needs to have full privilege access (Lookup.hasFullPrivilegeAccess()) -> Injected Invokers are only created for full privilege lookups. * The injected invoker is in the same runtime package and has the same protection domain. * It is already possible to obtain a Lookup for the injected invoker by calling MethodHandles.lookup() through a MethodHandle (yikes) [5]. I am concerned with this. I am inclined to say we need to fix JDK-8013527 if we fix this issue. This means, we only have to consider what additional privileges the injected invoker gets if it is also a nestmate of the lookup class. I don't see any problem with that, as you already have a full privilege lookup when the injected invoker is created. - Johannes PS.: JDK-8013527 is mildly related - as the @CallerSensitive methods in java.lang.reflect are "hyper-sensitive". This fix will get JDK-8013527 passed the IAE but the lookup class of the resulting Lookup object is the hidden class which is still incorrect. Mandy [1]: https://urldefense.com/v3/__https://gist.github.com/DasBrain/25c6738610c517ee375aacc86ffebd0c__;!!GqivPVa7Brio!IRaER_1fNHPfhaN4erhar7ZMzL5yVGyoWGcH2FS_SMH23wknQghpz2amisGRByQ4Yg$ [2]: https://urldefense.com/v3/__https://github.com/akx/jpexs-decompiler/blob/6c998254b9d5bdce80be1b92d34836820ee31a1d/libsrc/ffdec_lib/src/com/jpexs/decompiler/flash/configuration/Configuration.java*L869__;Iw!!GqivPVa7Brio!IRaER_1fNHPfhaN4erhar7ZMzL5yVGyoWGcH2FS_SMH23wknQghpz2amisHSLG47fg$ [3]: https://urldefense.com/v3/__https://github.com/Chisel-Team/Chisel/blob/1.12/dev/src/main/java/team/chisel/common/init/ChiselBlocks.java*L18__;Iw!!GqivPVa7Brio!IRaER_1fNHPfhaN4erhar7ZMzL5yVGyoWGcH2FS_SMH23wknQghpz2amisHY5Ta7RQ$ [4]: https://urldefense.com/v3/__https://github.com/DasBrain/jpex/tree/master/src/pw/dasbrain/jpexs/agent__;!!GqivPVa7Brio!IRaER_1fNHPfhaN4erhar7ZMzL5yVGyoWGcH2FS_SMH23wknQghpz2amisEYiprE4w$ [5]: https://urldefense.com/v3/__https://gist.github.com/DasBrain/142cb8ef9cc16e7469ac519790119e07__;!!GqivPVa7Brio!IRaER_1fNHPfhaN4erhar7ZMzL5yVGyoWGcH2FS_SMH23wknQghpz2amisEC0fauqw$
Re: RFR: 8257887: java/foreign/TestSegments.java test fails on 32-bit after JDK-8257186
On Tue, 8 Dec 2020 08:54:41 GMT, Aleksey Shipilev wrote: > See here: > https://github.com/mcimadamore/jdk/runs/1460615378 > > $ CONF=linux-x86-server-fastdebug make images run-test > TEST=java/foreign/TestSegments.java > > STDERR: > Invalid maximum heap size: -Xmx4G > The specified size exceeds the maximum representable size. > Error: Could not create the Java Virtual Machine. > Error: A fatal exception has occurred. Program will exit. > > Adding `@requires` in the same form other `java/foreign` tests have it skips > the test on 32-bit platforms. > > Additional testing: > - [x] Affected test on Linux x86_32 (skipped now) > - [x] Affected test on Linux x86_64 (still passes) Marked as reviewed by redestad (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/1688
Re: RFR: 8257596: Clarify trusted final fields for record classes
- Mail original - > De: "Mandy Chung" > À: "core-libs-dev" , "hotspot-dev" > > Envoyé: Mardi 8 Décembre 2020 23:57:39 > Objet: RFR: 8257596: Clarify trusted final fields for record classes Hi Mandy, > This is a follow-up on JDK-8255342 that removes non-specified JVM checks on > classes with Record attributes. That introduces a regression in > `InstanceKlass::is_record` that returns true on a non-record class which has > `RecordComponents` attribute present. This causes unexpected semantics in > `JVM_IsRecord` and `JVM_GetRecordComponents` and also a regression to trust > final fields for non-record classes. It's not an issue, the JVM view of what a record is and the JLS view of what a record is doesn't have to be identical, only aligned. It's fine for the VM to consider any class that have a record Attribute is a record. We already had this discussion on amber-spec-expert list, see https://mail.openjdk.java.net/pipermail/amber-spec-experts/2020-November/002630.html We already know that the JLS definition of a record will have to be changed for inline record (an inline record is not direct subclass of j.l.Record because you have the reference projection in the middle). The real issue is that the JIT optimisation and Field.set() should be aligned, VarHandle deosn't let you change a final field and Unsafe is unsafe, so the current problem is that Field.set() relies on the reflection api by calling Class.isRecord() which is not good because Classs.isRecord() returns the JLS view of the world not the JVM view of the world. As said in the mail chain above, for the JIT optimization, instead of listing all the new constructs, record, inline, etc, i propose to check the other way, only allow to set a final field is only allowed for plain old java class, so the VM should not have a method InstanceKlass::is_record but a method that return if a class is a plain class or not and this method should be called by the JIT and by Field.set (through a JVM entry point) so the fact the optimization will be done or not is not related to what the JLS think a record is, those are separated concern. The more we inject the Java (the language) semantics in the JVM the less it is useful for other languages that run one the JVM. Rémi > > I propose to change `InstanceKlass::is_record` to match the JLS semantic of a > record class, i.e. final direct subclass of `java.lang.Record` with the > presence of `RecordComponents` attribute. There is no change to JVM class > file > validation. Also I propose clearly define: >- `JVM_IsRecord` returns true if the given class is a record i.e. final and >direct subclass of `java.lang.Record` with `RecordComponents` attribute >- `JVM_GetRecordComponents` returns an `RecordComponents` array if >`RecordComponents` attribute is present; otherwise, returns NULL. This > does >not check if it's a record class or not. So it may return non-null on a >non-record class if it has `RecordComponents` attribute. So >`JVM_GetRecordComponents` returns the content of the classfile. Rémi > > - > > Commit messages: > - 8257596: Clarify trusted final fields for record classes > > Changes: https://git.openjdk.java.net/jdk/pull/1706/files > Webrev: https://webrevs.openjdk.java.net/?repo=jdk=1706=00 > Issue: https://bugs.openjdk.java.net/browse/JDK-8257596 > Stats: 60 lines in 4 files changed: 30 ins; 10 del; 20 mod > Patch: https://git.openjdk.java.net/jdk/pull/1706.diff > Fetch: git fetch https://git.openjdk.java.net/jdk pull/1706/head:pull/1706 > > PR: https://git.openjdk.java.net/jdk/pull/1706
Re: RFR: 8257450: Start of release updates for JDK 17 [v6]
> Start of JDK 17 updates. 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 14 additional commits since the last revision: - Get in JEP 390 changes. - Merge branch 'master' into JDK-8257450 - Merge branch 'master' into JDK-8257450 - Update symbol files for JDK 16b27. - Merge branch 'master' into JDK-8257450 - Merge branch 'master' into JDK-8257450 - Merge branch 'master' into JDK-8257450 - Update tests. - Merge branch 'master' into JDK-8257450 - Merge branch 'JDK-8257450' of https://github.com/jddarcy/jdk into JDK-8257450 - ... and 4 more: https://git.openjdk.java.net/jdk/compare/823053e1...57ba7b64 - Changes: - all: https://git.openjdk.java.net/jdk/pull/1531/files - new: https://git.openjdk.java.net/jdk/pull/1531/files/ff9b78ec..57ba7b64 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=1531=05 - incr: https://webrevs.openjdk.java.net/?repo=jdk=1531=04-05 Stats: 1178 lines in 148 files changed: 571 ins; 195 del; 412 mod Patch: https://git.openjdk.java.net/jdk/pull/1531.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1531/head:pull/1531 PR: https://git.openjdk.java.net/jdk/pull/1531
RFR: 8238781: [macos] jpackage tests failed due to "hdiutil: convert failed" in various ways
This is similar issue we used to have for hdiutil create/detach, but for "convert". Added same workaround to repeat command. I also added repeat for "udifrez" command just in case. - Commit messages: - 8238781: [macos] jpackage tests failed due to "hdiutil: convert failed" in various ways Changes: https://git.openjdk.java.net/jdk/pull/1687/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=1687=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8238781 Stats: 8 lines in 1 file changed: 6 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/1687.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1687/head:pull/1687 PR: https://git.openjdk.java.net/jdk/pull/1687
Integrated: 8257845: Integrate JEP 390
On Sat, 5 Dec 2020 01:46:31 GMT, Dan Smith wrote: > Integration of [JEP 390](https://bugs.openjdk.java.net/browse/JDK-8249100). > > Development has been broken into 5 tasks, each with its own JBS issue: > - [JDK-8252180](https://bugs.openjdk.java.net/browse/JDK-8252180): Deprecate > wrapper class constructors for removal (rriggs) > - [JDK-8254047](https://bugs.openjdk.java.net/browse/JDK-8254047): Revise > "value-based class" & apply to wrappers (dlsmith) > - [JDK-8252181](https://bugs.openjdk.java.net/browse/JDK-8252181):Define & > apply annotation jdk.internal.ValueBased (rriggs) > - [JDK-8252183](https://bugs.openjdk.java.net/browse/JDK-8252183): Add 'lint' > warning for @ValueBased classes (sadayapalam) > - [JDK-8257027](https://bugs.openjdk.java.net/browse/JDK-8257027): Diagnose > synchronization on @ValueBased classes (lfoltan) > > All changes have been previously reviewed and integrated into the [`jep390` > branch](https://github.com/openjdk/valhalla/tree/jep390) of the `valhalla` > repository. See the subtasks of the 5 JBS issues for these changes, including > discussion and links to reviews. (Reviewers: mchung, dlsmith, jlaskey, > rriggs, lfoltan, fparain, hseigel.) > > CSRs have also been completed or are nearly complete: > > - [JDK-8254324](https://bugs.openjdk.java.net/browse/JDK-8254324) for wrapper > class constructor deprecation > - [JDK-8254944](https://bugs.openjdk.java.net/browse/JDK-8254944) for > revisions to "value-based class" > - [JDK-8256023](https://bugs.openjdk.java.net/browse/JDK-8256023) for new > `javac` lint warnings > > Here's an overview of the files changed: > > - `src/hotspot`: implementing diagnostics when `monitorenter` is applied to > an instance of a class tagged with `jdk.internal.ValueBased`. This enhances > previous work that produced such diagnostics for the primitive wrapper > classes. > > - `src/java.base/share/classes/java/lang`: deprecating for removal the > wrapper class constructors; revising the definition of "value-based class" in > `ValueBased.html` and the description used by linking classes; applying > "value-based class" to the primitive wrapper classes; marking value-based > classes with `@jdk.internal.ValueBased`. > > - `src/java.base/share/classes/java/lang/constant`: no longer designating > these classes as "value-based", since they rely heavily on field inheritance. > > - `src/java.base/share/classes/java/time`: revising the description used to > link to `ValueBased.html`; marking value-based classes with > `@jdk.internal.ValueBased`. > > - `src/java.base/share/classes/java/util`: revising the description used to > link to `ValueBased.html`; marking value-based classes with > `@jdk.internal.ValueBased`. > > - `src/java.base/share/classes/jdk/internal`: define the > `@jdk.internal.ValueBased` annotation. > > - `src/java.management.rmi`: removing uses of wrapper class constructors. > > - `src/java.xml`: removing uses of wrapper class constructors. > > - `src/jdk.compiler`: implementing the `synchronization` lint category, which > reports attempts to synchronize on classes and interfaces annotated with > `@jdk.internal.ValueBased`. > > - `src/jdk.incubator.foreign`: revising the description used to link to > `ValueBased.html`. (Applying `@jdk.internal.ValueBased` would require a > special module export, which was not deemed necessary for an incubating API.) > > - `src/jdk.internal.vm.compiler`: suppressing `javac` deprecation and > synchronization warnings in tests > > - `src/jdk.jfr`: supplementary changes for HotSpot diagnostics > > - `test`: testing new `javac` and HotSpot behavior; removing usages of > deprecated wrapper class constructors from tests, or suppressing deprecation > warnings; revising the description used to link to `ValueBased.html`. This pull request has now been integrated. Changeset: 48d8650a Author:Dan Smith URL: https://git.openjdk.java.net/jdk/commit/48d8650a Stats: 902 lines in 114 files changed: 489 ins; 121 del; 292 mod 8257845: Integrate JEP 390 8254047: [JEP 390] Revise "value-based class" & apply to wrappers 8252181: [JEP 390] Define & apply annotation jdk.internal.ValueBased 8252183: [JEP 390] Add 'lint' warning for @ValueBased classes 8257027: [JEP 390] Diagnose synchronization on @ValueBased classes 8252180: [JEP 390] Deprecate wrapper class constructors for removal Co-authored-by: Roger Riggs Co-authored-by: Srikanth Adayapalam Co-authored-by: Lois Foltan Reviewed-by: rriggs, hseigel, mchung, darcy - PR: https://git.openjdk.java.net/jdk/pull/1636
RFR: 8257596: Clarify trusted final fields for record classes
This is a follow-up on JDK-8255342 that removes non-specified JVM checks on classes with Record attributes. That introduces a regression in `InstanceKlass::is_record` that returns true on a non-record class which has `RecordComponents` attribute present. This causes unexpected semantics in `JVM_IsRecord` and `JVM_GetRecordComponents` and also a regression to trust final fields for non-record classes. I propose to change `InstanceKlass::is_record` to match the JLS semantic of a record class, i.e. final direct subclass of `java.lang.Record` with the presence of `RecordComponents` attribute. There is no change to JVM class file validation. Also I propose clearly define: - `JVM_IsRecord` returns true if the given class is a record i.e. final and direct subclass of `java.lang.Record` with `RecordComponents` attribute - `JVM_GetRecordComponents` returns an `RecordComponents` array if `RecordComponents` attribute is present; otherwise, returns NULL. This does not check if it's a record class or not. So it may return non-null on a non-record class if it has `RecordComponents` attribute. So `JVM_GetRecordComponents` returns the content of the classfile. - Commit messages: - 8257596: Clarify trusted final fields for record classes Changes: https://git.openjdk.java.net/jdk/pull/1706/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=1706=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8257596 Stats: 60 lines in 4 files changed: 30 ins; 10 del; 20 mod Patch: https://git.openjdk.java.net/jdk/pull/1706.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1706/head:pull/1706 PR: https://git.openjdk.java.net/jdk/pull/1706
Re: RFR: 8257450: Start of release updates for JDK 17 [v5]
On Tue, 8 Dec 2020 17:10:30 GMT, Joe Darcy wrote: >> Start of JDK 17 updates. > > 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 12 additional commits since > the last revision: > > - Merge branch 'master' into JDK-8257450 > - Update symbol files for JDK 16b27. > - Merge branch 'master' into JDK-8257450 > - Merge branch 'master' into JDK-8257450 > - Merge branch 'master' into JDK-8257450 > - Update tests. > - Merge branch 'master' into JDK-8257450 > - Merge branch 'JDK-8257450' of https://github.com/jddarcy/jdk into > JDK-8257450 > - Merge branch 'JDK-8257450' of https://github.com/jddarcy/jdk into > JDK-8257450 > - JDK-8257450 > - ... and 2 more: > https://git.openjdk.java.net/jdk/compare/d1c29ce3...ff9b78ec Marked as reviewed by dholmes (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/1531
Re: RFR: JDK-8257086: Clarify differences between {Float, Double}.equals and ==
On Tue, 8 Dec 2020 17:59:41 GMT, Stuart Marks wrote:
>> Updates to the specifications of Double.{equals, compareTo} to explain more
>> explicitly why the obvious wrappers around "==" and "<"/"==" are not
>> sufficient for floating-point values.
>>
>> Once the wording is worked out, I'll replicate it for the analogous methods
>> on Float.
>
> I'll note initially that the original bug is about `equals` and `==` whereas
> this change also covers `compareTo` and additional comparison operators `<`
> and `>`. I believe covering this additional material **IS** important, as
> these concepts are all closely related.
>
> While this material is covering the right ground, I'd say that it's too
> verbose for a method specification. It feels like it's being compressed to
> fit into a method specification and thus doesn't do the topic justice.
>
> (One additional concept that ought to be covered is that `compareTo` is
> *consistent with equals*. This should be asserted in the method
> specification, but trying to explain it in a method specification seems
> difficult.)
>
> I'll suggest a couple alternatives. One is to put a full, combined
> explanation into class doc somewhere, say in `Double`. The various methods
> can then make some terse assertions and then refer to the combined
> explanation. `Float` could refer to `Double` instead of replicating the text.
>
> Another alternative is to put this explanation into the `java.lang` package
> specification. This might be a good fit, since there is already some
> explanation there of the boxed primitives.
>
>
> I'll note initially that the original bug is about `equals` and `==` whereas
> this change also covers `compareTo` and additional comparison operators `<`
> and `>`. I believe covering this additional material **IS** important, as
> these concepts are all closely related.
>
> While this material is covering the right ground, I'd say that it's too
> verbose for a method specification. It feels like it's being compressed to
> fit into a method specification and thus doesn't do the topic justice.
>
> (One additional concept that ought to be covered is that `compareTo` is
> _consistent with equals_. This should be asserted in the method
> specification, but trying to explain it in a method specification seems
> difficult.)
>
> I'll suggest a couple alternatives. One is to put a full, combined
> explanation into class doc somewhere, say in `Double`. The various methods
> can then make some terse assertions and then refer to the combined
> explanation. `Float` could refer to `Double` instead of replicating the text.
>
> Another alternative is to put this explanation into the `java.lang` package
> specification. This might be a good fit, since there is already some
> explanation there of the boxed primitives.
I added discussion of compareTo as well as equals to the scope of the bug since
they are sibling surprising deviations from what is expected and have the same
root cause. (I took care to say "incomplete order" rather than "partial order"
since a partial order requires reflexivity.)
The interface java.lang.Comparable gives a definition of "consistent with
equals." I didn't verify it doesn't have an anchor to link to, but we don't
typically link to the definition. However, I wouldn't be adverse to having
"consistent with equals" link to Comparable; that would be more obvious than
expecting the reader to follow the "Specified by: compareTo in interface
Comparable" link javadoc generates for the method.
I think the Float and Double equals and compareTo methods should at least get
pointers to any new section added -- I think a standalone section in the
package javadoc by itself would have low discoverability.
I'm make another iteration over the text; thanks.
-
PR: https://git.openjdk.java.net/jdk/pull/1699
Integrated: 8256299: Implement JEP 396: Strongly Encapsulate JDK Internals by Default
On Thu, 19 Nov 2020 16:49:30 GMT, Mark Reinhold wrote:
> Please review this implementation of JEP 396
> (https://openjdk.java.net/jeps/396).
> Alan Bateman is the original author; I’ve credited him in the commit metadata.
> Passes tiers 1-3 on {linux,macos,windows}-x64 and linux-aarch64.
This pull request has now been integrated.
Changeset: ed4c4ee7
Author:Mark Reinhold
URL: https://git.openjdk.java.net/jdk/commit/ed4c4ee7
Stats: 162 lines in 5 files changed: 24 ins; 73 del; 65 mod
8256299: Implement JEP 396: Strongly Encapsulate JDK Internals by Default
Co-authored-by: Alan Bateman
Reviewed-by: mchung, alanb
-
PR: https://git.openjdk.java.net/jdk/pull/1324
Re: RFR: 8257845: Integrate JEP 390
On Sat, 5 Dec 2020 01:46:31 GMT, Dan Smith wrote: > Integration of [JEP 390](https://bugs.openjdk.java.net/browse/JDK-8249100). > > Development has been broken into 5 tasks, each with its own JBS issue: > - [JDK-8252180](https://bugs.openjdk.java.net/browse/JDK-8252180): Deprecate > wrapper class constructors for removal (rriggs) > - [JDK-8254047](https://bugs.openjdk.java.net/browse/JDK-8254047): Revise > "value-based class" & apply to wrappers (dlsmith) > - [JDK-8252181](https://bugs.openjdk.java.net/browse/JDK-8252181):Define & > apply annotation jdk.internal.ValueBased (rriggs) > - [JDK-8252183](https://bugs.openjdk.java.net/browse/JDK-8252183): Add 'lint' > warning for @ValueBased classes (sadayapalam) > - [JDK-8257027](https://bugs.openjdk.java.net/browse/JDK-8257027): Diagnose > synchronization on @ValueBased classes (lfoltan) > > All changes have been previously reviewed and integrated into the [`jep390` > branch](https://github.com/openjdk/valhalla/tree/jep390) of the `valhalla` > repository. See the subtasks of the 5 JBS issues for these changes, including > discussion and links to reviews. (Reviewers: mchung, dlsmith, jlaskey, > rriggs, lfoltan, fparain, hseigel.) > > CSRs have also been completed or are nearly complete: > > - [JDK-8254324](https://bugs.openjdk.java.net/browse/JDK-8254324) for wrapper > class constructor deprecation > - [JDK-8254944](https://bugs.openjdk.java.net/browse/JDK-8254944) for > revisions to "value-based class" > - [JDK-8256023](https://bugs.openjdk.java.net/browse/JDK-8256023) for new > `javac` lint warnings > > Here's an overview of the files changed: > > - `src/hotspot`: implementing diagnostics when `monitorenter` is applied to > an instance of a class tagged with `jdk.internal.ValueBased`. This enhances > previous work that produced such diagnostics for the primitive wrapper > classes. > > - `src/java.base/share/classes/java/lang`: deprecating for removal the > wrapper class constructors; revising the definition of "value-based class" in > `ValueBased.html` and the description used by linking classes; applying > "value-based class" to the primitive wrapper classes; marking value-based > classes with `@jdk.internal.ValueBased`. > > - `src/java.base/share/classes/java/lang/constant`: no longer designating > these classes as "value-based", since they rely heavily on field inheritance. > > - `src/java.base/share/classes/java/time`: revising the description used to > link to `ValueBased.html`; marking value-based classes with > `@jdk.internal.ValueBased`. > > - `src/java.base/share/classes/java/util`: revising the description used to > link to `ValueBased.html`; marking value-based classes with > `@jdk.internal.ValueBased`. > > - `src/java.base/share/classes/jdk/internal`: define the > `@jdk.internal.ValueBased` annotation. > > - `src/java.management.rmi`: removing uses of wrapper class constructors. > > - `src/java.xml`: removing uses of wrapper class constructors. > > - `src/jdk.compiler`: implementing the `synchronization` lint category, which > reports attempts to synchronize on classes and interfaces annotated with > `@jdk.internal.ValueBased`. > > - `src/jdk.incubator.foreign`: revising the description used to link to > `ValueBased.html`. (Applying `@jdk.internal.ValueBased` would require a > special module export, which was not deemed necessary for an incubating API.) > > - `src/jdk.internal.vm.compiler`: suppressing `javac` deprecation and > synchronization warnings in tests > > - `src/jdk.jfr`: supplementary changes for HotSpot diagnostics > > - `test`: testing new `javac` and HotSpot behavior; removing usages of > deprecated wrapper class constructors from tests, or suppressing deprecation > warnings; revising the description used to link to `ValueBased.html`. Marked as reviewed by darcy (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/1636
Re: RFR: 8234131: Miscellaneous changes imported from jsr166 CVS 2020-12 [v3]
> 8234131: Miscellaneous changes imported from jsr166 CVS 2020-12 Martin Buchholz has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains one commit: JDK-8234131 - Changes: https://git.openjdk.java.net/jdk/pull/1647/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=1647=02 Stats: 312 lines in 41 files changed: 105 ins; 41 del; 166 mod Patch: https://git.openjdk.java.net/jdk/pull/1647.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1647/head:pull/1647 PR: https://git.openjdk.java.net/jdk/pull/1647
Re: RFR: JDK-8256950: Add record attribute support to symbol generator CreateSymbols [v5]
On Thu, 3 Dec 2020 09:22:18 GMT, Jan Lahoda wrote: >> Adding support for record classes in the historical data for ct.sym. This >> includes a few changes not strictly needed for the change: >> -updating and moving tests into test/langtools, so that it is easier to run >> them. >> -fixing Record attribute reading in javac's ClassReader (used for tests, but >> seems like the proper thing to do anyway). >> -fixing the -Xprint annotation processor to print record component >> annotations. >> >> Changes to jdk.jdeps' classfile library are needed so that the ct.sym >> creation works. > > Jan Lahoda has updated the pull request incrementally with one additional > commit since the last revision: > > Blank lines do not count for the text block indentation, removing them. Marked as reviewed by jjg (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/1480
Re: RFR: 8254350: CompletableFuture.get may swallow InterruptedException [v2]
On Tue, Dec 8, 2020 at 3:54 AM Daniel Fuchs wrote: > > > RandomFactory is probably overkill here as the test just needs to > exercise untimed and timed get. So just a random boolean rather than a wide > range of random values. > > OK. > There's a conflict between the desires to do more thorough testing and avoid flaky intermittent failures. Especially in j.u.concurrent we embrace test non-determinism, much of it comes from the concurrency we're testing, and retrieving a random seed for reproducibility is not worth the effort. No one does a good job of race prevention through testing.
Re: RFR: 8254350: CompletableFuture.get may swallow InterruptedException [v2]
On Mon, Dec 7, 2020 at 11:56 PM Alan Bateman wrote: > > > 37: // TODO: Rewrite as a CompletableFuture tck test ? > > Do you want the "TODO" in the commit? > > Yes!
Integrated: 8256515: javax.xml.XMLEventReader produces incorrect START_DOCUMENT event
On Wed, 4 Nov 2020 14:01:53 GMT, Marius Volkhart wrote: > The default implementation of javax.xml.stream.XMLEventReader produced a > StartDocument event that always indicated that the "standalone" attribute was > set. > > The root cause of this was that the > com.sun.xml.internal.stream.events.XMLEventAllocatorImpl always set the > "standalone" attribute, rather than asking streamReader.standaloneSet() > before setting the property of the StartDocumentEvent being created. This pull request has now been integrated. Changeset: c47ab5f6 Author:Marius Volkhart Committer: Joe Wang URL: https://git.openjdk.java.net/jdk/commit/c47ab5f6 Stats: 33 lines in 3 files changed: 25 ins; 0 del; 8 mod 8256515: javax.xml.XMLEventReader produces incorrect START_DOCUMENT event Reviewed-by: joehw - PR: https://git.openjdk.java.net/jdk/pull/1056
Re: RFR: 8052260: Reference.isEnqueued() spec does not match the long-standing behavior returning true iff it's in the ref queue [v3]
> `Reference::isEnqueued` method was never implemented as it was initially > specified since 1.2. The specification says that it tests if this reference > object has been enqueued, but the long-standing behavior is to test if this > reference object is in its associated queue, only reflecting the state at the > time when this method is executed. The implementation doesn't do what the > specification promised, which might cause serious bugs if unnoticed. For > example, an application that relies on this method to release critical > resources could cause serious performance issues, in particular when this > method is misused on Reference objects without an associated queue. > `Reference::refersTo(null)` is the better recommended way to test if a > Reference object has been cleared. > > This proposes to deprecate `Reference::isEnqueued`. Also the spec is updated > to match the long-standing behavior. > > CSR: https://bugs.openjdk.java.net/browse/JDK-8189386 Mandy Chung has updated the pull request incrementally with one additional commit since the last revision: improve the deprecation message per feedback - Changes: - all: https://git.openjdk.java.net/jdk/pull/1684/files - new: https://git.openjdk.java.net/jdk/pull/1684/files/b4f9b489..9acf8a56 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=1684=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk=1684=01-02 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/1684.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1684/head:pull/1684 PR: https://git.openjdk.java.net/jdk/pull/1684
Re: RFR: 8234131: Miscellaneous changes imported from jsr166 CVS 2020-12 [v2]
OK, rollback committed to CVS:
--- src/main/java/util/concurrent/ThreadPoolExecutor.java 27 Nov 2020
17:42:00 - 1.194
+++ src/main/java/util/concurrent/ThreadPoolExecutor.java 8 Dec 2020
20:31:54 -
@@ -1522,13 +1522,11 @@
// As a heuristic, prestart enough new workers (up to new
// core size) to handle the current number of tasks in
// queue, but stop if queue becomes empty while doing so.
-/*
int k = Math.min(delta, workQueue.size());
while (k-- > 0 && addWorker(null, true)) {
if (workQueue.isEmpty())
break;
}
-*/
}
}
On Tue, Dec 8, 2020 at 4:04 AM Doug Lea wrote:
>
> On 12/8/20 3:56 AM, Alan Bateman wrote:
> >> 1558: break;
> >> 1559: }
> >> 1560: */
> > Is this meant to be commented out?
> Yes, but It should be marked as a possible improvement, not yet
> committed. While this (prestarting) would improve performance in some
> scenarios, it may also disrupt expectations and even tooling in some
> existing usages, which we haven't fully checked out.
>
>
Re: RFR: 8256515: javax.xml.XMLEventReader produces incorrect START_DOCUMENT event [v4]
On Tue, 8 Dec 2020 19:10:37 GMT, Joe Wang wrote: >> Marius Volkhart has updated the pull request incrementally with two >> additional commits since the last revision: >> >> - fixup! Fix: javax.xml.stream.XMLEventReader produces incorrect >> START_DOCUMENT event >> - fixup! Fix: javax.xml.stream.XMLEventReader produces incorrect >> START_DOCUMENT event > > Marked as reviewed by joehw (Reviewer). Hi Marius, As the bot suggested, this PR is ready to be integrated. You may issue the integrate command when you're ready, I'll then sponsor it for you. Thanks for contributing the fix! Regards, Joe - PR: https://git.openjdk.java.net/jdk/pull/1056
Re: RFR: 8052260: Reference.isEnqueued() spec does not match the long-standing behavior returning true iff it's in the ref queue [v2]
On Tue, 8 Dec 2020 19:35:39 GMT, Alan Bateman wrote: >> Mandy Chung has updated the pull request with a new target base due to a >> merge or a rebase. The incremental webrev excludes the unrelated changes >> brought in by the merge/rebase. The pull request contains four additional >> commits since the last revision: >> >> - Merge branch 'master' of https://github.com/openjdk/jdk into isEnqueued >> - Merge branch 'master' of https://github.com/openjdk/jdk into isEnqueued >> - add suppress warnings >> - 8052260: Reference.isEnqueued() spec does not match the long-standing >> behavior returning true iff it's in the ref queue > > src/java.base/share/classes/java/lang/ref/Reference.java line 430: > >> 428: * This method was never implemented to test if a reference object >> has >> 429: * been cleared and enqueued as it was previously specified since >> 1.2. >> 430: * This method could be misused due to the inherent race condition > > A small suggestion is to restructure the first sentence of the deprecated > message to say "This method was originally specified to test .. but was never > implemented to do this test", otherwise looks okay. Yes, it reads better and we can also drop "as it was previously specified since 1.2". * This method was originally specified to test if a reference object has * been cleared and enqueued but was never implemented to do this test. - PR: https://git.openjdk.java.net/jdk/pull/1684
Re: RFR: 8246585: ForkJoin updates
On Tue, Dec 8, 2020 at 12:05 AM Aleksey Shipilev wrote: > On Sun, 6 Dec 2020 02:56:34 GMT, Martin Buchholz > wrote: > > > 8246585: ForkJoin updates > > It would be nice to get it tested with GH Actions. "Checks" tabs should > have those testing results automatically on push, but it is empty. Probably > because your fork is not configured for it. Please go to > https://github.com/Martin-Buchholz/jdk/actions -- and see if it says > anything suspicious? Triggering the (only) workflow manually against your > branch would help too. > Thanks for the handholding. I visited https://github.com/Martin-Buchholz/jdk/actions and it said Workflows aren't being run on this forked repository Because this repository contained workflow files when it was forked, we have disabled them from running on this fork. Make sure you understand the configured workflows and their expected usage before enabling Actions on this repository. They're now enabled. Do the skara docs need to clarify this?
Re: RFR: 8052260: Reference.isEnqueued() spec does not match the long-standing behavior returning true iff it's in the ref queue [v2]
On Tue, 8 Dec 2020 18:53:30 GMT, Mandy Chung wrote: >> `Reference::isEnqueued` method was never implemented as it was initially >> specified since 1.2. The specification says that it tests if this reference >> object has been enqueued, but the long-standing behavior is to test if this >> reference object is in its associated queue, only reflecting the state at >> the time when this method is executed. The implementation doesn't do what >> the specification promised, which might cause serious bugs if unnoticed. For >> example, an application that relies on this method to release critical >> resources could cause serious performance issues, in particular when this >> method is misused on Reference objects without an associated queue. >> `Reference::refersTo(null)` is the better recommended way to test if a >> Reference object has been cleared. >> >> This proposes to deprecate `Reference::isEnqueued`. Also the spec is >> updated to match the long-standing behavior. >> >> CSR: https://bugs.openjdk.java.net/browse/JDK-8189386 > > Mandy Chung has updated the pull request with a new target base due to a > merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains four additional > commits since the last revision: > > - Merge branch 'master' of https://github.com/openjdk/jdk into isEnqueued > - Merge branch 'master' of https://github.com/openjdk/jdk into isEnqueued > - add suppress warnings > - 8052260: Reference.isEnqueued() spec does not match the long-standing > behavior returning true iff it's in the ref queue src/java.base/share/classes/java/lang/ref/Reference.java line 430: > 428: * This method was never implemented to test if a reference object > has > 429: * been cleared and enqueued as it was previously specified since > 1.2. > 430: * This method could be misused due to the inherent race condition A small suggestion is to restructure the first sentence of the deprecated message to say "This method was originally specified to test .. but was never implemented to do this test", otherwise looks okay. - PR: https://git.openjdk.java.net/jdk/pull/1684
Re: RFR: JDK-8250768: javac should be adapted to changes in JEP 12 [v12]
> This is an update to javac and javadoc, to introduce support for Preview > APIs, and generally improve javac and javadoc behavior to more closely adhere > to JEP 12. > > The notable changes are: > > * adding support for Preview APIs (javac until now supported primarily only > preview language features, and APIs associated with preview language > features). This includes: > * the @PreviewFeature annotation has boolean attribute "reflective", > which should be true for reflective Preview APIs, false otherwise. This > replaces the existing "essentialAPI" attribute with roughly inverted meaning. > * the preview warnings for preview APIs are auto-suppressed as described > in the JEP 12. E.g. the module that declares the preview API is free to use > it without warnings > * improving error/warning messages. Please see [1] for a list of > cases/examples. > * class files are only marked as preview if they are using a preview > feature. [1] also shows if a class file is marked as preview or not. > * the PreviewFeature annotation has been moved to jdk.internal.javac package > (originally was in the jdk.internal package). > * Preview API in JDK's javadoc no longer needs to specify @preview tag in > the source files. javadoc will auto-generate a note for @PreviewFeature > elements, see e.g. [2] and [3] (non-reflective and reflective API, > respectively). A summary of preview elements is also provided [4]. Existing > uses of @preview have been updated/removed. > * non-JDK javadoc is also enhanced to auto-generate preview notes for uses > of Preview elements, and for declaring elements using preview language > features [5]. > > Please also see the CSR [6] for more information. > > [1] http://cr.openjdk.java.net/~jlahoda/8250768/JEP12-errors-warnings-v6.html > [2] > http://cr.openjdk.java.net/~jlahoda/8250768/jdk.javadoc.00/api/java.base/java/lang/Record.html > [3] > http://cr.openjdk.java.net/~jlahoda/8250768/jdk.javadoc.00/api/java.compiler/javax/lang/model/element/RecordComponentElement.html > [4] > http://cr.openjdk.java.net/~jlahoda/8250768/jdk.javadoc.00/api/preview-list.html > [5] http://cr.openjdk.java.net/~jlahoda/8250768/test.javadoc.00/ > [6] https://bugs.openjdk.java.net/browse/JDK-8250769 Jan Lahoda has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 55 commits: - Merging recent master changes into JDK-8250768 - Fixing navigator for the PREVIEW page. - Fixing typo. - Removing obsolette @PreviewFeature. - Merging master into JDK-8250768 - Removing unnecessary property keys. - Cleanup - removing unnecessary code. - Merging master into JDK-8250768-dev4 - Reflecting review comments. - Removing trailing whitespace. - ... and 45 more: https://git.openjdk.java.net/jdk/compare/044616bd...0c1c4d57 - Changes: https://git.openjdk.java.net/jdk/pull/703/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=703=11 Stats: 3711 lines in 132 files changed: 2726 ins; 692 del; 293 mod Patch: https://git.openjdk.java.net/jdk/pull/703.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/703/head:pull/703 PR: https://git.openjdk.java.net/jdk/pull/703
Re: RFR: 8257733: Move module-specific data from make to respective module [v2]
On Tue, 8 Dec 2020 17:33:16 GMT, Alan Bateman wrote: >> The mapping and nr tables, and the *-X.java.template files in >> make/data/charsetmapping are used to generate source code for the java.base >> and jdk.charsets modules. The stdcs-$OS files configure the package and >> module that each charset go into. If the tables used to generate the source >> files are moved to src/java.base then make/modules/jdk.charsets/Gensrc.gmk >> will probably need a new home too. > >> @AlanBateman The process of modularization was not fully completed with >> Project Jigsaw, and a few ugly warts remained. I was under the impression >> that these should be addressed in follow-up fixes, but this was >> unfortunately never done. Charsets and cldrconverter were both split between >> a core portion in java.base and the rest in jdk.charsets and jdk.localedata, >> respectively, but the split was never handled properly, but just "duct >> taped" in place. > > This is a complicated area of the build, not really a Project Jigsaw issue. > It's complicated because the source code for the charsets is generated at > build time and the set of non-standard charsets included in java.base varies > by platform, e.g. there's are several IBMxxx charsets in java.base when > building on AIX that are not interesting to include in java.base on other > platforms. This means we can't split up the mapping tables in > make/data/charsetmapping and put them in different directories. If you are > moving them into the src tree then src/java.base (as you have it) is best but > will still have the ugly wart that some of these mapping tables will be used > to generate code for the jdk.charsets module. @AlanBateman @mlchung @naotoj I can certainly anticipate follow-up cleanups on this patch. In fact, I think this patch has the potential to be a catalyst for such change, since the data that has been "hidden away" in `make` now becomes more visible to the teams that are capable of doing something about it. (With that said, of course the build team will assist in helping to clear up messy code structure, as we always do.) But I think we should be restrictive in trying too hard to make everything right in this single patch; it's better to get things approximately right and then go through the "warts" one by one and solve them properly. @AlanBateman What I meant by Jigsaw was that when the monolithic source code were modularized, the separation of concern between java.base and jdk.charsets/jdk.localedata was not complete, compared to (more or less) all other modules. It might very well be the case that we will never be able to make such a separation; but, prior to Jigsaw, this was not a "wart". It only became a code hygiene issue when some of the charsets and localedata was extraced from java.base. @mlchung My gut reaction is that we should not change idea.sh. First of all, kind of the purpose of this move is to make it clear to module developers the full set of materials their module consists of. That purpose would be sort of defeated, if we were to hide this in a popular IDE configuration. Secondly, I doubt this will add any performance difference. Listing additional files in the workspace is unlikely to do much, unless you actively open and/or interact with these files. But if you are worried, please fetch the PR (Skara adds instructions in the body) and try it out yourself! - PR: https://git.openjdk.java.net/jdk/pull/1611
Re: RFR: 8256515: javax.xml.XMLEventReader produces incorrect START_DOCUMENT event [v4]
On Tue, 8 Dec 2020 16:25:25 GMT, Marius Volkhart wrote: >> The default implementation of javax.xml.stream.XMLEventReader produced a >> StartDocument event that always indicated that the "standalone" attribute >> was set. >> >> The root cause of this was that the >> com.sun.xml.internal.stream.events.XMLEventAllocatorImpl always set the >> "standalone" attribute, rather than asking streamReader.standaloneSet() >> before setting the property of the StartDocumentEvent being created. > > Marius Volkhart has updated the pull request incrementally with two > additional commits since the last revision: > > - fixup! Fix: javax.xml.stream.XMLEventReader produces incorrect > START_DOCUMENT event > - fixup! Fix: javax.xml.stream.XMLEventReader produces incorrect > START_DOCUMENT event Marked as reviewed by joehw (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/1056
Re: RFR: 8052260: Reference.isEnqueued() spec does not match the long-standing behavior returning true iff it's in the ref queue [v2]
> `Reference::isEnqueued` method was never implemented as it was initially > specified since 1.2. The specification says that it tests if this reference > object has been enqueued, but the long-standing behavior is to test if this > reference object is in its associated queue, only reflecting the state at the > time when this method is executed. The implementation doesn't do what the > specification promised, which might cause serious bugs if unnoticed. For > example, an application that relies on this method to release critical > resources could cause serious performance issues, in particular when this > method is misused on Reference objects without an associated queue. > `Reference::refersTo(null)` is the better recommended way to test if a > Reference object has been cleared. > > This proposes to deprecate `Reference::isEnqueued`. Also the spec is updated > to match the long-standing behavior. > > CSR: https://bugs.openjdk.java.net/browse/JDK-8189386 Mandy Chung has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains four additional commits since the last revision: - Merge branch 'master' of https://github.com/openjdk/jdk into isEnqueued - Merge branch 'master' of https://github.com/openjdk/jdk into isEnqueued - add suppress warnings - 8052260: Reference.isEnqueued() spec does not match the long-standing behavior returning true iff it's in the ref queue - Changes: - all: https://git.openjdk.java.net/jdk/pull/1684/files - new: https://git.openjdk.java.net/jdk/pull/1684/files/7549cdc4..b4f9b489 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=1684=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=1684=00-01 Stats: 1325 lines in 35 files changed: 1010 ins; 175 del; 140 mod Patch: https://git.openjdk.java.net/jdk/pull/1684.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1684/head:pull/1684 PR: https://git.openjdk.java.net/jdk/pull/1684
Re: RFR: 8257887: java/foreign/TestSegments.java test fails on 32-bit after JDK-8257186
On Tue, 8 Dec 2020 17:17:54 GMT, Aditya Mandaleeka wrote: >> See here: >> https://github.com/mcimadamore/jdk/runs/1460615378 >> >> $ CONF=linux-x86-server-fastdebug make images run-test >> TEST=java/foreign/TestSegments.java >> >> STDERR: >> Invalid maximum heap size: -Xmx4G >> The specified size exceeds the maximum representable size. >> Error: Could not create the Java Virtual Machine. >> Error: A fatal exception has occurred. Program will exit. >> >> Adding `@requires` in the same form other `java/foreign` tests have it skips >> the test on 32-bit platforms. >> >> Additional testing: >> - [x] Affected test on Linux x86_32 (skipped now) >> - [x] Affected test on Linux x86_64 (still passes) > > Marked as reviewed by adityam (Author). Thanks folks! I need a formal Reviewer ack. @mcimadamore? This would make GH testing clean again. - PR: https://git.openjdk.java.net/jdk/pull/1688
Re: RFR: 8257733: Move module-specific data from make to respective module [v2]
On Tue, 8 Dec 2020 17:33:16 GMT, Alan Bateman wrote: >> The mapping and nr tables, and the *-X.java.template files in >> make/data/charsetmapping are used to generate source code for the java.base >> and jdk.charsets modules. The stdcs-$OS files configure the package and >> module that each charset go into. If the tables used to generate the source >> files are moved to src/java.base then make/modules/jdk.charsets/Gensrc.gmk >> will probably need a new home too. > >> @AlanBateman The process of modularization was not fully completed with >> Project Jigsaw, and a few ugly warts remained. I was under the impression >> that these should be addressed in follow-up fixes, but this was >> unfortunately never done. Charsets and cldrconverter were both split between >> a core portion in java.base and the rest in jdk.charsets and jdk.localedata, >> respectively, but the split was never handled properly, but just "duct >> taped" in place. > > This is a complicated area of the build, not really a Project Jigsaw issue. > It's complicated because the source code for the charsets is generated at > build time and the set of non-standard charsets included in java.base varies > by platform, e.g. there's are several IBMxxx charsets in java.base when > building on AIX that are not interesting to include in java.base on other > platforms. This means we can't split up the mapping tables in > make/data/charsetmapping and put them in different directories. If you are > moving them into the src tree then src/java.base (as you have it) is best but > will still have the ugly wart that some of these mapping tables will be used > to generate code for the jdk.charsets module. > @AlanBateman The process of modularization was not fully completed with > Project Jigsaw, and a few ugly warts remained. I was under the impression > that these should be addressed in follow-up fixes, but this was unfortunately > never done. Charsets and cldrconverter were both split between a core portion > in java.base and the rest in jdk.charsets and jdk.localedata, respectively, > but the split was never handled properly, but just "duct taped" in place. > > I chose to put the data files used for both java.base and the "additional" > modules in java.base, based on the comment that Naoto made in > https://mail.openjdk.java.net/pipermail/build-dev/2020-March/027044.html: > > > As to charsetmapping and cldrconverter, I believe they can reside in > > java.base, as jdk.charsets and jdk.localedata modules depend on it. > > Of course it would be preferable to make a proper split, but that requires > work done by the component teams to break the modules apart. > > Specifically for make/modules/jdk.charsets/Gensrc.gmk; the code in that file > is more or less duplicated in > make/modules/java.base/gensrc/GensrcCharsetMapping.gmk, since the same data > set is processed twice, once for java.base and once for jdk.charsets. I don't > think that means that make/modules/jdk.charsets/Gensrc.gmk should move to any > other place. I still stand by what I wrote above. It's best to put data in java.base for charsets/localedata. Otherwise we would have to duplicate some in each modules source directory. - PR: https://git.openjdk.java.net/jdk/pull/1611
Re: RFR: 8257620: Do not use objc_msgSend_stret to get macOS version
On Thu, 3 Dec 2020 06:50:11 GMT, Anton Kozlov wrote: >> Filed https://bugs.openjdk.java.net/browse/JDK-8257633 > > Thanks for taking care of those issues. To be clear, there is no real need to > bump the version for this PR, 10.9 is fine. This PR just proposes another way > to implement the workaround for 10.9. Hi, could I get review of the patch? - PR: https://git.openjdk.java.net/jdk/pull/1569
Re: RFR: 8256867: Classes with empty PermittedSubclasses attribute cannot be extended [v3]
On Tue, 8 Dec 2020 18:24:21 GMT, Mandy Chung wrote: >> Harold Seigel has updated the pull request incrementally with one additional >> commit since the last revision: >> >> 8256867: Classes with empty PermittedSubclasses attribute cannot be >> extended > > Marked as reviewed by mchung (Reviewer). Thanks Mandy, Chris, Lois, and Jan for reviewing this change. Harold - PR: https://git.openjdk.java.net/jdk/pull/1675
Re: RFR: 8257450: Start of release updates for JDK 17 [v5]
On Tue, 8 Dec 2020 17:10:30 GMT, Joe Darcy wrote: >> Start of JDK 17 updates. > > 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 12 additional commits since > the last revision: > > - Merge branch 'master' into JDK-8257450 > - Update symbol files for JDK 16b27. > - Merge branch 'master' into JDK-8257450 > - Merge branch 'master' into JDK-8257450 > - Merge branch 'master' into JDK-8257450 > - Update tests. > - Merge branch 'master' into JDK-8257450 > - Merge branch 'JDK-8257450' of https://github.com/jddarcy/jdk into > JDK-8257450 > - Merge branch 'JDK-8257450' of https://github.com/jddarcy/jdk into > JDK-8257450 > - JDK-8257450 > - ... and 2 more: > https://git.openjdk.java.net/jdk/compare/c3d62067...ff9b78ec Marked as reviewed by mchung (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/1531
Re: RFR: 8256867: Classes with empty PermittedSubclasses attribute cannot be extended [v3]
On Tue, 8 Dec 2020 14:57:26 GMT, Harold Seigel wrote: >> Please review this fix for JDK-8256867. This change no longer throws a >> ClassFormatError exception when loading a class whose PermittedSubclasses >> attribute is empty (contains no classes). Instead, the class is treated as >> a sealed class which cannot be extended nor implemented. This new behavior >> conforms to the JVM Spec. >> >> This change required changing Class.permittedSubclasses() to return an empty >> array for classes with empty PermittedSubclasses attributes, and to return >> null for non-sealed classes. >> >> This fix was tested with Mach5 tiers 1-2 on Linux, MacOS, and Windows, and >> tiers 3-5 on Linux x64. >> >> Thanks, Harold > > Harold Seigel has updated the pull request incrementally with one additional > commit since the last revision: > > 8256867: Classes with empty PermittedSubclasses attribute cannot be extended Marked as reviewed by mchung (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/1675
Re: RFR: 8257733: Move module-specific data from make to respective module [v2]
On Tue, 8 Dec 2020 18:16:15 GMT, Mandy Chung wrote: >> @wangweij Moving build tools is a related, but separate, question, which is >> addressed by https://bugs.openjdk.java.net/browse/JDK-8241463. >> >> I send out a review earlier this year (see >> https://mail.openjdk.java.net/pipermail/build-dev/2020-March/027038.html), >> but there were differing opinions on what the proper placement of these >> tools should be, and the discussion kind of fizzled out without reaching a >> conclusion. > > @magicus @erikj79 it's now clearly stated that you want everything under > `make` owned by the build team, which is fair. You are right that `make` has > been easily considered as a dumping ground and it's time to define a clean > structure. > > I reviewed this patch and this looks okay to me. Some follow-up questions > and follow-on cleanup for example "should jdwp.spec belong to `specs` > directory vs `data`? There are platform-specific data (such as charsets) > which has been special-case in the makefile and they need follow-on clean up. > I agree that this should be cleaned up by the component teams and not part > of this PR. With these new files showing up in under `src` directory, should `bin/idea.sh` be changed to exclude `data` to avoid incurring costs in loading JDK project from IDE that many of us use for development? - PR: https://git.openjdk.java.net/jdk/pull/1611
Re: RFR: 8257733: Move module-specific data from make to respective module [v2]
On Tue, 8 Dec 2020 16:19:05 GMT, Magnus Ihse Bursie wrote: >> Is there a plan to move make/jdk/src/classes/build/tools/intpoly into >> java.base as well? >> >> Update: I see all subdirs in tools are still there. > > @wangweij Moving build tools is a related, but separate, question, which is > addressed by https://bugs.openjdk.java.net/browse/JDK-8241463. > > I send out a review earlier this year (see > https://mail.openjdk.java.net/pipermail/build-dev/2020-March/027038.html), > but there were differing opinions on what the proper placement of these tools > should be, and the discussion kind of fizzled out without reaching a > conclusion. @magicus @erikj79 it's now clearly stated that you want everything under `make` owned by the build team, which is fair. You are right that `make` has been easily considered as a dumping ground and it's time to define a clean structure. I reviewed this patch and this looks okay to me. Some follow-up questions and follow-on cleanup for example "should jdwp.spec belong to `specs` directory vs `data`? There are platform-specific data (such as charsets) which has been special-case in the makefile and they need follow-on clean up. I agree that this should be cleaned up by the component teams and not part of this PR. - PR: https://git.openjdk.java.net/jdk/pull/1611
Integrated: 8200102: NativeLibraryTest.java fails intermittently, unloaded count is not same as expected
On Fri, 4 Dec 2020 18:23:55 GMT, Brent Christian wrote: > Please review this fix for the intermittently-failing > java/lang/ClassLoader/nativeLibrary/NativeLibraryTest.java. > > The change replaces System.gc()+sleep() with the more robust gcAwait() > mechanic used elsewhere in the test base, [as pointed out by > Martin](https://bugs.openjdk.java.net/browse/JDK-8200102?focusedCommentId=14382648=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14382648)(thanks!). > > The new version of the test passes 100 times out of 100 on the test farm. This pull request has now been integrated. Changeset: 1a9ed92d Author:Brent Christian URL: https://git.openjdk.java.net/jdk/commit/1a9ed92d Stats: 12 lines in 1 file changed: 4 ins; 1 del; 7 mod 8200102: NativeLibraryTest.java fails intermittently, unloaded count is not same as expected Reviewed-by: mchung, naoto - PR: https://git.openjdk.java.net/jdk/pull/1630
Re: RFR: 8256867: Classes with empty PermittedSubclasses attribute cannot be extended [v3]
On Tue, 8 Dec 2020 17:38:12 GMT, Chris Hegarty wrote:
>> src/java.base/share/classes/java/lang/Class.java line 4399:
>>
>>> 4397: * that is {@link #isSealed()} returns {@code false}, then this
>>> method returns {@code null}.
>>> 4398: * Conversely, if {@link #isSealed()} returns {@code true}, then
>>> this method
>>> 4399: * returns a non-null value.
>>
>> @ChrisHegarty minor but I prefer a simpler alternative to address your
>> concern is to add a link to the reference to "sealed class or interface" to
>> `Class::isSealed` as follows:
>> implement this class or interface if it is {@linkplain #isSealed() sealed}.
>> The order of such elements is unspecified. If this {@code Class} object
>> represents a primitive type, is unspecified. The array is empty if this
>> {@linkplain #isSealed() sealed} class or interface has no permitted
>> subclass.
>> If this {@code Class} object represents a primitive type, {@code void}, an
>> array type,
>> or a class or interface that is not sealed, then this method returns {@code
>> null}.
>
> There is certainly a little redundancy in the additional spec wording
> that I proposed. In my view it is worth it, as it allows the reader to
> more easily grok the null versus empty array for non-sealed classes,
> without having to navigate between the pair of methods.
I see this new pattern introduced in `getRecordComponents`. You may consider
if this pattern should consistently applied in other `Class` APIs such as
`getEnumConstants`.
-
PR: https://git.openjdk.java.net/jdk/pull/1675
Re: RFR: JDK-8257086: Clarify differences between {Float, Double}.equals and ==
On Tue, 8 Dec 2020 16:29:49 GMT, Joe Darcy wrote:
> Updates to the specifications of Double.{equals, compareTo} to explain more
> explicitly why the obvious wrappers around "==" and "<"/"==" are not
> sufficient for floating-point values.
>
> Once the wording is worked out, I'll replicate it for the analogous methods
> on Float.
I'll note initially that the original bug is about `equals` and `==` whereas
this change also covers `compareTo` and additional comparison operators `<` and
`>`. I believe covering this additional material **IS** important, as these
concepts are all closely related.
While this material is covering the right ground, I'd say that it's too verbose
for a method specification. It feels like it's being compressed to fit into a
method specification and thus doesn't do the topic justice.
(One additional concept that ought to be covered is that `compareTo` is
*consistent with equals*. This should be asserted in the method specification,
but trying to explain it in a method specification seems difficult.)
I'll suggest a couple alternatives. One is to put a full, combined explanation
into class doc somewhere, say in `Double`. The various methods can then make
some terse assertions and then refer to the combined explanation. `Float` could
refer to `Double` instead of replicating the text.
Another alternative is to put this explanation into the `java.lang` package
specification. This might be a good fit, since there is already some
explanation there of the boxed primitives.
-
PR: https://git.openjdk.java.net/jdk/pull/1699
Re: RFR: 8256867: Classes with empty PermittedSubclasses attribute cannot be extended [v3]
On Tue, 8 Dec 2020 17:18:20 GMT, Mandy Chung wrote:
>> Harold Seigel has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> 8256867: Classes with empty PermittedSubclasses attribute cannot be
>> extended
>
> src/java.base/share/classes/java/lang/Class.java line 4399:
>
>> 4397: * that is {@link #isSealed()} returns {@code false}, then this
>> method returns {@code null}.
>> 4398: * Conversely, if {@link #isSealed()} returns {@code true}, then
>> this method
>> 4399: * returns a non-null value.
>
> @ChrisHegarty minor but I prefer a simpler alternative to address your
> concern is to add a link to the reference to "sealed class or interface" to
> `Class::isSealed` as follows:
> implement this class or interface if it is {@linkplain #isSealed() sealed}.
> The order of such elements is unspecified. If this {@code Class} object
> represents a primitive type, is unspecified. The array is empty if this
> {@linkplain #isSealed() sealed} class or interface has no permitted subclass.
> If this {@code Class} object represents a primitive type, {@code void}, an
> array type,
> or a class or interface that is not sealed, then this method returns {@code
> null}.
There is certainly a little redundancy in the additional spec wording
that I proposed. In my view it is worth it, as it allows the reader to
more easily grok the null versus empty array for non-sealed classes,
without having to navigate between the pair of methods.
-
PR: https://git.openjdk.java.net/jdk/pull/1675
Re: RFR: 8257733: Move module-specific data from make to respective module [v2]
On Tue, 8 Dec 2020 12:15:38 GMT, Alan Bateman wrote: >> Also, to clarify, for me there is a fundamental difference between >> `src/$MODULE` and `make/modules/$MODULE`. The former is the home of files >> that are part of the module, owned by the content team, and the `$MODULE` >> part is essential to delineate this content. The latter is owned by the >> build team, and is just a convenient way to organize the build system within >> the `make` directory. So it's clearly a no-no to put anything but `.gmk` >> files in `make/modules/$MODULE`. > > The mapping and nr tables, and the *-X.java.template files in > make/data/charsetmapping are used to generate source code for the java.base > and jdk.charsets modules. The stdcs-$OS files configure the package and > module that each charset go into. If the tables used to generate the source > files are moved to src/java.base then make/modules/jdk.charsets/Gensrc.gmk > will probably need a new home too. > @AlanBateman The process of modularization was not fully completed with > Project Jigsaw, and a few ugly warts remained. I was under the impression > that these should be addressed in follow-up fixes, but this was unfortunately > never done. Charsets and cldrconverter were both split between a core portion > in java.base and the rest in jdk.charsets and jdk.localedata, respectively, > but the split was never handled properly, but just "duct taped" in place. This is a complicated area of the build, not really a Project Jigsaw issue. It's complicated because the source code for the charsets is generated at build time and the set of non-standard charsets included in java.base varies by platform, e.g. there's are several IBMxxx charsets in java.base when building on AIX that are not interesting to include in java.base on other platforms. This means we can't split up the mapping tables in make/data/charsetmapping and put them in different directories. If you are moving them into the src tree then src/java.base (as you have it) is best but will still have the ugly wart that some of these mapping tables will be used to generate code for the jdk.charsets module. - PR: https://git.openjdk.java.net/jdk/pull/1611
Re: RFR: 8257876: Avoid Reference.isEnqueued in tests
On Tue, 8 Dec 2020 09:52:51 GMT, Kim Barrett wrote: > Please review this change that eliminates the use of Reference.isEnqueued by > tests. There were three tests using it: > > vmTestbase/gc/gctests/ReferencesGC/ReferencesGC.java > vmTestbase/gc/gctests/WeakReferenceGC/WeakReferenceGC.java > jdk/java/lang/ref/ReferenceEnqueue.java > > In each of them, some combination of using Reference.refersTo and > ReferenceQueue.remove with a timeout were used to eliminate the use of > Reference.isEnqueued. > > I also cleaned up ReferencesGC.java in various respects. It contained > several bits of dead code, and the failure checks were made stronger. > > Testing: > mach5 tier1 > Locally (linux-x64) ran all three tests with each GC (including Shenandoah). Marked as reviewed by mchung (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/1691
Re: RFR: 8256867: Classes with empty PermittedSubclasses attribute cannot be extended [v3]
On Tue, 8 Dec 2020 14:57:26 GMT, Harold Seigel wrote:
>> Please review this fix for JDK-8256867. This change no longer throws a
>> ClassFormatError exception when loading a class whose PermittedSubclasses
>> attribute is empty (contains no classes). Instead, the class is treated as
>> a sealed class which cannot be extended nor implemented. This new behavior
>> conforms to the JVM Spec.
>>
>> This change required changing Class.permittedSubclasses() to return an empty
>> array for classes with empty PermittedSubclasses attributes, and to return
>> null for non-sealed classes.
>>
>> This fix was tested with Mach5 tiers 1-2 on Linux, MacOS, and Windows, and
>> tiers 3-5 on Linux x64.
>>
>> Thanks, Harold
>
> Harold Seigel has updated the pull request incrementally with one additional
> commit since the last revision:
>
> 8256867: Classes with empty PermittedSubclasses attribute cannot be extended
src/java.base/share/classes/java/lang/Class.java line 4399:
> 4397: * that is {@link #isSealed()} returns {@code false}, then this
> method returns {@code null}.
> 4398: * Conversely, if {@link #isSealed()} returns {@code true}, then
> this method
> 4399: * returns a non-null value.
@ChrisHegarty minor but I prefer a simpler alternative to address your concern
is to add a link to the reference to "sealed class or interface" to
`Class::isSealed` as follows:
implement this class or interface if it is {@linkplain #isSealed() sealed}.
The order of such elements is unspecified. If this {@code Class} object
represents a primitive type, is unspecified. The array is empty if this
{@linkplain #isSealed() sealed} class or interface has no permitted subclass.
If this {@code Class} object represents a primitive type, {@code void}, an
array type,
or a class or interface that is not sealed, then this method returns {@code
null}.
-
PR: https://git.openjdk.java.net/jdk/pull/1675
Re: RFR: 6882207: Convert javap to use diamond operator internally
On Mon, 7 Dec 2020 20:30:07 GMT, Andrey Turbanov wrote: > 6882207: Convert javap to use diamond operator internally Nice; thanks - Marked as reviewed by jjg (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/1677
Re: RFR: 8257887: java/foreign/TestSegments.java test fails on 32-bit after JDK-8257186
On Tue, 8 Dec 2020 08:54:41 GMT, Aleksey Shipilev wrote: > See here: > https://github.com/mcimadamore/jdk/runs/1460615378 > > $ CONF=linux-x86-server-fastdebug make images run-test > TEST=java/foreign/TestSegments.java > > STDERR: > Invalid maximum heap size: -Xmx4G > The specified size exceeds the maximum representable size. > Error: Could not create the Java Virtual Machine. > Error: A fatal exception has occurred. Program will exit. > > Adding `@requires` in the same form other `java/foreign` tests have it skips > the test on 32-bit platforms. > > Additional testing: > - [x] Affected test on Linux x86_32 (skipped now) > - [x] Affected test on Linux x86_64 (still passes) Marked as reviewed by adityam (Author). - PR: https://git.openjdk.java.net/jdk/pull/1688
Re: RFR: JDK-8257086: Clarify differences between {Float, Double}.equals and ==
On Tue, 8 Dec 2020 16:29:49 GMT, Joe Darcy wrote:
> Updates to the specifications of Double.{equals, compareTo} to explain more
> explicitly why the obvious wrappers around "==" and "<"/"==" are not
> sufficient for floating-point values.
>
> Once the wording is worked out, I'll replicate it for the analogous methods
> on Float.
src/java.base/share/classes/java/lang/Double.java line 811:
> 809: *
> 810: * also has the value {@code true}. However, there are two
> 811: * exceptions where the properties of an equivalence relations are
type: relations -> relation.
src/java.base/share/classes/java/lang/Double.java line 1008:
> 1006: * This method imposes a total order on {@code Double} objects
> 1007: * with two differences compared to the incomplete order defined the
> 1008: * by Java language numerical comparison operators ({@code <, <=,
Typo: defined the by Java -> defined by the Java.
-
PR: https://git.openjdk.java.net/jdk/pull/1699
Re: RFR: 8257450: Start of release updates for JDK 17 [v5]
> Start of JDK 17 updates. 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 12 additional commits since the last revision: - Merge branch 'master' into JDK-8257450 - Update symbol files for JDK 16b27. - Merge branch 'master' into JDK-8257450 - Merge branch 'master' into JDK-8257450 - Merge branch 'master' into JDK-8257450 - Update tests. - Merge branch 'master' into JDK-8257450 - Merge branch 'JDK-8257450' of https://github.com/jddarcy/jdk into JDK-8257450 - Merge branch 'JDK-8257450' of https://github.com/jddarcy/jdk into JDK-8257450 - JDK-8257450 - ... and 2 more: https://git.openjdk.java.net/jdk/compare/e5462611...ff9b78ec - Changes: - all: https://git.openjdk.java.net/jdk/pull/1531/files - new: https://git.openjdk.java.net/jdk/pull/1531/files/342c8650..ff9b78ec Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=1531=04 - incr: https://webrevs.openjdk.java.net/?repo=jdk=1531=03-04 Stats: 1427 lines in 38 files changed: 1106 ins; 191 del; 130 mod Patch: https://git.openjdk.java.net/jdk/pull/1531.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1531/head:pull/1531 PR: https://git.openjdk.java.net/jdk/pull/1531
Re: A Bug involving MethodHandles, Nestmates, Reflection and @CallerSensitive
On 08-Dec-20 5:40, Mandy Chung wrote: Thanks David. I was about to create one. This is indeed a gap in injecting this trampoline class for supporting @CallerSensitive methods. The InjectedInvoker ensures that the InjectedInvoker class has the same class loader as the lookup class. W.r.t. your patch, it seems okay but I have to consider and think through the security implication carefully. You mentioned "Some old software loves to set static final fields through reflection" - can you share some example libraries about this? This is really ugly hack!! Mandy Not sure if I read this correctly as "please share some example of code that tries to do that" or "please share code that you write to fix that". So I do both. Setting static final fields does not work [1]. It probably never really did. It usually seems to work - but there is no guarantee that it actually does (like undefined behavior). JPEXS [2] for example used that for it's configuration. Also some old Minecraft Forge version (a Minecraft mod loader / mod API) depends on this. Example use [3], but they do really ugly things. So, I said I develop agents to get old stuff running on a current Java version. Why? Fun, I guess. I also learn a lot a about what are the main comparability problems with newer Java versions. Pros of writing an agent: * I don't need the source code. * I don't need to setup a build environment with all dependencies, lombok and who knows what else is required. In all, it's faster for me. And I then have a list of problems - and how they can be solved. I did publish my JPESX agent here [4]. But yeah, it's an ugly hack. For the nestmate security consideration, the following invariants should already hold: * To call a @CallerSensitive method, the Lookup needs to have full privilege access (Lookup.hasFullPrivilegeAccess()) -> Injected Invokers are only created for full privilege lookups. * The injected invoker is in the same runtime package and has the same protection domain. * It is already possible to obtain a Lookup for the injected invoker by calling MethodHandles.lookup() through a MethodHandle (yikes) [5]. This means, we only have to consider what additional privileges the injected invoker gets if it is also a nestmate of the lookup class. I don't see any problem with that, as you already have a full privilege lookup when the injected invoker is created. - Johannes PS.: JDK-8013527 is mildly related - as the @CallerSensitive methods in java.lang.reflect are "hyper-sensitive". [1]: https://gist.github.com/DasBrain/25c6738610c517ee375aacc86ffebd0c [2]: https://github.com/akx/jpexs-decompiler/blob/6c998254b9d5bdce80be1b92d34836820ee31a1d/libsrc/ffdec_lib/src/com/jpexs/decompiler/flash/configuration/Configuration.java#L869 [3]: https://github.com/Chisel-Team/Chisel/blob/1.12/dev/src/main/java/team/chisel/common/init/ChiselBlocks.java#L18 [4]: https://github.com/DasBrain/jpex/tree/master/src/pw/dasbrain/jpexs/agent [5]: https://gist.github.com/DasBrain/142cb8ef9cc16e7469ac519790119e07
Re: RFR: JDK-8212035 : merge jdk.test.lib.util.SimpleHttpServer with jaxp.library.SimpleHttpServer [v2]
> jaxp.library.SimpleHttpServer > https://bugs.openjdk.java.net/browse/JDK-8212035 Mahendra Chhipa has updated the pull request incrementally with one additional commit since the last revision: Implemented the review comments. - Changes: - all: https://git.openjdk.java.net/jdk/pull/1632/files - new: https://git.openjdk.java.net/jdk/pull/1632/files/f48a3f7a..71fc7e9f Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=1632=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=1632=00-01 Stats: 145 lines in 3 files changed: 26 ins; 33 del; 86 mod Patch: https://git.openjdk.java.net/jdk/pull/1632.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1632/head:pull/1632 PR: https://git.openjdk.java.net/jdk/pull/1632
Re: RFR: JDK-8212035 : merge jdk.test.lib.util.SimpleHttpServer with jaxp.library.SimpleHttpServer [v2]
On Fri, 4 Dec 2020 20:45:24 GMT, Daniel Fuchs wrote: >> Mahendra Chhipa has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Implemented the review comments. > > test/lib/jdk/test/lib/net/SimpleHttpServer.java line 95: > >> 93: return _httpserver.getAddress().getPort(); >> 94: } >> 95: > > There are many issues with this class - using "localhost" and binding to the > wildcard address among others. > Having instance variables that are not final but are accessed by potentially > multiple threads is another. > I could also mention not using try-with-resources or the odd _name convention. > It will need to be modernized if you want to put it in jdk.test.lib.net; Thanks. Now this class is modernized in next patch. - PR: https://git.openjdk.java.net/jdk/pull/1632
Re: RFR: JDK-8212035 : merge jdk.test.lib.util.SimpleHttpServer with jaxp.library.SimpleHttpServer [v2]
On Fri, 4 Dec 2020 20:48:01 GMT, Daniel Fuchs wrote:
>> test/jdk/sun/net/www/protocol/jar/MultiReleaseJarURLConnection.java line 80:
>>
>>> 78: creator.buildSignedMultiReleaseJar();
>>> 79:
>>> 80: server = new
>>> SimpleHttpServer(TESTCONTEXT,System.getProperty("user.dir", "."));
>>
>> Please add space after comma.
>
> I believe the InetAddress parameter should be preserved in order to allow
> test to specifically bind to the loopback address instead of the wildcard
> (binding to the wildcard has been a source of test instability in the past).
Thanks. Review comments are fixed in next patch
-
PR: https://git.openjdk.java.net/jdk/pull/1632
RFR: JDK-8257086: Clarify differences between {Float, Double}.equals and ==
Updates to the specifications of Double.{equals, compareTo} to explain more
explicitly why the obvious wrappers around "==" and "<"/"==" are not sufficient
for floating-point values.
Once the wording is worked out, I'll replicate it for the analogous methods on
Float.
-
Commit messages:
- Fix whitespace
- Initial work for JDK-8257086.
Changes: https://git.openjdk.java.net/jdk/pull/1699/files
Webrev: https://webrevs.openjdk.java.net/?repo=jdk=1699=00
Issue: https://bugs.openjdk.java.net/browse/JDK-8257086
Stats: 65 lines in 1 file changed: 47 ins; 0 del; 18 mod
Patch: https://git.openjdk.java.net/jdk/pull/1699.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/1699/head:pull/1699
PR: https://git.openjdk.java.net/jdk/pull/1699
Re: RFR: 8257639: Update usage of "type" terminology in java.lang.Enum & java.lang.Record [v2]
On Tue, 8 Dec 2020 16:21:09 GMT, Julia Boes wrote: >> This change applies a stricter semantic distinction of 'type' versus 'class >> and interface'. This is based on the JLS changes described in the >> "Consistent Class and Interface Terminology" document: >> https://download.java.net/java/early_access/jdk16/docs/specs/class-terminology-jls.html. >> >> The following rules were applied: >> - 'class' and/or 'interface' are used when referring to the class/interface >> itself >> - 'type' is used when referring to the type of a variable or expression > > Julia Boes has updated the pull request incrementally with one additional > commit since the last revision: > > adjust 1 change in Enum.java Marked as reviewed by chegar (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/1670
Re: RFR: 8257639: Update usage of "type" terminology in java.lang.Enum & java.lang.Record [v2]
On Tue, 8 Dec 2020 16:21:09 GMT, Julia Boes wrote: >> This change applies a stricter semantic distinction of 'type' versus 'class >> and interface'. This is based on the JLS changes described in the >> "Consistent Class and Interface Terminology" document: >> https://download.java.net/java/early_access/jdk16/docs/specs/class-terminology-jls.html. >> >> The following rules were applied: >> - 'class' and/or 'interface' are used when referring to the class/interface >> itself >> - 'type' is used when referring to the type of a variable or expression > > Julia Boes has updated the pull request incrementally with one additional > commit since the last revision: > > adjust 1 change in Enum.java Marked as reviewed by dfuchs (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/1670
Re: RFR: 8256515: javax.xml.XMLEventReader produces incorrect START_DOCUMENT event [v4]
> The default implementation of javax.xml.stream.XMLEventReader produced a > StartDocument event that always indicated that the "standalone" attribute was > set. > > The root cause of this was that the > com.sun.xml.internal.stream.events.XMLEventAllocatorImpl always set the > "standalone" attribute, rather than asking streamReader.standaloneSet() > before setting the property of the StartDocumentEvent being created. Marius Volkhart has updated the pull request incrementally with two additional commits since the last revision: - fixup! Fix: javax.xml.stream.XMLEventReader produces incorrect START_DOCUMENT event - fixup! Fix: javax.xml.stream.XMLEventReader produces incorrect START_DOCUMENT event - Changes: - all: https://git.openjdk.java.net/jdk/pull/1056/files - new: https://git.openjdk.java.net/jdk/pull/1056/files/0fa81e46..68ab39aa Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=1056=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk=1056=02-03 Stats: 79 lines in 4 files changed: 25 ins; 46 del; 8 mod Patch: https://git.openjdk.java.net/jdk/pull/1056.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1056/head:pull/1056 PR: https://git.openjdk.java.net/jdk/pull/1056
Re: RFR: JDK-8257539: tools/jpackage/windows/WinL10nTest.java unpack.bat failed with Exit code: 1618
On Tue, 8 Dec 2020 14:56:20 GMT, Andy Herrick wrote: >> Looks like test failed due to: >> [Fatal Error] b.wxl:3:1: XML document structures must start and end within >> the same entity. >> So, I am not sure how increased repeat will help. Do we know why it failed >> to parse XML? > >> >> >> Looks like test failed due to: >> [Fatal Error] b.wxl:3:1: XML document structures must start and end within >> the same entity. >> So, I am not sure how increased repeat will help. Do we know why it failed >> to parse XML? > > If you scroll down from that error - you can see that that is the expected > error and the return code from jpackage is asserted to be 1, and it is > asserted that the resulting WinL10NTest.msi does not exist. > > the @Parameters for "data" cause instance of this test to be run 8 times with > different args. This instances is expected to fail since allWxlFilesValid is > false. > later in the log > (https://mach5.us.oracle.com:10060/api/v1/results/mach5-one-jdk-16+26-1740-tier2-20201124-1335-16116386-open_test_jdk_tier2_part2-windows-x64-125-1606226621-1556/log) > you can see the timeout, where unpack.bat is run three times with 3 second > delay and returns 1618 each time: > **13:58:30.303] TRACE: exec: Execute [cmd /c > .\\test.3563aceb\\unpacked-msi\\unpack.bat](3); discard I/O... > [13:58:33.469] TRACE: exec: Done. Exit code: 1618 > [13:58:36.492] TRACE: exec: Execute [cmd /c > .\\test.3563aceb\\unpacked-msi\\unpack.bat](3); discard I/O... > [13:58:39.636] TRACE: exec: Done. Exit code: 1618 > [13:58:42.652] TRACE: exec: Execute [cmd /c > .\\test.3563aceb\\unpacked-msi\\unpack.bat](3); discard I/O... > [13:58:45.803] TRACE: exec: Done. Exit code: 1618 > [13:58:48.832] ERROR: Expected [0]. Actual [1618]: Check command [cmd /c > .\\test.3563aceb\\unpacked-msi\\unpack.bat](3) exited with 0 code > [13:58:48.832] [ FAILED ] WinL10nTest([name=a.wxl; culture=fr, name=b.wxl; > culture=fr](length=2), fr;en-us, null).test; checks=17 > [13:58:48.832] [ RUN ] WinL10nTest([name=a.wxl; culture=fr, name=b.wxl; > culture=it, name=c.wxl; culture=fr, name=d.wxl; culture=it](length=4), > fr;it;en-us,** > > running unpack.bat with msiexec command in it succeed for one test instance > after the expected failure, then got 1618 return on the second test instance > after the expected failure with the above timeout. I converted to draft because somehow merging with master caused a mess - since is simple change in one test file I may create a new branch and new pull request cleanly again. - PR: https://git.openjdk.java.net/jdk/pull/1676
Re: RFR: 8256515: javax.xml.XMLEventReader produces incorrect START_DOCUMENT event [v4]
On Thu, 3 Dec 2020 23:40:40 GMT, Joe Wang wrote:
>> Marius Volkhart has updated the pull request incrementally with two
>> additional commits since the last revision:
>>
>> - fixup! Fix: javax.xml.stream.XMLEventReader produces incorrect
>> START_DOCUMENT event
>> - fixup! Fix: javax.xml.stream.XMLEventReader produces incorrect
>> START_DOCUMENT event
>
> src/java.xml/share/classes/com/sun/xml/internal/stream/events/XMLEventAllocatorImpl.java
> line 136:
>
>> 134: if (streamReader.standaloneSet()) {
>> 135: sdEvent.setStandalone(streamReader.isStandalone());
>> 136: }
>
> The change looked fine at the first glance. However, when I looked at your
> test, I noticed that in your first test case, isStandalone will return true.
> This is because standalone was initiated as true and with the added
> condition, setStandalone is skipped.
>
> To fix the issue, consider, instead of making it conditional, adding
> standaloneSet to the setStandalone method.
>
> Pls update the copyright year at line 2, e.g. 2016 -> 2020.
Updates made. I understood your comment about modifying the `setStandalone`
method to mean `StartDocumentEvent#setStandalone`. If it was something else,
please let me know!
-
PR: https://git.openjdk.java.net/jdk/pull/1056
Re: RFR: 8257733: Move module-specific data from make to respective module [v2]
On Tue, 8 Dec 2020 15:25:47 GMT, Weijun Wang wrote: >> The mapping and nr tables, and the *-X.java.template files in >> make/data/charsetmapping are used to generate source code for the java.base >> and jdk.charsets modules. The stdcs-$OS files configure the package and >> module that each charset go into. If the tables used to generate the source >> files are moved to src/java.base then make/modules/jdk.charsets/Gensrc.gmk >> will probably need a new home too. > > Is there a plan to move make/jdk/src/classes/build/tools/intpoly into > java.base as well? > > Update: I see all subdirs in tools are still there. @wangweij Moving build tools is a related, but separate, question, which is addressed by https://bugs.openjdk.java.net/browse/JDK-8241463. I send out a review earlier this year (see https://mail.openjdk.java.net/pipermail/build-dev/2020-March/027038.html), but there were differing opinions on what the proper placement of these tools should be, and the discussion kind of fizzled out without reaching a conclusion. - PR: https://git.openjdk.java.net/jdk/pull/1611
Re: RFR: 8257639: Update usage of "type" terminology in java.lang.Enum & java.lang.Record [v2]
> This change applies a stricter semantic distinction of 'type' versus 'class > and interface'. This is based on the JLS changes described in the "Consistent > Class and Interface Terminology" document: > https://download.java.net/java/early_access/jdk16/docs/specs/class-terminology-jls.html. > > The following rules were applied: > - 'class' and/or 'interface' are used when referring to the class/interface > itself > - 'type' is used when referring to the type of a variable or expression Julia Boes has updated the pull request incrementally with one additional commit since the last revision: adjust 1 change in Enum.java - Changes: - all: https://git.openjdk.java.net/jdk/pull/1670/files - new: https://git.openjdk.java.net/jdk/pull/1670/files/6a2a55ea..36b7ceb0 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=1670=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=1670=00-01 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/1670.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1670/head:pull/1670 PR: https://git.openjdk.java.net/jdk/pull/1670
Re: RFR: 8257733: Move module-specific data from make to respective module [v2]
On Tue, 8 Dec 2020 12:15:38 GMT, Alan Bateman wrote: >> Also, to clarify, for me there is a fundamental difference between >> `src/$MODULE` and `make/modules/$MODULE`. The former is the home of files >> that are part of the module, owned by the content team, and the `$MODULE` >> part is essential to delineate this content. The latter is owned by the >> build team, and is just a convenient way to organize the build system within >> the `make` directory. So it's clearly a no-no to put anything but `.gmk` >> files in `make/modules/$MODULE`. > > The mapping and nr tables, and the *-X.java.template files in > make/data/charsetmapping are used to generate source code for the java.base > and jdk.charsets modules. The stdcs-$OS files configure the package and > module that each charset go into. If the tables used to generate the source > files are moved to src/java.base then make/modules/jdk.charsets/Gensrc.gmk > will probably need a new home too. @AlanBateman The process of modularization was not fully completed with Project Jigsaw, and a few ugly warts remained. I was under the impression that these should be addressed in follow-up fixes, but this was unfortunately never done. Charsets and cldrconverter were both split between a core portion in java.base and the rest in jdk.charsets and jdk.localedata, respectively, but the split was never handled properly, but just "duct taped" in place. I chose to put the data files used for both java.base and the "additional" modules in java.base, based on the comment that Naoto made in https://mail.openjdk.java.net/pipermail/build-dev/2020-March/027044.html: > As to charsetmapping and cldrconverter, I believe they can reside in java.base, as jdk.charsets and jdk.localedata modules depend on it. Of course it would be preferable to make a proper split, but that requires work done by the component teams to break the modules apart. Specifically for make/modules/jdk.charsets/Gensrc.gmk; the code in that file is more or less duplicated in make/modules/java.base/gensrc/GensrcCharsetMapping.gmk, since the same data set is processed twice, once for java.base and once for jdk.charsets. I don't think that means that make/modules/jdk.charsets/Gensrc.gmk should move to any other place. - PR: https://git.openjdk.java.net/jdk/pull/1611
Re: RFR: 8257733: Move module-specific data from make to respective module [v2]
On Tue, 8 Dec 2020 12:15:38 GMT, Alan Bateman wrote: >> Also, to clarify, for me there is a fundamental difference between >> `src/$MODULE` and `make/modules/$MODULE`. The former is the home of files >> that are part of the module, owned by the content team, and the `$MODULE` >> part is essential to delineate this content. The latter is owned by the >> build team, and is just a convenient way to organize the build system within >> the `make` directory. So it's clearly a no-no to put anything but `.gmk` >> files in `make/modules/$MODULE`. > > The mapping and nr tables, and the *-X.java.template files in > make/data/charsetmapping are used to generate source code for the java.base > and jdk.charsets modules. The stdcs-$OS files configure the package and > module that each charset go into. If the tables used to generate the source > files are moved to src/java.base then make/modules/jdk.charsets/Gensrc.gmk > will probably need a new home too. Is there a plan to move make/jdk/src/classes/build/tools/intpoly into java.base as well? - PR: https://git.openjdk.java.net/jdk/pull/1611
Re: RFR: 8247373: ArraysSupport.newLength doc, test, and exception message [v2]
On Tue, 8 Dec 2020 06:14:35 GMT, Stuart Marks wrote: >> This rewrites the doc of ArraysSupport.newLength, adds detail to the >> exception message, and adds a test. In addition to some renaming and a bit >> of refactoring of the actual code, I also made two changes of substance to >> the code: >> >> 1. I fixed a problem with overflow checking. In the original code, if >> oldLength and prefGrowth were both very large (say, Integer.MAX_VALUE), this >> method could return a negative value. It turns out that writing tests helps >> find bugs! >> >> 2. Under the old policy, if oldLength and minGrowth required a length above >> SOFT_MAX_ARRAY_LENGTH but not above Integer.MAX_VALUE, this method would >> return Integer.MAX_VALUE. That doesn't make any sense, because attempting to >> allocate an array of that length will almost certainly cause the Hotspot to >> throw OOME because its implementation limit was exceeded. Instead, if the >> required length is in this range, this method returns that required length. >> >> Separately, I'll work on retrofitting various call sites around the JDK to >> use this method. > > Stuart Marks has updated the pull request incrementally with one additional > commit since the last revision: > > fix typo, clarify asserts disabled, test prefGrowth==0 Marked as reviewed by rriggs (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/1617
Re: RFR: 8247373: ArraysSupport.newLength doc, test, and exception message [v2]
On Tue, 8 Dec 2020 00:48:55 GMT, Stuart Marks wrote: >> The algorithm can be well defined for minGrowth and prefGrowth == 0 without >> extra checks or exceptions with a careful look at the inequality checks. >> For example, as currently coded, if both are zero, it returns >> SOFT_MAX_ARRAY_LENGTH. >> Changing the `0 < prefLength` to `0 <= prefLength` would return minGrowth >> and avoid a very large but unintentional allocation. > > That's only true if oldLength is zero. The behavior is different if oldLength > is positive. In any case, this method is called when the array needs to > **grow**, which means the caller needs to reallocate and copy. If the caller > passes zero for both minGrowth and prefGrowth, the caller doesn't need to > reallocate and copy, and there's no point in it calling this method in the > first place. Not calling for a zero value is usually an optimization, not a necessity to work around an inconsistency or unpredictability in the API or the range of arguments it accepts. - PR: https://git.openjdk.java.net/jdk/pull/1617
Re: RFR: JDK-8257539: tools/jpackage/windows/WinL10nTest.java unpack.bat failed with Exit code: 1618 [v2]
> increase retries and timeout increasing in runMsiexecWithRetries Andy Herrick has updated the pull request incrementally with 18 additional commits since the last revision: - 8256149: Weird AST structure for incomplete member select Reviewed-by: vromero - 8257194: Add 'foreign linker API' in 'jdk.incubator.foreign' module desc/summary Reviewed-by: jvernee, shade - 8257848: -XX:CompileCommand=blackhole,* should be diagnostic Reviewed-by: vlivanov - 8242258: (jrtfs) Path::toUri throws AssertionError for malformed input Reviewed-by: alanb - 8254733: HotSpot Style Guide should permit using range-based for loops Reviewed-by: dholmes, pliden, jrose, dcubed, iklam, eosterlund, tschatzl, kvn - 8253644: C2: assert(skeleton_predicate_has_opaque(iff)) failed: unexpected Reviewed-by: roland, kvn - 8256411: Based anonymous classes have a weird end position Reviewed-by: vromero - 8257813: [redo] C2: Filter type in PhiNode::Value() for induction variables of trip-counted integer loops Reviewed-by: chagedorn, kvn - 8257769: Cipher.getParameters() throws NPE for ChaCha20-Poly1305 Reviewed-by: mullan, valeriep - 8257855: Example SafeVarargsNotApplicableToRecordAccessors breaks test tools/javac/diags/CheckExamples.java Reviewed-by: jjg - ... and 8 more: https://git.openjdk.java.net/jdk/compare/7c4743c5...5d065497 - Changes: - all: https://git.openjdk.java.net/jdk/pull/1676/files - new: https://git.openjdk.java.net/jdk/pull/1676/files/7c4743c5..5d065497 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=1676=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=1676=00-01 Stats: 1938 lines in 49 files changed: 1465 ins; 276 del; 197 mod Patch: https://git.openjdk.java.net/jdk/pull/1676.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1676/head:pull/1676 PR: https://git.openjdk.java.net/jdk/pull/1676
Re: RFR: 8256867: Classes with empty PermittedSubclasses attribute cannot be extended [v3]
On Tue, 8 Dec 2020 14:57:26 GMT, Harold Seigel wrote: >> Please review this fix for JDK-8256867. This change no longer throws a >> ClassFormatError exception when loading a class whose PermittedSubclasses >> attribute is empty (contains no classes). Instead, the class is treated as >> a sealed class which cannot be extended nor implemented. This new behavior >> conforms to the JVM Spec. >> >> This change required changing Class.permittedSubclasses() to return an empty >> array for classes with empty PermittedSubclasses attributes, and to return >> null for non-sealed classes. >> >> This fix was tested with Mach5 tiers 1-2 on Linux, MacOS, and Windows, and >> tiers 3-5 on Linux x64. >> >> Thanks, Harold > > Harold Seigel has updated the pull request incrementally with one additional > commit since the last revision: > > 8256867: Classes with empty PermittedSubclasses attribute cannot be extended Marked as reviewed by chegar (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/1675
Re: RFR: JDK-8257539: tools/jpackage/windows/WinL10nTest.java unpack.bat failed with Exit code: 1618
On Tue, 8 Dec 2020 03:08:59 GMT, Alexander Matveev wrote: > > > Looks like test failed due to: > [Fatal Error] b.wxl:3:1: XML document structures must start and end within > the same entity. > So, I am not sure how increased repeat will help. Do we know why it failed to > parse XML? If you scroll down from that error - you can see that that is the expected error and the return code from jpackage is asserted to be 1, and it is asserted that the resulting WinL10NTest.msi does not exist. the @Parameters for "data" cause instance of this test to be run 8 times with different args. This instances is expected to fail since allWxlFilesValid is false. later in the log (https://mach5.us.oracle.com:10060/api/v1/results/mach5-one-jdk-16+26-1740-tier2-20201124-1335-16116386-open_test_jdk_tier2_part2-windows-x64-125-1606226621-1556/log) you can see the timeout, where unpack.bat is run three times with 3 second delay and returns 1618 each time: **13:58:30.303] TRACE: exec: Execute [cmd /c .\\test.3563aceb\\unpacked-msi\\unpack.bat](3); discard I/O... [13:58:33.469] TRACE: exec: Done. Exit code: 1618 [13:58:36.492] TRACE: exec: Execute [cmd /c .\\test.3563aceb\\unpacked-msi\\unpack.bat](3); discard I/O... [13:58:39.636] TRACE: exec: Done. Exit code: 1618 [13:58:42.652] TRACE: exec: Execute [cmd /c .\\test.3563aceb\\unpacked-msi\\unpack.bat](3); discard I/O... [13:58:45.803] TRACE: exec: Done. Exit code: 1618 [13:58:48.832] ERROR: Expected [0]. Actual [1618]: Check command [cmd /c .\\test.3563aceb\\unpacked-msi\\unpack.bat](3) exited with 0 code [13:58:48.832] [ FAILED ] WinL10nTest([name=a.wxl; culture=fr, name=b.wxl; culture=fr](length=2), fr;en-us, null).test; checks=17 [13:58:48.832] [ RUN ] WinL10nTest([name=a.wxl; culture=fr, name=b.wxl; culture=it, name=c.wxl; culture=fr, name=d.wxl; culture=it](length=4), fr;it;en-us,** running unpack.bat with msiexec command in it succeed for one test instance after the expected failure, then got 1618 return on the second test instance after the expected failure with the above timeout. - PR: https://git.openjdk.java.net/jdk/pull/1676
Re: RFR: 8256867: Classes with empty PermittedSubclasses attribute cannot be extended [v3]
> Please review this fix for JDK-8256867. This change no longer throws a > ClassFormatError exception when loading a class whose PermittedSubclasses > attribute is empty (contains no classes). Instead, the class is treated as a > sealed class which cannot be extended nor implemented. This new behavior > conforms to the JVM Spec. > > This change required changing Class.permittedSubclasses() to return an empty > array for classes with empty PermittedSubclasses attributes, and to return > null for non-sealed classes. > > This fix was tested with Mach5 tiers 1-2 on Linux, MacOS, and Windows, and > tiers 3-5 on Linux x64. > > Thanks, Harold Harold Seigel has updated the pull request incrementally with one additional commit since the last revision: 8256867: Classes with empty PermittedSubclasses attribute cannot be extended - Changes: - all: https://git.openjdk.java.net/jdk/pull/1675/files - new: https://git.openjdk.java.net/jdk/pull/1675/files/de461457..2ce2a993 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=1675=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk=1675=01-02 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/1675.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1675/head:pull/1675 PR: https://git.openjdk.java.net/jdk/pull/1675
Re: RFR: 8256867: Classes with empty PermittedSubclasses attribute cannot be extended [v2]
On Tue, 8 Dec 2020 14:37:52 GMT, Chris Hegarty wrote:
>> Harold Seigel has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> 8256867: Classes with empty PermittedSubclasses attribute cannot be
>> extended
>
> src/java.base/share/classes/java/lang/Class.java line 4399:
>
>> 4397: * that is {@link #isSealed()} returns {@code false}, then this
>> method returns {@code null}.
>> 4398: * Conversely, if {@link #isSealed()} returns {@code true}, then
>> this method
>> 4399: * returns a non-null value."
>
> Trivially, the trailing quote can be removed.
Thanks Chris! I'll remove the trailing quote.
-
PR: https://git.openjdk.java.net/jdk/pull/1675
Re: RFR: 8256867: Classes with empty PermittedSubclasses attribute cannot be extended [v2]
On Tue, 8 Dec 2020 14:10:26 GMT, Harold Seigel wrote:
>> Please review this fix for JDK-8256867. This change no longer throws a
>> ClassFormatError exception when loading a class whose PermittedSubclasses
>> attribute is empty (contains no classes). Instead, the class is treated as
>> a sealed class which cannot be extended nor implemented. This new behavior
>> conforms to the JVM Spec.
>>
>> This change required changing Class.permittedSubclasses() to return an empty
>> array for classes with empty PermittedSubclasses attributes, and to return
>> null for non-sealed classes.
>>
>> This fix was tested with Mach5 tiers 1-2 on Linux, MacOS, and Windows, and
>> tiers 3-5 on Linux x64.
>>
>> Thanks, Harold
>
> Harold Seigel has updated the pull request incrementally with one additional
> commit since the last revision:
>
> 8256867: Classes with empty PermittedSubclasses attribute cannot be extended
Thanks Harold. LGTM.
src/java.base/share/classes/java/lang/Class.java line 4399:
> 4397: * that is {@link #isSealed()} returns {@code false}, then this
> method returns {@code null}.
> 4398: * Conversely, if {@link #isSealed()} returns {@code true}, then
> this method
> 4399: * returns a non-null value."
Trivially, the trailing quote can be removed.
-
Marked as reviewed by chegar (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/1675
Re: RFR: 8256867: Classes with empty PermittedSubclasses attribute cannot be extended [v2]
On Tue, 8 Dec 2020 09:30:59 GMT, Chris Hegarty wrote:
>> src/java.base/share/classes/java/lang/Class.java line 4396:
>>
>>> 4394: * is unspecified. If this {@code Class} object represents a
>>> primitive type,
>>> 4395: * {@code void}, an array type, or a class or interface that is
>>> not sealed,
>>> 4396: * then null is returned.
>>
>> nit: s/null/`{@code null}`
>>
>> I'd suggest to clarify if this sealed class or interface has no permitted
>> subclass, something like this:
>> Returns an array containing {@code Class} objects representing the
>> direct subinterfaces or subclasses permitted to extend or
>> implement this class or interface if it is sealed. The order of such
>> elements
>> is unspecified. The array is empty if this sealed class or interface has no
>> permitted subclass.
>>
>> `@return` needs to be revised as well:
>> @return an array of {@code Class} objects of the permitted subclasses of
>> this sealed class or interface,
>> or {@null} if this class or interface is not sealed
>
> Mandy's suggested wording is good.
>
> I would like to add one more additional point of clarification. It would
> be good to strongly connect `isSealed` and `getPermittedClasses` in a
> first-class way in normative spec ( similar to isRecord and
> getRecordComponents ).
>
> For example,
>
> to `isSealed` add: "getPermittedSubclasses returns a non-null but possibly
>empty value for a sealed class."
>
> to `getPermittedSubclasses`: "If this class is not a sealed class, that is
> {@link
> * #isSealed()} returns {@code false}, then this method returns {@code
> null}.
> * Conversely, if {@link #isSealed()} returns {@code true}, then this
> method
> * returns a non-null value."
Please review the updated commit. It incorporates the changes to the comments
in Class.java suggested by Mandy and Chris.
Thanks, Harold
-
PR: https://git.openjdk.java.net/jdk/pull/1675
Re: RFR: 8256867: Classes with empty PermittedSubclasses attribute cannot be extended [v2]
On Tue, 8 Dec 2020 00:09:34 GMT, Mandy Chung wrote: >> src/hotspot/share/prims/jvm.cpp line 2130: >> >>> 2128: JvmtiVMObjectAllocEventCollector oam; >>> 2129: Array* subclasses = ik->permitted_subclasses(); >>> 2130: int length = subclasses == NULL ? 0 : subclasses->length(); >> >> Minor comment - you don't really need the check of subclasses == NULL here >> since subclasses will never be NULL. You could just assign length to >> subclasses->length(); > > +1. is_sealed returns true iff `_permitted_subclasses != NULL` Thanks for the reviews. I removed the check of subclasses == NULL in the updated commit. - PR: https://git.openjdk.java.net/jdk/pull/1675
Re: RFR: 8256867: Classes with empty PermittedSubclasses attribute cannot be extended [v2]
> Please review this fix for JDK-8256867. This change no longer throws a > ClassFormatError exception when loading a class whose PermittedSubclasses > attribute is empty (contains no classes). Instead, the class is treated as a > sealed class which cannot be extended nor implemented. This new behavior > conforms to the JVM Spec. > > This change required changing Class.permittedSubclasses() to return an empty > array for classes with empty PermittedSubclasses attributes, and to return > null for non-sealed classes. > > This fix was tested with Mach5 tiers 1-2 on Linux, MacOS, and Windows, and > tiers 3-5 on Linux x64. > > Thanks, Harold Harold Seigel has updated the pull request incrementally with one additional commit since the last revision: 8256867: Classes with empty PermittedSubclasses attribute cannot be extended - Changes: - all: https://git.openjdk.java.net/jdk/pull/1675/files - new: https://git.openjdk.java.net/jdk/pull/1675/files/89c61b95..de461457 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=1675=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=1675=00-01 Stats: 12 lines in 2 files changed: 6 ins; 0 del; 6 mod Patch: https://git.openjdk.java.net/jdk/pull/1675.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1675/head:pull/1675 PR: https://git.openjdk.java.net/jdk/pull/1675
Re: RFR: 8256867: Classes with empty PermittedSubclasses attribute cannot be extended [v2]
On Tue, 8 Dec 2020 14:07:08 GMT, Harold Seigel wrote: >> Please review this fix for JDK-8256867. This change no longer throws a >> ClassFormatError exception when loading a class whose PermittedSubclasses >> attribute is empty (contains no classes). Instead, the class is treated as >> a sealed class which cannot be extended nor implemented. This new behavior >> conforms to the JVM Spec. >> >> This change required changing Class.permittedSubclasses() to return an empty >> array for classes with empty PermittedSubclasses attributes, and to return >> null for non-sealed classes. >> >> This fix was tested with Mach5 tiers 1-2 on Linux, MacOS, and Windows, and >> tiers 3-5 on Linux x64. >> >> Thanks, Harold > > Harold Seigel has updated the pull request incrementally with one additional > commit since the last revision: > > 8256867: Classes with empty PermittedSubclasses attribute cannot be extended The langtools changes look good to me - thanks! - Marked as reviewed by jlahoda (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/1675
Re: RFR: 8257733: Move module-specific data from make to respective module [v2]
On 2020-12-08 00:30, Magnus Ihse Bursie wrote:
On Tue, 8 Dec 2020 02:40:43 GMT, Mandy Chung wrote:
I have reviewed all lines in the patch file with or near instances of
`jdk.compiler`
Hi Magnus,
I see the motivation of moving these build files for better identification of
ownership. Placing them under
`src/$MODULE/{share,$OS}/data` is one option. Given that skara will
automatically determine appropriate mailing lists of a PR, it seems that
`make/modules/$MODULE/data` can be another alternative that skara can include
this pattern in the mailing list configuration?? I don't yet have a strong
preference while I don't consider everything under `make` must be owned by the
build team though. Do you see one option is better than the other?
@mlchung If you don't have any strong preference, I implore you to accept this
change. I **vastly** prefer to move the data out of `make`!
This is not just about Skara tooling. When working with make files, autoconf and shell scripts,
there is no fancy IDE support, so you are stuck with simple text editors and tools like `grep`.
I've lost count on how many times I've had my grep searches blow up, since I happened to find e.g.
a string in `tzdata` and get hundreds or more of hits. :-( And I do believe we will get a better
code structure if the build team "owns" `make`; or at least has a vested interest in
what's in that directory. We still suffer a lot of the old "I don't know where to put this
file, so I'll just put it in make cause nobody cares about it anyway" mentality, but I've been
working for quite some time to make that list of misplaced files shorter and shorter.
I strongly agree with Magnus for all the same reasons. To me, the data
files are clearly a form of source code that should be considered owned
by the component teams. I'm honestly perplexed over why this is being
argued against.
/Erik
-
PR: https://git.openjdk.java.net/jdk/pull/1611
Re: RFR: 8257639: Update usage of "type" terminology in java.lang.Enum & java.lang.Record
On Tue, 8 Dec 2020 10:41:06 GMT, Julia Boes wrote: >> src/java.base/share/classes/java/lang/Enum.java line 62: >> >>> 60: * java.util.EnumMap map} implementations are available. >>> 61: * >>> 62: * @param The enum class subclass >> >> I wonder about this one, given that `` is a type. It sounds strange to me >> - even though I understand that what is meant is that this type should point >> to a class (a subclass of `java.lang.Enum`). > > Makes sense. I suggest the following instead: `The type of enum subclass`. Maybe: "`The type of the enum subclass.`" would sound better - or just "`The enum subclass`". - PR: https://git.openjdk.java.net/jdk/pull/1670
Re: Has it been considered to add inverse methods to collections which are in Optional?
- Mail original -
> De: "Dave Franken"
> À: "core-libs-dev"
> Envoyé: Mardi 8 Décembre 2020 13:17:25
> Objet: Has it been considered to add inverse methods to collections which are
> in Optional?
> Dear all,
Hi Dave,
>
> The class java.util.Optional could be considered a collection with a limit
> on how many values it can hold, minimum 0, maximum 1.
> Likewise, any class implementing the regular java.util.Collection has a
> minimum, 0 and a maximum > 1.
Optional is more considered as a Stream with 0 or 1 element.
The primary usage of Optional is to temporary wraps a value (or not) as the
result of a computation so it's not really like a collection which is a more
"permanent" storage.
>
> While Optional does not implement Collection, it could be seen as a
> pseudo-collection in this regard.
> Optional has a convenience method for which there is an inverse,
> isPresent() which is the inverse of isEmpty().
>
> Has it been considered to add such a method (or other convenience methods)
> to collections as well?
> For instance, java.util.Collection has the method isEmpty() which behaves
> exactly like Optional's isEmpty(), but it does not have an inverse
> counterpart.
Adding methods like notContains or notFilter have been considered several times
in the past and rejected because it's too many methods with no real gain. I'm
sure you can dig some old emails on lambda-dev and see the first iteration of
the Stream API, it was full of convenient methods like that.
I'm sure Stuart or Brian have a template answer for that :)
>
> I think it would be logical to add such as convenience method to
> Collection, because we have a precedent with Optional.
> We could add `bikeshed name here, as an example I'm going to use
> hasElements()' to java.util.Collection and, for simplicity's sake make the
> default implementation just call !isEmpty(); obviously implementations
> could offer optimized versions if necessary.
>
> It would improve readability where we have the negation of isEmpty(), e.g.:
>
> if (!myList.isEmpty()) {
> }
>
> // vs.
> if (myList.hasElements()) {
> }
>
> Note that in the first example, which is the current way of checking
> whether a (non null) collection has any elements, the negation and the
> method it negates are separated by the variable name.
> If the variable would instead be another method call or any other, longer,
> expression, this would make the example even less readable as the negation
> and final method call are further apart.
>
> The second example has the advantage of more clearly expressing the intent
> of the user - which is to check if the collection has any elements.
> Moreover, it offers matching functionality for something which already
> exists in the JDK core libs, Optional's isPresent().
Apart what i've said above, Java inherits from C its weird compact syntax, "if
(!myList.isEmpty())" instead of "if myList is not empty" like in HyperCard, i'm
not sure that trying to solve that by adding one or more method is not trying
to put lipstick to a pig. We are used to that pig since a long time to the
point we don't think of it as a pig.
>
> If this has already been discussed and dismissed, I'm sorry for bringing it
> up again and if anybody could point me to the conclusion and the reasoning
> behind it, I would be grateful.
>
> Kind regards,
regards,
>
> Dave Franken
Rémi
Re: RFR: 8257733: Move module-specific data from make to respective module [v2]
On Tue, 8 Dec 2020 08:32:28 GMT, Magnus Ihse Bursie wrote: >> @mlchung If you don't have any strong preference, I implore you to accept >> this change. I **vastly** prefer to move the data out of `make`! >> >> This is not just about Skara tooling. When working with make files, autoconf >> and shell scripts, there is no fancy IDE support, so you are stuck with >> simple text editors and tools like `grep`. I've lost count on how many times >> I've had my grep searches blow up, since I happened to find e.g. a string in >> `tzdata` and get hundreds or more of hits. :-( And I do believe we will get >> a better code structure if the build team "owns" `make`; or at least has a >> vested interest in what's in that directory. We still suffer a lot of the >> old "I don't know where to put this file, so I'll just put it in make cause >> nobody cares about it anyway" mentality, but I've been working for quite >> some time to make that list of misplaced files shorter and shorter. > > Also, to clarify, for me there is a fundamental difference between > `src/$MODULE` and `make/modules/$MODULE`. The former is the home of files > that are part of the module, owned by the content team, and the `$MODULE` > part is essential to delineate this content. The latter is owned by the build > team, and is just a convenient way to organize the build system within the > `make` directory. So it's clearly a no-no to put anything but `.gmk` files in > `make/modules/$MODULE`. The mapping and nr tables, and the *-X.java.template files in make/data/charsetmapping are used to generate source code for the java.base and jdk.charsets modules. The stdcs-$OS files configure the package and module that each charset go into. If the tables used to generate the source files are moved to src/java.base then make/modules/jdk.charsets/Gensrc.gmk will probably need a new home too. - PR: https://git.openjdk.java.net/jdk/pull/1611
Integrated: 8257194: Add 'foreign linker API' in 'jdk.incubator.foreign' module desc/summary
On Mon, 7 Dec 2020 17:38:40 GMT, Maurizio Cimadamore wrote: > This simple patch updates the jaavdoc in the module-info file of > jdk.incubator.foreign to reflect the fact that support of foreign function > calls has been added. This pull request has now been integrated. Changeset: a7080247 Author:Maurizio Cimadamore URL: https://git.openjdk.java.net/jdk/commit/a7080247 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod 8257194: Add 'foreign linker API' in 'jdk.incubator.foreign' module desc/summary Reviewed-by: jvernee, shade - PR: https://git.openjdk.java.net/jdk/pull/1671
Has it been considered to add inverse methods to collections which are in Optional?
Dear all,
The class java.util.Optional could be considered a collection with a limit
on how many values it can hold, minimum 0, maximum 1.
Likewise, any class implementing the regular java.util.Collection has a
minimum, 0 and a maximum > 1.
While Optional does not implement Collection, it could be seen as a
pseudo-collection in this regard.
Optional has a convenience method for which there is an inverse,
isPresent() which is the inverse of isEmpty().
Has it been considered to add such a method (or other convenience methods)
to collections as well?
For instance, java.util.Collection has the method isEmpty() which behaves
exactly like Optional's isEmpty(), but it does not have an inverse
counterpart.
I think it would be logical to add such as convenience method to
Collection, because we have a precedent with Optional.
We could add `bikeshed name here, as an example I'm going to use
hasElements()' to java.util.Collection and, for simplicity's sake make the
default implementation just call !isEmpty(); obviously implementations
could offer optimized versions if necessary.
It would improve readability where we have the negation of isEmpty(), e.g.:
if (!myList.isEmpty()) {
}
// vs.
if (myList.hasElements()) {
}
Note that in the first example, which is the current way of checking
whether a (non null) collection has any elements, the negation and the
method it negates are separated by the variable name.
If the variable would instead be another method call or any other, longer,
expression, this would make the example even less readable as the negation
and final method call are further apart.
The second example has the advantage of more clearly expressing the intent
of the user - which is to check if the collection has any elements.
Moreover, it offers matching functionality for something which already
exists in the JDK core libs, Optional's isPresent().
If this has already been discussed and dismissed, I'm sorry for bringing it
up again and if anybody could point me to the conclusion and the reasoning
behind it, I would be grateful.
Kind regards,
Dave Franken
Re: RFR: 8234131: Miscellaneous changes imported from jsr166 CVS 2020-12 [v2]
On 12/8/20 3:56 AM, Alan Bateman wrote: 1558: break; 1559: } 1560: */ Is this meant to be commented out? Yes, but It should be marked as a possible improvement, not yet committed. While this (prestarting) would improve performance in some scenarios, it may also disrupt expectations and even tooling in some existing usages, which we haven't fully checked out.
Re: RFR: 8254350: CompletableFuture.get may swallow InterruptedException [v2]
On Tue, 8 Dec 2020 10:35:20 GMT, Alan Bateman wrote:
>> test/jdk/java/util/concurrent/CompletableFuture/LostInterrupt.java line 49:
>>
>>> 47:
>>> 48: public static void main(String[] args) throws Exception {
>>> 49: ThreadLocalRandom rnd = ThreadLocalRandom.current();
>>
>> Hi Martin,
>>
>> Is using a `ThreadLocalRandom` important for the test logic?
>> I was wondering whether it would be better to use ( `* @library /test/lib`)
>> `jdk.test.lib.RandomFactory`,
>> which prints the random seed in the output so that you can reproduce with
>> the same pseudo-random
>> sequence in case of test failures.
>>
>> best regards,
>>
>> -- daniel
>
> RandomFactory is probably overkill here as the test just needs to exercise
> untimed and timed get. So just a random boolean rather than a wide range of
> random values.
OK.
-
PR: https://git.openjdk.java.net/jdk/pull/1651
Re: RFR: 8234131: Miscellaneous changes imported from jsr166 CVS 2020-12 [v2]
On Tue, 8 Dec 2020 08:53:38 GMT, Alan Bateman wrote: >> Martin Buchholz has refreshed the contents of this pull request, and >> previous commits have been removed. The incremental views will show >> differences compared to the previous content of the PR. > > src/java.base/share/classes/java/util/concurrent/ThreadPoolExecutor.java line > 1560: > >> 1558: break; >> 1559: } >> 1560: */ > > Is this meant to be commented out? This code was also commented out in the original CVS repo; here's the diff: http://gee.cs.oswego.edu/cgi-bin/viewcvs.cgi/jsr166/src/main/java/util/concurrent/ThreadPoolExecutor.java?r1=1.193=1.194 The message for the `1.194` revision suggests we should NOT expect code changes. I've double-checked my patch, which is at least partially responsible for the `1.194` revision, and couldn't find that commenting out part. - PR: https://git.openjdk.java.net/jdk/pull/1647
RFR: 8165276: Spec states that invoke the premain method in an agent cl…
This change have been already reviewed by Mandy, Alan and David. Now, the PR approval is needed. The push was postponed because the CSR was not approved at that time (it is now): https://bugs.openjdk.java.net/browse/JDK-8248189 Investigation of existing popular java agents was requested by Joe. -- The java.lang.instrument spec: https://docs.oracle.com/en/java/javase/15/docs/api/java.instrument/java/lang/instrument/package-summary.html Summary: The java.lang.instrument spec clearly states: "The agent class must implement a public static premain method similar in principle to the main application entry point." Current implementation of sun/instrument/InstrumentationImpl.java allows the premain method be non-public which violates the spec. This fix aligns the implementation with the spec. Testing: A mach5 run of jdk_instrument tests is in progress. - Commit messages: - JDK-8165276 Spec states that invoke the premain method in an agent class if it's public but implementation differs Changes: https://git.openjdk.java.net/jdk/pull/1694/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=1694=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8165276 Stats: 356 lines in 26 files changed: 251 ins; 56 del; 49 mod Patch: https://git.openjdk.java.net/jdk/pull/1694.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1694/head:pull/1694 PR: https://git.openjdk.java.net/jdk/pull/1694
Withdrawn: 8257885: [TESTBUG] java/foreign/TestSegments.java fails on x86_32
On Tue, 8 Dec 2020 08:59:25 GMT, Jie Fu wrote:
> Hi all,
>
> java/foreign/TestSegments.java fails on x86_32 due to '-Xmx4G'.
> The reason is that -Xmx4G is invalid maximum heap size for 32-bit platforms.
> The current implimentation only supports maximum 3800M on 32-bit systems [1].
>
> The fix just change '-Xmx4G' to '-Xmx3500M'.
>
> Testing:
> - java/foreign/TestSegments.java on Linux/x86_{32,64}
>
> Thanks.
> Best regards,
> Jie
>
> [1]
> https://github.com/openjdk/jdk/blob/master/src/hotspot/os/posix/os_posix.cpp#L567
This pull request has been closed without being integrated.
-
PR: https://git.openjdk.java.net/jdk/pull/1689
Re: RFR: 8257887: java/foreign/TestSegments.java test fails on 32-bit after JDK-8257186
On Tue, 8 Dec 2020 08:54:41 GMT, Aleksey Shipilev wrote: > See here: > https://github.com/mcimadamore/jdk/runs/1460615378 > > $ CONF=linux-x86-server-fastdebug make images run-test > TEST=java/foreign/TestSegments.java > > STDERR: > Invalid maximum heap size: -Xmx4G > The specified size exceeds the maximum representable size. > Error: Could not create the Java Virtual Machine. > Error: A fatal exception has occurred. Program will exit. > > Adding `@requires` in the same form other `java/foreign` tests have it skips > the test on 32-bit platforms. > > Additional testing: > - [x] Affected test on Linux x86_32 (skipped now) > - [x] Affected test on Linux x86_64 (still passes) The test is brittle on 32-bit platforms (see https://github.com/openjdk/jdk/pull/1689 ). I agree with @shipilev . - Marked as reviewed by jiefu (Committer). PR: https://git.openjdk.java.net/jdk/pull/1688
Re: RFR: 8257885: [TESTBUG] java/foreign/TestSegments.java fails on x86_32
On Tue, 8 Dec 2020 10:17:48 GMT, Aleksey Shipilev wrote: > > > `-Xm3800M` could still fail on many 32-bit systems, for example where > > > native libraries are located in the middle of address space. I think it > > > is safer to `@require` it. In fact, I did it an hour ago here #1688 :) > > > > > > The test just limits the maximum memory to be used, not the minimum memory. > > So I think the foreign api should also work on 32-bit platforms. > > Well, I think the test still fails with your fix: > https://github.com/DamonFool/jdk/runs/1516430124 -- as I suspected it would :( OK. It seems the test is brittle on 32-bit platforms. I agree with you now. Thanks. - PR: https://git.openjdk.java.net/jdk/pull/1689
Re: RFR: 8257639: Update usage of "type" terminology in java.lang.Enum & java.lang.Record
On Mon, 7 Dec 2020 18:22:33 GMT, Daniel Fuchs wrote: >> This change applies a stricter semantic distinction of 'type' versus 'class >> and interface'. This is based on the JLS changes described in the >> "Consistent Class and Interface Terminology" document: >> https://download.java.net/java/early_access/jdk16/docs/specs/class-terminology-jls.html. >> >> The following rules were applied: >> - 'class' and/or 'interface' are used when referring to the class/interface >> itself >> - 'type' is used when referring to the type of a variable or expression > > src/java.base/share/classes/java/lang/Enum.java line 62: > >> 60: * java.util.EnumMap map} implementations are available. >> 61: * >> 62: * @param The enum class subclass > > I wonder about this one, given that `` is a type. It sounds strange to me > - even though I understand that what is meant is that this type should point > to a class (a subclass of `java.lang.Enum`). Makes sense. I suggest the following instead: `The type of enum subclass`. - PR: https://git.openjdk.java.net/jdk/pull/1670
Re: RFR: 8254350: CompletableFuture.get may swallow InterruptedException [v2]
On Tue, 8 Dec 2020 10:19:22 GMT, Daniel Fuchs wrote:
>> Martin Buchholz has refreshed the contents of this pull request, and
>> previous commits have been removed. The incremental views will show
>> differences compared to the previous content of the PR.
>
> test/jdk/java/util/concurrent/CompletableFuture/LostInterrupt.java line 49:
>
>> 47:
>> 48: public static void main(String[] args) throws Exception {
>> 49: ThreadLocalRandom rnd = ThreadLocalRandom.current();
>
> Hi Martin,
>
> Is using a `ThreadLocalRandom` important for the test logic?
> I was wondering whether it would be better to use ( `* @library /test/lib`)
> `jdk.test.lib.RandomFactory`,
> which prints the random seed in the output so that you can reproduce with the
> same pseudo-random
> sequence in case of test failures.
>
> best regards,
>
> -- daniel
RandomFactory is probably overkill here as the test just needs to exercise
untimed and timed get. So just a random boolean rather than a wide range of
random values.
-
PR: https://git.openjdk.java.net/jdk/pull/1651
Re: RFR: 8254350: CompletableFuture.get may swallow InterruptedException [v2]
On Tue, 8 Dec 2020 06:30:28 GMT, Martin Buchholz wrote:
>> 8254350: CompletableFuture.get may swallow InterruptedException
>
> Martin Buchholz has refreshed the contents of this pull request, and previous
> commits have been removed. The incremental views will show differences
> compared to the previous content of the PR.
test/jdk/java/util/concurrent/CompletableFuture/LostInterrupt.java line 49:
> 47:
> 48: public static void main(String[] args) throws Exception {
> 49: ThreadLocalRandom rnd = ThreadLocalRandom.current();
Hi Martin,
Is using a `ThreadLocalRandom` important for the test logic?
I was wondering whether it would be better to use ( `* @library /test/lib`)
`jdk.test.lib.RandomFactory`,
which prints the random seed in the output so that you can reproduce with the
same pseudo-random
sequence in case of test failures.
best regards,
-- daniel
-
PR: https://git.openjdk.java.net/jdk/pull/1651
Re: RFR: 8257885: [TESTBUG] java/foreign/TestSegments.java fails on x86_32
On Tue, 8 Dec 2020 09:22:05 GMT, Jie Fu wrote: > > `-Xm3800M` could still fail on many 32-bit systems, for example where > > native libraries are located in the middle of address space. I think it is > > safer to `@require` it. In fact, I did it an hour ago here #1688 :) > > The test just limits the maximum memory to be used, not the minimum memory. > So I think the foreign api should also work on 32-bit platforms. Well, I think the test still fails with your fix: https://github.com/DamonFool/jdk/runs/1516430124 -- as I suspected it would :( - PR: https://git.openjdk.java.net/jdk/pull/1689
RFR: 8257876: Avoid Reference.isEnqueued in tests
Please review this change that eliminates the use of Reference.isEnqueued by tests. There were three tests using it: vmTestbase/gc/gctests/ReferencesGC/ReferencesGC.java vmTestbase/gc/gctests/WeakReferenceGC/WeakReferenceGC.java jdk/java/lang/ref/ReferenceEnqueue.java In each of them, some combination of using Reference.refersTo and ReferenceQueue.remove with a timeout were used to eliminate the use of Reference.isEnqueued. I also cleaned up ReferencesGC.java in various respects. It contained several bits of dead code, and the failure checks were made stronger. Testing: mach5 tier1 Locally (linux-x64) ran all three tests with each GC (including Shenandoah). - Commit messages: - update WeakReferenceGC test - update ReferenceQueue test - update ReferencesGC test Changes: https://git.openjdk.java.net/jdk/pull/1691/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=1691=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8257876 Stats: 102 lines in 3 files changed: 21 ins; 39 del; 42 mod Patch: https://git.openjdk.java.net/jdk/pull/1691.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1691/head:pull/1691 PR: https://git.openjdk.java.net/jdk/pull/1691
Integrated: 8242258: (jrtfs) Path::toUri throws AssertionError for malformed input
On Mon, 7 Dec 2020 16:35:52 GMT, Athijegannathan Sundararajan wrote: > Safe URI encode logic adopted from UnixUriUtils. This pull request has now been integrated. Changeset: d2b66196 Author:Athijegannathan Sundararajan URL: https://git.openjdk.java.net/jdk/commit/d2b66196 Stats: 218 lines in 2 files changed: 206 ins; 5 del; 7 mod 8242258: (jrtfs) Path::toUri throws AssertionError for malformed input Reviewed-by: alanb - PR: https://git.openjdk.java.net/jdk/pull/1669
Re: RFR: 8257887: java/foreign/TestSegments.java test fails on 32-bit after JDK-8257186
On Tue, 8 Dec 2020 08:54:41 GMT, Aleksey Shipilev wrote: > See here: > https://github.com/mcimadamore/jdk/runs/1460615378 > > $ CONF=linux-x86-server-fastdebug make images run-test > TEST=java/foreign/TestSegments.java > > STDERR: > Invalid maximum heap size: -Xmx4G > The specified size exceeds the maximum representable size. > Error: Could not create the Java Virtual Machine. > Error: A fatal exception has occurred. Program will exit. > > Adding `@requires` in the same form other `java/foreign` tests have it skips > the test on 32-bit platforms. > > Additional testing: > - [x] Affected test on Linux x86_32 (skipped now) > - [x] Affected test on Linux x86_64 (still passes) The original test doesn't seem to be designed for 64-bit platforms only. So it may not a good idea to skip it for 32-bit platforms. Thanks. - PR: https://git.openjdk.java.net/jdk/pull/1688
Re: RFR: 8256867: Classes with empty PermittedSubclasses attribute cannot be extended
On Mon, 7 Dec 2020 23:47:40 GMT, Mandy Chung wrote:
>> Please review this fix for JDK-8256867. This change no longer throws a
>> ClassFormatError exception when loading a class whose PermittedSubclasses
>> attribute is empty (contains no classes). Instead, the class is treated as
>> a sealed class which cannot be extended nor implemented. This new behavior
>> conforms to the JVM Spec.
>>
>> This change required changing Class.permittedSubclasses() to return an empty
>> array for classes with empty PermittedSubclasses attributes, and to return
>> null for non-sealed classes.
>>
>> This fix was tested with Mach5 tiers 1-2 on Linux, MacOS, and Windows, and
>> tiers 3-5 on Linux x64.
>>
>> Thanks, Harold
>
> src/java.base/share/classes/java/lang/Class.java line 4396:
>
>> 4394: * is unspecified. If this {@code Class} object represents a
>> primitive type,
>> 4395: * {@code void}, an array type, or a class or interface that is
>> not sealed,
>> 4396: * then null is returned.
>
> nit: s/null/`{@code null}`
>
> I'd suggest to clarify if this sealed class or interface has no permitted
> subclass, something like this:
> Returns an array containing {@code Class} objects representing the
> direct subinterfaces or subclasses permitted to extend or
> implement this class or interface if it is sealed. The order of such elements
> is unspecified. The array is empty if this sealed class or interface has no
> permitted subclass.
>
> `@return` needs to be revised as well:
> @return an array of {@code Class} objects of the permitted subclasses of this
> sealed class or interface,
> or {@null} if this class or interface is not sealed
Mandy's suggested wording is good.
I would like to add one more additional point of clarification. It would
be good to strongly connect `isSealed` and `getPermittedClasses` in a
first-class way in normative spec ( similar to isRecord and
getRecordComponents ).
For example,
to `isSealed` add: "getPermittedSubclasses returns a non-null but possibly
empty value for a sealed class."
to `getPermittedSubclasses`: "If this class is not a sealed class, that is
{@link
* #isSealed()} returns {@code false}, then this method returns {@code
null}.
* Conversely, if {@link #isSealed()} returns {@code true}, then this method
* returns a non-null value."
-
PR: https://git.openjdk.java.net/jdk/pull/1675
Re: RFR: 8257885: [TESTBUG] java/foreign/TestSegments.java fails on x86_32
On Tue, 8 Dec 2020 09:07:27 GMT, Aleksey Shipilev wrote: > `-Xm3800M` could still fail on many 32-bit systems, for example where native > libraries are located in the middle of address space. I think it is safer to > `@require` it. In fact, I did it an hour ago here #1688 :) The test just limits the maximum memory to be used, not the minimum memory. So I think the foreign api should also work on 32-bit platforms. - PR: https://git.openjdk.java.net/jdk/pull/1689
Re: RFR: 8257885: [TESTBUG] java/foreign/TestSegments.java fails on x86_32
On Tue, 8 Dec 2020 08:59:25 GMT, Jie Fu wrote:
> Hi all,
>
> java/foreign/TestSegments.java fails on x86_32 due to '-Xmx4G'.
> The reason is that -Xmx4G is invalid maximum heap size for 32-bit platforms.
> The current implimentation only supports maximum 3800M on 32-bit systems [1].
>
> The fix just change '-Xmx4G' to '-Xmx3500M'.
>
> Testing:
> - java/foreign/TestSegments.java on Linux/x86_{32,64}
>
> Thanks.
> Best regards,
> Jie
>
> [1]
> https://github.com/openjdk/jdk/blob/master/src/hotspot/os/posix/os_posix.cpp#L567
`-Xm3800M` could still fail on many 32-bit systems, for example where native
libraries are located in the middle of address space. I think it is safer to
`@require` it. In fact, I did it an hour ago here #1688 :)
-
PR: https://git.openjdk.java.net/jdk/pull/1689
