Re: RFR: JDK-8266918: merge_stack in check_code.c add NULL check [v2]
On Fri, 4 Jun 2021 14:10:20 GMT, Matthias Baesken wrote: >> Hello, please review this small Sonar finding. >> Sonar reports a potential NULL pointer dereference here : >> https://sonarcloud.io/project/issues?id=shipilev_jdk=c=AXck8CPLBBG2CXpcnh_z=false=MAJOR=BUG >> "Access to field 'item' results in a dereference of a null pointer (loaded >> from variable 'new')" >> It would be better to add a check . > > Matthias Baesken has updated the pull request incrementally with one > additional commit since the last revision: > > Replace check by assertion Marked as reviewed by clanger (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/3979
Re: RFR: 8236569: -Xss not multiple of 4K does not work for the main thread on macOS [v4]
> …d on macOS
>
> This patch simply round up the specified stack size to multiple of the system
> page size.
>
> Test is trivial, simply run java with -Xss option against following code. On
> MacOS, before the fix, running with `-Xss159k` and `-Xss160k` would get
> `7183` and `649` respectively. After fix, both would output `649`, while
> `-Xss161k` would be same as `-Xss164k` and see 691 as the output.
>
> ```code:java
> public class StackLeak {
> public int depth = 0;
> public void stackLeak() {
> depth++;
> stackLeak();
> }
>
> public static void main(String[] args) {
> var test = new StackLeak();
> try {
> test.stackLeak();
> } catch (Throwable e) {
> System.out.println(test.depth);
> }
> }
> }
Henry Jen has updated the pull request incrementally with one additional commit
since the last revision:
Change java -X output for -Xss
-
Changes:
- all: https://git.openjdk.java.net/jdk/pull/4256/files
- new: https://git.openjdk.java.net/jdk/pull/4256/files/1e96be94..764a1f93
Webrevs:
- full: https://webrevs.openjdk.java.net/?repo=jdk=4256=03
- incr: https://webrevs.openjdk.java.net/?repo=jdk=4256=02-03
Stats: 2 lines in 1 file changed: 2 ins; 0 del; 0 mod
Patch: https://git.openjdk.java.net/jdk/pull/4256.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/4256/head:pull/4256
PR: https://git.openjdk.java.net/jdk/pull/4256
Re: RFR: 8174222: LambdaMetafactory: validate inputs and improve documentation [v4]
On Sat, 5 Jun 2021 00:15:25 GMT, Dan Smith wrote: >> Standardizes and better specifies the errors thrown by LambdaMetafactory, >> including for inputs that would not be generated by javac. >> >> Does some renaming of core parameters/fields to better reflect their purpose. >> >> In the implementation, stops using ACC_BRIDGE for any methods, which is not >> a good fit for what these methods do (they don't delegate to each other, but >> all invoke the same implementation method). > > Dan Smith has updated the pull request incrementally with two additional > commits since the last revision: > > - Fix stray renaming > - Apply renamings to LambdaProxyClassArchive Thanks for going beyond the java code. Looks good. - Marked as reviewed by mchung (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/4346
Re: RFR: 8174222: LambdaMetafactory: validate inputs and improve documentation [v3]
On Fri, 4 Jun 2021 23:41:06 GMT, Dan Smith wrote:
>> src/java.base/share/classes/java/lang/invoke/LambdaMetafactory.java line 312:
>>
>>> 310: * @return a CallSite whose target can be used to perform capture,
>>> generating
>>> 311: * instances of the interface named by {@code factoryType}
>>> 312: * @throws LambdaConversionException If {@code caller} does not
>>> have full privilege
>>
>> Suggestion:
>>
>> * @throws LambdaConversionException If {@code caller} does not have
>> {@linkplain Lookup#hasFullPrivilegeAccess full privilege}
>
> It's hard to see in the diff, but this link just appeared in the javadoc a
> few lines above. Stylistically, doesn't seem like it should be linked again.
I didn't realize it's already linked, thanks. So what you have is fine then.
-
PR: https://git.openjdk.java.net/jdk/pull/4346
Re: RFR: 8236569: -Xss not multiple of 4K does not work for the main thread on macOS [v3]
> …d on macOS
>
> This patch simply round up the specified stack size to multiple of the system
> page size.
>
> Test is trivial, simply run java with -Xss option against following code. On
> MacOS, before the fix, running with `-Xss159k` and `-Xss160k` would get
> `7183` and `649` respectively. After fix, both would output `649`, while
> `-Xss161k` would be same as `-Xss164k` and see 691 as the output.
>
> ```code:java
> public class StackLeak {
> public int depth = 0;
> public void stackLeak() {
> depth++;
> stackLeak();
> }
>
> public static void main(String[] args) {
> var test = new StackLeak();
> try {
> test.stackLeak();
> } catch (Throwable e) {
> System.out.println(test.depth);
> }
> }
> }
Henry Jen has updated the pull request with a new target base due to a merge or
a rebase. The incremental webrev excludes the unrelated changes brought in by
the merge/rebase. The pull request contains four additional commits since the
last revision:
- Merge
- Only try to round-up when current value failed
- Avoid overflow on page size
- JDK-8236569: -Xss not multiple of 4K does not work for the main thread on
macOS
-
Changes:
- all: https://git.openjdk.java.net/jdk/pull/4256/files
- new: https://git.openjdk.java.net/jdk/pull/4256/files/39b041d7..1e96be94
Webrevs:
- full: https://webrevs.openjdk.java.net/?repo=jdk=4256=02
- incr: https://webrevs.openjdk.java.net/?repo=jdk=4256=01-02
Stats: 485003 lines in 2409 files changed: 450413 ins; 28438 del; 6152 mod
Patch: https://git.openjdk.java.net/jdk/pull/4256.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/4256/head:pull/4256
PR: https://git.openjdk.java.net/jdk/pull/4256
Re: RFR: 8174222: LambdaMetafactory: validate inputs and improve documentation [v4]
> Standardizes and better specifies the errors thrown by LambdaMetafactory, > including for inputs that would not be generated by javac. > > Does some renaming of core parameters/fields to better reflect their purpose. > > In the implementation, stops using ACC_BRIDGE for any methods, which is not a > good fit for what these methods do (they don't delegate to each other, but > all invoke the same implementation method). Dan Smith has updated the pull request incrementally with two additional commits since the last revision: - Fix stray renaming - Apply renamings to LambdaProxyClassArchive - Changes: - all: https://git.openjdk.java.net/jdk/pull/4346/files - new: https://git.openjdk.java.net/jdk/pull/4346/files/b8b4ac14..9d722edf Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=4346=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk=4346=02-03 Stats: 94 lines in 4 files changed: 0 ins; 0 del; 94 mod Patch: https://git.openjdk.java.net/jdk/pull/4346.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4346/head:pull/4346 PR: https://git.openjdk.java.net/jdk/pull/4346
Re: RFR: 8174222: LambdaMetafactory: validate inputs and improve documentation [v3]
On Fri, 4 Jun 2021 22:09:39 GMT, Mandy Chung wrote: > Can you also rename the parameter names in > `java.lang.invoke.LambdaProxyClassArchive` methods to match the new ones? Hmm, that expands the reach of the patch a bit—into native HotSpot code—but I've given it a try. Let me know if it looks like I've broken something. :-) - PR: https://git.openjdk.java.net/jdk/pull/4346
Integrated: 8263512: [macos_aarch64] issues with calling va_args functions from invoke_native
On Thu, 22 Apr 2021 08:19:53 GMT, Nick Gasson wrote: > macOS on Apple silicon uses slightly different ABI conventions to the > standard AArch64 ABI. The differences are outlined in [1]. In > particular in the standard (AAPCS) ABI, variadic arguments may be passed > in either registers or on the stack following the normal calling > convention. To handle this, va_list is a struct containing separate > pointers for arguments located in integer registers, floating point > registers, and on the stack. Apple's ABI simplifies this by passing all > variadic arguments on the stack and the va_list type becomes a simple > char* pointer. > > This patch adds a new MacOsAArch64 CABI type and MacOsAArch64Linker to > represent the new ABI variant on macOS. StackVaList is based on > WinVaList lightly modified to handle the different TypeClasses on > AArch64. The original AArch64Linker is renamed to AapcsLinker and is > currently used for all non-Mac platforms. I think we also need to add a > WinAArch64 CABI but I haven't yet been able to test on a Windows system > so will do that later. > > The macOS ABI also uses a different method of spilling arguments to the > stack (the standard ABI pads each argument to a multiple of 8 byte stack > slots, but the Mac ABI packs arguments according to their natural > alignment). None of the existing tests exercise this so I'll open a new > JBS issue and work on that separately. > > Tested jdk_foreign on macOS AArch64, Linux AArch64, and Linux X86_64. > > [1] > https://developer.apple.com/documentation/xcode/writing_arm64_code_for_apple_platforms This pull request has now been integrated. Changeset: 76b54a19 Author:Nick Gasson URL: https://git.openjdk.java.net/jdk/commit/76b54a19955cd93f071cf1fb45c6d01bb57b84eb Stats: 778 lines in 15 files changed: 571 ins; 115 del; 92 mod 8263512: [macos_aarch64] issues with calling va_args functions from invoke_native Reviewed-by: jvernee - PR: https://git.openjdk.java.net/jdk/pull/3617
Re: RFR: 8174222: LambdaMetafactory: validate inputs and improve documentation [v3]
On Fri, 4 Jun 2021 22:06:49 GMT, Mandy Chung wrote:
>> Dan Smith has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Address reviewer comments
>
> src/java.base/share/classes/java/lang/invoke/LambdaMetafactory.java line 312:
>
>> 310: * @return a CallSite whose target can be used to perform capture,
>> generating
>> 311: * instances of the interface named by {@code factoryType}
>> 312: * @throws LambdaConversionException If {@code caller} does not
>> have full privilege
>
> Suggestion:
>
> * @throws LambdaConversionException If {@code caller} does not have
> {@linkplain Lookup#hasFullPrivilegeAccess full privilege}
It's hard to see in the diff, but this link just appeared in the javadoc a few
lines above. Stylistically, doesn't seem like it should be linked again.
-
PR: https://git.openjdk.java.net/jdk/pull/4346
Re: RFR: 8174222: LambdaMetafactory: validate inputs and improve documentation [v3]
On Fri, 4 Jun 2021 22:04:19 GMT, Dan Smith wrote:
>> Standardizes and better specifies the errors thrown by LambdaMetafactory,
>> including for inputs that would not be generated by javac.
>>
>> Does some renaming of core parameters/fields to better reflect their purpose.
>>
>> In the implementation, stops using ACC_BRIDGE for any methods, which is not
>> a good fit for what these methods do (they don't delegate to each other, but
>> all invoke the same implementation method).
>
> Dan Smith has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Address reviewer comments
Can you also rename the parameter names in
`java.lang.invoke.LambdaProxyClassArchive` methods to match the new ones?
src/java.base/share/classes/java/lang/invoke/LambdaMetafactory.java line 312:
> 310: * @return a CallSite whose target can be used to perform capture,
> generating
> 311: * instances of the interface named by {@code factoryType}
> 312: * @throws LambdaConversionException If {@code caller} does not have
> full privilege
Suggestion:
* @throws LambdaConversionException If {@code caller} does not have
{@linkplain Lookup#hasFullPrivilegeAccess full privilege}
-
PR: https://git.openjdk.java.net/jdk/pull/4346
Re: RFR: 8174222: LambdaMetafactory: validate inputs and improve documentation [v2]
On Fri, 4 Jun 2021 06:16:45 GMT, Peter Levart wrote: >> Dan Smith has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Fix accidentally commented-out parts of test > > src/java.base/share/classes/java/lang/invoke/AbstractValidatingLambdaMetafactory.java > line 69: > >> 67: final boolean isSerializable; // Should the returned >> instance be serializable >> 68: final Class[] interfaces; // Additional interfaces >> to be implemented >> 69: final MethodType[] bridges; // Signatures of >> additional methods to bridge > > If you are removing ACC_BRIDGE from additional generated methods, then > perhaps this parameter name could also be changed? `altMethods` (as > alternative methods perhaps?) Yes, it was sort of a half-done move. You're right, we should rename these (and, more importantly, in the public LambdaMetafactory API). Done. Also renamed `interfaces` --> `altInterfaces`. - PR: https://git.openjdk.java.net/jdk/pull/4346
Re: RFR: 8174222: LambdaMetafactory: validate inputs and improve documentation [v3]
> Standardizes and better specifies the errors thrown by LambdaMetafactory, > including for inputs that would not be generated by javac. > > Does some renaming of core parameters/fields to better reflect their purpose. > > In the implementation, stops using ACC_BRIDGE for any methods, which is not a > good fit for what these methods do (they don't delegate to each other, but > all invoke the same implementation method). Dan Smith has updated the pull request incrementally with one additional commit since the last revision: Address reviewer comments - Changes: - all: https://git.openjdk.java.net/jdk/pull/4346/files - new: https://git.openjdk.java.net/jdk/pull/4346/files/4b8d0dab..b8b4ac14 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=4346=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk=4346=01-02 Stats: 73 lines in 4 files changed: 0 ins; 0 del; 73 mod Patch: https://git.openjdk.java.net/jdk/pull/4346.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4346/head:pull/4346 PR: https://git.openjdk.java.net/jdk/pull/4346
Re: RFR: 8268083: JDK-8267706 breaks bin/idea.sh on a Mac
On Fri, 4 Jun 2021 21:23:27 GMT, Nikita Gubarkov wrote: > I got rid of `realpath` usage as discussed in > https://github.com/openjdk/jdk/pull/4190 and used `RelativePath` macro > instead, however there were quite a few problems with this macro, here's the > example: > > $(call RelativePath,/foo/bar,/foo/bar/baz) -> "..//foo/bar" > $(call RelativePath,/foo/bar/baz/,/foo/bar/baz) -> SEGFAULT > $(call RelativePath,/foo/bar/baz/banan,/foo/bar/) -> "./baz/banan" > $(call RelativePath,/foo/bar/baz,/foo/bar/banan) -> "../baz" > > As you can see, 1st case is just plain wrong, 2nd crashes make because of > infinite loop, 3rd can be simplified and all of them have leading whitespaces > First commit in this PR fixes all these issues and adds corresponding test > cases and second commit replaces usage of `realpath` in idea.sh with > `RelativePath` macro in idea.gmk and fixes problems, when paths are > incorrectly treated by IDEA Nice work, this looks great! Thank you for improving the RelativePath macro. Since this is touching some very core and sensitive build functionality, I'm going to run it through all our internal builds just to be safe. - PR: https://git.openjdk.java.net/jdk/pull/4369
Re: RFR: 8174222: LambdaMetafactory: validate inputs and improve documentation [v2]
On Fri, 4 Jun 2021 01:22:05 GMT, liach
wrote:
>> Dan Smith has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Fix accidentally commented-out parts of test
>
> src/java.base/share/classes/java/lang/invoke/LambdaMetafactory.java line 547:
>
>> 545: throw new IllegalArgumentException("argument has wrong
>> type");
>> 546: }
>> 547: return type.cast(result);
>
> Can we just use a `return (T) result` as there will always be a checked cast
> on the caller's end, and this call seems redundant? The only shortcoming is
> that the call will raise an unchecked warning that needs to be suppressed. Or
> is this negligible after hotspot optimization?
Eh, the scale here is quite small (never more than, say, 20 items), and an
instanceof + a cast is a routine coding style that I'd expect to be optimized
away. Doesn't seem worth the maintenance noise of a `@SuppressWarnings`.
-
PR: https://git.openjdk.java.net/jdk/pull/4346
Re: RFR: 8174222: LambdaMetafactory: validate inputs and improve documentation [v2]
On Fri, 4 Jun 2021 00:08:41 GMT, Mandy Chung wrote:
>> Dan Smith has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Fix accidentally commented-out parts of test
>
> src/java.base/share/classes/java/lang/invoke/LambdaMetafactory.java line 457:
>
>> 455: * @return a CallSite whose target can be used to perform capture,
>> generating
>> 456: * instances of the interface named by {@code factoryType}
>> 457: * @throws LambdaConversionException If {@code caller} does not
>> have private
>
> One additional comment:
>
> The lookup access has been extended since 14 to include `MODULE` and
> `ORIGINAL` access.
> `Lookup::hasFullPrivilegeAccess` returns true if the lookup has `PRIVATE` and
> `MODULE` which means that this lookup is not teleported from another module
> via `Lookup::in` and `MethodHandles::privateLookupIn`.
>
> What privilege do you expect the `caller` lookup should have? I believe full
> privilege access is the appropriate privilege. The `ORIGINAL` access is
> stricter as the lookup must be created from the original lookup class.
>
> [1] shows what access a `Lookup` has when being produced via different APIs.
>
> [1]
> https://download.java.net/java/early_access/jdk17/docs/api/java.base/java/lang/invoke/MethodHandles.Lookup.html#access-modes
Thanks, yes I think `hasFullPrivilegeAccess` is what we want. I updated the
javadoc, along with the actual check. Thanks for catching!
> test/jdk/java/lang/invoke/lambda/MetafactoryArgValidationTest.java line 2:
>
>> 1: /*
>> 2: * Copyright (c) 2019, Oracle and/or its affiliates. All rights reserved.
>
> copyright year needs update.
Fixed. And I bumped it on the other files, too.
-
PR: https://git.openjdk.java.net/jdk/pull/4346
RFR: 8268083: JDK-8267706 breaks bin/idea.sh on a Mac
I got rid of `realpath` usage as discussed in https://github.com/openjdk/jdk/pull/4190 and used `RelativePath` macro instead, however there were quite a few problems with this macro, here's the example: $(call RelativePath,/foo/bar,/foo/bar/baz) -> "..//foo/bar" $(call RelativePath,/foo/bar/baz/,/foo/bar/baz) -> SEGFAULT $(call RelativePath,/foo/bar/baz/banan,/foo/bar/) -> "./baz/banan" $(call RelativePath,/foo/bar/baz,/foo/bar/banan) -> "../baz" As you can see, 1st case is just plain wrong, 2nd crashes make because of infinite loop, 3rd can be simplified and all of them have leading whitespaces First commit in this PR fixes all these issues and adds corresponding test cases and second commit replaces usage of `realpath` in idea.sh with `RelativePath` macro in idea.gmk and fixes problems, when paths are incorrectly treated by IDEA - Commit messages: - 8268083: Got rid of realpath usage in bin/idea.sh - 8268083: Fix FindCommonPathPrefix and RelativePath macros in make/common/Utils.gmk Changes: https://git.openjdk.java.net/jdk/pull/4369/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=4369=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8268083 Stats: 219 lines in 11 files changed: 107 ins; 47 del; 65 mod Patch: https://git.openjdk.java.net/jdk/pull/4369.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4369/head:pull/4369 PR: https://git.openjdk.java.net/jdk/pull/4369
Re: Comment on JDK-826722: SoftReference not cleared on OutOfMemoryError: Requested array size exceeds VM limit
Hi Mandy,
the OOME thrown for VM limits reasons is not related to any purported
heap exhaustion but to the VM refusing to allocate an array of size
Integer.MAX_VALUE or Integer.MAX_VALUE - 1, *even* if there's plenty of
space.
For example, with 8 GiB of heap and a size of Integer.MAX_VALUE - 2 the
small program runs without fuss:
java -XX:+UseG1GC -Xms8g -Xmx8g -cp ... Softly 2147483645
But when the argument is increased by 1 to Integer.MAX_VALUE - 1
java -XX:+UseG1GC -Xms8g -Xmx8g -cp ... Softly 2147483646
you immediately get:
Exception in thread "main" java.lang.AssertionError: non-null referent
at Softly.main(Softly.java:26)
Caused by: java.lang.OutOfMemoryError: Requested array size exceeds VM limit
at Softly.main(Softly.java:15)
In other words, OOME is "abused" in such cases. A VM limit error should
really throw another kind of error, not OOME, because contrary to its
name there's not necessarily a lack of memory space, as shown here.
To parallel current behavior, thus, the spec should be amended as
proposed "... an OutOfMemoryError caused by Java heap space exhaustion."
or a similar wording.
Alternatively, to maintain the current spec untouched, the VM should
throw another kind of error for VM limits. Not sure if this has any
adverse impact on existing code in the wild.
Greetings
Raffaello
On 2021-06-04 21:16, Mandy Chung wrote:
I'm not sure if the spec should be updated. JDK-8267222 needs the GC
team to evaluate.
I have added my comment in this JBS issue.
The SoftReference spec has the guarantee:
“All soft references to softly-reachable objects are guaranteed to
have been cleared before the virtual machine throws an OutOfMemoryError.”
This is a reasonable guarantee expected by design in response to memory
demand.
For the OOME thrown due to "requested array size exceeds VM limit", it
seems that this is a fast-path throwing OOME without really going
through the object allocation request (where reference processing will
be performed in GC cycle).
The question to the GC team is whether VM implementation can and should
support this soft reference guarantee. Note that the soft reference
objects are cleared as specified, the large object allocation exceeding
VM limit would fail any way. If the implementation is feasible, I'm
inclined to clear the soft reference objects when OOME is thrown as
specified even the object allocation request is known to fail.
Mandy
On 6/3/21 11:57 AM, Raffaello Giulietti wrote:
Hi,
upon reading [1] I tried a similar scenario, but where OOME are caused
by "Java heap space" exhaustion rather than by VM limits.
import java.lang.ref.SoftReference;
import java.text.DecimalFormat;
import java.util.ArrayList;
public class Softly {
public static void main(String[] args) {
var size = Integer.parseInt(args[0]);
var format = new DecimalFormat("#,###");
var news = 0;
var ref = new SoftReference<>(new ArrayList<>());
for (;;) {
byte[] b = null;
try {
b = new byte[size];
++news;
ref.get().add(b);
} catch (NullPointerException __) {
System.out.format("totSize = %20s, allocations =
%d\n", format.format((long) news * size), news);
ref = new SoftReference<>(new ArrayList<>());
ref.get().add(b);
} catch (OutOfMemoryError e) {
if (ref.refersTo(null)) {
throw new AssertionError("allocations =
%d".formatted((news)), e);
}
throw new AssertionError("non-null referent", e);
}
}
}
}
E.g.,
java -XX:+UseG1GC -Xms1g -Xmx1g -cp ... Softly 8
Depending on the collector and how tight the heap is, I sometimes
observe a "Java heap space" OOME but then the referent of ref is null.
I never observed a OOME with a non-null referent for ref. Hence, in
scenarios where OOME are caused by heap exhaustion, soft refs seem to
work as advertised.
Tried on AdoptOpenJDK-16.0.1+9 with SerialGC, ParallelGC, G1GC, ZGC
and ShenandoahGC with either -Xms1g/-Xmx1g or -Xms2g/-Xmx2g (small
heaps) and various byte[] sizes.
Thus, the current wording in SoftReference's javadoc:
"All soft references to softly-reachable objects are guaranteed to
have been cleared before the virtual machine throws an OutOfMemoryError."
could be amended to read:
"All soft references to softly-reachable objects are guaranteed to
have been cleared before the virtual machine throws an
OutOfMemoryError caused by Java heap space exhaustion."
Greetings
Raffaello
[1] https://bugs.openjdk.java.net/browse/JDK-8267222
Re: RFR: 8262891: Compiler implementation for Pattern Matching for switch (Preview) [v12]
On Fri, 4 Jun 2021 18:23:28 GMT, Vicente Romero wrote:
>> Jan Lahoda has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Fixing typo.
>
> test/langtools/tools/javac/patterns/SealedTypeChanges.java line 71:
>
>> 69: }
>> 70:
>> 71: sealed interface SealedTypeChangesIntf permits SealedTypeChanges.A {}
>
> just for completeness shouldn't we have a test with sealed, non-abstract
> classes?
Note that for sealed non-abstract classes, the permits is not checked (as an
instance of the non-abstract class may be created and passed to the switch, the
switch needs to contain a case that will cover the class anyway). I've added
tests that check the behavior for abstract class, and non-abstract classes
(error is produced in the latter case).
-
PR: https://git.openjdk.java.net/jdk/pull/3863
Re: RFR: 8262891: Compiler implementation for Pattern Matching for switch (Preview) [v13]
> This is a preview of a patch implementing JEP 406: Pattern Matching for > switch (Preview): > https://bugs.openjdk.java.net/browse/JDK-8213076 > > The current draft of the specification is here: > http://cr.openjdk.java.net/~gbierman/jep406/jep406-20210430/specs/patterns-switch-jls.html > > A summary of notable parts of the patch: > -to support cases expressions and patterns in cases, there is a new common > superinterface for expressions and patterns, `CaseLabelTree`, which > expressions and patterns implement, and a list of case labels is returned > from `CaseTree.getLabels()`. > -to support `case default`, there is an implementation of `CaseLabelTree` > that represents it (`DefaultCaseLabelTree`). It is used also to represent the > conventional `default` internally and in the newly added methods. > -in the parser, parenthesized patterns and expressions need to be > disambiguated when parsing case labels. > -Lower has been enhanced to handle `case null` for ordinary (boxed-primitive, > String, enum) switches. This is a bit tricky for boxed primitives, as there > is no value that is not part of the input domain so that could be used to > represent `case null`. Requires a bit shuffling with values. > -TransPatterns has been enhanced to handle the pattern matching switch. It > produces code that delegates to a new bootstrap method, that will classify > the input value to the switch and return the case number, to which the switch > then jumps. To support guards, the switches (and the bootstrap method) are > restartable. The bootstrap method as such is written very simply so far, but > could be much more optimized later. > -nullable type patterns are `case String s, null`/`case null, String s`/`case > null: case String s:`/`case String s: case null:`, handling of these required > a few tricks in `Attr`, `Flow` and `TransPatterns`. > > The specdiff for the change is here (to be updated): > http://cr.openjdk.java.net/~jlahoda/8265981/specdiff.preview.01/overview-summary.html Jan Lahoda has updated the pull request incrementally with two additional commits since the last revision: - Applying review feedback. - Tweaking javadoc. - Changes: - all: https://git.openjdk.java.net/jdk/pull/3863/files - new: https://git.openjdk.java.net/jdk/pull/3863/files/8d4c02b4..e3c29759 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=3863=12 - incr: https://webrevs.openjdk.java.net/?repo=jdk=3863=11-12 Stats: 125 lines in 8 files changed: 91 ins; 10 del; 24 mod Patch: https://git.openjdk.java.net/jdk/pull/3863.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3863/head:pull/3863 PR: https://git.openjdk.java.net/jdk/pull/3863
Re: RFR: 8262891: Compiler implementation for Pattern Matching for switch (Preview) [v12]
On Fri, 4 Jun 2021 15:46:32 GMT, Paul Sandoz wrote:
>> Jan Lahoda has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Fixing typo.
>
> test/langtools/tools/javac/patterns/DisambiguateParenthesizedPattern.java
> line 72:
>
>> 70: SwitchTree st = (SwitchTree)
>> method.getBody().getStatements().get(0);
>> 71: CaseLabelTree label = st.getCases().get(0).getLabels().get(0);
>> 72: ExpressionType actualType = switch (label) {
>
> Should the test be careful of using a pattern match switch?
I don't think using the new feature in the tests is problematic (esp. javac
tests related to the feature). It helps to ensure the feature really works on
real code.
-
PR: https://git.openjdk.java.net/jdk/pull/3863
Re: RFR: 8262891: Compiler implementation for Pattern Matching for switch (Preview) [v12]
On Fri, 4 Jun 2021 09:46:31 GMT, Jan Lahoda wrote: >> This is a preview of a patch implementing JEP 406: Pattern Matching for >> switch (Preview): >> https://bugs.openjdk.java.net/browse/JDK-8213076 >> >> The current draft of the specification is here: >> http://cr.openjdk.java.net/~gbierman/jep406/jep406-20210430/specs/patterns-switch-jls.html >> >> A summary of notable parts of the patch: >> -to support cases expressions and patterns in cases, there is a new common >> superinterface for expressions and patterns, `CaseLabelTree`, which >> expressions and patterns implement, and a list of case labels is returned >> from `CaseTree.getLabels()`. >> -to support `case default`, there is an implementation of `CaseLabelTree` >> that represents it (`DefaultCaseLabelTree`). It is used also to represent >> the conventional `default` internally and in the newly added methods. >> -in the parser, parenthesized patterns and expressions need to be >> disambiguated when parsing case labels. >> -Lower has been enhanced to handle `case null` for ordinary >> (boxed-primitive, String, enum) switches. This is a bit tricky for boxed >> primitives, as there is no value that is not part of the input domain so >> that could be used to represent `case null`. Requires a bit shuffling with >> values. >> -TransPatterns has been enhanced to handle the pattern matching switch. It >> produces code that delegates to a new bootstrap method, that will classify >> the input value to the switch and return the case number, to which the >> switch then jumps. To support guards, the switches (and the bootstrap >> method) are restartable. The bootstrap method as such is written very simply >> so far, but could be much more optimized later. >> -nullable type patterns are `case String s, null`/`case null, String >> s`/`case null: case String s:`/`case String s: case null:`, handling of >> these required a few tricks in `Attr`, `Flow` and `TransPatterns`. >> >> The specdiff for the change is here (to be updated): >> http://cr.openjdk.java.net/~jlahoda/8265981/specdiff.preview.01/overview-summary.html > > Jan Lahoda has updated the pull request incrementally with one additional > commit since the last revision: > > Fixing typo. Thanks a lot for all the feedback. I've tried to do the requested changes in the recent commits. - PR: https://git.openjdk.java.net/jdk/pull/3863
Re: Comment on JDK-826722: SoftReference not cleared on OutOfMemoryError: Requested array size exceeds VM limit
I'm not sure if the spec should be updated. JDK-8267222 needs the GC
team to evaluate.
I have added my comment in this JBS issue.
The SoftReference spec has the guarantee:
“All soft references to softly-reachable objects are guaranteed to
have been cleared before the virtual machine throws an OutOfMemoryError.”
This is a reasonable guarantee expected by design in response to memory
demand.
For the OOME thrown due to "requested array size exceeds VM limit", it
seems that this is a fast-path throwing OOME without really going
through the object allocation request (where reference processing will
be performed in GC cycle).
The question to the GC team is whether VM implementation can and should
support this soft reference guarantee. Note that the soft reference
objects are cleared as specified, the large object allocation exceeding
VM limit would fail any way. If the implementation is feasible, I'm
inclined to clear the soft reference objects when OOME is thrown as
specified even the object allocation request is known to fail.
Mandy
On 6/3/21 11:57 AM, Raffaello Giulietti wrote:
Hi,
upon reading [1] I tried a similar scenario, but where OOME are caused
by "Java heap space" exhaustion rather than by VM limits.
import java.lang.ref.SoftReference;
import java.text.DecimalFormat;
import java.util.ArrayList;
public class Softly {
public static void main(String[] args) {
var size = Integer.parseInt(args[0]);
var format = new DecimalFormat("#,###");
var news = 0;
var ref = new SoftReference<>(new ArrayList<>());
for (;;) {
byte[] b = null;
try {
b = new byte[size];
++news;
ref.get().add(b);
} catch (NullPointerException __) {
System.out.format("totSize = %20s, allocations =
%d\n", format.format((long) news * size), news);
ref = new SoftReference<>(new ArrayList<>());
ref.get().add(b);
} catch (OutOfMemoryError e) {
if (ref.refersTo(null)) {
throw new AssertionError("allocations =
%d".formatted((news)), e);
}
throw new AssertionError("non-null referent", e);
}
}
}
}
E.g.,
java -XX:+UseG1GC -Xms1g -Xmx1g -cp ... Softly 8
Depending on the collector and how tight the heap is, I sometimes
observe a "Java heap space" OOME but then the referent of ref is null.
I never observed a OOME with a non-null referent for ref. Hence, in
scenarios where OOME are caused by heap exhaustion, soft refs seem to
work as advertised.
Tried on AdoptOpenJDK-16.0.1+9 with SerialGC, ParallelGC, G1GC, ZGC
and ShenandoahGC with either -Xms1g/-Xmx1g or -Xms2g/-Xmx2g (small
heaps) and various byte[] sizes.
Thus, the current wording in SoftReference's javadoc:
"All soft references to softly-reachable objects are guaranteed to
have been cleared before the virtual machine throws an OutOfMemoryError."
could be amended to read:
"All soft references to softly-reachable objects are guaranteed to
have been cleared before the virtual machine throws an
OutOfMemoryError caused by Java heap space exhaustion."
Greetings
Raffaello
[1] https://bugs.openjdk.java.net/browse/JDK-8267222
Re: RFR: 8236569: -Xss not multiple of 4K does not work for the main thread on macOS [v2]
> …d on macOS
>
> This patch simply round up the specified stack size to multiple of the system
> page size.
>
> Test is trivial, simply run java with -Xss option against following code. On
> MacOS, before the fix, running with `-Xss159k` and `-Xss160k` would get
> `7183` and `649` respectively. After fix, both would output `649`, while
> `-Xss161k` would be same as `-Xss164k` and see 691 as the output.
>
> ```code:java
> public class StackLeak {
> public int depth = 0;
> public void stackLeak() {
> depth++;
> stackLeak();
> }
>
> public static void main(String[] args) {
> var test = new StackLeak();
> try {
> test.stackLeak();
> } catch (Throwable e) {
> System.out.println(test.depth);
> }
> }
> }
Henry Jen has updated the pull request incrementally with one additional commit
since the last revision:
Avoid overflow on page size
-
Changes:
- all: https://git.openjdk.java.net/jdk/pull/4256/files
- new: https://git.openjdk.java.net/jdk/pull/4256/files/fb9b22d5..39b041d7
Webrevs:
- full: https://webrevs.openjdk.java.net/?repo=jdk=4256=01
- incr: https://webrevs.openjdk.java.net/?repo=jdk=4256=00-01
Stats: 15 lines in 2 files changed: 8 ins; 0 del; 7 mod
Patch: https://git.openjdk.java.net/jdk/pull/4256.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/4256/head:pull/4256
PR: https://git.openjdk.java.net/jdk/pull/4256
Re: RFR: 8262891: Compiler implementation for Pattern Matching for switch (Preview) [v12]
On Fri, 4 Jun 2021 09:46:31 GMT, Jan Lahoda wrote:
>> This is a preview of a patch implementing JEP 406: Pattern Matching for
>> switch (Preview):
>> https://bugs.openjdk.java.net/browse/JDK-8213076
>>
>> The current draft of the specification is here:
>> http://cr.openjdk.java.net/~gbierman/jep406/jep406-20210430/specs/patterns-switch-jls.html
>>
>> A summary of notable parts of the patch:
>> -to support cases expressions and patterns in cases, there is a new common
>> superinterface for expressions and patterns, `CaseLabelTree`, which
>> expressions and patterns implement, and a list of case labels is returned
>> from `CaseTree.getLabels()`.
>> -to support `case default`, there is an implementation of `CaseLabelTree`
>> that represents it (`DefaultCaseLabelTree`). It is used also to represent
>> the conventional `default` internally and in the newly added methods.
>> -in the parser, parenthesized patterns and expressions need to be
>> disambiguated when parsing case labels.
>> -Lower has been enhanced to handle `case null` for ordinary
>> (boxed-primitive, String, enum) switches. This is a bit tricky for boxed
>> primitives, as there is no value that is not part of the input domain so
>> that could be used to represent `case null`. Requires a bit shuffling with
>> values.
>> -TransPatterns has been enhanced to handle the pattern matching switch. It
>> produces code that delegates to a new bootstrap method, that will classify
>> the input value to the switch and return the case number, to which the
>> switch then jumps. To support guards, the switches (and the bootstrap
>> method) are restartable. The bootstrap method as such is written very simply
>> so far, but could be much more optimized later.
>> -nullable type patterns are `case String s, null`/`case null, String
>> s`/`case null: case String s:`/`case String s: case null:`, handling of
>> these required a few tricks in `Attr`, `Flow` and `TransPatterns`.
>>
>> The specdiff for the change is here (to be updated):
>> http://cr.openjdk.java.net/~jlahoda/8265981/specdiff.preview.01/overview-summary.html
>
> Jan Lahoda has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Fixing typo.
test/langtools/tools/javac/patterns/SealedTypeChanges.java line 58:
> 56: void statement(SealedTypeChangesIntf obj) {
> 57: switch (obj) {
> 58: case A a -> System.err.println(1);
what about having tests with a case that matches the sealed class?
test/langtools/tools/javac/patterns/SealedTypeChanges.java line 71:
> 69: }
> 70:
> 71: sealed interface SealedTypeChangesIntf permits SealedTypeChanges.A {}
just for completeness shouldn't we have a test with sealed, non-abstract
classes?
-
PR: https://git.openjdk.java.net/jdk/pull/3863
Integrated: 8268151: Vector API toShuffle optimization
On Thu, 3 Jun 2021 00:29:00 GMT, Sandhya Viswanathan wrote: > The Vector API toShuffle method can be optimized using existing vector > conversion intrinsic. > > The following changes are made: > 1) vector.toShuffle java implementation is changed to call > VectorSupport.convert. > 2) The conversion intrinsic (inline_vector_convert()) in vectorIntrinsics.cpp > is changed to allow shuffle as a destination type. > 3) The shuffle.toVector intrinsic (inline_vector_shuffle_to_vector()) in > vectorIntrinsics.cpp now explicitly generates conversion node instead of > performing conversion during unbox. This is to remove unnecessary boxing > during back to back vector.toShuffle and shuffle.toVector calls. > > Best Regards, > Sandhya This pull request has now been integrated. Changeset: 20b63127 Author:Sandhya Viswanathan URL: https://git.openjdk.java.net/jdk/commit/20b631278c0c89ccd9c16f2a29d47eb8414aacd5 Stats: 399 lines in 41 files changed: 165 ins; 197 del; 37 mod 8268151: Vector API toShuffle optimization Reviewed-by: psandoz, vlivanov - PR: https://git.openjdk.java.net/jdk/pull/4326
Re: RFR: 8268151: Vector API toShuffle optimization [v2]
On Fri, 4 Jun 2021 13:03:24 GMT, Vladimir Ivanov wrote: >> Sandhya Viswanathan has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Implement review comments > > Looks good. > > One inefficiency I noticed is that repeated `toVector()`/`toShuffle` leave a > trail of redundant `VectorCastB2X`/`VectorCast[S..L]2X` nodes behind. @iwanowww @XiaohongGong Thanks a lot for the review. @iwanowww I will take up the redundant VectorCastB2X/VectorCast[S..L]2X conversion optimizations separately. - PR: https://git.openjdk.java.net/jdk/pull/4326
Re: RFR: 8195129: System.load() fails to load from unicode paths [v5]
On Fri, 4 Jun 2021 13:36:27 GMT, Maxim Kartashev wrote: >> Character strings within JVM are produced and consumed in several formats. >> Strings come from/to Java in the UTF8 format and POSIX APIs (like fprintf() >> or dlopen()) consume strings also in UTF8. On Windows, however, the >> situation is far less simple: some new(er) APIs expect UTF16 (wide-character >> strings), some older APIs can only work with strings in a "platform" format, >> where not all UTF8 characters can be represented; which ones can depends on >> the current "code page". >> >> This commit switches the Windows version of native library loading code to >> using the new UTF16 API `LoadLibraryW()` and attempts to streamline the use >> of various string formats in the surrounding code. >> >> Namely, exception messages are made to consume strings explicitly in the >> UTF8 format, while logging functions (that end up using legacy Windows API) >> are made to consume "platform" strings in most cases. One exception is >> `JVM_LoadLibrary()` logging where the UTF8 name of the library is logged, >> which can, of course, be fixed, but was considered not worth the additional >> code (NB: this isn't a new bug). >> >> The test runs in a separate JVM in order to make NIO happy about non-ASCII >> characters in the file name; tests are executed with LC_ALL=C and that >> doesn't let NIO work with non-ASCII file names even on Linux or MacOS. >> >> Tested by running `test/hotspot/jtreg:tier1` on Linux and >> `jtreg:test/hotspot/jtreg/runtime` on Windows 10. The new test (` >> jtreg:test/hotspot/jtreg/runtime/jni/loadLibraryUnicode`) was explicitly ran >> on those platforms as well. >> >> Results from Linux: >> >> Test summary >> == >>TEST TOTAL PASS FAIL ERROR >> >>jtreg:test/hotspot/jtreg:tier1 1784 1784 0 0 >> >> == >> TEST SUCCESS >> >> >> Building target 'run-test-only' in configuration >> 'linux-x86_64-server-release' >> Test selection 'jtreg:test/hotspot/jtreg/runtime/jni/loadLibraryUnicode', >> will run: >> * jtreg:test/hotspot/jtreg/runtime/jni/loadLibraryUnicode >> >> Running test 'jtreg:test/hotspot/jtreg/runtime/jni/loadLibraryUnicode' >> Passed: runtime/jni/loadLibraryUnicode/LoadLibraryUnicodeTest.java >> Test results: passed: 1 >> >> >> Results from Windows 10: >> >> Test summary >> == >>TEST TOTAL PASS FAIL ERROR >>jtreg:test/hotspot/jtreg/runtime746 746 0 0 >> == >> TEST SUCCESS >> Finished building target 'run-test-only' in configuration >> 'windows-x86_64-server-fastdebug' >> >> >> Building target 'run-test-only' in configuration >> 'windows-x86_64-server-fastdebug' >> Test selection 'test/hotspot/jtreg/runtime/jni/loadLibraryUnicode', will run: >> * jtreg:test/hotspot/jtreg/runtime/jni/loadLibraryUnicode >> >> Running test 'jtreg:test/hotspot/jtreg/runtime/jni/loadLibraryUnicode' >> Passed: runtime/jni/loadLibraryUnicode/LoadLibraryUnicodeTest.java >> Test results: passed: 1 > > Maxim Kartashev has updated the pull request incrementally with one > additional commit since the last revision: > > Updated the test to run on Windows only and to use a character from the > supplementary plane in the path name. src/java.base/share/native/libjava/jni_util.c line 680: > 678: utf8Encoding).z; > 679: return isUTF8EncodingSupported; > 680: } It would be moot if we choose not to modify the JVM_ interface to standard UTF-8, but this function is not necessary. UTF-8 encoding is guaranteed in every Java implementation. - PR: https://git.openjdk.java.net/jdk/pull/4169
Re: RFR: 8195129: System.load() fails to load from unicode paths [v3]
On Fri, 4 Jun 2021 14:00:25 GMT, Maxim Kartashev wrote: >> Not an expert by my understanding is that the VM only deals with modified >> UTF-8, as does JNI. So the incoming string should be modified-UTF8 IMO and >> then converted to UTF16. >> >> That said, this is shared code being modified on the JDK side so you can't >> just change the type of string being passed in without updating all the >> implementations of os::dll_load to support that! > > I think we need to establish some common ground before proceeding further > with this fix. It's a bit of a long read; please, bear with me. > > The path name starts its life as a `jstring` in > `Java_jdk_internal_loader_NativeLibraries_load()`, its encoding is irrelevant > at this point. > > Next, the name has to be passed down to `JVM_LoadLibrary()` that takes > `char*`. So we need to convert form `jstring` to `char*` (point (a)). > Following that, `os::dll_load()` that actually performs loading in a > platform-specific manner also receives `char*`. All platform implementations > of `os::dll_load()` pass the path name down to their respective platform's > APIs unmodified, but I think that's just incidental and here we have another > possible point of conversion (point (b)). Other consumers of the path name > are exception(c) and logging(d) messages; they also take `char*`, but > potentially of a different encoding. > > Let me try to enumerate all conceivably valid conversions for > `JVM_LoadLibrary()` consumption (point (a)): > 1. jstring -> platform-specific encoding (status quo meaning possibly lossy > encoding on Windows and UTF-8 elsewhere AFAICT), > 2. jstring -> modified UTF-8, > 3. jstring -> UTF-8. > > This bug [8195129](https://bugs.openjdk.java.net/browse/JDK-8195129) occurs > because conversion (1) may loose information on Windows if the platform > encoding happens to be NOT UTF-8 (which it often - or even always - is). So > that's a no-go and we are left with either (2) or (3). > > On MacOS and Linux, "platform" encoding already is UTF-8 and since all the > platform APIs happily consume UTF-8, no further conversion is necessary > (neither for actual library loading, nor for log or exception messages; the > latter have to convert to UTF-16, but do that under the hood). > > On Windows, we require at least these variants of the path name: > 1. UTF16 for library loading (Unicode Windows API), > 2. "platform" encoding for logging (yes, loosing information here, but that's > tolerable), > 3. "platform" (lossy) or UTF8 (lossless) encoding for exception messages > (prefer lossless). > > This is what's behind my choice of UTF-8 for the path name encoding as it > gets passed down to `JVM_LoadLibrary()`. We can go with modified UTF-8, of > course, in which case all platforms - not just Windows - will have to do the > conversion on their own, loosing the benefit of the knowledge about the > original string encoding (the String.coder field of jstring). I think I am hesitant to change the JVM interface from modified UTF-8 to standard UTF-8, as it would be the only location in JNI/JVM interface that uses the standard UTF-8. Instead, I would implement `convert_UTF8_to_UTF16` or rather `convert_mUTF8_to_UTF16` with a fairly simple arithmetic logic. - PR: https://git.openjdk.java.net/jdk/pull/4169
Integrated: 8199318: add idempotent copy operation for Map.Entry
On Wed, 2 Jun 2021 00:39:25 GMT, Stuart Marks wrote: > I'm fixing this along with a couple intertwined issues. > > 1. Add Map.Entry::copyOf as an idempotent copy operation. > > 2. Clarify that AbstractMap.SimpleImmutableEntry is itself unmodifiable (not > really immutable) but that subclasses can be modifiable. > > 3. Clarify some confusing, historical wording about Map.Entry instances being > obtainable only via iteration of a Map's entry-set view. This was never > really true, since anyone could implement the Map.Entry interface, and it > certainly was not true since JDK 1.6 when SimpleEntry and > SimpleImmutableEntry were added. This pull request has now been integrated. Changeset: cd0678fc Author:Stuart Marks URL: https://git.openjdk.java.net/jdk/commit/cd0678fcf6bc00ecda3e61d959617c67d02dba3c Stats: 141 lines in 3 files changed: 120 ins; 2 del; 19 mod 8199318: add idempotent copy operation for Map.Entry Reviewed-by: alanb, psandoz, dfuchs - PR: https://git.openjdk.java.net/jdk/pull/4295
Re: RFR: 8262891: Compiler implementation for Pattern Matching for switch (Preview) [v12]
On Fri, 4 Jun 2021 09:46:31 GMT, Jan Lahoda wrote:
>> This is a preview of a patch implementing JEP 406: Pattern Matching for
>> switch (Preview):
>> https://bugs.openjdk.java.net/browse/JDK-8213076
>>
>> The current draft of the specification is here:
>> http://cr.openjdk.java.net/~gbierman/jep406/jep406-20210430/specs/patterns-switch-jls.html
>>
>> A summary of notable parts of the patch:
>> -to support cases expressions and patterns in cases, there is a new common
>> superinterface for expressions and patterns, `CaseLabelTree`, which
>> expressions and patterns implement, and a list of case labels is returned
>> from `CaseTree.getLabels()`.
>> -to support `case default`, there is an implementation of `CaseLabelTree`
>> that represents it (`DefaultCaseLabelTree`). It is used also to represent
>> the conventional `default` internally and in the newly added methods.
>> -in the parser, parenthesized patterns and expressions need to be
>> disambiguated when parsing case labels.
>> -Lower has been enhanced to handle `case null` for ordinary
>> (boxed-primitive, String, enum) switches. This is a bit tricky for boxed
>> primitives, as there is no value that is not part of the input domain so
>> that could be used to represent `case null`. Requires a bit shuffling with
>> values.
>> -TransPatterns has been enhanced to handle the pattern matching switch. It
>> produces code that delegates to a new bootstrap method, that will classify
>> the input value to the switch and return the case number, to which the
>> switch then jumps. To support guards, the switches (and the bootstrap
>> method) are restartable. The bootstrap method as such is written very simply
>> so far, but could be much more optimized later.
>> -nullable type patterns are `case String s, null`/`case null, String
>> s`/`case null: case String s:`/`case String s: case null:`, handling of
>> these required a few tricks in `Attr`, `Flow` and `TransPatterns`.
>>
>> The specdiff for the change is here (to be updated):
>> http://cr.openjdk.java.net/~jlahoda/8265981/specdiff.preview.01/overview-summary.html
>
> Jan Lahoda has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Fixing typo.
I reviewed the `java.base` change namely, SwitchBootstraps.java. Looks good.
src/java.base/share/classes/java/lang/runtime/SwitchBootstraps.java line 47:
> 45: * of the {@code switch}, implicitly numbered sequentially from {@code
> [0..N)}.
> 46: *
> 47: * The bootstrap call site accepts a single parameter of the type of
> the
It takes 2 parameters (not single parameter). Perhaps you can take out this
paragraph since it's specified in the `typeSwitch` method.
src/java.base/share/classes/java/lang/runtime/SwitchBootstraps.java line 110:
> 108: * @return a {@code CallSite} returning the first matching element
> as described above
> 109: *
> 110: * @throws NullPointerException if any argument is null
Suggestion:
* @throws NullPointerException if any argument is {@code null}
same formatting nit for other occurrenace of "null"
-
Marked as reviewed by mchung (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/3863
Integrated: 8268222: javax/xml/jaxp/unittest/transform/Bug6216226Test.java failed, cannot delete file
On Fri, 4 Jun 2021 05:14:13 GMT, Joe Wang wrote: > Revert changes in StreamResult.java made through > https://github.com/openjdk/jdk/pull/4318 since it was creating a file stream > on behalf of the Transformer, which resulted in a leaking file handle because > the Transformer would only close files it opened. > > This change instead replace the problematic file-uri-url-file conversion code > with nio Paths. While we generally don't make such changes if it's not > necessary as Apache still supports older versions of the JDK, we are okay > with a code level 8. This pull request has now been integrated. Changeset: b27599b3 Author:Joe Wang URL: https://git.openjdk.java.net/jdk/commit/b27599b3ec3fd344fa9fa97b7ecde85d5662ca6c Stats: 37 lines in 2 files changed: 2 ins; 24 del; 11 mod 8268222: javax/xml/jaxp/unittest/transform/Bug6216226Test.java failed, cannot delete file Reviewed-by: dfuchs - PR: https://git.openjdk.java.net/jdk/pull/4353
Re: RFR: 8268222: javax/xml/jaxp/unittest/transform/Bug6216226Test.java failed, cannot delete file
On Fri, 4 Jun 2021 08:50:09 GMT, Daniel Fuchs wrote: >> Revert changes in StreamResult.java made through >> https://github.com/openjdk/jdk/pull/4318 since it was creating a file stream >> on behalf of the Transformer, which resulted in a leaking file handle >> because the Transformer would only close files it opened. >> >> This change instead replace the problematic file-uri-url-file conversion >> code with nio Paths. While we generally don't make such changes if it's not >> necessary as Apache still supports older versions of the JDK, we are okay >> with a code level 8. > > src/java.xml/share/classes/com/sun/org/apache/xalan/internal/xsltc/trax/TransformerImpl.java > line 52: > >> 50: import java.net.UnknownServiceException; >> 51: import java.nio.file.Path; >> 52: import java.nio.file.Paths; > > Nit: you should not need to use Paths. `Paths.get(URI)` just calls > `Path.of(URI)` Right, but Path.of was introduced in JDK 11. I hope to avoid more advanced features if we can keep the code level at 8. There had been previous cases where we stayed at 8. So far, only a few classes need to be modified for java.xml to compile at 8. - PR: https://git.openjdk.java.net/jdk/pull/4353
Re: RFR: 8262891: Compiler implementation for Pattern Matching for switch (Preview) [v12]
On Fri, 4 Jun 2021 09:46:31 GMT, Jan Lahoda wrote:
>> This is a preview of a patch implementing JEP 406: Pattern Matching for
>> switch (Preview):
>> https://bugs.openjdk.java.net/browse/JDK-8213076
>>
>> The current draft of the specification is here:
>> http://cr.openjdk.java.net/~gbierman/jep406/jep406-20210430/specs/patterns-switch-jls.html
>>
>> A summary of notable parts of the patch:
>> -to support cases expressions and patterns in cases, there is a new common
>> superinterface for expressions and patterns, `CaseLabelTree`, which
>> expressions and patterns implement, and a list of case labels is returned
>> from `CaseTree.getLabels()`.
>> -to support `case default`, there is an implementation of `CaseLabelTree`
>> that represents it (`DefaultCaseLabelTree`). It is used also to represent
>> the conventional `default` internally and in the newly added methods.
>> -in the parser, parenthesized patterns and expressions need to be
>> disambiguated when parsing case labels.
>> -Lower has been enhanced to handle `case null` for ordinary
>> (boxed-primitive, String, enum) switches. This is a bit tricky for boxed
>> primitives, as there is no value that is not part of the input domain so
>> that could be used to represent `case null`. Requires a bit shuffling with
>> values.
>> -TransPatterns has been enhanced to handle the pattern matching switch. It
>> produces code that delegates to a new bootstrap method, that will classify
>> the input value to the switch and return the case number, to which the
>> switch then jumps. To support guards, the switches (and the bootstrap
>> method) are restartable. The bootstrap method as such is written very simply
>> so far, but could be much more optimized later.
>> -nullable type patterns are `case String s, null`/`case null, String
>> s`/`case null: case String s:`/`case String s: case null:`, handling of
>> these required a few tricks in `Attr`, `Flow` and `TransPatterns`.
>>
>> The specdiff for the change is here (to be updated):
>> http://cr.openjdk.java.net/~jlahoda/8265981/specdiff.preview.01/overview-summary.html
>
> Jan Lahoda has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Fixing typo.
A really nice feature, and a significant amount of work in javac. I mostly
focused on the bootstrap and API aspects, and left some minor comments (most of
which you can choose to apply or not as you see fit).
I suspect the bootstrap might evolve as we get feedback and switch is enhanced
with further forms of matching. But, at the moment it looks good.
src/java.base/share/classes/java/lang/runtime/SwitchBootstraps.java line 87:
> 85: * returns {@literal -1}.
> 86: *
> 87: * the {@code target} is not {@code null}, then the method of the
> call site
Suggestion:
* If the {@code target} is not {@code null}, then the method of the call
site
src/java.base/share/classes/java/lang/runtime/SwitchBootstraps.java line 92:
> 90: *
> 91: * the element is of type {@code Class} and the target value
> 92: * is a subtype of this {@code Class}; or
Suggestion:
* the element is of type {@code Class} that is assignable
* from the target's class; or
src/java.base/share/classes/java/lang/runtime/SwitchBootstraps.java line 162:
> 160: return i;
> 161: } else {
> 162: if (label instanceof Integer constant) {
Minor suggestion: use `else if` rather than nest
src/java.base/share/classes/java/lang/runtime/SwitchBootstraps.java line 166:
> 164: return i;
> 165: }
> 166: if (target instanceof Character input &&
> constant.intValue() == input.charValue()) {
Use `else if`
src/jdk.compiler/share/classes/com/sun/source/tree/CaseLabelTree.java line 31:
> 29:
> 30: /**A marker interface for {@code Tree}s that may be used as {@link
> CaseTree} labels.
> 31: *
Suggestion:
/**
* A marker interface for {@code Tree}s that may be used as {@link CaseTree}
labels.
*
src/jdk.compiler/share/classes/com/sun/source/tree/DefaultCaseLabelTree.java
line 30:
> 28:
> 29: /** A case label that marks {@code default} in {@code case null, default}.
> 30: * @since 17
Suggestion:
/**
* A case label that marks {@code default} in {@code case null, default}.
*
* @since 17
test/jdk/java/lang/runtime/SwitchBootstrapsTest.java line 55:
> 53: catch (NoSuchMethodException | IllegalAccessException e) {
> 54: throw new RuntimeException(e);
> 55: }
Suggestion:
catch (ReflectiveOperationException e) {
throw new AssertionError(e, "Should not happen");
}
test/jdk/java/lang/runtime/SwitchBootstrapsTest.java line 73:
> 71: }
> 72:
> 73: public void testTypes() throws Throwable {
As a follow up issue consider adding tests for `null` values
Re: RFR: 8268124: Update java.lang to use switch expressions [v3]
On Thu, 3 Jun 2021 11:35:13 GMT, Rémi Forax wrote: >> My mistake. I've replaced the colon now with the lambda operator. See a8706b0 > >> My mistake. I've replaced the colon now with the lambda operator. > > Drive by comment, in term of name, `->` is the arrow operator not the lambda > operator. > - lambda = parameters + arrow + code > - arrow case = case + arrow + code > > The difference is important because a lambda is a function while an arrow > case is a block. Ah OK, good to know. Thanks Remi! - PR: https://git.openjdk.java.net/jdk/pull/4312
Re: RFR: 8263512: [macos_aarch64] issues with calling va_args functions from invoke_native [v6]
On Fri, 4 Jun 2021 15:10:18 GMT, Nick Gasson wrote: >> macOS on Apple silicon uses slightly different ABI conventions to the >> standard AArch64 ABI. The differences are outlined in [1]. In >> particular in the standard (AAPCS) ABI, variadic arguments may be passed >> in either registers or on the stack following the normal calling >> convention. To handle this, va_list is a struct containing separate >> pointers for arguments located in integer registers, floating point >> registers, and on the stack. Apple's ABI simplifies this by passing all >> variadic arguments on the stack and the va_list type becomes a simple >> char* pointer. >> >> This patch adds a new MacOsAArch64 CABI type and MacOsAArch64Linker to >> represent the new ABI variant on macOS. StackVaList is based on >> WinVaList lightly modified to handle the different TypeClasses on >> AArch64. The original AArch64Linker is renamed to AapcsLinker and is >> currently used for all non-Mac platforms. I think we also need to add a >> WinAArch64 CABI but I haven't yet been able to test on a Windows system >> so will do that later. >> >> The macOS ABI also uses a different method of spilling arguments to the >> stack (the standard ABI pads each argument to a multiple of 8 byte stack >> slots, but the Mac ABI packs arguments according to their natural >> alignment). None of the existing tests exercise this so I'll open a new >> JBS issue and work on that separately. >> >> Tested jdk_foreign on macOS AArch64, Linux AArch64, and Linux X86_64. >> >> [1] >> https://developer.apple.com/documentation/xcode/writing_arm64_code_for_apple_platforms > > Nick Gasson has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains ten commits: > > - Fix build after merge > - Merge master > - Redundant cast to long > - Merge master > >Change-Id: I49bef0437b4c47bef8bf74d192299d06b25e1555 >CustomizedGitHooks: yes > - Update test/jdk/java/foreign/valist/VaListTest.java > >Co-authored-by: Jorn Vernee > - No variadic upcalls > >Change-Id: Ibf91c570c88be2587e9e23240477c4a5cc56b4c5 >CustomizedGitHooks: yes > - Fixes after JEP integratioN > >Change-Id: Iaa13b3869522c8814c3f7ef4c1eac8e8267657e6 >CustomizedGitHooks: yes > - merge master > >Change-Id: Ic06fec084099ff2053dd251a255cbbf4a64a59d7 >CustomizedGitHooks: yes > - 8263512: [macos_aarch64] issues with calling va_args functions from > invoke_native > >macOS on Apple silicon uses slightly different ABI conventions to the >standard AArch64 ABI. The differences are outlined in [1]. In >particular in the standard (AAPCS) ABI, variadic arguments may be passed >in either registers or on the stack following the normal calling >convention. To handle this, va_list is a struct containing separate >pointers for arguments located in integer registers, floating point >registers, and on the stack. Apple's ABI simplifies this by passing all >variadic arguments on the stack and the va_list type becomes a simple >char* pointer. > >This patch adds a new MacOsAArch64 CABI type and MacOsAArch64Linker to >represent the new ABI variant on macOS. StackVaList is based on >WinVaList lightly modified to handle the different TypeClasses on >AArch64. The original AArch64Linker is renamed to AapcsLinker and is >currently used for all non-Mac platforms. I think we also need to add a >WinAArch64 CABI but I haven't yet been able to test on a Windows system >so will do that later. > >The macOS ABI also uses a different method of spilling arguments to the >stack (the standard ABI pads each argument to a multiple of 8 byte stack >slots, but the Mac ABI packs arguments according to their natural >alignment). None of the existing tests exercise this so I'll open a new >JBS issue and work on that separately. > >Tested jdk_foreign on macOS AArch64, Linux AArch64, and Linux X86_64. Marked as reviewed by jvernee (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/3617
Re: RFR: 8263512: [macos_aarch64] issues with calling va_args functions from invoke_native [v3]
On Fri, 4 Jun 2021 11:07:27 GMT, Jorn Vernee wrote:
>> test/jdk/java/foreign/valist/VaListTest.java line 706:
>>
>>> 704: vaList.skip(BigPoint_LAYOUT);
>>> 705: assertEquals((long) vaList.vargAsLong(C_LONG), 42);
>>> 706: })},
>>
>> Looks like a carrier size mismatch here:
>>
>>
>> java.lang.IllegalArgumentException: Carrier size mismatch: long !=
>> b32[abi/kind=LONG]
>> at
>> jdk.incubator.foreign/jdk.internal.foreign.Utils.checkPrimitiveCarrierCompat(Utils.java:111)
>> at
>> jdk.incubator.foreign/jdk.internal.foreign.abi.SharedUtils.checkCompatibleType(SharedUtils.java:231)
>> at
>> jdk.incubator.foreign/jdk.internal.foreign.abi.x64.windows.WinVaList.read(WinVaList.java:114)
>> at
>> jdk.incubator.foreign/jdk.internal.foreign.abi.x64.windows.WinVaList.read(WinVaList.java:109)
>> at
>> jdk.incubator.foreign/jdk.internal.foreign.abi.x64.windows.WinVaList.vargAsLong(WinVaList.java:84)
>> at VaListTest.lambda$upcalls$58(VaListTest.java:705)
>> at
>> jdk.incubator.foreign/jdk.internal.foreign.abi.ProgrammableInvoker.invokeNative(Native
>> Method)
>> at
>> jdk.incubator.foreign/jdk.internal.foreign.abi.ProgrammableInvoker.invokeMoves(ProgrammableInvoker.java:290)
>> at VaListTest.testUpcall(VaListTest.java:530)
>>
>>
>> Need to use `C_LONG_LONG` to be cross-platform compatible. (sorry for not
>> noticing this).
>>
>> Suggestion:
>>
>> { linkVaListCB("upcallBigStructPlusScalar"),
>> VaListConsumer.mh(vaList -> {
>> MemorySegment struct =
>> vaList.vargAsSegment(BigPoint_LAYOUT, ResourceScope.newImplicitScope());
>> assertEquals((long) VH_BigPoint_x.get(struct), 8);
>> assertEquals((long) VH_BigPoint_y.get(struct), 16);
>>
>> assertEquals((long) vaList.vargAsLong(C_LONG_LONG), 42);
>> })},
>> { linkVaListCB("upcallBigStructPlusScalar"),
>> VaListConsumer.mh(vaList -> {
>> vaList.skip(BigPoint_LAYOUT);
>> assertEquals((long) vaList.vargAsLong(C_LONG_LONG), 42);
>> })},
>
> Also, it looks like the cast to `(long)` on the `vaList.vargAsLong` lines is
> redundant (thanks IntelliJ).
Thanks for your help! These are both fixed now.
-
PR: https://git.openjdk.java.net/jdk/pull/3617
Re: RFR: 8263512: [macos_aarch64] issues with calling va_args functions from invoke_native
On Fri, 4 Jun 2021 12:57:17 GMT, Maurizio Cimadamore wrote: > > That fix has a switch on the ABI type in the SystemLookup class (a new class > introduced by that fix). I believe that switch will no longer compile with > the changes in this PR as the ABI enum constants have changed - hopefully the > fix should be easy - e.g. just replace the case label with references AARCH64 > with a compound case label which covers both ARM/linux and ARM/MacOS. OK sure, the last commit fixes that. - PR: https://git.openjdk.java.net/jdk/pull/3617
Re: RFR: 8263512: [macos_aarch64] issues with calling va_args functions from invoke_native [v6]
> macOS on Apple silicon uses slightly different ABI conventions to the > standard AArch64 ABI. The differences are outlined in [1]. In > particular in the standard (AAPCS) ABI, variadic arguments may be passed > in either registers or on the stack following the normal calling > convention. To handle this, va_list is a struct containing separate > pointers for arguments located in integer registers, floating point > registers, and on the stack. Apple's ABI simplifies this by passing all > variadic arguments on the stack and the va_list type becomes a simple > char* pointer. > > This patch adds a new MacOsAArch64 CABI type and MacOsAArch64Linker to > represent the new ABI variant on macOS. StackVaList is based on > WinVaList lightly modified to handle the different TypeClasses on > AArch64. The original AArch64Linker is renamed to AapcsLinker and is > currently used for all non-Mac platforms. I think we also need to add a > WinAArch64 CABI but I haven't yet been able to test on a Windows system > so will do that later. > > The macOS ABI also uses a different method of spilling arguments to the > stack (the standard ABI pads each argument to a multiple of 8 byte stack > slots, but the Mac ABI packs arguments according to their natural > alignment). None of the existing tests exercise this so I'll open a new > JBS issue and work on that separately. > > Tested jdk_foreign on macOS AArch64, Linux AArch64, and Linux X86_64. > > [1] > https://developer.apple.com/documentation/xcode/writing_arm64_code_for_apple_platforms Nick Gasson has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains ten commits: - Fix build after merge - Merge master - Redundant cast to long - Merge master Change-Id: I49bef0437b4c47bef8bf74d192299d06b25e1555 CustomizedGitHooks: yes - Update test/jdk/java/foreign/valist/VaListTest.java Co-authored-by: Jorn Vernee - No variadic upcalls Change-Id: Ibf91c570c88be2587e9e23240477c4a5cc56b4c5 CustomizedGitHooks: yes - Fixes after JEP integratioN Change-Id: Iaa13b3869522c8814c3f7ef4c1eac8e8267657e6 CustomizedGitHooks: yes - merge master Change-Id: Ic06fec084099ff2053dd251a255cbbf4a64a59d7 CustomizedGitHooks: yes - 8263512: [macos_aarch64] issues with calling va_args functions from invoke_native macOS on Apple silicon uses slightly different ABI conventions to the standard AArch64 ABI. The differences are outlined in [1]. In particular in the standard (AAPCS) ABI, variadic arguments may be passed in either registers or on the stack following the normal calling convention. To handle this, va_list is a struct containing separate pointers for arguments located in integer registers, floating point registers, and on the stack. Apple's ABI simplifies this by passing all variadic arguments on the stack and the va_list type becomes a simple char* pointer. This patch adds a new MacOsAArch64 CABI type and MacOsAArch64Linker to represent the new ABI variant on macOS. StackVaList is based on WinVaList lightly modified to handle the different TypeClasses on AArch64. The original AArch64Linker is renamed to AapcsLinker and is currently used for all non-Mac platforms. I think we also need to add a WinAArch64 CABI but I haven't yet been able to test on a Windows system so will do that later. The macOS ABI also uses a different method of spilling arguments to the stack (the standard ABI pads each argument to a multiple of 8 byte stack slots, but the Mac ABI packs arguments according to their natural alignment). None of the existing tests exercise this so I'll open a new JBS issue and work on that separately. Tested jdk_foreign on macOS AArch64, Linux AArch64, and Linux X86_64. - Changes: https://git.openjdk.java.net/jdk/pull/3617/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=3617=05 Stats: 778 lines in 15 files changed: 571 ins; 115 del; 92 mod Patch: https://git.openjdk.java.net/jdk/pull/3617.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3617/head:pull/3617 PR: https://git.openjdk.java.net/jdk/pull/3617
Re: RFR: JDK-8266918: merge_stack in check_code.c add NULL check [v2]
On Fri, 4 Jun 2021 14:10:20 GMT, Matthias Baesken wrote: >> Hello, please review this small Sonar finding. >> Sonar reports a potential NULL pointer dereference here : >> https://sonarcloud.io/project/issues?id=shipilev_jdk=c=AXck8CPLBBG2CXpcnh_z=false=MAJOR=BUG >> "Access to field 'item' results in a dereference of a null pointer (loaded >> from variable 'new')" >> It would be better to add a check . > > Matthias Baesken has updated the pull request incrementally with one > additional commit since the last revision: > > Replace check by assertion The change looks good to me. - Marked as reviewed by rschmelter (Committer). PR: https://git.openjdk.java.net/jdk/pull/3979
Re: RFR: JDK-8266918: merge_stack in check_code.c add NULL check [v2]
> Hello, please review this small Sonar finding. > Sonar reports a potential NULL pointer dereference here : > https://sonarcloud.io/project/issues?id=shipilev_jdk=c=AXck8CPLBBG2CXpcnh_z=false=MAJOR=BUG > "Access to field 'item' results in a dereference of a null pointer (loaded > from variable 'new')" > It would be better to add a check . Matthias Baesken has updated the pull request incrementally with one additional commit since the last revision: Replace check by assertion - Changes: - all: https://git.openjdk.java.net/jdk/pull/3979/files - new: https://git.openjdk.java.net/jdk/pull/3979/files/eeff35ef..d58c996e Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=3979=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=3979=00-01 Stats: 2 lines in 1 file changed: 1 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/3979.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3979/head:pull/3979 PR: https://git.openjdk.java.net/jdk/pull/3979
Re: RFR: 8195129: System.load() fails to load from unicode paths [v3]
On Fri, 4 Jun 2021 02:12:42 GMT, David Holmes wrote: >> I am not sure we can pass non `modified UTF-8` through `JVM_LoadLibrary()`. >> Probably some VM folks can enlighten here? > > Not an expert by my understanding is that the VM only deals with modified > UTF-8, as does JNI. So the incoming string should be modified-UTF8 IMO and > then converted to UTF16. > > That said, this is shared code being modified on the JDK side so you can't > just change the type of string being passed in without updating all the > implementations of os::dll_load to support that! I think we need to establish some common ground before proceeding further with this fix. It's a bit of a long read; please, bear with me. The path name starts its life as a `jstring` in `Java_jdk_internal_loader_NativeLibraries_load()`, its encoding is irrelevant at this point. Next, the name has to be passed down to `JVM_LoadLibrary()` that takes `char*`. So we need to convert form `jstring` to `char*` (point (a)). Following that, `os::dll_load()` that actually performs loading in a platform-specific manner also receives `char*`. All platform implementations of `os::dll_load()` pass the path name down to their respective platform's APIs unmodified, but I think that's just incidental and here we have another possible point of conversion (point (b)). Other consumers of the path name are exception(c) and logging(d) messages; they also take `char*`, but potentially of a different encoding. Let me try to enumerate all conceivably valid conversions for `JVM_LoadLibrary()` consumption (point (a)): 1. jstring -> platform-specific encoding (status quo meaning possibly lossy encoding on Windows and UTF-8 elsewhere AFAICT), 2. jstring -> modified UTF-8, 3. jstring -> UTF-8. This bug [8195129](https://bugs.openjdk.java.net/browse/JDK-8195129) occurs because conversion (1) may loose information on Windows if the platform encoding happens to be NOT UTF-8 (which it often - or even always - is). So that's a no-go and we are left with either (2) or (3). On MacOS and Linux, "platform" encoding already is UTF-8 and since all the platform APIs happily consume UTF-8, no further conversion is necessary (neither for actual library loading, nor for log or exception messages; the latter have to convert to UTF-16, but do that under the hood). On Windows, we require at least these variants of the path name: 1. UTF16 for library loading (Unicode Windows API), 2. "platform" encoding for logging (yes, loosing information here, but that's tolerable), 3. "platform" (lossy) or UTF8 (lossless) encoding for exception messages (prefer lossless). This is what's behind my choice of UTF-8 for the path name encoding as it gets passed down to `JVM_LoadLibrary()`. We can go with modified UTF-8, of course, in which case all platforms - not just Windows - will have to do the conversion on their own, loosing the benefit of the knowledge about the original string encoding (the String.coder field of jstring). - PR: https://git.openjdk.java.net/jdk/pull/4169
Re: RFR: 8195129: System.load() fails to load from unicode paths [v5]
> Character strings within JVM are produced and consumed in several formats. > Strings come from/to Java in the UTF8 format and POSIX APIs (like fprintf() > or dlopen()) consume strings also in UTF8. On Windows, however, the situation > is far less simple: some new(er) APIs expect UTF16 (wide-character strings), > some older APIs can only work with strings in a "platform" format, where not > all UTF8 characters can be represented; which ones can depends on the current > "code page". > > This commit switches the Windows version of native library loading code to > using the new UTF16 API `LoadLibraryW()` and attempts to streamline the use > of various string formats in the surrounding code. > > Namely, exception messages are made to consume strings explicitly in the UTF8 > format, while logging functions (that end up using legacy Windows API) are > made to consume "platform" strings in most cases. One exception is > `JVM_LoadLibrary()` logging where the UTF8 name of the library is logged, > which can, of course, be fixed, but was considered not worth the additional > code (NB: this isn't a new bug). > > The test runs in a separate JVM in order to make NIO happy about non-ASCII > characters in the file name; tests are executed with LC_ALL=C and that > doesn't let NIO work with non-ASCII file names even on Linux or MacOS. > > Tested by running `test/hotspot/jtreg:tier1` on Linux and > `jtreg:test/hotspot/jtreg/runtime` on Windows 10. The new test (` > jtreg:test/hotspot/jtreg/runtime/jni/loadLibraryUnicode`) was explicitly ran > on those platforms as well. > > Results from Linux: > > Test summary > == >TEST TOTAL PASS FAIL ERROR > >jtreg:test/hotspot/jtreg:tier1 1784 1784 0 0 > > == > TEST SUCCESS > > > Building target 'run-test-only' in configuration 'linux-x86_64-server-release' > Test selection 'jtreg:test/hotspot/jtreg/runtime/jni/loadLibraryUnicode', > will run: > * jtreg:test/hotspot/jtreg/runtime/jni/loadLibraryUnicode > > Running test 'jtreg:test/hotspot/jtreg/runtime/jni/loadLibraryUnicode' > Passed: runtime/jni/loadLibraryUnicode/LoadLibraryUnicodeTest.java > Test results: passed: 1 > > > Results from Windows 10: > > Test summary > == >TEST TOTAL PASS FAIL ERROR >jtreg:test/hotspot/jtreg/runtime746 746 0 0 > == > TEST SUCCESS > Finished building target 'run-test-only' in configuration > 'windows-x86_64-server-fastdebug' > > > Building target 'run-test-only' in configuration > 'windows-x86_64-server-fastdebug' > Test selection 'test/hotspot/jtreg/runtime/jni/loadLibraryUnicode', will run: > * jtreg:test/hotspot/jtreg/runtime/jni/loadLibraryUnicode > > Running test 'jtreg:test/hotspot/jtreg/runtime/jni/loadLibraryUnicode' > Passed: runtime/jni/loadLibraryUnicode/LoadLibraryUnicodeTest.java > Test results: passed: 1 Maxim Kartashev has updated the pull request incrementally with one additional commit since the last revision: Updated the test to run on Windows only and to use a character from the supplementary plane in the path name. - Changes: - all: https://git.openjdk.java.net/jdk/pull/4169/files - new: https://git.openjdk.java.net/jdk/pull/4169/files/97c918ca..a5d45dca Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=4169=04 - incr: https://webrevs.openjdk.java.net/?repo=jdk=4169=03-04 Stats: 8 lines in 2 files changed: 1 ins; 5 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/4169.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4169/head:pull/4169 PR: https://git.openjdk.java.net/jdk/pull/4169
Re: RFR: 8195129: System.load() fails to load from unicode paths [v3]
On Thu, 3 Jun 2021 17:51:54 GMT, Naoto Sato wrote: > I think we can simply limit the test platform only to Windows in @requires > tag in the test. Also, I would see the test case using some supplementary > characters. Done. - PR: https://git.openjdk.java.net/jdk/pull/4169
Re: RFR: 8268151: Vector API toShuffle optimization [v2]
On Thu, 3 Jun 2021 21:43:19 GMT, Sandhya Viswanathan wrote: >> The Vector API toShuffle method can be optimized using existing vector >> conversion intrinsic. >> >> The following changes are made: >> 1) vector.toShuffle java implementation is changed to call >> VectorSupport.convert. >> 2) The conversion intrinsic (inline_vector_convert()) in >> vectorIntrinsics.cpp is changed to allow shuffle as a destination type. >> 3) The shuffle.toVector intrinsic (inline_vector_shuffle_to_vector()) in >> vectorIntrinsics.cpp now explicitly generates conversion node instead of >> performing conversion during unbox. This is to remove unnecessary boxing >> during back to back vector.toShuffle and shuffle.toVector calls. >> >> Best Regards, >> Sandhya > > Sandhya Viswanathan has updated the pull request incrementally with one > additional commit since the last revision: > > Implement review comments Looks good. One inefficiency I noticed is that repeated `toVector()`/`toShuffle` leave a trail of redundant `VectorCastB2X`/`VectorCast[S..L]2X` nodes behind. - Marked as reviewed by vlivanov (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/4326
Re: RFR: 8263512: [macos_aarch64] issues with calling va_args functions from invoke_native
On Fri, 4 Jun 2021 10:06:26 GMT, Nick Gasson wrote:
>> The JEP has been integrated, so we can pick this back up (and handle it as a
>> bug for 17 even after the fork).
>>
>> Thank you for your patience.
>
> Thanks @JornVernee! I noticed VaListTest has started failing on Windows with
> this error:
>
> test
> VaListTest.testUpcall(java.lang.invoke.BoundMethodHandle$Species_LLL@198ebce4,
> MethodHandle(VaList)void): success
> test
> VaListTest.testUpcall(java.lang.invoke.BoundMethodHandle$Species_LLL@7a97cd30,
> MethodHandle(VaList)void): success
> Uncaught exception:
> java.lang.IllegalArgumentException
> {0xd6506c20} - klass: 'java/lang/IllegalArgumentException'
> #
> # A fatal error has been detected by the Java Runtime Environment:
> #
> # Internal Error (universalUpcallHandler.cpp:113), pid=13972, tid=23500
> # Error: ShouldNotReachHere()
> #
>
>
> I guess it must be related to the two new cases I added and the Windows code
> is now throwing an IllegalArgumentException but I can't see where from. Any
> ideas?
@nick-arm, I've just integrated a fix which, I believe will create a minor
build issue with the changes in this patch:
https://git.openjdk.java.net/jdk/pull/4316
That fix has a switch on the ABI type in the SystemLookup class (a new class
introduced by that fix). I believe that switch will no longer compile with the
changes in this PR as the ABI enum constants have changed - hopefully the fix
should be easy - e.g. just replace the case label with references AARCH64 with
a compound case label which covers both ARM/linux and ARM/MacOS.
-
PR: https://git.openjdk.java.net/jdk/pull/3617
Integrated: 8268129: LibraryLookup::ofDefault leaks symbols from loaded libraries
On Wed, 2 Jun 2021 17:19:06 GMT, Maurizio Cimadamore wrote: > This patch overhauls the library loading mechanism used by the Foreign Linker > API. We realized that, while handy, the *default* lookup abstraction > (`LibraryLookup::ofDefault`) was behaving inconsistentlt across platforms. > > This patch replaces `LibraryLookup` with a simpler `SymbolLookup` > abstraction, a functional interface. Crucially, `SymbolLookup` does not > concern with library loading, only symbol lookup. For this reason, two > factories are added: > > * `SymbolLookup::loaderLookup` - which obtains a lookup that can be used to > lookup symbols in libraries loaded by current loader > * `CLinker::systemLookup` - a more stable replacement for the *default* > lookup, which looks for symbols in libc.so (or its equivalent in other > platforms). The contents of this lookup are unspecified. > > Both factories are *restricted*, so they can only be called when > `--enable-native-access` is set. This pull request has now been integrated. Changeset: 59a539fe Author:Maurizio Cimadamore URL: https://git.openjdk.java.net/jdk/commit/59a539fef12dec6ba8af8a41000829402e7e9b72 Stats: 1351 lines in 47 files changed: 626 ins; 621 del; 104 mod 8268129: LibraryLookup::ofDefault leaks symbols from loaded libraries Reviewed-by: jvernee, psandoz - PR: https://git.openjdk.java.net/jdk/pull/4316
Re: RFR: 8263512: [macos_aarch64] issues with calling va_args functions from invoke_native [v5]
> macOS on Apple silicon uses slightly different ABI conventions to the > standard AArch64 ABI. The differences are outlined in [1]. In > particular in the standard (AAPCS) ABI, variadic arguments may be passed > in either registers or on the stack following the normal calling > convention. To handle this, va_list is a struct containing separate > pointers for arguments located in integer registers, floating point > registers, and on the stack. Apple's ABI simplifies this by passing all > variadic arguments on the stack and the va_list type becomes a simple > char* pointer. > > This patch adds a new MacOsAArch64 CABI type and MacOsAArch64Linker to > represent the new ABI variant on macOS. StackVaList is based on > WinVaList lightly modified to handle the different TypeClasses on > AArch64. The original AArch64Linker is renamed to AapcsLinker and is > currently used for all non-Mac platforms. I think we also need to add a > WinAArch64 CABI but I haven't yet been able to test on a Windows system > so will do that later. > > The macOS ABI also uses a different method of spilling arguments to the > stack (the standard ABI pads each argument to a multiple of 8 byte stack > slots, but the Mac ABI packs arguments according to their natural > alignment). None of the existing tests exercise this so I'll open a new > JBS issue and work on that separately. > > Tested jdk_foreign on macOS AArch64, Linux AArch64, and Linux X86_64. > > [1] > https://developer.apple.com/documentation/xcode/writing_arm64_code_for_apple_platforms Nick Gasson has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains seven commits: - Redundant cast to long - Merge master Change-Id: I49bef0437b4c47bef8bf74d192299d06b25e1555 CustomizedGitHooks: yes - Update test/jdk/java/foreign/valist/VaListTest.java Co-authored-by: Jorn Vernee - No variadic upcalls Change-Id: Ibf91c570c88be2587e9e23240477c4a5cc56b4c5 CustomizedGitHooks: yes - Fixes after JEP integratioN Change-Id: Iaa13b3869522c8814c3f7ef4c1eac8e8267657e6 CustomizedGitHooks: yes - merge master Change-Id: Ic06fec084099ff2053dd251a255cbbf4a64a59d7 CustomizedGitHooks: yes - 8263512: [macos_aarch64] issues with calling va_args functions from invoke_native macOS on Apple silicon uses slightly different ABI conventions to the standard AArch64 ABI. The differences are outlined in [1]. In particular in the standard (AAPCS) ABI, variadic arguments may be passed in either registers or on the stack following the normal calling convention. To handle this, va_list is a struct containing separate pointers for arguments located in integer registers, floating point registers, and on the stack. Apple's ABI simplifies this by passing all variadic arguments on the stack and the va_list type becomes a simple char* pointer. This patch adds a new MacOsAArch64 CABI type and MacOsAArch64Linker to represent the new ABI variant on macOS. StackVaList is based on WinVaList lightly modified to handle the different TypeClasses on AArch64. The original AArch64Linker is renamed to AapcsLinker and is currently used for all non-Mac platforms. I think we also need to add a WinAArch64 CABI but I haven't yet been able to test on a Windows system so will do that later. The macOS ABI also uses a different method of spilling arguments to the stack (the standard ABI pads each argument to a multiple of 8 byte stack slots, but the Mac ABI packs arguments according to their natural alignment). None of the existing tests exercise this so I'll open a new JBS issue and work on that separately. Tested jdk_foreign on macOS AArch64, Linux AArch64, and Linux X86_64. - Changes: https://git.openjdk.java.net/jdk/pull/3617/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=3617=04 Stats: 777 lines in 14 files changed: 571 ins; 115 del; 91 mod Patch: https://git.openjdk.java.net/jdk/pull/3617.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3617/head:pull/3617 PR: https://git.openjdk.java.net/jdk/pull/3617
Re: RFR: 8263512: [macos_aarch64] issues with calling va_args functions from invoke_native [v4]
> macOS on Apple silicon uses slightly different ABI conventions to the > standard AArch64 ABI. The differences are outlined in [1]. In > particular in the standard (AAPCS) ABI, variadic arguments may be passed > in either registers or on the stack following the normal calling > convention. To handle this, va_list is a struct containing separate > pointers for arguments located in integer registers, floating point > registers, and on the stack. Apple's ABI simplifies this by passing all > variadic arguments on the stack and the va_list type becomes a simple > char* pointer. > > This patch adds a new MacOsAArch64 CABI type and MacOsAArch64Linker to > represent the new ABI variant on macOS. StackVaList is based on > WinVaList lightly modified to handle the different TypeClasses on > AArch64. The original AArch64Linker is renamed to AapcsLinker and is > currently used for all non-Mac platforms. I think we also need to add a > WinAArch64 CABI but I haven't yet been able to test on a Windows system > so will do that later. > > The macOS ABI also uses a different method of spilling arguments to the > stack (the standard ABI pads each argument to a multiple of 8 byte stack > slots, but the Mac ABI packs arguments according to their natural > alignment). None of the existing tests exercise this so I'll open a new > JBS issue and work on that separately. > > Tested jdk_foreign on macOS AArch64, Linux AArch64, and Linux X86_64. > > [1] > https://developer.apple.com/documentation/xcode/writing_arm64_code_for_apple_platforms Nick Gasson has updated the pull request incrementally with one additional commit since the last revision: Update test/jdk/java/foreign/valist/VaListTest.java Co-authored-by: Jorn Vernee - Changes: - all: https://git.openjdk.java.net/jdk/pull/3617/files - new: https://git.openjdk.java.net/jdk/pull/3617/files/251aae68..185f114f Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=3617=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk=3617=02-03 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/3617.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3617/head:pull/3617 PR: https://git.openjdk.java.net/jdk/pull/3617
Re: RFR: 8268113: Re-use Long.hashCode() where possible [v4]
> There is a few JDK classes duplicating the contents of Long.hashCode() for > hash code calculation. They should explicitly delegate to Long.hashCode(). Сергей Цыпанов has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains four additional commits since the last revision: - Merge branch 'master' into 8268113 - 8268113: Inline local vars where reasonable - 8268113: Delegate to Double.hashCode() - 8268113: Re-use Long.hashCode() where possible - Changes: - all: https://git.openjdk.java.net/jdk/pull/4309/files - new: https://git.openjdk.java.net/jdk/pull/4309/files/df8be00a..7dc5020e Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=4309=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk=4309=02-03 Stats: 454615 lines in 1482 files changed: 442781 ins; 7489 del; 4345 mod Patch: https://git.openjdk.java.net/jdk/pull/4309.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4309/head:pull/4309 PR: https://git.openjdk.java.net/jdk/pull/4309
Re: RFR: 8263561: Re-examine uses of LinkedList [v2]
> After I've renamed remove branch GitHub for some reason has closed original > https://github.com/openjdk/jdk/pull/2744, so I've decided to recreate it. Сергей Цыпанов has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains seven commits: - Merge branch 'master' into 8263561 - Merge branch 'master' into 8263561 # Conflicts: #src/java.base/unix/classes/sun/net/dns/ResolverConfigurationImpl.java - Merge branch 'master' into purge-linked-list - 8263561: Use sized constructor where reasonable - 8263561: Use interface List instead of particular type where possible - 8263561: Rename requestList -> requests - 8263561: Re-examine uses of LinkedList - Changes: https://git.openjdk.java.net/jdk/pull/4304/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=4304=01 Stats: 48 lines in 9 files changed: 0 ins; 2 del; 46 mod Patch: https://git.openjdk.java.net/jdk/pull/4304.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4304/head:pull/4304 PR: https://git.openjdk.java.net/jdk/pull/4304
Re: RFR: 8263512: [macos_aarch64] issues with calling va_args functions from invoke_native [v3]
On Thu, 3 Jun 2021 03:28:56 GMT, Nick Gasson wrote: >> macOS on Apple silicon uses slightly different ABI conventions to the >> standard AArch64 ABI. The differences are outlined in [1]. In >> particular in the standard (AAPCS) ABI, variadic arguments may be passed >> in either registers or on the stack following the normal calling >> convention. To handle this, va_list is a struct containing separate >> pointers for arguments located in integer registers, floating point >> registers, and on the stack. Apple's ABI simplifies this by passing all >> variadic arguments on the stack and the va_list type becomes a simple >> char* pointer. >> >> This patch adds a new MacOsAArch64 CABI type and MacOsAArch64Linker to >> represent the new ABI variant on macOS. StackVaList is based on >> WinVaList lightly modified to handle the different TypeClasses on >> AArch64. The original AArch64Linker is renamed to AapcsLinker and is >> currently used for all non-Mac platforms. I think we also need to add a >> WinAArch64 CABI but I haven't yet been able to test on a Windows system >> so will do that later. >> >> The macOS ABI also uses a different method of spilling arguments to the >> stack (the standard ABI pads each argument to a multiple of 8 byte stack >> slots, but the Mac ABI packs arguments according to their natural >> alignment). None of the existing tests exercise this so I'll open a new >> JBS issue and work on that separately. >> >> Tested jdk_foreign on macOS AArch64, Linux AArch64, and Linux X86_64. >> >> [1] >> https://developer.apple.com/documentation/xcode/writing_arm64_code_for_apple_platforms > > Nick Gasson has updated the pull request incrementally with one additional > commit since the last revision: > > No variadic upcalls > > Change-Id: Ibf91c570c88be2587e9e23240477c4a5cc56b4c5 > CustomizedGitHooks: yes Looks good - except for the issue raised by Jorn (carrier mismatch on test) - PR: https://git.openjdk.java.net/jdk/pull/3617
Re: RFR: 8263512: [macos_aarch64] issues with calling va_args functions from invoke_native [v3]
On Fri, 4 Jun 2021 11:04:33 GMT, Jorn Vernee wrote:
>> Nick Gasson has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> No variadic upcalls
>>
>> Change-Id: Ibf91c570c88be2587e9e23240477c4a5cc56b4c5
>> CustomizedGitHooks: yes
>
> test/jdk/java/foreign/valist/VaListTest.java line 706:
>
>> 704: vaList.skip(BigPoint_LAYOUT);
>> 705: assertEquals((long) vaList.vargAsLong(C_LONG), 42);
>> 706: })},
>
> Looks like a carrier size mismatch here:
>
>
> java.lang.IllegalArgumentException: Carrier size mismatch: long !=
> b32[abi/kind=LONG]
> at
> jdk.incubator.foreign/jdk.internal.foreign.Utils.checkPrimitiveCarrierCompat(Utils.java:111)
> at
> jdk.incubator.foreign/jdk.internal.foreign.abi.SharedUtils.checkCompatibleType(SharedUtils.java:231)
> at
> jdk.incubator.foreign/jdk.internal.foreign.abi.x64.windows.WinVaList.read(WinVaList.java:114)
> at
> jdk.incubator.foreign/jdk.internal.foreign.abi.x64.windows.WinVaList.read(WinVaList.java:109)
> at
> jdk.incubator.foreign/jdk.internal.foreign.abi.x64.windows.WinVaList.vargAsLong(WinVaList.java:84)
> at VaListTest.lambda$upcalls$58(VaListTest.java:705)
> at
> jdk.incubator.foreign/jdk.internal.foreign.abi.ProgrammableInvoker.invokeNative(Native
> Method)
> at
> jdk.incubator.foreign/jdk.internal.foreign.abi.ProgrammableInvoker.invokeMoves(ProgrammableInvoker.java:290)
> at VaListTest.testUpcall(VaListTest.java:530)
>
>
> Need to use `C_LONG_LONG` to be cross-platform compatible. (sorry for not
> noticing this).
>
> Suggestion:
>
> { linkVaListCB("upcallBigStructPlusScalar"),
> VaListConsumer.mh(vaList -> {
> MemorySegment struct =
> vaList.vargAsSegment(BigPoint_LAYOUT, ResourceScope.newImplicitScope());
> assertEquals((long) VH_BigPoint_x.get(struct), 8);
> assertEquals((long) VH_BigPoint_y.get(struct), 16);
>
> assertEquals((long) vaList.vargAsLong(C_LONG_LONG), 42);
> })},
> { linkVaListCB("upcallBigStructPlusScalar"),
> VaListConsumer.mh(vaList -> {
> vaList.skip(BigPoint_LAYOUT);
> assertEquals((long) vaList.vargAsLong(C_LONG_LONG), 42);
> })},
Also, it looks like the cast to `(long)` on the `vaList.vargAsLong` lines is
redundant (thanks IntelliJ).
-
PR: https://git.openjdk.java.net/jdk/pull/3617
Re: RFR: 8263512: [macos_aarch64] issues with calling va_args functions from invoke_native [v3]
On Thu, 3 Jun 2021 03:28:56 GMT, Nick Gasson wrote:
>> macOS on Apple silicon uses slightly different ABI conventions to the
>> standard AArch64 ABI. The differences are outlined in [1]. In
>> particular in the standard (AAPCS) ABI, variadic arguments may be passed
>> in either registers or on the stack following the normal calling
>> convention. To handle this, va_list is a struct containing separate
>> pointers for arguments located in integer registers, floating point
>> registers, and on the stack. Apple's ABI simplifies this by passing all
>> variadic arguments on the stack and the va_list type becomes a simple
>> char* pointer.
>>
>> This patch adds a new MacOsAArch64 CABI type and MacOsAArch64Linker to
>> represent the new ABI variant on macOS. StackVaList is based on
>> WinVaList lightly modified to handle the different TypeClasses on
>> AArch64. The original AArch64Linker is renamed to AapcsLinker and is
>> currently used for all non-Mac platforms. I think we also need to add a
>> WinAArch64 CABI but I haven't yet been able to test on a Windows system
>> so will do that later.
>>
>> The macOS ABI also uses a different method of spilling arguments to the
>> stack (the standard ABI pads each argument to a multiple of 8 byte stack
>> slots, but the Mac ABI packs arguments according to their natural
>> alignment). None of the existing tests exercise this so I'll open a new
>> JBS issue and work on that separately.
>>
>> Tested jdk_foreign on macOS AArch64, Linux AArch64, and Linux X86_64.
>>
>> [1]
>> https://developer.apple.com/documentation/xcode/writing_arm64_code_for_apple_platforms
>
> Nick Gasson has updated the pull request incrementally with one additional
> commit since the last revision:
>
> No variadic upcalls
>
> Change-Id: Ibf91c570c88be2587e9e23240477c4a5cc56b4c5
> CustomizedGitHooks: yes
test/jdk/java/foreign/valist/VaListTest.java line 706:
> 704: vaList.skip(BigPoint_LAYOUT);
> 705: assertEquals((long) vaList.vargAsLong(C_LONG), 42);
> 706: })},
Looks like a carrier size mismatch here:
java.lang.IllegalArgumentException: Carrier size mismatch: long !=
b32[abi/kind=LONG]
at
jdk.incubator.foreign/jdk.internal.foreign.Utils.checkPrimitiveCarrierCompat(Utils.java:111)
at
jdk.incubator.foreign/jdk.internal.foreign.abi.SharedUtils.checkCompatibleType(SharedUtils.java:231)
at
jdk.incubator.foreign/jdk.internal.foreign.abi.x64.windows.WinVaList.read(WinVaList.java:114)
at
jdk.incubator.foreign/jdk.internal.foreign.abi.x64.windows.WinVaList.read(WinVaList.java:109)
at
jdk.incubator.foreign/jdk.internal.foreign.abi.x64.windows.WinVaList.vargAsLong(WinVaList.java:84)
at VaListTest.lambda$upcalls$58(VaListTest.java:705)
at
jdk.incubator.foreign/jdk.internal.foreign.abi.ProgrammableInvoker.invokeNative(Native
Method)
at
jdk.incubator.foreign/jdk.internal.foreign.abi.ProgrammableInvoker.invokeMoves(ProgrammableInvoker.java:290)
at VaListTest.testUpcall(VaListTest.java:530)
Need to use `C_LONG_LONG` to be cross-platform compatible. (sorry for not
noticing this).
Suggestion:
{ linkVaListCB("upcallBigStructPlusScalar"),
VaListConsumer.mh(vaList -> {
MemorySegment struct =
vaList.vargAsSegment(BigPoint_LAYOUT, ResourceScope.newImplicitScope());
assertEquals((long) VH_BigPoint_x.get(struct), 8);
assertEquals((long) VH_BigPoint_y.get(struct), 16);
assertEquals((long) vaList.vargAsLong(C_LONG_LONG), 42);
})},
{ linkVaListCB("upcallBigStructPlusScalar"),
VaListConsumer.mh(vaList -> {
vaList.skip(BigPoint_LAYOUT);
assertEquals((long) vaList.vargAsLong(C_LONG_LONG), 42);
})},
-
PR: https://git.openjdk.java.net/jdk/pull/3617
RFR: 8268227: java/foreign/TestUpcall.java still times out
Turns out that adding more timeout is a lost cause here. The root cause of the slowdown when running the test in debug build is: https://bugs.openjdk.java.net/browse/JDK-8266074 Which has also caused related test issues: https://bugs.openjdk.java.net/browse/JDK-8268095 So, the fix (at least temporarily) is to run method handle-heavy tests with the -XX:-VerifyDependency options. On my machine, execution time of these tests on debug goes from 10 minutes down to less than 1. Since `-XX:-VerifyDependencies` cannot be specified on non-debug build, the `-XX:+IgnoreUnrecognizedVMOptions` is also passed (thanks Vlad for the tip!). - Commit messages: - Disable VerifyDependencies on TestUpcall/Downcall (when in debug mode) Changes: https://git.openjdk.java.net/jdk/pull/4355/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=4355=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8268227 Stats: 2 lines in 2 files changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/4355.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4355/head:pull/4355 PR: https://git.openjdk.java.net/jdk/pull/4355
Re: RFR: 8263512: [macos_aarch64] issues with calling va_args functions from invoke_native
On Fri, 4 Jun 2021 10:06:26 GMT, Nick Gasson wrote:
>> The JEP has been integrated, so we can pick this back up (and handle it as a
>> bug for 17 even after the fork).
>>
>> Thank you for your patience.
>
> Thanks @JornVernee! I noticed VaListTest has started failing on Windows with
> this error:
>
> test
> VaListTest.testUpcall(java.lang.invoke.BoundMethodHandle$Species_LLL@198ebce4,
> MethodHandle(VaList)void): success
> test
> VaListTest.testUpcall(java.lang.invoke.BoundMethodHandle$Species_LLL@7a97cd30,
> MethodHandle(VaList)void): success
> Uncaught exception:
> java.lang.IllegalArgumentException
> {0xd6506c20} - klass: 'java/lang/IllegalArgumentException'
> #
> # A fatal error has been detected by the Java Runtime Environment:
> #
> # Internal Error (universalUpcallHandler.cpp:113), pid=13972, tid=23500
> # Error: ShouldNotReachHere()
> #
>
>
> I guess it must be related to the two new cases I added and the Windows code
> is now throwing an IllegalArgumentException but I can't see where from. Any
> ideas?
Hey @nick-arm I'm on Windows, so I can take a look here. We recently added a
patch that handles these exceptions better and actually prints the stack trace
as well.
-
PR: https://git.openjdk.java.net/jdk/pull/3617
Re: RFR: 8263512: [macos_aarch64] issues with calling va_args functions from invoke_native
On Wed, 2 Jun 2021 13:42:22 GMT, Jorn Vernee wrote:
>> macOS on Apple silicon uses slightly different ABI conventions to the
>> standard AArch64 ABI. The differences are outlined in [1]. In
>> particular in the standard (AAPCS) ABI, variadic arguments may be passed
>> in either registers or on the stack following the normal calling
>> convention. To handle this, va_list is a struct containing separate
>> pointers for arguments located in integer registers, floating point
>> registers, and on the stack. Apple's ABI simplifies this by passing all
>> variadic arguments on the stack and the va_list type becomes a simple
>> char* pointer.
>>
>> This patch adds a new MacOsAArch64 CABI type and MacOsAArch64Linker to
>> represent the new ABI variant on macOS. StackVaList is based on
>> WinVaList lightly modified to handle the different TypeClasses on
>> AArch64. The original AArch64Linker is renamed to AapcsLinker and is
>> currently used for all non-Mac platforms. I think we also need to add a
>> WinAArch64 CABI but I haven't yet been able to test on a Windows system
>> so will do that later.
>>
>> The macOS ABI also uses a different method of spilling arguments to the
>> stack (the standard ABI pads each argument to a multiple of 8 byte stack
>> slots, but the Mac ABI packs arguments according to their natural
>> alignment). None of the existing tests exercise this so I'll open a new
>> JBS issue and work on that separately.
>>
>> Tested jdk_foreign on macOS AArch64, Linux AArch64, and Linux X86_64.
>>
>> [1]
>> https://developer.apple.com/documentation/xcode/writing_arm64_code_for_apple_platforms
>
> The JEP has been integrated, so we can pick this back up (and handle it as a
> bug for 17 even after the fork).
>
> Thank you for your patience.
Thanks @JornVernee! I noticed VaListTest has started failing on Windows with
this error:
test
VaListTest.testUpcall(java.lang.invoke.BoundMethodHandle$Species_LLL@198ebce4,
MethodHandle(VaList)void): success
test
VaListTest.testUpcall(java.lang.invoke.BoundMethodHandle$Species_LLL@7a97cd30,
MethodHandle(VaList)void): success
Uncaught exception:
java.lang.IllegalArgumentException
{0xd6506c20} - klass: 'java/lang/IllegalArgumentException'
#
# A fatal error has been detected by the Java Runtime Environment:
#
# Internal Error (universalUpcallHandler.cpp:113), pid=13972, tid=23500
# Error: ShouldNotReachHere()
#
I guess it must be related to the two new cases I added and the Windows code is
now throwing an IllegalArgumentException but I can't see where from. Any ideas?
-
PR: https://git.openjdk.java.net/jdk/pull/3617
Re: RFR: 8262891: Compiler implementation for Pattern Matching for switch (Preview) [v12]
> This is a preview of a patch implementing JEP 406: Pattern Matching for > switch (Preview): > https://bugs.openjdk.java.net/browse/JDK-8213076 > > The current draft of the specification is here: > http://cr.openjdk.java.net/~gbierman/jep406/jep406-20210430/specs/patterns-switch-jls.html > > A summary of notable parts of the patch: > -to support cases expressions and patterns in cases, there is a new common > superinterface for expressions and patterns, `CaseLabelTree`, which > expressions and patterns implement, and a list of case labels is returned > from `CaseTree.getLabels()`. > -to support `case default`, there is an implementation of `CaseLabelTree` > that represents it (`DefaultCaseLabelTree`). It is used also to represent the > conventional `default` internally and in the newly added methods. > -in the parser, parenthesized patterns and expressions need to be > disambiguated when parsing case labels. > -Lower has been enhanced to handle `case null` for ordinary (boxed-primitive, > String, enum) switches. This is a bit tricky for boxed primitives, as there > is no value that is not part of the input domain so that could be used to > represent `case null`. Requires a bit shuffling with values. > -TransPatterns has been enhanced to handle the pattern matching switch. It > produces code that delegates to a new bootstrap method, that will classify > the input value to the switch and return the case number, to which the switch > then jumps. To support guards, the switches (and the bootstrap method) are > restartable. The bootstrap method as such is written very simply so far, but > could be much more optimized later. > -nullable type patterns are `case String s, null`/`case null, String s`/`case > null: case String s:`/`case String s: case null:`, handling of these required > a few tricks in `Attr`, `Flow` and `TransPatterns`. > > The specdiff for the change is here (to be updated): > http://cr.openjdk.java.net/~jlahoda/8265981/specdiff.preview.01/overview-summary.html Jan Lahoda has updated the pull request incrementally with one additional commit since the last revision: Fixing typo. - Changes: - all: https://git.openjdk.java.net/jdk/pull/3863/files - new: https://git.openjdk.java.net/jdk/pull/3863/files/216b87c2..8d4c02b4 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=3863=11 - incr: https://webrevs.openjdk.java.net/?repo=jdk=3863=10-11 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/3863.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3863/head:pull/3863 PR: https://git.openjdk.java.net/jdk/pull/3863
Re: RFR: 8262891: Compiler implementation for Pattern Matching for switch (Preview) [v11]
On Fri, 4 Jun 2021 09:01:27 GMT, Jan Lahoda wrote: >> This is a preview of a patch implementing JEP 406: Pattern Matching for >> switch (Preview): >> https://bugs.openjdk.java.net/browse/JDK-8213076 >> >> The current draft of the specification is here: >> http://cr.openjdk.java.net/~gbierman/jep406/jep406-20210430/specs/patterns-switch-jls.html >> >> A summary of notable parts of the patch: >> -to support cases expressions and patterns in cases, there is a new common >> superinterface for expressions and patterns, `CaseLabelTree`, which >> expressions and patterns implement, and a list of case labels is returned >> from `CaseTree.getLabels()`. >> -to support `case default`, there is an implementation of `CaseLabelTree` >> that represents it (`DefaultCaseLabelTree`). It is used also to represent >> the conventional `default` internally and in the newly added methods. >> -in the parser, parenthesized patterns and expressions need to be >> disambiguated when parsing case labels. >> -Lower has been enhanced to handle `case null` for ordinary >> (boxed-primitive, String, enum) switches. This is a bit tricky for boxed >> primitives, as there is no value that is not part of the input domain so >> that could be used to represent `case null`. Requires a bit shuffling with >> values. >> -TransPatterns has been enhanced to handle the pattern matching switch. It >> produces code that delegates to a new bootstrap method, that will classify >> the input value to the switch and return the case number, to which the >> switch then jumps. To support guards, the switches (and the bootstrap >> method) are restartable. The bootstrap method as such is written very simply >> so far, but could be much more optimized later. >> -nullable type patterns are `case String s, null`/`case null, String >> s`/`case null: case String s:`/`case String s: case null:`, handling of >> these required a few tricks in `Attr`, `Flow` and `TransPatterns`. >> >> The specdiff for the change is here (to be updated): >> http://cr.openjdk.java.net/~jlahoda/8265981/specdiff.preview.01/overview-summary.html > > Jan Lahoda has updated the pull request incrementally with two additional > commits since the last revision: > > - Tweaking SwitchBootstraps javadoc, as suggested. > - Improving javadoc. Re-approving to keep the bots happy - Marked as reviewed by mcimadamore (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/3863
Re: RFR: 8262891: Compiler implementation for Pattern Matching for switch (Preview) [v11]
> This is a preview of a patch implementing JEP 406: Pattern Matching for > switch (Preview): > https://bugs.openjdk.java.net/browse/JDK-8213076 > > The current draft of the specification is here: > http://cr.openjdk.java.net/~gbierman/jep406/jep406-20210430/specs/patterns-switch-jls.html > > A summary of notable parts of the patch: > -to support cases expressions and patterns in cases, there is a new common > superinterface for expressions and patterns, `CaseLabelTree`, which > expressions and patterns implement, and a list of case labels is returned > from `CaseTree.getLabels()`. > -to support `case default`, there is an implementation of `CaseLabelTree` > that represents it (`DefaultCaseLabelTree`). It is used also to represent the > conventional `default` internally and in the newly added methods. > -in the parser, parenthesized patterns and expressions need to be > disambiguated when parsing case labels. > -Lower has been enhanced to handle `case null` for ordinary (boxed-primitive, > String, enum) switches. This is a bit tricky for boxed primitives, as there > is no value that is not part of the input domain so that could be used to > represent `case null`. Requires a bit shuffling with values. > -TransPatterns has been enhanced to handle the pattern matching switch. It > produces code that delegates to a new bootstrap method, that will classify > the input value to the switch and return the case number, to which the switch > then jumps. To support guards, the switches (and the bootstrap method) are > restartable. The bootstrap method as such is written very simply so far, but > could be much more optimized later. > -nullable type patterns are `case String s, null`/`case null, String s`/`case > null: case String s:`/`case String s: case null:`, handling of these required > a few tricks in `Attr`, `Flow` and `TransPatterns`. > > The specdiff for the change is here (to be updated): > http://cr.openjdk.java.net/~jlahoda/8265981/specdiff.preview.01/overview-summary.html Jan Lahoda has updated the pull request incrementally with two additional commits since the last revision: - Tweaking SwitchBootstraps javadoc, as suggested. - Improving javadoc. - Changes: - all: https://git.openjdk.java.net/jdk/pull/3863/files - new: https://git.openjdk.java.net/jdk/pull/3863/files/fa50b5fb..216b87c2 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=3863=10 - incr: https://webrevs.openjdk.java.net/?repo=jdk=3863=09-10 Stats: 66 lines in 2 files changed: 40 ins; 9 del; 17 mod Patch: https://git.openjdk.java.net/jdk/pull/3863.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3863/head:pull/3863 PR: https://git.openjdk.java.net/jdk/pull/3863
Re: RFR: 8268222: javax/xml/jaxp/unittest/transform/Bug6216226Test.java failed, cannot delete file
On Fri, 4 Jun 2021 05:14:13 GMT, Joe Wang wrote:
> Revert changes in StreamResult.java made through
> https://github.com/openjdk/jdk/pull/4318 since it was creating a file stream
> on behalf of the Transformer, which resulted in a leaking file handle because
> the Transformer would only close files it opened.
>
> This change instead replace the problematic file-uri-url-file conversion code
> with nio Paths. While we generally don't make such changes if it's not
> necessary as Apache still supports older versions of the JDK, we are okay
> with a code level 8.
Using `Path` instead of trying to handcraft a URL is a much better alternative.
src/java.xml/share/classes/com/sun/org/apache/xalan/internal/xsltc/trax/TransformerImpl.java
line 52:
> 50: import java.net.UnknownServiceException;
> 51: import java.nio.file.Path;
> 52: import java.nio.file.Paths;
Nit: you should not need to use Paths. `Paths.get(URI)` just calls
`Path.of(URI)`
src/java.xml/share/classes/com/sun/org/apache/xalan/internal/xsltc/trax/TransformerImpl.java
line 516:
> 514: if (systemId.startsWith("file:")) {
> 515: try{
> 516: Path p = Paths.get(new URI(systemId));
I suggest using `Path.of(new URI(systemId));` here.
-
Marked as reviewed by dfuchs (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/4353
Re: RFR: 8174222: LambdaMetafactory: validate inputs and improve documentation [v2]
On Thu, 3 Jun 2021 20:40:23 GMT, Dan Smith wrote: >> Standardizes and better specifies the errors thrown by LambdaMetafactory, >> including for inputs that would not be generated by javac. >> >> Does some renaming of core parameters/fields to better reflect their purpose. >> >> In the implementation, stops using ACC_BRIDGE for any methods, which is not >> a good fit for what these methods do (they don't delegate to each other, but >> all invoke the same implementation method). > > Dan Smith has updated the pull request incrementally with one additional > commit since the last revision: > > Fix accidentally commented-out parts of test src/java.base/share/classes/java/lang/invoke/AbstractValidatingLambdaMetafactory.java line 69: > 67: final boolean isSerializable; // Should the returned > instance be serializable > 68: final Class[] interfaces; // Additional interfaces to > be implemented > 69: final MethodType[] bridges; // Signatures of additional > methods to bridge If you are removing ACC_BRIDGE from additional generated methods, then perhaps this parameter name could also be changed? `altMethods` (as alternative methods perhaps?) - PR: https://git.openjdk.java.net/jdk/pull/4346
