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. The pull request contains one new > commit since the last revision: > > JDK-8254350 Marked as reviewed by alanb (Reviewer). test/jdk/java/util/concurrent/CompletableFuture/LostInterrupt.java line 37: > 35: */ > 36: > 37: // TODO: Rewrite as a CompletableFuture tck test ? Do you want the "TODO" in the commit? - PR: https://git.openjdk.java.net/jdk/pull/1651
Re: RFR: 8242258: (jrtfs) Path::toUri throws AssertionError for malformed input [v2]
On Tue, 8 Dec 2020 03:00:34 GMT, Athijegannathan Sundararajan wrote: >> Safe URI encode logic adopted from UnixUriUtils. > > Athijegannathan Sundararajan has updated the pull request incrementally with > one additional commit since the last revision: > > renamed the test as per review comment. Marked as reviewed by alanb (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/1669
Re: RFR: 8242258: (jrtfs) Path::toUri throws AssertionError for malformed input [v2]
On Tue, 8 Dec 2020 02:55:43 GMT, Athijegannathan Sundararajan
wrote:
>> test/jdk/jdk/internal/jrtfs/Test8242258.java line 60:
>>
>>> 58: { "xyz ", "jrt:/xyz%20" },
>>> 59: { "xy z", "jrt:/xy%20z" },
>>> 60: };
>>
>> One other thing we test here is a malformed escape pair, e.g. "jrt:/%5" and
>> check that getPath(URI) throws IAE.
>
> URI.create(String) method itself checks for malformed escape pairs. Do we
> need anything additional here?
The bug report is Path -> URI but any testing here has to do round-trip (as you
have done). My comment about the malformed escaped pair is that we don't seem
to have tests for this case, or did I missed it? The example I tased was %5
which isn't a complete pair and will be rejected. In any case, I think what you
have is fine for now.
-
PR: https://git.openjdk.java.net/jdk/pull/1669
Re: RFR: 8257450: Start of release updates for JDK 17 [v2]
On Mon, 7 Dec 2020 21:14:55 GMT, Joe Darcy wrote:
>> test/jdk/java/lang/module/ClassFileVersionsTest.java line 107:
>>
>>> 105: { 61, 0, Set.of(STATIC, TRANSITIVE) },
>>> 106:
>>> 107: { 62, 0, Set.of()}, // JDK 18
>>
>> This seems unduly repetitive. Could this be dynamically generated, perhaps
>> in a future release?
>
> I've had similar thoughts; that strikes me as a fine RFE for after this fork.
> I see what the code is doing, but haven't delved into the module system
> details to understand exactly the rationale for these tests. In any case,
> filed the RFE JDK-8257856: "Make ClassFileVersionsTest.java robust to JDK
> version updates."
There was a change to JVMS 4.7.25 in Java 10 to add a rule for the
requires_flags that are allowed. This is why this test started was created to
test 53.0 vs. 54.0 class files. It wasn't intended to be a burden to update at
each release so I'll re-implement it.
-
PR: https://git.openjdk.java.net/jdk/pull/1531
Re: RFR: 8254350: CompletableFuture.get may swallow InterruptedException [v2]
> 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. The pull request contains one new commit since the last revision: JDK-8254350 - Changes: - all: https://git.openjdk.java.net/jdk/pull/1651/files - new: https://git.openjdk.java.net/jdk/pull/1651/files/4e32f8c1..762a1715 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=1651=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=1651=00-01 Stats: 0 lines in 0 files changed: 0 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/1651.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1651/head:pull/1651 PR: https://git.openjdk.java.net/jdk/pull/1651
Re: RFR: 8257450: Start of release updates for JDK 17 [v4]
> 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 10 additional commits since the last revision: - 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 - JDK-8257450 - JDK-8257450 - Changes: - all: https://git.openjdk.java.net/jdk/pull/1531/files - new: https://git.openjdk.java.net/jdk/pull/1531/files/96e75b08..342c8650 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=1531=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk=1531=02-03 Stats: 3 lines in 1 file changed: 0 ins; 3 del; 0 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: RFR: 8247373: ArraysSupport.newLength doc, test, and exception message [v2]
> 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 - Changes: - all: https://git.openjdk.java.net/jdk/pull/1617/files - new: https://git.openjdk.java.net/jdk/pull/1617/files/03b7922f..bacb5f91 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=1617=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=1617=00-01 Stats: 3 lines in 2 files changed: 2 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/1617.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1617/head:pull/1617 PR: https://git.openjdk.java.net/jdk/pull/1617
Re: MatchResult support for named groups
OK, thanks, good information. Clearly support for named groups is the most important thing missing from MatchResult. s'marks On 12/4/20 11:43 PM, Cay Horstmann wrote: Hi Stuart, 1: If there is no match at all, then results yields the empty stream. I don't think anything else is required. 2/3: I wrote a fair number of regex patterns in Java, ever since they appeared in 2002. I can say with confidence that I never once used hitEnd/requireEnd, or seen it used. I note they occur in one file in the JDK 11 source, in the Scanner class. But not in a loop that fetches all matches, but after a single call to find or lookingAt. I think these are exceedingly uncommon. In contrast, looping over matcher.find() and extracting groups is common, and named groups are a best practice (https://urldefense.com/v3/__https://eslint.org/docs/rules/prefer-named-capture-group__;!!GqivPVa7Brio!I3YBH6KonWfqm4zW7pQRatPsLcj4rRjGOveB6NWQedZVU8BeJ3hknZcPy7rC1G2fug$ ). Cheers, Cay Il 04/12/2020 19:53, Stuart Marks ha scritto: Hi Cay, Thanks for mentioning this. It's good to know that adding this provides value to people who are actually trying to use this stuff (as opposed to adding stuff merely for the sake of completeness, as often seems to arise). I've added some notes to JDK-8065554. Looking at this more closely, it seems to me that MatchResult ought to include more match-result-related information that's currently only in Matcher, namely: 1. whether there was a match at all 2. hitEnd 3. requireEnd If you have any thoughts on these, please let me know. s'marks On 12/2/20 2:53 AM, Cay Horstmann wrote: Hello, I'd like to raise awareness for https://bugs.openjdk.java.net/browse/JDK-8180352 https://bugs.openjdk.java.net/browse/JDK-8072984 https://bugs.openjdk.java.net/browse/JDK-8065554 These all ask for MatchResult.group(String name). What they don't mention is that this is more urgent in light of the methods Stream Matcher.results() // https://bugs.openjdk.java.net/browse/JDK-8071479 Stream Scanner.findAll(Pattern pattern) // https://bugs.openjdk.java.net/browse/JDK-8072722 In particular, Matcher.results() seems a cleaner way of collecting match results than calling while (matcher.find()). But then MatchResult needs to support the same queries that Matcher provides. I believe the only missing one is group(String name). Cheers, Cay NB. There are related requests that ask for finding group names in patterns, or for correlating group names and numbers. I have formed no opinion on their merits.
Re: RFR: 8234131: Miscellaneous changes imported from jsr166 CVS 2020-12 [v2]
> 8234131: Miscellaneous changes imported from jsr166 CVS 2020-12 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. The pull request contains one new commit since the last revision: JDK-8234131 - Changes: - all: https://git.openjdk.java.net/jdk/pull/1647/files - new: https://git.openjdk.java.net/jdk/pull/1647/files/ed43b3fe..a6d85863 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=1647=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=1647=00-01 Stats: 0 lines in 0 files changed: 0 ins; 0 del; 0 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: 8246585: ForkJoin updates [v2]
> 8246585: ForkJoin updates 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. The pull request contains one new commit since the last revision: JDK-8246585 - Changes: - all: https://git.openjdk.java.net/jdk/pull/1646/files - new: https://git.openjdk.java.net/jdk/pull/1646/files/0ae46847..d025f68b Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=1646=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=1646=00-01 Stats: 5 lines in 1 file changed: 0 ins; 0 del; 5 mod Patch: https://git.openjdk.java.net/jdk/pull/1646.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1646/head:pull/1646 PR: https://git.openjdk.java.net/jdk/pull/1646
Re: RFR: 8246677: LinkedTransferQueue and SynchronousQueue synchronization updates [v2]
> 8246677: LinkedTransferQueue and SynchronousQueue synchronization updates 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. The pull request contains one new commit since the last revision: JDK-8246677 - Changes: - all: https://git.openjdk.java.net/jdk/pull/1645/files - new: https://git.openjdk.java.net/jdk/pull/1645/files/13eb0fa8..b8baa921 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=1645=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=1645=00-01 Stats: 0 lines in 0 files changed: 0 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/1645.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1645/head:pull/1645 PR: https://git.openjdk.java.net/jdk/pull/1645
Re: RFR: 8255917: runtime/cds/SharedBaseAddress.java failed "assert(reserved_rgn != 0LL) failed: No reserved region" [v3]
> Hi, Please review > Windows mapping for file into memory could not happen to reserved memory. > In mapping CDS archive we first reserve enough memory then before mapping, > release them. For cds archive and using class space, need split the whole > space into two, that is, release the whole reserved space and do reservation > to the two split spaces again, which is problematic that there is possibility > other thread or system can kick in to take the released space. > The fix is the first step of two steps: >1) Do not split reserved memory; >2) Remove splitting memory. >This fix is first step, for Windows and use requested mapping address, > reserved for cds archive and ccs on a contiguous space separately, so there > is no need to call split. If any reservation failed, release them, go to > other way, but do not do the 'real' split either. For Windows (and using > class space), the reserved space will be released anyway. > > Tests:tier1-5,tier7 Yumin Qi has updated the pull request incrementally with 32 additional commits since the last revision: - Add total_space_rs, total reserved space to release_reserved_spaces and reserve_address_space_for_archives, made changes to check failed output on test. - 8253762: JFR: getField(String) should be able to access subfields Reviewed-by: mgronlun - 8257670: sun/security/ssl/SSLSocketImpl/SSLSocketLeak.java reports leaks Reviewed-by: jnimeh - 8257796: [TESTBUG] TestUseSHA512IntrinsicsOptionOnSupportedCPU.java fails on x86_32 Reviewed-by: kvn - 8257211: C2: Enable call devirtualization during post-parse phase Reviewed-by: kvn, neliasso, thartmann - 8257572: Deprecate the archaic signal-chaining interfaces: sigset and signal Reviewed-by: ihse, alanb, dcubed, erikj - 8257718: LogCompilation: late_inline doesnt work right for JDK 8 logs Reviewed-by: redestad, kvn - 8257799: Update JLS cross-references in java.compiler Reviewed-by: jjg - 8254939: macOS: unused function 'replicate4_imm' Reviewed-by: redestad, thartmann - 8257805: Add compiler/blackhole tests to tier1 Reviewed-by: kvn - ... and 22 more: https://git.openjdk.java.net/jdk/compare/dd9ae050...f7958306 - Changes: - all: https://git.openjdk.java.net/jdk/pull/1657/files - new: https://git.openjdk.java.net/jdk/pull/1657/files/dd9ae050..f7958306 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=1657=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk=1657=01-02 Stats: 8052 lines in 156 files changed: 4548 ins; 2755 del; 749 mod Patch: https://git.openjdk.java.net/jdk/pull/1657.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1657/head:pull/1657 PR: https://git.openjdk.java.net/jdk/pull/1657
A Bug involving MethodHandles, Nestmates, Reflection and @CallerSensitive
Let's start with the reproducer:
public class NestmateBug {
private static int field = 0;
public static void main(String[] args) throws Throwable {
Lookup l = MethodHandles.lookup();
Field f = NestmateBug.class.getDeclaredField("field");
MethodHandle mh = l.findVirtual(Field.class, "setInt",
methodType(void.class, Object.class, int.class));
int newValue = 5;
mh.invokeExact(f, (Object) null, newValue);
}
}
This throws a IAE in the last line:
class test.se15.NestmateBug$$InjectedInvoker/0x000800bb5840 (in
module test.se15) cannot access a member of class test.se15.NestmateBug
(in module test.se15) with modifiers "private static"
at
java.base/jdk.internal.reflect.Reflection.newIllegalAccessException(Reflection.java:385)
at
java.base/java.lang.reflect.AccessibleObject.checkAccess(AccessibleObject.java:693)
at java.base/java.lang.reflect.Field.checkAccess(Field.java:1096)
at java.base/java.lang.reflect.Field.setInt(Field.java:976)
at test.se15/test.se15.NestmateBug.main(NestmateBug.java:17)
The reason for this behaviour is:
* Field.setInt (without setAccessible) requires that the caller class is
in the same nest for private members.
* Field.setInt is @CallerSensitive
* MethodHandles will bind to the caller by injecting an invoker for
@CallerSensitive methods.
* The injected invoker is NOT a nestmate of the original lookup class.
* The access check of Field.setInt fails - as the injected invoker is
now the caller.
This is important because:
* Some old software loves to set static final fields through reflection.
* To do that, it usually hacks into Field.modifiers. Which is filtered
iirc since Java 12.
* I write Java agents to fix such things - removing final from static
fields, and intercept Class.getDeclaredField and Field.setInt by
replacing it with an invokedynamic instruction.
* The original target is passed as a MethodHandle bootstrap argument. In
the case of Field.setInt, it is guarded with a
MethodHandles.guardWithTest() - calling the original target if the Field
is not my canary.
Suggested fix:
* Make the injected invoker a nestmate of lookup class.
I did prepare a fix for that [1], but AFAIK the bug first needs to be
filled in the bug tracker.
And I don't have an account.
- Johannes
[1]:
https://github.com/DasBrain/jdk/commit/3c4bb20c8e4cd9086e934128e5cb085a5cfbdb94
RFR: 8254350: CompletableFuture.get may swallow InterruptedException
8254350: CompletableFuture.get may swallow InterruptedException - Commit messages: - JDK-8254350 Changes: https://git.openjdk.java.net/jdk/pull/1651/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=1651=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8254350 Stats: 178 lines in 3 files changed: 170 ins; 5 del; 3 mod Patch: https://git.openjdk.java.net/jdk/pull/1651.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1651/head:pull/1651 PR: https://git.openjdk.java.net/jdk/pull/1651
RFR: 8246585: ForkJoin updates
8246585: ForkJoin updates - Commit messages: - JDK-8246585 Changes: https://git.openjdk.java.net/jdk/pull/1646/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=1646=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8246585 Stats: 3114 lines in 6 files changed: 1135 ins; 890 del; 1089 mod Patch: https://git.openjdk.java.net/jdk/pull/1646.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1646/head:pull/1646 PR: https://git.openjdk.java.net/jdk/pull/1646
Re: RFR: 8052260: Reference.isEnqueued() spec does not match the long-standing behavior returning true iff it's in the ref queue
On Tue, 8 Dec 2020 05:01:08 GMT, Kim Barrett 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 > > This change looks good. There are a couple of tests that are using > isEnqueued. > vmTestbase/gc/gctests/ReferencesGC/ReferencesGC.java > jdk/java/lang/ref/ReferenceEnqueue.java > https://bugs.openjdk.java.net/browse/JDK-8257876 > (I'm working on this.) Thanks, Kim. It's good to update the tests (thanks for working on it). - PR: https://git.openjdk.java.net/jdk/pull/1684
RFR: 8246677: LinkedTransferQueue and SynchronousQueue synchronization updates
8246677: LinkedTransferQueue and SynchronousQueue synchronization updates - Commit messages: - JDK-8246677 Changes: https://git.openjdk.java.net/jdk/pull/1645/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=1645=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8246677 Stats: 538 lines in 3 files changed: 143 ins; 294 del; 101 mod Patch: https://git.openjdk.java.net/jdk/pull/1645.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1645/head:pull/1645 PR: https://git.openjdk.java.net/jdk/pull/1645
RFR: 8234131: Miscellaneous changes imported from jsr166 CVS 2020-12
8234131: Miscellaneous changes imported from jsr166 CVS 2020-12 - Commit messages: - JDK-8234131 Changes: https://git.openjdk.java.net/jdk/pull/1647/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=1647=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8234131 Stats: 314 lines in 41 files changed: 107 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: 8052260: Reference.isEnqueued() spec does not match the long-standing behavior returning true iff it's in the ref queue
On Tue, 8 Dec 2020 02:36:01 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 This change looks good. There are a couple of tests that are using isEnqueued. vmTestbase/gc/gctests/ReferencesGC/ReferencesGC.java jdk/java/lang/ref/ReferenceEnqueue.java https://bugs.openjdk.java.net/browse/JDK-8257876 (I'm working on this.) - Marked as reviewed by kbarrett (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/1684
Re: A Bug involving MethodHandles, Nestmates, Reflection and @CallerSensitive
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
On 12/7/20 8:13 PM, David Holmes wrote:
Hi Johannes,
I've filed this bug report for you:
https://bugs.openjdk.java.net/browse/JDK-8257874
Thanks,
David
On 8/12/2020 11:20 am, Johannes Kuhn wrote:
Let's start with the reproducer:
public class NestmateBug {
private static int field = 0;
public static void main(String[] args) throws Throwable {
Lookup l = MethodHandles.lookup();
Field f = NestmateBug.class.getDeclaredField("field");
MethodHandle mh = l.findVirtual(Field.class, "setInt",
methodType(void.class, Object.class, int.class));
int newValue = 5;
mh.invokeExact(f, (Object) null, newValue);
}
}
This throws a IAE in the last line:
class test.se15.NestmateBug$$InjectedInvoker/0x000800bb5840 (in
module test.se15) cannot access a member of class
test.se15.NestmateBug (in module test.se15) with modifiers "private
static"
at
java.base/jdk.internal.reflect.Reflection.newIllegalAccessException(Reflection.java:385)
at
java.base/java.lang.reflect.AccessibleObject.checkAccess(AccessibleObject.java:693)
at java.base/java.lang.reflect.Field.checkAccess(Field.java:1096)
at java.base/java.lang.reflect.Field.setInt(Field.java:976)
at test.se15/test.se15.NestmateBug.main(NestmateBug.java:17)
The reason for this behaviour is:
* Field.setInt (without setAccessible) requires that the caller class
is in the same nest for private members.
* Field.setInt is @CallerSensitive
* MethodHandles will bind to the caller by injecting an invoker for
@CallerSensitive methods.
* The injected invoker is NOT a nestmate of the original lookup class.
* The access check of Field.setInt fails - as the injected invoker is
now the caller.
This is important because:
* Some old software loves to set static final fields through reflection.
* To do that, it usually hacks into Field.modifiers. Which is
filtered iirc since Java 12.
* I write Java agents to fix such things - removing final from static
fields, and intercept Class.getDeclaredField and Field.setInt by
replacing it with an invokedynamic instruction.
* The original target is passed as a MethodHandle bootstrap argument.
In the case of Field.setInt, it is guarded with a
MethodHandles.guardWithTest() - calling the original target if the
Field is not my canary.
Suggested fix:
* Make the injected invoker a nestmate of lookup class.
I did prepare a fix for that [1], but AFAIK the bug first needs to be
filled in the bug tracker.
And I don't have an account.
- Johannes
[1]:
https://github.com/DasBrain/jdk/commit/3c4bb20c8e4cd9086e934128e5cb085a5cfbdb94
Re: A Bug involving MethodHandles, Nestmates, Reflection and @CallerSensitive
Hi Johannes,
I've filed this bug report for you:
https://bugs.openjdk.java.net/browse/JDK-8257874
Thanks,
David
On 8/12/2020 11:20 am, Johannes Kuhn wrote:
Let's start with the reproducer:
public class NestmateBug {
private static int field = 0;
public static void main(String[] args) throws Throwable {
Lookup l = MethodHandles.lookup();
Field f = NestmateBug.class.getDeclaredField("field");
MethodHandle mh = l.findVirtual(Field.class, "setInt",
methodType(void.class, Object.class, int.class));
int newValue = 5;
mh.invokeExact(f, (Object) null, newValue);
}
}
This throws a IAE in the last line:
class test.se15.NestmateBug$$InjectedInvoker/0x000800bb5840 (in
module test.se15) cannot access a member of class test.se15.NestmateBug
(in module test.se15) with modifiers "private static"
at
java.base/jdk.internal.reflect.Reflection.newIllegalAccessException(Reflection.java:385)
at
java.base/java.lang.reflect.AccessibleObject.checkAccess(AccessibleObject.java:693)
at java.base/java.lang.reflect.Field.checkAccess(Field.java:1096)
at java.base/java.lang.reflect.Field.setInt(Field.java:976)
at test.se15/test.se15.NestmateBug.main(NestmateBug.java:17)
The reason for this behaviour is:
* Field.setInt (without setAccessible) requires that the caller class is
in the same nest for private members.
* Field.setInt is @CallerSensitive
* MethodHandles will bind to the caller by injecting an invoker for
@CallerSensitive methods.
* The injected invoker is NOT a nestmate of the original lookup class.
* The access check of Field.setInt fails - as the injected invoker is
now the caller.
This is important because:
* Some old software loves to set static final fields through reflection.
* To do that, it usually hacks into Field.modifiers. Which is filtered
iirc since Java 12.
* I write Java agents to fix such things - removing final from static
fields, and intercept Class.getDeclaredField and Field.setInt by
replacing it with an invokedynamic instruction.
* The original target is passed as a MethodHandle bootstrap argument. In
the case of Field.setInt, it is guarded with a
MethodHandles.guardWithTest() - calling the original target if the Field
is not my canary.
Suggested fix:
* Make the injected invoker a nestmate of lookup class.
I did prepare a fix for that [1], but AFAIK the bug first needs to be
filled in the bug tracker.
And I don't have an account.
- Johannes
[1]:
https://github.com/DasBrain/jdk/commit/3c4bb20c8e4cd9086e934128e5cb085a5cfbdb94
Re: RFR: 8257450: Start of release updates for JDK 17 [v3]
> 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 ten additional commits since the last revision: - 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 - JDK-8257450 - JDK-8257450 - Changes: - all: https://git.openjdk.java.net/jdk/pull/1531/files - new: https://git.openjdk.java.net/jdk/pull/1531/files/f6a64473..96e75b08 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=1531=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk=1531=01-02 Stats: 851 lines in 29 files changed: 560 ins; 163 del; 128 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: RFR: 8251989: Hex formatting and parsing utility [v17]
On Fri, 4 Dec 2020 22:13:21 GMT, Roger Riggs wrote:
>> src/java.base/share/classes/java/util/HexFormat.java line 936:
>>
>>> 934: * if any of the characters is not a hexadecimal character
>>> 935: */
>>> 936: public int fromHexDigits(CharSequence string) {
>>
>> I recommend this group of methods include an apinote explaining the
>> differences in behavior of compared to parseInt(s, 16) and
>> parseUnsignedInt(s, 16).
>
> Will add:
> * @apiNote
> * {@link Integer#parseInt(String, int) Integer.parseInt(s, 16)} and
> * {@link Integer#parseUnsignedInt(String, int)
> Integer.pareUnsignedInt(s, 16)}
> * are similar but allow all Unicode hexadecimal digits allowed by
> * {@link Character#digit(char, int) Character.digit(ch, 16)}.
> * {@code HexFormat} uses only Latin1 hexadecimal characters "0-9, "A-F",
> and "a-f".
> * {@link Integer#parseInt(String, int)} can parse signed hexadecimal
> strings.
> And similar text for Long#parseLong and Long.parseUnsignedLong
Okay; however, I suggest saying more on the signed/unsigned behavior.
-
PR: https://git.openjdk.java.net/jdk/pull/482
Re: RFR: JDK-8257539: tools/jpackage/windows/WinL10nTest.java unpack.bat failed with Exit code: 1618
On Mon, 7 Dec 2020 20:29:56 GMT, Andy Herrick wrote: > increase retries and timeout increasing in runMsiexecWithRetries 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? - PR: https://git.openjdk.java.net/jdk/pull/1676
Re: RFR: 8242258: (jrtfs) Path::toUri throws AssertionError for malformed input [v2]
On Mon, 7 Dec 2020 19:59:40 GMT, Alan Bateman wrote:
>> Athijegannathan Sundararajan has updated the pull request incrementally with
>> one additional commit since the last revision:
>>
>> renamed the test as per review comment.
>
> test/jdk/jdk/internal/jrtfs/Test8242258.java line 40:
>
>> 38: import static org.testng.Assert.assertEquals;
>> 39:
>> 40: public class Test8242258 {
>
> I think it would be better to create something like UriTests that we can add
> further tests for jrtfs URIs as they arise.
I'll renamed the test
> test/jdk/jdk/internal/jrtfs/Test8242258.java line 60:
>
>> 58: { "xyz ", "jrt:/xyz%20" },
>> 59: { "xy z", "jrt:/xy%20z" },
>> 60: };
>
> One other thing we test here is a malformed escape pair, e.g. "jrt:/%5" and
> check that getPath(URI) throws IAE.
URI.create(String) method itself checks for malformed escape pairs. Do we need
anything additional here?
-
PR: https://git.openjdk.java.net/jdk/pull/1669
Re: RFR: 8242258: (jrtfs) Path::toUri throws AssertionError for malformed input [v2]
> Safe URI encode logic adopted from UnixUriUtils. Athijegannathan Sundararajan has updated the pull request incrementally with one additional commit since the last revision: renamed the test as per review comment. - Changes: - all: https://git.openjdk.java.net/jdk/pull/1669/files - new: https://git.openjdk.java.net/jdk/pull/1669/files/b3d0a927..4d7417f8 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=1669=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=1669=00-01 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/1669.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1669/head:pull/1669 PR: https://git.openjdk.java.net/jdk/pull/1669
Re: RFR: 8257733: Move module-specific data from make to respective module [v2]
On Mon, 7 Dec 2020 19:31:59 GMT, Jonathan Gibbons wrote:
>> Magnus Ihse Bursie has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Move to share/data, and move jdwp.spec to java.se
>
> 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?
-
PR: https://git.openjdk.java.net/jdk/pull/1611
RFR: 8052260: Reference.isEnqueued() spec does not match the long-standing behavior returning true iff it's in the ref queue
`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 - Commit messages: - add suppress warnings - 8052260: Reference.isEnqueued() spec does not match the long-standing behavior returning true iff it's in the ref queue Changes: https://git.openjdk.java.net/jdk/pull/1684/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=1684=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8052260 Stats: 28 lines in 2 files changed: 22 ins; 0 del; 6 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: 8247373: ArraysSupport.newLength doc, test, and exception message
On Mon, 7 Dec 2020 16:05:11 GMT, Roger Riggs wrote: >> The origin of this code is in collections like ArrayList that have an >> existing array (hence oldLength >= 0) and that need it to grow (hence >> minGrowth > 0). Those were the prevailing assumptions in the code from which >> this was derived, so they turn into preconditions here. I don't see the need >> to try to make this handle any more cases than it currently does. Doing so >> complicates the analysis and possibly the code as well. Certainly a bug in a >> caller might be difficult to track down, but I don't want to add argument >> checking or to provide "reasonable" behavior in the face of unreasonable >> inputs. This is an internal method; bugs in callers should be fixed. > > 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. - PR: https://git.openjdk.java.net/jdk/pull/1617
Re: RFR: 8256867: Classes with empty PermittedSubclasses attribute cannot be extended
On Mon, 7 Dec 2020 21:29:23 GMT, Lois Foltan 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/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` - PR: https://git.openjdk.java.net/jdk/pull/1675
Re: RFR: 8256867: Classes with empty PermittedSubclasses attribute cannot be extended
On Mon, 7 Dec 2020 19:51:38 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 Other changes look okay except the spec of `Class::getPermittedSubclasses` - PR: https://git.openjdk.java.net/jdk/pull/1675
Re: RFR: 8200102: NativeLibraryTest.java fails intermittently, unloaded count is not same as expected [v3]
On Mon, 7 Dec 2020 23:52:29 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. > > Brent Christian 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 five additional > commits since the last revision: > > - Merge branch 'master' into 8200102 > - Copyright date and indentation cleanup > - Merge branch 'master' into 8200102 > - Use existing ForceGC class in the test lib > - Add gcAwait() mechanism Marked as reviewed by mchung (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/1630
Re: RFR: 8200102: NativeLibraryTest.java fails intermittently, unloaded count is not same as expected [v3]
On Mon, 7 Dec 2020 23:52:29 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. > > Brent Christian 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 five additional > commits since the last revision: > > - Merge branch 'master' into 8200102 > - Copyright date and indentation cleanup > - Merge branch 'master' into 8200102 > - Use existing ForceGC class in the test lib > - Add gcAwait() mechanism Looks good! - Marked as reviewed by naoto (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/1630
Re: RFR: 8256867: Classes with empty PermittedSubclasses attribute cannot be extended
On Mon, 7 Dec 2020 19:51:38 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
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
-
PR: https://git.openjdk.java.net/jdk/pull/1675
Re: RFR: 8200102: NativeLibraryTest.java fails intermittently, unloaded count is not same as expected [v3]
> 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. Brent Christian 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 five additional commits since the last revision: - Merge branch 'master' into 8200102 - Copyright date and indentation cleanup - Merge branch 'master' into 8200102 - Use existing ForceGC class in the test lib - Add gcAwait() mechanism - Changes: - all: https://git.openjdk.java.net/jdk/pull/1630/files - new: https://git.openjdk.java.net/jdk/pull/1630/files/a6d58148..91814d19 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=1630=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk=1630=01-02 Stats: 804 lines in 24 files changed: 519 ins; 160 del; 125 mod Patch: https://git.openjdk.java.net/jdk/pull/1630.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1630/head:pull/1630 PR: https://git.openjdk.java.net/jdk/pull/1630
Re: RFR: 8200102: NativeLibraryTest.java fails intermittently, unloaded count is not same as expected [v2]
On Mon, 7 Dec 2020 20:39:34 GMT, Naoto Sato wrote:
>> Brent Christian 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 three additional
>> commits since the last revision:
>>
>> - Merge branch 'master' into 8200102
>> - Use existing ForceGC class in the test lib
>> - Add gcAwait() mechanism
>
> test/jdk/java/lang/ClassLoader/nativeLibrary/NativeLibraryTest.java line 72:
>
>> 70: if (!gc.await(() -> finalCount == unloadedCount)) {
>> 71: throw new RuntimeException("Expected unloaded=" + count +
>> 72: " but got=" + unloadedCount);
>
> Could be left unchanged.
True
-
PR: https://git.openjdk.java.net/jdk/pull/1630
Re: RFR: 8200102: NativeLibraryTest.java fails intermittently, unloaded count is not same as expected [v2]
On Mon, 7 Dec 2020 20:38:31 GMT, Naoto Sato wrote: >> Brent Christian 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 three additional >> commits since the last revision: >> >> - Merge branch 'master' into 8200102 >> - Use existing ForceGC class in the test lib >> - Add gcAwait() mechanism > > test/jdk/java/lang/ClassLoader/nativeLibrary/NativeLibraryTest.java line 2: > >> 1: /* >> 2: * Copyright (c) 2020, Oracle and/or its affiliates. All rights reserved. > > Missin 2017? Yes, you're right - PR: https://git.openjdk.java.net/jdk/pull/1630
Re: RFR: 8257450: Start of release updates for JDK 17 [v2]
On Mon, 7 Dec 2020 20:20:58 GMT, Jesper Wilhelmsson wrote: >> 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 eight additional commits >> since the last revision: >> >> - 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 >> - JDK-8257450 >> - JDK-8257450 > > src/java.compiler/share/classes/javax/lang/model/SourceVersion.java line 234: > >> 232: * @since 17 >> 233: */ >> 234: RELEASE_17; > > Would it make sense to have a RELEASE_LATEST for the cases that are just > updated to the latest release every six months? That kind of design was considered and rejected with the API was initially added. The use of enum constants in annotations must be an actual enum constant, not just a static final field pointing to a particular enum value. It would be possible to conceptually alias RELEASE_LATEST with whatever actual constant was the latest (16, then 17, then 18...), but that would cause issues with other uses of the API. - PR: https://git.openjdk.java.net/jdk/pull/1531
Re: RFR: 8256867: Classes with empty PermittedSubclasses attribute cannot be extended
On Mon, 7 Dec 2020 21:30:18 GMT, Lois Foltan 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 > > Changes looks good Harold. > Lois Hi Harold, > There is no CSR because I viewed this as a bug fix / spec conformance issue. > But, I can write a CSR if you think one is needed. This spec change needs a CSR since the return value is changed if this class is not sealed. Please create one. - PR: https://git.openjdk.java.net/jdk/pull/1675
Re: RFR: 8256867: Classes with empty PermittedSubclasses attribute cannot be extended
On Mon, 7 Dec 2020 21:18:45 GMT, Joe Darcy 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 > > Is there already CSR coverage of this change? There is no CSR because I viewed this as a bug fix / spec conformance issue. But, I can write a CSR if you think one is needed. - PR: https://git.openjdk.java.net/jdk/pull/1675
Re: RFR: 8256867: Classes with empty PermittedSubclasses attribute cannot be extended
On Mon, 7 Dec 2020 19:51:38 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 Changes looks good Harold. Lois 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(); - Marked as reviewed by lfoltan (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/1675
Re: RFR: 8256867: Classes with empty PermittedSubclasses attribute cannot be extended
On Mon, 7 Dec 2020 19:51:38 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 Is there already CSR coverage of this change? - PR: https://git.openjdk.java.net/jdk/pull/1675
Re: RFR: 8257450: Start of release updates for JDK 17 [v2]
On Mon, 7 Dec 2020 20:08:59 GMT, Jonathan Gibbons wrote:
>> 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 eight additional commits
>> since the last revision:
>>
>> - 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
>> - JDK-8257450
>> - JDK-8257450
>
> test/jdk/java/lang/module/ClassFileVersionsTest.java line 107:
>
>> 105: { 61, 0, Set.of(STATIC, TRANSITIVE) },
>> 106:
>> 107: { 62, 0, Set.of()}, // JDK 18
>
> This seems unduly repetitive. Could this be dynamically generated, perhaps in
> a future release?
I've had similar thoughts; that strikes me as a fine RFE for after this fork. I
see what the code is doing, but haven't delved into the module system details
to understand exactly the rationale for these tests. In any case, filed the RFE
JDK-8257856: "Make ClassFileVersionsTest.java robust to JDK version updates."
-
PR: https://git.openjdk.java.net/jdk/pull/1531
Re: RFR: 8200102: NativeLibraryTest.java fails intermittently, unloaded count is not same as expected [v2]
On Mon, 7 Dec 2020 19:30:28 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.
>
> Brent Christian 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 three additional
> commits since the last revision:
>
> - Merge branch 'master' into 8200102
> - Use existing ForceGC class in the test lib
> - Add gcAwait() mechanism
Looks good to me (with trivial nits).
test/jdk/java/lang/ClassLoader/nativeLibrary/NativeLibraryTest.java line 2:
> 1: /*
> 2: * Copyright (c) 2020, Oracle and/or its affiliates. All rights reserved.
Missin 2017?
test/jdk/java/lang/ClassLoader/nativeLibrary/NativeLibraryTest.java line 72:
> 70: if (!gc.await(() -> finalCount == unloadedCount)) {
> 71: throw new RuntimeException("Expected unloaded=" + count +
> 72: " but got=" + unloadedCount);
Could be left unchanged.
-
Marked as reviewed by naoto (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/1630
RFR: JDK-8257539: tools/jpackage/windows/WinL10nTest.java unpack.bat failed with Exit code: 1618
increase retries and timeout increasing in runMsiexecWithRetries - Commit messages: - JDK-8257539: tools/jpackage/windows/WinL10nTest.java unpack.bat failed with Exit code: 1618 Changes: https://git.openjdk.java.net/jdk/pull/1676/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=1676=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8257539 Stats: 3 lines in 1 file changed: 1 ins; 0 del; 2 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
RFR: 6882207: Convert javap to use diamond operator internally
6882207: Convert javap to use diamond operator internally - Commit messages: - 6882207: Convert javap to use diamond operator internally Changes: https://git.openjdk.java.net/jdk/pull/1677/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=1677=00 Issue: https://bugs.openjdk.java.net/browse/JDK-6882207 Stats: 7 lines in 3 files changed: 0 ins; 0 del; 7 mod Patch: https://git.openjdk.java.net/jdk/pull/1677.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1677/head:pull/1677 PR: https://git.openjdk.java.net/jdk/pull/1677
Re: RFR: 8257450: Start of release updates for JDK 17 [v2]
On Mon, 7 Dec 2020 19:38:41 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 eight additional commits since > the last revision: > > - 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 > - JDK-8257450 > - JDK-8257450 Marked as reviewed by jwilhelm (Reviewer). src/java.compiler/share/classes/javax/lang/model/SourceVersion.java line 234: > 232: * @since 17 > 233: */ > 234: RELEASE_17; Would it make sense to have a RELEASE_LATEST for the cases that are just updated to the latest release every six months? - PR: https://git.openjdk.java.net/jdk/pull/1531
Re: RFR: 8257450: Start of release updates for JDK 17 [v2]
On Mon, 7 Dec 2020 19:38:41 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 eight additional commits since > the last revision: > > - 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 > - JDK-8257450 > - JDK-8257450 Marked as reviewed by jlahoda (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/1531
Re: RFR: 8257450: Start of release updates for JDK 17 [v2]
On Mon, 7 Dec 2020 19:38:41 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 eight additional commits since
> the last revision:
>
> - 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
> - JDK-8257450
> - JDK-8257450
I've read all the files, and approve all the langtools related ones (i.e. not
hotspot)
As you've noticed elsewhere, there's a pending conflict with Magnus' work to
move files around.
src/jdk.compiler/share/classes/com/sun/tools/javac/code/Source.java line 108:
> 106:
> 107: /**
> 108: * 16, tbd
The "tbd" can presumably be filled in now
test/jdk/java/lang/module/ClassFileVersionsTest.java line 107:
> 105: { 61, 0, Set.of(STATIC, TRANSITIVE) },
> 106:
> 107: { 62, 0, Set.of()}, // JDK 18
This seems unduly repetitive. Could this be dynamically generated, perhaps in a
future release?
test/langtools/tools/javac/preview/classReaderTest/Client.preview.out line 1:
> 1: - compiler.warn.preview.feature.use.classfile: Bar.class, 17
Is this a test can could be automated? (i.e. no manual change per release?)
test/langtools/tools/javac/versions/Versions.java line 26:
> 24: /*
> 25: * @test
> 26: * @bug 4981566 5028634 5094412 6304984 7025786 7025789 8001112 8028545
> 8000961 8030610 8028546 8188870 8173382 8173382 8193290 8205619 8028563
> 8245147 8245586 8257453
long lines are annoying
test/langtools/tools/javac/versions/Versions.java line 297:
> 295:expectedPass(args, List.of("New7.java", "New8.java",
> "New10.java", "New11.java",
> 296: "New14.java", "New15.java"));
> 297: expectedFail(args, List.of("New16.java"));
(minor) looks like bad indentation
-
Marked as reviewed by jjg (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/1531
Re: RFR: 8166026: Refactor java/lang shell tests to java [v3]
On Mon, 7 Dec 2020 15:46:11 GMT, Roger Riggs wrote:
>> Ivan Šipka has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> 8166026: Refactor java/lang shell tests to java
>
> test/jdk/java/lang/Thread/UncaughtExceptionsTest.java line 68:
>
>> 66: @Test(dataProvider = "testCases")
>> 67: public void test(String className, int exitValue, String
>> stdOutMatch, String stdErrMatch) throws Throwable {
>> 68: ProcessBuilder processBuilder =
>> ProcessTools.createJavaProcessBuilder(String.format("Seppuku$%s",className));
>
> The class renaming looks incomplete. Here and in the error texts above.
> Seppuku does not look right as the host class for the test, they are nested
> classes of UncaughtExceptionsTest.
Indeed. Running tests was succeeding because old class files were in place. So
running tests is insufficient, one should run them with a pristine directory
every time.
-
PR: https://git.openjdk.java.net/jdk/pull/1578
Re: RFR: 8166026: Refactor java/lang shell tests to java [v4]
> Refactor `test/jdk/java/lang/Thread/UncaughtExceptions.sh` as java test. Ivan Šipka has updated the pull request incrementally with two additional commits since the last revision: - 8166026: Refactor java/lang shell tests to java - 8166026: Refactor java/lang shell tests to java - Changes: - all: https://git.openjdk.java.net/jdk/pull/1578/files - new: https://git.openjdk.java.net/jdk/pull/1578/files/05e3dd2f..c94ef2c3 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=1578=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk=1578=02-03 Stats: 4 lines in 1 file changed: 0 ins; 0 del; 4 mod Patch: https://git.openjdk.java.net/jdk/pull/1578.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1578/head:pull/1578 PR: https://git.openjdk.java.net/jdk/pull/1578
Re: RFR: 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.
test/jdk/jdk/internal/jrtfs/Test8242258.java line 40:
> 38: import static org.testng.Assert.assertEquals;
> 39:
> 40: public class Test8242258 {
I think it would be better to create something like UriTests that we can add
further tests for jrtfs URIs as they arise.
test/jdk/jdk/internal/jrtfs/Test8242258.java line 60:
> 58: { "xyz ", "jrt:/xyz%20" },
> 59: { "xy z", "jrt:/xy%20z" },
> 60: };
One other thing we test here is a malformed escape pair, e.g. "jrt:/%5" and
check that getPath(URI) throws IAE.
-
PR: https://git.openjdk.java.net/jdk/pull/1669
Re: RFR: 8200102: NativeLibraryTest.java fails intermittently, unloaded count is not same as expected [v2]
On Mon, 7 Dec 2020 19:30:28 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. > > Brent Christian 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 three additional > commits since the last revision: > > - Merge branch 'master' into 8200102 > - Use existing ForceGC class in the test lib > - Add gcAwait() mechanism Marked as reviewed by mchung (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/1630
Re: RFR: 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. Marked as reviewed by jvernee (Committer). - PR: https://git.openjdk.java.net/jdk/pull/1671
RFR: 8256867: Classes with empty PermittedSubclasses attribute cannot be extended
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 - Commit messages: - 8256867: Classes with empty PermittedSubclasses attribute cannot be extended Changes: https://git.openjdk.java.net/jdk/pull/1675/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=1675=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8256867 Stats: 156 lines in 8 files changed: 100 ins; 13 del; 43 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: 8257450: Start of release updates for JDK 17 [v2]
On Mon, 7 Dec 2020 16:10:42 GMT, Jan Lahoda wrote: > > > The langtools changes look fine to me. There were additions/changes to jcod > files under `test/hotspot/jtreg/runtime/sealedClasses/` made under > JDK-8246778, so these may need an update. Sorry about that. After a merge, the tests in that directory still pass. Will keep merging in new changes and do at least more more test run before pushing. Thanks. - PR: https://git.openjdk.java.net/jdk/pull/1531
Re: RFR: 8257450: Start of release updates for JDK 17 [v2]
> 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 eight additional commits since the last revision: - 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 - JDK-8257450 - JDK-8257450 - Changes: - all: https://git.openjdk.java.net/jdk/pull/1531/files - new: https://git.openjdk.java.net/jdk/pull/1531/files/4187d66f..f6a64473 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=1531=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=1531=00-01 Stats: 12681 lines in 254 files changed: 8255 ins; 3285 del; 1141 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: RFR: 8257733: Move module-specific data from make to respective module [v2]
On Mon, 7 Dec 2020 14:27:45 GMT, Magnus Ihse Bursie wrote: >> A lot (but not all) of the data in make/data is tied to a specific module. >> For instance, the publicsuffixlist is used by java.base, and fontconfig by >> java.desktop. (A few directories, like mainmanifest, is *actually* used by >> make for the whole build.) >> >> These data files should move to the module they belong to. The are, after >> all, "source code" for that module that is "compiler" into resulting >> deliverables, for that module. (But the "source code" language is not Java >> or C, but typically a highly domain specific language or data format, and >> the "compilation" is, often, a specialized transformation.) >> >> This misplacement of the data directory is most visible at code review time. >> When such data is changed, most of the time build-dev (or the new build >> label) is involved, even though this has nothing to do with the build. While >> this is annoying, a worse problem is if the actual team that needs to review >> the patch (i.e., the team owning the module) is missed in the review. > > Magnus Ihse Bursie has updated the pull request incrementally with one > additional commit since the last revision: > > Move to share/data, and move jdwp.spec to java.se I have reviewed all lines in the patch file with or near instances of `jdk.compiler` - Marked as reviewed by jjg (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/1611
Re: RFR: 8200102: NativeLibraryTest.java fails intermittently, unloaded count is not same as expected [v2]
> 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. Brent Christian 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 three additional commits since the last revision: - Merge branch 'master' into 8200102 - Use existing ForceGC class in the test lib - Add gcAwait() mechanism - Changes: - all: https://git.openjdk.java.net/jdk/pull/1630/files - new: https://git.openjdk.java.net/jdk/pull/1630/files/a66308e8..a6d58148 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=1630=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=1630=00-01 Stats: 10552 lines in 197 files changed: 6632 ins; 3120 del; 800 mod Patch: https://git.openjdk.java.net/jdk/pull/1630.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1630/head:pull/1630 PR: https://git.openjdk.java.net/jdk/pull/1630
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: > - Deprecate wrapper class constructors for removal (rriggs) > - Revise "value-based class" & apply to wrappers (dlsmith) > - Define & apply annotation jdk.internal.ValueBased (rriggs) > - Add 'lint' warning for @ValueBased classes (sadayapalam) > - 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`. core-libs and hotspot change look okay. - Marked as reviewed by mchung (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/1636
Re: RFR: 8257639: Update usage of "type" terminology in java.lang.Enum & java.lang.Record
On Mon, 7 Dec 2020 17:31:34 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 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`). - PR: https://git.openjdk.java.net/jdk/pull/1670
Re: RFR: 8255918: XMLStreamFilterImpl constructor consumes XMLStreamException [v2]
On Wed, 18 Nov 2020 07:18:16 GMT, Joe Wang wrote: >> Michael Edgar has updated the pull request with a new target base due to a >> merge or a rebase. The pull request now contains two commits: >> >> - Update test for jtreg >> - 8255918: Throw XMLStreamExceptions from XMLStreamFilterImpl constructor > > test/jdk/javax/xml/jaxp/parsers/8255918/XMLStreamReaderFilterTest.java line > 38: > >> 36: >> 37: static final String XMLSOURCE1 = "\n" >> 38: + " \n" > > Change to sth like the following: > /* > * @test > * @bug 8255918 > * @library /javax/xml/jaxp/libs /javax/xml/jaxp/unittest > * @run testng stream.XMLStreamFilterTest.XMLStreamReaderFilterTest > * @summary (general description is fine as future tests can be added, but > add javadoc to the test itself) > */ This comment was shown under "static final String XMLSOURCE1" in the email, though in "Files changed" pane it still is right below the class description "JDK-8255918". The comment meant to be class description, replacing "JDK-8255918". - PR: https://git.openjdk.java.net/jdk/pull/1209
Integrated: 8255619: Localized WinResources.properties have MsiInstallerStrings_en.wxl resource
On Sat, 5 Dec 2020 05:33:25 GMT, Alexander Matveev wrote: > Fixed by using correct localized resources. This pull request has now been integrated. Changeset: a265c201 Author:Alexander Matveev URL: https://git.openjdk.java.net/jdk/commit/a265c201 Stats: 2 lines in 2 files changed: 0 ins; 0 del; 2 mod 8255619: Localized WinResources.properties have MsiInstallerStrings_en.wxl resource Reviewed-by: herrick, naoto, asemenyuk - PR: https://git.openjdk.java.net/jdk/pull/1638
Re: RFR: 8255918: XMLStreamFilterImpl constructor consumes XMLStreamException [v2]
On Sun, 6 Dec 2020 00:00:24 GMT, Michael Edgar
wrote:
>> Allow `XMLStreamException` to be thrown to the application when a filtered
>> `XMLStreamReader` encounters an `XMLStreamException` advancing the wrapped
>> `XMLStreamReader`.
>>
>> Note, this PR includes a method signature change (constructor of
>> `com.sun.org.apache.xerces.internal.impl.XMLStreamFilterImpl`).
>
> Michael Edgar has updated the pull request with a new target base due to a
> merge or a rebase. The pull request now contains two commits:
>
> - Update test for jtreg
> - 8255918: Throw XMLStreamExceptions from XMLStreamFilterImpl constructor
I left comments the other day but forgot to submit. I've adjusted them based on
your new changes.
src/java.xml/share/classes/com/sun/org/apache/xerces/internal/impl/XMLStreamFilterImpl.java
line 68:
> 66: *the filter to apply to the reader
> 67: * @throws XMLStreamException
> 68: * when an XMLStreamException is thrown when
{@code XMLStreamException} is preferable.
test/jdk/javax/xml/jaxp/parsers/8255918/XMLStreamReaderFilterTest.java line 1:
> 1: /*
Please move the test to
test/jaxp/javax/xml/jaxp/unittest/stream/XMLStreamFilterTest
Once this is done, run tier2 test instead of tier1.
test/jdk/javax/xml/jaxp/parsers/8255918/XMLStreamReaderFilterTest.java line 38:
> 36:
> 37: static final String XMLSOURCE1 = "\n"
> 38: + " \n"
Change to sth like the following:
/*
* @test
* @bug 8255918
* @library /javax/xml/jaxp/libs /javax/xml/jaxp/unittest
* @run testng stream.XMLStreamFilterTest.XMLStreamReaderFilterTest
* @summary (general description is fine as future tests can be added, but add
javadoc to the test itself)
*/
test/jdk/javax/xml/jaxp/parsers/8255918/XMLStreamReaderFilterTest.java line 22:
> 20: * or visit www.oracle.com if you need additional information or have any
> 21: * questions.
> 22: */
Add package stream.XMLStreamFilterTest;
test/jdk/javax/xml/jaxp/parsers/8255918/XMLStreamReaderFilterTest.java line 75:
> 73:
> 74: assertTrue(thrown instanceof XMLStreamException, "Missing or
> unexpected exception type: " + String.valueOf(thrown));
> 75: assertEquals(EXPECTED_MESSAGE1, thrown.getMessage(), "Unexpected
> exception message: " + thrown.getMessage());
It would be good/sufficient to verify XMLStreamException with assertThrows.
Checking the message details can be fragile.
test/jdk/javax/xml/jaxp/parsers/8255918/XMLStreamReaderFilterTest.java line 58:
> 56: * @modules java.xml
> 57: * @run testng/othervm XMLStreamReaderFilterTest
> 58: *
Tags are added in the class description. Change this to a TestCase javadoc or
note.
-
PR: https://git.openjdk.java.net/jdk/pull/1209
RFR: 8253797: [cgroups v2] Account for the fact that swap accounting is disabled on some systems
This has been implemented for cgroups v1 with [JDK-8250984](https://bugs.openjdk.java.net/browse/JDK-8250984) but was lacking some tooling support for cgroups v2. With podman 2.2.0 release this could now be implemented (and tested). The idea is the same as for the cgroups v1 fix. If we've got no swap limit capabilities, return the memory limit only. Note that for cgroups v2 doesn't implement CgroupV1Metrics (obviously) and, thus, doesn't have `getMemoryAndSwapFailCount()` and `getMemoryAndSwapMaxUsage()`. Testing: - [x] submit testing - [x] container tests on cgroups v2 with swapaccount=0. - [x] Manual container tests involving `-XshowSettings:system` on cgroups v2. Thoughts? - Commit messages: - 8253797: [cgroups v2] Account for the fact that swap accounting is disabled on some systems Changes: https://git.openjdk.java.net/jdk/pull/1672/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=1672=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8253797 Stats: 45 lines in 2 files changed: 24 ins; 1 del; 20 mod Patch: https://git.openjdk.java.net/jdk/pull/1672.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1672/head:pull/1672 PR: https://git.openjdk.java.net/jdk/pull/1672
RFR: 8257639: Update usage of "type" terminology in java.lang.Enum & java.lang.Record
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 - Commit messages: - replace one more occurrence - change type to class in java.lang.Record - change type to class in Enum.java Changes: https://git.openjdk.java.net/jdk/pull/1670/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=1670=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8257639 Stats: 24 lines in 2 files changed: 0 ins; 0 del; 24 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
RFR: 8257194: Add 'foreign linker API' in 'jdk.incubator.foreign' module desc/summary
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. - Commit messages: - Fix module-info javadoc Changes: https://git.openjdk.java.net/jdk/pull/1671/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=1671=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8257194 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/1671.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1671/head:pull/1671 PR: https://git.openjdk.java.net/jdk/pull/1671
Re: RFR: 8256515: javax.xml.XMLEventReader produces incorrect START_DOCUMENT event [v3]
On Fri, 4 Dec 2020 00:32:26 GMT, Joe Wang wrote:
>> Marius Volkhart has updated the pull request with a new target base due to a
>> merge or a rebase. The pull request now contains three commits:
>>
>> - Adjust test so it works with jtreg
>> - Fix: javax.xml.stream.XMLEventReader produces incorrect START_DOCUMENT
>> event
>>
>>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.
>> - Add test for XmlInputFactory
>
> test/jdk/javax/xml/jaxp/8256515/XmlInputFactoryTest.java line 25:
>
>> 23: var factory = XMLInputFactory.newInstance();
>> 24: var xml = """
>> 25: """;
>
> There are three test cases here. Let's use DataProvider, sth. like {" version='1.0'?>", false, false}. The test then will take three parameters,
> e.g. xml, standalone, standaloneSet, and make two assertEquals
And while we are here, I'm not against using new features, we actually have
done a lot over time. But unless there's a clear advantage, I'd keep the code
(esp. tests) compatible with 8 since it's very likely changes will be
backported. With that, I would recommend not using var and text blocks.
-
PR: https://git.openjdk.java.net/jdk/pull/1056
Re: RFR: 8256515: javax.xml.XMLEventReader produces incorrect START_DOCUMENT event [v3]
On Sat, 5 Dec 2020 12:02:26 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 with a new target base due to a
> merge or a rebase. The pull request now contains three commits:
>
> - Adjust test so it works with jtreg
> - Fix: javax.xml.stream.XMLEventReader produces incorrect START_DOCUMENT
> event
>
>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.
> - Add test for XmlInputFactory
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.
test/jdk/javax/xml/jaxp/8256515/XmlInputFactoryTest.java line 22:
> 20:
> 21: @Test
> 22: void startDocumentEvent_standaloneSet() throws XMLStreamException {
Instead of creating a new test, add this test case to:
test/jaxp/javax/xml/jaxp/unittest/stream/XMLEventReaderTest/EventReaderTest.java
update the copyright year: add "2020,"
update class description: add 8256515 to the bug tag: @bug 8204329 8256515
change the name of the test case to sth like: testStandaloneSet
test/jdk/javax/xml/jaxp/8256515/XmlInputFactoryTest.java line 25:
> 23: var factory = XMLInputFactory.newInstance();
> 24: var xml = """
> 25: """;
There are three test cases here. Let's use DataProvider, sth. like {"", false, false}. The test then will take three parameters, e.g.
xml, standalone, standaloneSet, and make two assertEquals
-
PR: https://git.openjdk.java.net/jdk/pull/1056
Integrated: 8257186: Size of heap segments is not computed correctlyFix overflow in size computation for heap segments
On Thu, 26 Nov 2020 18:29:42 GMT, Maurizio Cimadamore wrote: > There is a subtle bug in the heap segment factories: the byte size is > computed using an int multiplication instead of a long multiplication. > Because of that, it is possible to observe overflow when creating segments > out of arrays whose carrier has a byte size greater than one. This pull request has now been integrated. Changeset: bbc44f57 Author:Maurizio Cimadamore URL: https://git.openjdk.java.net/jdk/commit/bbc44f57 Stats: 29 lines in 2 files changed: 21 ins; 0 del; 8 mod 8257186: Size of heap segments is not computed correctlyFix overflow in size computation for heap segments Reviewed-by: jvernee, chegar - PR: https://git.openjdk.java.net/jdk/pull/1466
Re: RFR: 8255619: Localized WinResources.properties have MsiInstallerStrings_en.wxl resource
On Sat, 5 Dec 2020 05:33:25 GMT, Alexander Matveev wrote: > Fixed by using correct localized resources. Marked as reviewed by asemenyuk (Committer). - PR: https://git.openjdk.java.net/jdk/pull/1638
RFR: 8242258: (jrtfs) Path::toUri throws AssertionError for malformed input
Safe URI encode logic adopted from UnixUriUtils. - Commit messages: - 8242258: (jrtfs) Path::toUri throws AssertionError for malformed input Changes: https://git.openjdk.java.net/jdk/pull/1669/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=1669=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8242258 Stats: 218 lines in 2 files changed: 206 ins; 5 del; 7 mod Patch: https://git.openjdk.java.net/jdk/pull/1669.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1669/head:pull/1669 PR: https://git.openjdk.java.net/jdk/pull/1669
Re: RFR: 8252180: [JEP 390] Deprecate wrapper class constructors for removal
On Mon, 7 Dec 2020 15:50:55 GMT, Harold Seigel 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: >> - Deprecate wrapper class constructors for removal (rriggs) >> - Revise "value-based class" & apply to wrappers (dlsmith) >> - Define & apply annotation jdk.internal.ValueBased (rriggs) >> - Add 'lint' warning for @ValueBased classes (sadayapalam) >> - 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`. > > The hotspot changes look good. In a future change, could you add additional > classes, such as ProcessHandle to test TestSyncOnValueBasedClassEvent. > Currently, it only tests primitive classes. @hseigel thank you for the review. I have created https://bugs.openjdk.java.net/browse/JDK-8257836 as an RFE to address additional testing. Lois - PR: https://git.openjdk.java.net/jdk/pull/1636
Withdrawn: 8246778: Compiler implementation for Sealed Classes (Second Preview)
On Mon, 16 Nov 2020 13:30:06 GMT, Vicente Romero wrote: > Please review the code for the second iteration of sealed classes. In this > iteration we are: > > - Enhancing narrowing reference conversion to allow for stricter checking of > cast conversions with respect to sealed type hierarchies > - Also local classes are not considered when determining implicitly declared > permitted direct subclasses of a sealed class or sealed interface > - renaming Class::permittedSubclasses to Class::getPermittedSubclasses, still > in the same method, the return type has been changed to Class[] instead of > the previous ClassDesc[] > - adding code to make sure that annotations can't be sealed > - improving some tests > > TIA > > Related specs: > [Sealed Classes > JSL](http://cr.openjdk.java.net/~gbierman/jep397/jep397-20201104/specs/sealed-classes-jls.html) > [Sealed Classes > JVMS](http://cr.openjdk.java.net/~gbierman/jep397/jep397-20201104/specs/sealed-classes-jvms.html) > [Additional: Contextual > Keywords](http://cr.openjdk.java.net/~gbierman/jep397/jep397-20201104/specs/contextual-keywords-jls.html) This pull request has been closed without being integrated. - PR: https://git.openjdk.java.net/jdk/pull/1227
Re: RFR: 8257450: Start of release updates for JDK 17
On Sat, 5 Dec 2020 03:17:56 GMT, Mikael Vidstedt wrote: >> Start of JDK 17 updates. > > Marked as reviewed by mikael (Reviewer). The langtools changes look fine to me. There were additions/changes to jcod files under `test/hotspot/jtreg/runtime/sealedClasses/` made under JDK-8246778, so these may need an update. Sorry about that. - PR: https://git.openjdk.java.net/jdk/pull/1531
Re: RFR: 8247373: ArraysSupport.newLength doc, test, and exception message
On Sat, 5 Dec 2020 02:37:55 GMT, Stuart Marks wrote: >> Is there a reason the code would not naturally work when either min or >> preferred is zero? >> Why are their preconditions? What do they allow? >> These methods are used in enough places that a slip in any of the clients >> would be trouble and hard to track down. > > The origin of this code is in collections like ArrayList that have an > existing array (hence oldLength >= 0) and that need it to grow (hence > minGrowth > 0). Those were the prevailing assumptions in the code from which > this was derived, so they turn into preconditions here. I don't see the need > to try to make this handle any more cases than it currently does. Doing so > complicates the analysis and possibly the code as well. Certainly a bug in a > caller might be difficult to track down, but I don't want to add argument > checking or to provide "reasonable" behavior in the face of unreasonable > inputs. This is an internal method; bugs in callers should be fixed. The algorithm can be well defined for minGrowth and prefGrowth == 0 without extra checks or exceptions with a careful look at the inequality checks. - PR: https://git.openjdk.java.net/jdk/pull/1617
Re: RFR: 8252180: [JEP 390] Deprecate wrapper class constructors for removal
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: > - Deprecate wrapper class constructors for removal (rriggs) > - Revise "value-based class" & apply to wrappers (dlsmith) > - Define & apply annotation jdk.internal.ValueBased (rriggs) > - Add 'lint' warning for @ValueBased classes (sadayapalam) > - 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`. The hotspot changes look good. In a future change, could you add additional classes, such as ProcessHandle to test TestSyncOnValueBasedClassEvent. Currently, it only tests primitive classes. - Marked as reviewed by hseigel (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/1636
Re: RFR: 8166026: Refactor java/lang shell tests to java [v3]
On Sun, 6 Dec 2020 17:29:27 GMT, Ivan Šipka wrote:
>> Refactor `test/jdk/java/lang/Thread/UncaughtExceptions.sh` as java test.
>
> Ivan Šipka has updated the pull request incrementally with one additional
> commit since the last revision:
>
> 8166026: Refactor java/lang shell tests to java
test/jdk/java/lang/Thread/UncaughtExceptionsTest.java line 68:
> 66: @Test(dataProvider = "testCases")
> 67: public void test(String className, int exitValue, String stdOutMatch,
> String stdErrMatch) throws Throwable {
> 68: ProcessBuilder processBuilder =
> ProcessTools.createJavaProcessBuilder(String.format("Seppuku$%s",className));
The class renaming looks incomplete. Here and in the error texts above.
Seppuku does not look right as the host class for the test, they are nested
classes of UncaughtExceptionsTest.
-
PR: https://git.openjdk.java.net/jdk/pull/1578
Re: RFR: 8252180: [JEP 390] Deprecate wrapper class constructors for removal
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: > - Deprecate wrapper class constructors for removal (rriggs) > - Revise "value-based class" & apply to wrappers (dlsmith) > - Define & apply annotation jdk.internal.ValueBased (rriggs) > - Add 'lint' warning for @ValueBased classes (sadayapalam) > - 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`. For the core libraries parts looks ok. - Marked as reviewed by rriggs (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/1636
Re: RFR: 8257733: Move module-specific data from make to respective module
On Fri, 4 Dec 2020 15:17:06 GMT, Magnus Ihse Bursie wrote:
>> Regarding the chosen layout. Did you consider following the existing pattern
>> of src//{share,}/data?
>
> @erikj79 Good point, that makes much more sense.
I'm not sure about the formal process for suggesting changes to a delivered
JEP, but this is the text I suggest should replace the current definition of
the new scheme:
src/$MODULE/{share,$OS}/classes/$PACKAGE/*.java
native/include/*.{h,hpp}
$LIBRARY/*.{c,cpp}
conf/*
legal/*
data/*
man/*
lib/*
where:
- $MODULE is a module name (_e.g._, `java.base`);
- The `share` directory contains shared, cross-platform code, as
before;
- The `$OS` directory contains operating-system-specific code, as
before, where `$OS` is one of `unix`, `windows`, _etc._;
- The `classes` directory contains Java source files and resource files
organized into a directory tree reflecting their API `$PACKAGE`
hierarchy, as before;
- The `native` directory contains C or C++ source files, as before but
organized differently:
- The `include` directory contains C or C++ header files intended to
be exported for external use (_e.g._, `jni.h`);
- C or C++ source files are placed in a `$LIBRARY` directory, whose
name is that of the shared library or DLL into which the compiled
code will be linked (_e.g._, `libjava` or `libawt`); and, finally,
- The `conf` directory contains configuration files meant to be edited
by end users (_e.g._, `net.properties`).
- The `legal` directory contains legal notices.
- The `data` directory contains data files needed for building the module.
- The `man` directory contains man pages in nroff or markdown format.
- The `lib` directory contains configuration files not meant to be edited
by end users.
Rendered as markdown, it would look somewhat like this:
src/$MODULE/{share,$OS}/classes/$PACKAGE/*.java
native/include/*.{h,hpp}
$LIBRARY/*.{c,cpp}
conf/*
legal/*
data/*
man/*
lib/*
where:
- $MODULE is a module name (_e.g._, `java.base`);
- The `share` directory contains shared, cross-platform code, as
before;
- The `$OS` directory contains operating-system-specific code, as
before, where `$OS` is one of `unix`, `windows`, _etc._;
- The `classes` directory contains Java source files and resource files
organized into a directory tree reflecting their API `$PACKAGE`
hierarchy, as before;
- The `native` directory contains C or C++ source files, as before but
organized differently:
- The `include` directory contains C or C++ header files intended to
be exported for external use (_e.g._, `jni.h`);
- C or C++ source files are placed in a `$LIBRARY` directory, whose
name is that of the shared library or DLL into which the compiled
code will be linked (_e.g._, `libjava` or `libawt`); and, finally,
- The `conf` directory contains configuration files meant to be edited
by end users (_e.g._, `net.properties`).
- The `legal` directory contains legal notices.
- The `data` directory contains data files needed for building the module.
- The `man` directory contains man pages in nroff or markdown format.
- The `lib` directory contains configuration files not meant to be edited
by end users.
-
PR: https://git.openjdk.java.net/jdk/pull/1611
Re: RFR: 8257733: Move module-specific data from make to respective module
On Mon, 7 Dec 2020 14:20:07 GMT, Magnus Ihse Bursie wrote:
>> @erikj79 Good point, that makes much more sense.
>
> I'm not sure about the formal process for suggesting changes to a delivered
> JEP, but this is the text I suggest should replace the current definition of
> the new scheme:
>
> src/$MODULE/{share,$OS}/classes/$PACKAGE/*.java
> native/include/*.{h,hpp}
>$LIBRARY/*.{c,cpp}
> conf/*
> legal/*
> data/*
> man/*
> lib/*
>
> where:
>
> - $MODULE is a module name (_e.g._, `java.base`);
>
> - The `share` directory contains shared, cross-platform code, as
> before;
>
> - The `$OS` directory contains operating-system-specific code, as
> before, where `$OS` is one of `unix`, `windows`, _etc._;
>
> - The `classes` directory contains Java source files and resource files
> organized into a directory tree reflecting their API `$PACKAGE`
> hierarchy, as before;
>
> - The `native` directory contains C or C++ source files, as before but
> organized differently:
>
> - The `include` directory contains C or C++ header files intended to
> be exported for external use (_e.g._, `jni.h`);
>
> - C or C++ source files are placed in a `$LIBRARY` directory, whose
> name is that of the shared library or DLL into which the compiled
> code will be linked (_e.g._, `libjava` or `libawt`); and, finally,
>
> - The `conf` directory contains configuration files meant to be edited
> by end users (_e.g._, `net.properties`).
>
> - The `legal` directory contains legal notices.
>
> - The `data` directory contains data files needed for building the module.
>
> - The `man` directory contains man pages in nroff or markdown format.
>
> - The `lib` directory contains configuration files not meant to be edited
> by end users.
>
> Rendered as markdown, it would look somewhat like this:
>
> src/$MODULE/{share,$OS}/classes/$PACKAGE/*.java
> native/include/*.{h,hpp}
>$LIBRARY/*.{c,cpp}
> conf/*
> legal/*
> data/*
> man/*
> lib/*
>
> where:
>
> - $MODULE is a module name (_e.g._, `java.base`);
>
> - The `share` directory contains shared, cross-platform code, as
> before;
>
> - The `$OS` directory contains operating-system-specific code, as
> before, where `$OS` is one of `unix`, `windows`, _etc._;
>
> - The `classes` directory contains Java source files and resource files
> organized into a directory tree reflecting their API `$PACKAGE`
> hierarchy, as before;
>
> - The `native` directory contains C or C++ source files, as before but
> organized differently:
>
> - The `include` directory contains C or C++ header files intended to
> be exported for external use (_e.g._, `jni.h`);
>
> - C or C++ source files are placed in a `$LIBRARY` directory, whose
> name is that of the shared library or DLL into which the compiled
> code will be linked (_e.g._, `libjava` or `libawt`); and, finally,
>
> - The `conf` directory contains configuration files meant to be edited
> by end users (_e.g._, `net.properties`).
>
> - The `legal` directory contains legal notices.
>
> - The `data` directory contains data files needed for building the module.
>
> - The `man` directory contains man pages in nroff or markdown format.
>
> - The `lib` directory contains configuration files not meant to be edited
> by end users.
I hope I understood the purpose of the `lib` directory correctly. Our only
instance of this is for `java.base/*/lib/security/default.policy`.
I also noted that jdk.hotspot.agent violates this scheme by having a top-level
`test` and `doc` directories, apart from `share` and the OS directories. The
purposes of these two directories are not clear to me. The tests in `test` are
definitely never executed. I don't know if this is an omission, or if they
should be removed. The documentation in `doc` is not exported to the end
product, nor is it references in any developer documentation. That too should
either be removed, or moved to a more suitable home.
-
PR: https://git.openjdk.java.net/jdk/pull/1611
Re: RFR: 8257733: Move module-specific data from make to respective module [v2]
> A lot (but not all) of the data in make/data is tied to a specific module. > For instance, the publicsuffixlist is used by java.base, and fontconfig by > java.desktop. (A few directories, like mainmanifest, is *actually* used by > make for the whole build.) > > These data files should move to the module they belong to. The are, after > all, "source code" for that module that is "compiler" into resulting > deliverables, for that module. (But the "source code" language is not Java or > C, but typically a highly domain specific language or data format, and the > "compilation" is, often, a specialized transformation.) > > This misplacement of the data directory is most visible at code review time. > When such data is changed, most of the time build-dev (or the new build > label) is involved, even though this has nothing to do with the build. While > this is annoying, a worse problem is if the actual team that needs to review > the patch (i.e., the team owning the module) is missed in the review. Magnus Ihse Bursie has updated the pull request incrementally with one additional commit since the last revision: Move to share/data, and move jdwp.spec to java.se - Changes: - all: https://git.openjdk.java.net/jdk/pull/1611/files - new: https://git.openjdk.java.net/jdk/pull/1611/files/8e775e40..f0047704 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=1611=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=1611=00-01 Stats: 56 lines in 1565 files changed: 1 ins; 0 del; 55 mod Patch: https://git.openjdk.java.net/jdk/pull/1611.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1611/head:pull/1611 PR: https://git.openjdk.java.net/jdk/pull/1611
Integrated: 8246778: Compiler implementation for Sealed Classes (Second Preview)
On Fri, 27 Nov 2020 16:57:54 GMT, Jan Lahoda wrote: > This pull request replaces https://github.com/openjdk/jdk/pull/1227. > > From the original PR: > >> Please review the code for the second iteration of sealed classes. In this >> iteration we are: >> >> * Enhancing narrowing reference conversion to allow for stricter >> checking of cast conversions with respect to sealed type hierarchies >> >> * Also local classes are not considered when determining implicitly >> declared permitted direct subclasses of a sealed class or sealed interface >> >> * renaming Class::permittedSubclasses to Class::getPermittedSubclasses, >> still in the same method, the return type has been changed to Class[] >> instead of the previous ClassDesc[] >> >> * adding code to make sure that annotations can't be sealed >> >> * improving some tests >> >> >> TIA >> >> Related specs: >> [Sealed Classes >> JSL](http://cr.openjdk.java.net/~gbierman/jep397/jep397-20201104/specs/sealed-classes-jls.html) >> [Sealed Classes >> JVMS](http://cr.openjdk.java.net/~gbierman/jep397/jep397-20201104/specs/sealed-classes-jvms.html) >> [Additional: Contextual >> Keywords](http://cr.openjdk.java.net/~gbierman/jep397/jep397-20201104/specs/contextual-keywords-jls.html) > > This PR strives to reflect the review comments from 1227: > * adjustments to javadoc of j.l.Class methods > * package access checks in Class.getPermittedSubclasses() > * fixed to the narrowing conversion/castability as pointed out by Maurizio This pull request has now been integrated. Changeset: 637b0c64 Author:Jan Lahoda URL: https://git.openjdk.java.net/jdk/commit/637b0c64 Stats: 1138 lines in 16 files changed: 1055 ins; 11 del; 72 mod 8246778: Compiler implementation for Sealed Classes (Second Preview) Co-authored-by: Vicente Romero Co-authored-by: Harold Seigel Reviewed-by: lfoltan, mchung, alanb, mcimadamore, chegar - PR: https://git.openjdk.java.net/jdk/pull/1483
Integrated: 8257184: Upstream 8252504: Add a method to MemoryLayout which returns a offset-computing method handle
On Thu, 26 Nov 2020 19:24:16 GMT, Jorn Vernee wrote: > This upstreams the patch from: > https://github.com/openjdk/panama-foreign/pull/396 > > There were only some minor merge conflicts due to imports and some tests > being replaced by java/foreign/TestNulls. All tests still pass, no other > changes were needed. > > CSR: https://bugs.openjdk.java.net/browse/JDK-8257187 This pull request has now been integrated. Changeset: 04ce8e38 Author:Jorn Vernee URL: https://git.openjdk.java.net/jdk/commit/04ce8e38 Stats: 321 lines in 4 files changed: 264 ins; 7 del; 50 mod 8257184: Upstream 8252504: Add a method to MemoryLayout which returns a offset-computing method handle Reviewed-by: mcimadamore, chegar - PR: https://git.openjdk.java.net/jdk/pull/1468
Integrated: 8255560: Class::isRecord should check that the current class is final and not abstract
On Tue, 1 Dec 2020 19:34:11 GMT, Chris Hegarty wrote: > Update Class::isRecord to only return true for classes that are final. > > The removal of non-specified JVM checks on classes with a Record Attribute > (see JDK-8255342), has resulted in more types of loadable classes that may > contain a Record Attribute. Since these checks are not performed by the JVM > anymore, Class::isRecord, and by extension Class::getRecordComponents, may > return true or component values, respectively, for classes that are not > well-formed record classes (as per the JLS), .e.g. non-final or abstract > classes, that contain a record Attribute. > > Core Reflection, Class::isRecord, already asserts checks that the JVM does > not, e.g. that the direct superclass is java.lang.Record. Some points from > the Java Language Specification for record classes: > > 1. It is a compile-time error if a record declaration has the modifier > abstract. > 2. A record declaration is implicitly final. > 3. The direct superclass type of a record class is Record. > > Class::isRecord already ensures no.3. This issue proposes to add explicit > checks in Core Reflection to ensure no.1 and no.2, since the JVM now allows > such classes that contain a Record Attribute to be loaded. This pull request has now been integrated. Changeset: 5a03e476 Author:Chris Hegarty URL: https://git.openjdk.java.net/jdk/commit/5a03e476 Stats: 230 lines in 3 files changed: 208 ins; 3 del; 19 mod 8255560: Class::isRecord should check that the current class is final and not abstract Reviewed-by: mchung, darcy - PR: https://git.openjdk.java.net/jdk/pull/1543
Re: RFR: 8255560: Class::isRecord should check that the current class is final and not abstract [v5]
> Update Class::isRecord to only return true for classes that are final. > > The removal of non-specified JVM checks on classes with a Record Attribute > (see JDK-8255342), has resulted in more types of loadable classes that may > contain a Record Attribute. Since these checks are not performed by the JVM > anymore, Class::isRecord, and by extension Class::getRecordComponents, may > return true or component values, respectively, for classes that are not > well-formed record classes (as per the JLS), .e.g. non-final or abstract > classes, that contain a record Attribute. > > Core Reflection, Class::isRecord, already asserts checks that the JVM does > not, e.g. that the direct superclass is java.lang.Record. Some points from > the Java Language Specification for record classes: > > 1. It is a compile-time error if a record declaration has the modifier > abstract. > 2. A record declaration is implicitly final. > 3. The direct superclass type of a record class is Record. > > Class::isRecord already ensures no.3. This issue proposes to add explicit > checks in Core Reflection to ensure no.1 and no.2, since the JVM now allows > such classes that contain a Record Attribute to be loaded. Chris Hegarty has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains six commits: - Remove unused test helper method - Merge branch 'master' into isRecord - fix issue in ByteCodeLoader - review comments - Mandy's review comments - Initial changes for 8255560 Class::isRecord should check that the current class is final and not abstract - Changes: https://git.openjdk.java.net/jdk/pull/1543/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=1543=04 Stats: 230 lines in 3 files changed: 208 ins; 3 del; 19 mod Patch: https://git.openjdk.java.net/jdk/pull/1543.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1543/head:pull/1543 PR: https://git.openjdk.java.net/jdk/pull/1543
Integrated: 8256679: Update serialization javadoc once JOSS changes for records are complete
On Wed, 2 Dec 2020 14:51:23 GMT, Julia Boes wrote: > Now that the changes for record serialization are integrated into the Java > Object Serialization Specification, this change updates the serialization > javadocs in ObjectInputStream, ObjectOutputStream, Serializable and > java.lang.Record. Additionally, the suppression of preview related warnings > is removed in ObjectOutputStream and ObjectStreamClass. > > CSR: https://bugs.openjdk.java.net/browse/JDK-8257592 This pull request has now been integrated. Changeset: d05401d8 Author:Julia Boes URL: https://git.openjdk.java.net/jdk/commit/d05401d8 Stats: 58 lines in 5 files changed: 8 ins; 34 del; 16 mod 8256679: Update serialization javadoc once JOSS changes for records are complete Reviewed-by: chegar, rriggs - PR: https://git.openjdk.java.net/jdk/pull/1564
