Re: RFR: 8159023: Engineering notation of DecimalFormat does not work as documented [v3]
> Please review this PR which updates the Scientific Notation section of > Decimal Format. It aims to resolve > [JDK-8159023](https://bugs.openjdk.org/browse/JDK-8159023) as well as > [JDK-6282188](https://bugs.openjdk.org/browse/JDK-6282188). > > **Scientific Notation** in Decimal Format contains the definition for a > scientific notation formatted number's mantissa as such: _The number of > significant digits in the mantissa is the sum of the minimum integer and > maximum fraction digits, and is unaffected by the maximum integer digits. For > example, 12345 formatted with "##0.##E0" is "12.3E3"._ > > Both the definition and example are incorrect, as the actual result is > "12.E345". > > The following example data show that scientific notation formatted numbers do > not adhere to that definition. As, according to the definition, the following > numbers should have 3 significant digits, but in reality, they have up to 5. > > 123 formatted by ##0.#E0 is 123E0 > 1234 formatted by ##0.#E0 is 1.234E3 > 12345 formatted by ##0.#E0 is 12.34E3 > 123456 formatted by ##0.#E0 is 123.5E3 > 1234567 formatted by ##0.#E0 is 1.235E6 > 12345678 formatted by ##0.#E0 is 12.35E6 > 123456789 formatted by ##0.#E0 is 123.5E6 > > > The actual definition of the mantissa, as well as examples and further > context are included in this change. In addition, a test has been added that > tests various patterns to numbers and ensures the format follows the new > definition's mathematical expression. Justin Lu has updated the pull request incrementally with one additional commit since the last revision: Review: replace counting with isDigit, use all caps static final vars - Changes: - all: https://git.openjdk.org/jdk/pull/14066/files - new: https://git.openjdk.org/jdk/pull/14066/files/faa32ec6..dd516b63 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=14066=02 - incr: https://webrevs.openjdk.org/?repo=jdk=14066=01-02 Stats: 13 lines in 1 file changed: 0 ins; 5 del; 8 mod Patch: https://git.openjdk.org/jdk/pull/14066.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14066/head:pull/14066 PR: https://git.openjdk.org/jdk/pull/14066
Re: RFR: 8159023: Engineering notation of DecimalFormat does not work as documented [v2]
On Mon, 22 May 2023 23:31:37 GMT, Naoto Sato wrote:
>> Justin Lu has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Review: Use Static dfs, error message, and exponent digit. Hard code min
>> and max digits instead of calculating at runtime
>
> test/jdk/java/text/Format/DecimalFormat/MantissaDigits.java line 64:
>
>> 62: for (double number : numbers) {
>> 63: // Count the significant digits in the pre-formatted number
>> 64: int originalNumDigits = String.valueOf(number)
>
> It's OK as it stands, but you could write:
>
> str.chars().filter(Character::isDigit).count()
>
> which might be more readable
Like this much better, incorporated!
-
PR Review Comment: https://git.openjdk.org/jdk/pull/14066#discussion_r1201538926
Re: RFR: 8307990: jspawnhelper must close its writing side of a pipe before reading from it [v4]
On Mon, 22 May 2023 10:22:16 GMT, Volker Simonis wrote:
>> Since JDK13, executing commands in a sub-process defaults to the so called
>> `POSIX_SPAWN` launching mechanism (i.e.
>> `-Djdk.lang.Process.launchMechanism=POSIX_SPAWN`) on Linux. This works by
>> using `posix_spawn(3)` to firstly start a tiny helper program called
>> `jspawnhelper` in a subprocess. In a second step, `jspawnhelper` reads the
>> command data from the parent Java process over a Unix pipe and finally
>> executes (i.e. `execvp(3)`) the requested command.
>>
>> In cases where the parent process terminates abnormally before
>> `jspawnhelper` has read all the expected data from the pipe, `jspawnhelper`
>> will block indefinitely on the reading end of the pipe. This is especially
>> harmful if the parent process had open sockets, because in that case, the
>> forked `jspawnhelper` process will inherit them and keep all the
>> corresponding ports open effectively preventing other processes to bind to
>> them. Notice that this is not an abstract scenario. We've observed this
>> regularly in production with services which couldn't be restarted after a
>> crash after migrating to JDK 17.
>>
>> The fix of the issue is rather trivial. `jspawnhelper` has to close its
>> writing end of the pipe which connects it with the parent Java process
>> *before* starting to read from that pipe such that reads from the pipe can
>> immediately return with EOF if the parent process terminates abnormally.
>>
>> Also did some cleanup:
>> - in `jspawnhelper.c` correctly chek the result of `readFully()`
>> - in `ProcessImpl_md.c` use `restartableWrite()` instead of `write()` to
>> write the command data from the parent process to `jspawnhelper`
>> - in `childproc.{c,h}` remove remaining traces of `MODE_CLONE` because
>> that's long gone.
>
> Volker Simonis has updated the pull request with a new target base due to a
> merge or a rebase. The pull request now contains five commits:
>
> - Merge branch 'master' into JDK-8307990
> - Address comments from tstuefe and RogerRiggs
> - REALLY adding the test :)
> - Added test case
> - 8307990: jspawnhelper must close its writing side of a pipe before reading
> from it
>
Without further investigation on my part, I don't know if this fix falls more
squarely in the "obviously no bugs" or "no obvious bugs" category.
I left a comment on the release note issue suggesting a description of the
solution or implications of the solution be added to the note. It could be also
be helpful to state under what conditions the problem being solved is more
likely to manifest, if that is known.
For whenever this goes back, highlighting the change on a quality-discuss
message could help validate the change doesn't have unexpected side-effects.
-
PR Comment: https://git.openjdk.org/jdk/pull/13956#issuecomment-1558529211
Integrated: 8308046: Move Solaris related charsets from java.base to jdk.charsets module
On Mon, 15 May 2023 00:28:41 GMT, Ichiroh Takiguchi wrote: > According to "JDK 20 Internationalization Guide" > https://docs.oracle.com/en/java/javase/20/intl/supported-encodings.html > Following Solaris related charsets are in "contained in jdk.charsets module" > list. > > - PCK (x-PCK) > - EUC_JP_Solaris (x-eucJP-Open) > - Big5_Solaris (x-Big5-Solaris) > > These are not supported by Linux platform, so they should not be in java.base > module. > > Note: > GHA Linux x86 builds were failed. > I think it's not related by my modified code. > I opened [JDK-8308051](https://bugs.openjdk.org/browse/JDK-8308051) GHA: > Linux x86 builds failure This pull request has now been integrated. Changeset: 5d8ba938 Author:Ichiroh Takiguchi URL: https://git.openjdk.org/jdk/commit/5d8ba938bef162b74816147eb1002a0620a419ba Stats: 21 lines in 4 files changed: 0 ins; 6 del; 15 mod 8308046: Move Solaris related charsets from java.base to jdk.charsets module Reviewed-by: naoto - PR: https://git.openjdk.org/jdk/pull/13973
Re: RFR: 8308046: Move Solaris related charsets from java.base to jdk.charsets module [v2]
On Sat, 20 May 2023 17:26:53 GMT, Naoto Sato wrote: >> Hello @naotoj . >> I'd like to confirm about DoubleByte-X.java.template and >> EUC_JP.java.template. >> I think the values are package-private. >> Even if class is changed to `public`, the classes in`sun.nio.cs.ext` package >> could not access to these values in `sun.nio.cs` package... >> I may be misunderstanding your suggestion, could you tell me more ? > >> I think the values are package-private. Even if class is changed to >> `public`, the classes in`sun.nio.cs.ext` package could not access to these >> values in `sun.nio.cs` package... > > I meant making those package-private fields public. I believe it's OK because > java.base/sun.nio.cs package is only exported to jdk.charsets module. Hello @naotoj . I appreciate your attention about JBS side. I changed title and description, add noreg-cleanup label. - PR Comment: https://git.openjdk.org/jdk/pull/13973#issuecomment-1558228901
Re: RFR: 8159023: Engineering notation of DecimalFormat does not work as documented [v2]
On Mon, 22 May 2023 23:03:10 GMT, Justin Lu wrote:
>> Please review this PR which updates the Scientific Notation section of
>> Decimal Format. It aims to resolve
>> [JDK-8159023](https://bugs.openjdk.org/browse/JDK-8159023) as well as
>> [JDK-6282188](https://bugs.openjdk.org/browse/JDK-6282188).
>>
>> **Scientific Notation** in Decimal Format contains the definition for a
>> scientific notation formatted number's mantissa as such: _The number of
>> significant digits in the mantissa is the sum of the minimum integer and
>> maximum fraction digits, and is unaffected by the maximum integer digits.
>> For example, 12345 formatted with "##0.##E0" is "12.3E3"._
>>
>> Both the definition and example are incorrect, as the actual result is
>> "12.E345".
>>
>> The following example data show that scientific notation formatted numbers
>> do not adhere to that definition. As, according to the definition, the
>> following numbers should have 3 significant digits, but in reality, they
>> have up to 5.
>>
>> 123 formatted by ##0.#E0 is 123E0
>> 1234 formatted by ##0.#E0 is 1.234E3
>> 12345 formatted by ##0.#E0 is 12.34E3
>> 123456 formatted by ##0.#E0 is 123.5E3
>> 1234567 formatted by ##0.#E0 is 1.235E6
>> 12345678 formatted by ##0.#E0 is 12.35E6
>> 123456789 formatted by ##0.#E0 is 123.5E6
>>
>>
>> The actual definition of the mantissa, as well as examples and further
>> context are included in this change. In addition, a test has been added that
>> tests various patterns to numbers and ensures the format follows the new
>> definition's mathematical expression.
>
> Justin Lu has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Review: Use Static dfs, error message, and exponent digit. Hard code min
> and max digits instead of calculating at runtime
Thanks, Justin. Left some super-nits.
test/jdk/java/text/Format/DecimalFormat/MantissaDigits.java line 52:
> 50: 1.1234, 1., 1.412, 222.333, -771.
> 51: };
> 52: private static final DecimalFormatSymbols dfs = new
> DecimalFormatSymbols(Locale.US);
The `static final` field is better uppercased.
test/jdk/java/text/Format/DecimalFormat/MantissaDigits.java line 64:
> 62: for (double number : numbers) {
> 63: // Count the significant digits in the pre-formatted number
> 64: int originalNumDigits = String.valueOf(number)
It's OK as it stands, but you could write:
str.chars().filter(Character::isDigit).count()
which might be more readable
-
PR Review: https://git.openjdk.org/jdk/pull/14066#pullrequestreview-1437867369
PR Review Comment: https://git.openjdk.org/jdk/pull/14066#discussion_r1201295121
PR Review Comment: https://git.openjdk.org/jdk/pull/14066#discussion_r1201295601
Re: RFR: 8159023: Engineering notation of DecimalFormat does not work as documented [v2]
On Mon, 22 May 2023 20:41:29 GMT, Naoto Sato wrote: >> Justin Lu has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Review: Use Static dfs, error message, and exponent digit. Hard code min >> and max digits instead of calculating at runtime > > test/jdk/java/text/Format/DecimalFormat/MantissaDigits.java line 89: > >> 87: // Test the new definition of the Mantissa >> 88: Integer calculatedDigits = Math >> 89: .min(Math.max(zeroes, originalNumDigits), >> (hashes+zeroes)); > > Could we make those expected numbers into the `Arguments` so that there is no > need to calculate in the test case at runtime? I think that would be more > readable. Thanks for the review, addressed both suggestions in the most recent commit. - PR Review Comment: https://git.openjdk.org/jdk/pull/14066#discussion_r1201272651
Re: RFR: 8159023: Engineering notation of DecimalFormat does not work as documented [v2]
> Please review this PR which updates the Scientific Notation section of > Decimal Format. It aims to resolve > [JDK-8159023](https://bugs.openjdk.org/browse/JDK-8159023) as well as > [JDK-6282188](https://bugs.openjdk.org/browse/JDK-6282188). > > **Scientific Notation** in Decimal Format contains the definition for a > scientific notation formatted number's mantissa as such: _The number of > significant digits in the mantissa is the sum of the minimum integer and > maximum fraction digits, and is unaffected by the maximum integer digits. For > example, 12345 formatted with "##0.##E0" is "12.3E3"._ > > Both the definition and example are incorrect, as the actual result is > "12.E345". > > The following example data show that scientific notation formatted numbers do > not adhere to that definition. As, according to the definition, the following > numbers should have 3 significant digits, but in reality, they have up to 5. > > 123 formatted by ##0.#E0 is 123E0 > 1234 formatted by ##0.#E0 is 1.234E3 > 12345 formatted by ##0.#E0 is 12.34E3 > 123456 formatted by ##0.#E0 is 123.5E3 > 1234567 formatted by ##0.#E0 is 1.235E6 > 12345678 formatted by ##0.#E0 is 12.35E6 > 123456789 formatted by ##0.#E0 is 123.5E6 > > > The actual definition of the mantissa, as well as examples and further > context are included in this change. In addition, a test has been added that > tests various patterns to numbers and ensures the format follows the new > definition's mathematical expression. Justin Lu has updated the pull request incrementally with one additional commit since the last revision: Review: Use Static dfs, error message, and exponent digit. Hard code min and max digits instead of calculating at runtime - Changes: - all: https://git.openjdk.org/jdk/pull/14066/files - new: https://git.openjdk.org/jdk/pull/14066/files/e6cbbeea..faa32ec6 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=14066=01 - incr: https://webrevs.openjdk.org/?repo=jdk=14066=00-01 Stats: 35 lines in 1 file changed: 6 ins; 12 del; 17 mod Patch: https://git.openjdk.org/jdk/pull/14066.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14066/head:pull/14066 PR: https://git.openjdk.org/jdk/pull/14066
Re: RFR: 8303040: linux PPC64le: Implementation of Foreign Function & Memory API (Preview) [v35]
On Mon, 22 May 2023 22:29:18 GMT, Martin Doerr wrote:
>> Implementation of "Foreign Function & Memory API" for linux on Power (Little
>> Endian) according to "Power Architecture 64-Bit ELF V2 ABI Specification".
>>
>> This PR does not include code for VaList support because it's supposed to
>> get removed by [JDK-8299736](https://bugs.openjdk.org/browse/JDK-8299736).
>> I've kept the related tests disabled for this platform and throw an
>> exception instead. Note that the ABI doesn't precisely specify variable
>> argument lists. Instead, it refers to `` (2.2.4 Variable Argument
>> Lists).
>>
>> Big Endian support is implemented to some extend, but not complete. E.g.
>> structs with size not divisible by 8 are not passed correctly (see
>> `useABIv2` in CallArranger.java). Big Endian is excluded by selecting
>> `ARCH.equals("ppc64le")` (CABI.java) only.
>>
>> There's another limitation: This PR only accepts structures with size
>> divisible by 4. (An `IllegalArgumentException` gets thrown otherwise.) I
>> think arbitrary sizes are not usable on other platforms, either, because
>> `SharedUtils.primitiveCarrierForSize` only accepts powers of 2. Update:
>> Resolved after merging of
>> [JDK-8303017](https://bugs.openjdk.org/browse/JDK-8303017)
>>
>> The ABI has some tricky corner cases related to HFA (Homogeneous Float
>> Aggregate). The same argument may need to get passed in both, a FP reg and a
>> GP reg or stack slot (see "no partial DW rule"). This cases are not covered
>> by the existing tests.
>>
>> I had to make changes to shared code and code for other platforms:
>> 1. Pass type information when creating `VMStorage` objects from `VMReg`.
>> This is needed for the following reasons:
>> - PPC64 ABI requires integer types to get extended to 64 bit (also see
>> CCallingConventionRequiresIntsAsLongs in existing hotspot code). We need to
>> know the type or at least the bit width for that.
>> - Floating point load / store instructions need the correct width to select
>> between the correct IEEE 754 formats. The register representation in single
>> FP registers is always IEEE 754 double precision on PPC64.
>> - Big Endian also needs usage of the precise size. Storing 8 Bytes and
>> loading 4 Bytes yields different values than on Little Endian!
>> 2. It happens that a `NativeMemorySegmentImpl` is used as a raw pointer
>> (with byteSize() == 0) while running TestUpcallScope. Hence, existing size
>> checks don't work (see MemorySegment.java). As a workaround, I'm just
>> skipping the check in this particular case. Please check if this makes sense
>> or if there's a better fix (possibly as separat...
>
> Martin Doerr has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Parameter Save Area is the correct name.
Thanks for publishing our discussion, here. The unnecessary PSA affects other
areas of hotspot much more than Panama. Yes, we should file an RFE. I think one
for hotspot is sufficient as the downcall stub is part of it. I don't think it
needs extra treatment.
-
PR Comment: https://git.openjdk.org/jdk/pull/12708#issuecomment-1558139323
Re: RFR: 8303040: linux PPC64le: Implementation of Foreign Function & Memory API (Preview) [v34]
On Mon, 22 May 2023 21:49:27 GMT, Richard Reingruber wrote:
>> Martin Doerr has updated the pull request with a new target base due to a
>> merge or a rebase. The pull request now contains 41 commits:
>>
>> - Adaptation for JDK-8308276.
>> - Merge remote-tracking branch 'origin' into PPC64_Panama
>> - Add comment about Register Save Area.
>> - Replace abstract method useABIv2().
>> - Cleanup imports, improve comments, updates from other platforms.
>> - Add NONZERO check for downcall_stub_address_offset_in_bytes().
>> - Replace NULL by nullptr.
>> - libTestHFA: Add explicit type conversion to avoid build warning.
>> - Add test case for passing a double value in a GP register. Use better
>> instructions for moving between FP and GP reg. Improve comments.
>> - Merge remote-tracking branch 'origin' into PPC64_Panama
>> - ... and 31 more: https://git.openjdk.org/jdk/compare/939344b8...08a5c143
>
> src/java.base/share/classes/jdk/internal/foreign/abi/ppc64/CallArranger.java
> line 205:
>
>> 203: stack = stackAlloc(4, 4);
>> 204: } else {
>> 205: stack = stackAlloc(is32Bit ? 4 : 8, STACK_SLOT_SIZE);
>
> This looks like a stack slot is always allocated. Please explain that for ABI
> V2 this is actually only required if it is know from a prototype that not all
> parameters can be passed in registers and that we plan to change this.
This basically computes the stack layout. We need to count all slots to get the
right offset for the registers which actually get written on stack. The first
such register will hit native_abi_minframe_size + 8 slots. If fewer registers
are used, the counted stack slots will not be used.
The decision whether we allocate the Parameter Save Area or not is done in the
downcall stub and doesn't depend on the stackAllocs.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/12708#discussion_r1201251283
Re: RFR: 8303040: linux PPC64le: Implementation of Foreign Function & Memory API (Preview) [v33]
On Mon, 22 May 2023 21:22:50 GMT, Richard Reingruber wrote: >> Martin Doerr has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Add comment about Register Save Area. > > src/hotspot/cpu/ppc/downcallLinker_ppc.cpp line 162: > >> 160: assert(_abi._shadow_space_bytes == frame::native_abi_minframe_size, >> "expected space according to ABI"); >> 161: // Note: For ABIv2, we only need (_input_registers.length() > 8) ? >> _input_registers.length() : 0 >> 162: int register_save_area_slots = MAX2(_input_registers.length(), 8); > > Both specs, ABI V1 and V2, call this "Parameter Save Area" we should use the > same name. > Suggestion: > > int parameter_save_area_slots = MAX2(_input_registers.length(), 8); Thanks! Changed. - PR Review Comment: https://git.openjdk.org/jdk/pull/12708#discussion_r1201234986
Re: RFR: 8303040: linux PPC64le: Implementation of Foreign Function & Memory API (Preview) [v35]
> Implementation of "Foreign Function & Memory API" for linux on Power (Little
> Endian) according to "Power Architecture 64-Bit ELF V2 ABI Specification".
>
> This PR does not include code for VaList support because it's supposed to get
> removed by [JDK-8299736](https://bugs.openjdk.org/browse/JDK-8299736). I've
> kept the related tests disabled for this platform and throw an exception
> instead. Note that the ABI doesn't precisely specify variable argument lists.
> Instead, it refers to `` (2.2.4 Variable Argument Lists).
>
> Big Endian support is implemented to some extend, but not complete. E.g.
> structs with size not divisible by 8 are not passed correctly (see `useABIv2`
> in CallArranger.java). Big Endian is excluded by selecting
> `ARCH.equals("ppc64le")` (CABI.java) only.
>
> There's another limitation: This PR only accepts structures with size
> divisible by 4. (An `IllegalArgumentException` gets thrown otherwise.) I
> think arbitrary sizes are not usable on other platforms, either, because
> `SharedUtils.primitiveCarrierForSize` only accepts powers of 2. Update:
> Resolved after merging of
> [JDK-8303017](https://bugs.openjdk.org/browse/JDK-8303017)
>
> The ABI has some tricky corner cases related to HFA (Homogeneous Float
> Aggregate). The same argument may need to get passed in both, a FP reg and a
> GP reg or stack slot (see "no partial DW rule"). This cases are not covered
> by the existing tests.
>
> I had to make changes to shared code and code for other platforms:
> 1. Pass type information when creating `VMStorage` objects from `VMReg`. This
> is needed for the following reasons:
> - PPC64 ABI requires integer types to get extended to 64 bit (also see
> CCallingConventionRequiresIntsAsLongs in existing hotspot code). We need to
> know the type or at least the bit width for that.
> - Floating point load / store instructions need the correct width to select
> between the correct IEEE 754 formats. The register representation in single
> FP registers is always IEEE 754 double precision on PPC64.
> - Big Endian also needs usage of the precise size. Storing 8 Bytes and
> loading 4 Bytes yields different values than on Little Endian!
> 2. It happens that a `NativeMemorySegmentImpl` is used as a raw pointer (with
> byteSize() == 0) while running TestUpcallScope. Hence, existing size checks
> don't work (see MemorySegment.java). As a workaround, I'm just skipping the
> check in this particular case. Please check if this makes sense or if there's
> a better fix (possibly as separate RFE). Update: T...
Martin Doerr has updated the pull request incrementally with one additional
commit since the last revision:
Parameter Save Area is the correct name.
-
Changes:
- all: https://git.openjdk.org/jdk/pull/12708/files
- new: https://git.openjdk.org/jdk/pull/12708/files/08a5c143..b912155b
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk=12708=34
- incr: https://webrevs.openjdk.org/?repo=jdk=12708=33-34
Stats: 5 lines in 1 file changed: 2 ins; 0 del; 3 mod
Patch: https://git.openjdk.org/jdk/pull/12708.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/12708/head:pull/12708
PR: https://git.openjdk.org/jdk/pull/12708
Re: RFR: 8308293: A linker should expose the layouts it supports [v3]
On Fri, 19 May 2023 23:08:00 GMT, Paul Sandoz wrote: >>> This look much better. Can we strengthen the specification of >>> `canonicalLayouts` in accordance with the class specification >> >> We can't do more in that method javadoc, think, as that has to be general >> enough for all linkers. I think the rules set up in that method javadoc are >> good - e.g. the set of layouts should be stable (both in terms of names and >> layout types). >> >> What we can do is to sprinkle some wording in the `nativeLinker` factory - >> e.g. `the native linker is guaranteed to provide canonical layouts for basic >> C types `. > >> > This look much better. Can we strengthen the specification of >> > `canonicalLayouts` in accordance with the class specification >> >> We can't do more in that method javadoc, think, as that has to be general >> enough for all linkers. I think the rules set up in that method javadoc are >> good - e.g. the set of layouts should be stable (both in terms of names and >> layout types). >> >> What we can do is to sprinkle some wording in the `nativeLinker` factory - >> e.g. `the native linker is guaranteed to provide canonical layouts for basic >> C types `. > > Yes, that's better. @PaulSandoz after thinking some more, it seems a bit ad-hoc to guarantee a canonical for "unsigned short", but not for other unsigned types? Possible alternatives (beside keeping what we have in this PR): * guarantee also all the other unsigned types (e.g. char, int, long and long long) * do not guarantee unsigned short - but provide a mapping for it anyways * do not guarantee unsigned short, and do not provide a mapping for it - this means that JAVA_CHAR will not be usable when linking What do you think? - PR Comment: https://git.openjdk.org/jdk/pull/14037#issuecomment-1558110434
Re: RFR: 8303040: linux PPC64le: Implementation of Foreign Function & Memory API (Preview) [v33]
On Mon, 22 May 2023 16:06:02 GMT, Martin Doerr wrote:
>> Implementation of "Foreign Function & Memory API" for linux on Power (Little
>> Endian) according to "Power Architecture 64-Bit ELF V2 ABI Specification".
>>
>> This PR does not include code for VaList support because it's supposed to
>> get removed by [JDK-8299736](https://bugs.openjdk.org/browse/JDK-8299736).
>> I've kept the related tests disabled for this platform and throw an
>> exception instead. Note that the ABI doesn't precisely specify variable
>> argument lists. Instead, it refers to `` (2.2.4 Variable Argument
>> Lists).
>>
>> Big Endian support is implemented to some extend, but not complete. E.g.
>> structs with size not divisible by 8 are not passed correctly (see
>> `useABIv2` in CallArranger.java). Big Endian is excluded by selecting
>> `ARCH.equals("ppc64le")` (CABI.java) only.
>>
>> There's another limitation: This PR only accepts structures with size
>> divisible by 4. (An `IllegalArgumentException` gets thrown otherwise.) I
>> think arbitrary sizes are not usable on other platforms, either, because
>> `SharedUtils.primitiveCarrierForSize` only accepts powers of 2. Update:
>> Resolved after merging of
>> [JDK-8303017](https://bugs.openjdk.org/browse/JDK-8303017)
>>
>> The ABI has some tricky corner cases related to HFA (Homogeneous Float
>> Aggregate). The same argument may need to get passed in both, a FP reg and a
>> GP reg or stack slot (see "no partial DW rule"). This cases are not covered
>> by the existing tests.
>>
>> I had to make changes to shared code and code for other platforms:
>> 1. Pass type information when creating `VMStorage` objects from `VMReg`.
>> This is needed for the following reasons:
>> - PPC64 ABI requires integer types to get extended to 64 bit (also see
>> CCallingConventionRequiresIntsAsLongs in existing hotspot code). We need to
>> know the type or at least the bit width for that.
>> - Floating point load / store instructions need the correct width to select
>> between the correct IEEE 754 formats. The register representation in single
>> FP registers is always IEEE 754 double precision on PPC64.
>> - Big Endian also needs usage of the precise size. Storing 8 Bytes and
>> loading 4 Bytes yields different values than on Little Endian!
>> 2. It happens that a `NativeMemorySegmentImpl` is used as a raw pointer
>> (with byteSize() == 0) while running TestUpcallScope. Hence, existing size
>> checks don't work (see MemorySegment.java). As a workaround, I'm just
>> skipping the check in this particular case. Please check if this makes sense
>> or if there's a better fix (possibly as separat...
>
> Martin Doerr has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Add comment about Register Save Area.
src/hotspot/cpu/ppc/downcallLinker_ppc.cpp line 161:
> 159: // (native_abi_reg_args is native_abi_minframe plus space for 8
> argument register spill slots)
> 160: assert(_abi._shadow_space_bytes == frame::native_abi_minframe_size,
> "expected space according to ABI");
> 161: // Note: For ABIv2, we only need (_input_registers.length() > 8) ?
> _input_registers.length() : 0
This is hard to understand. It should be explained that we allocate a PSA even
though ABI V2 only requires it if not all parameters can be passed in registers.
src/hotspot/cpu/ppc/downcallLinker_ppc.cpp line 162:
> 160: assert(_abi._shadow_space_bytes == frame::native_abi_minframe_size,
> "expected space according to ABI");
> 161: // Note: For ABIv2, we only need (_input_registers.length() > 8) ?
> _input_registers.length() : 0
> 162: int register_save_area_slots = MAX2(_input_registers.length(), 8);
Both specs, ABI V1 and V2, call this "Parameter Save Area" we should use the
same name.
Suggestion:
int parameter_save_area_slots = MAX2(_input_registers.length(), 8);
-
PR Review Comment: https://git.openjdk.org/jdk/pull/12708#discussion_r1201132931
PR Review Comment: https://git.openjdk.org/jdk/pull/12708#discussion_r1201128718
Re: RFR: 8303040: linux PPC64le: Implementation of Foreign Function & Memory API (Preview) [v34]
On Mon, 22 May 2023 21:36:12 GMT, Martin Doerr wrote:
>> Implementation of "Foreign Function & Memory API" for linux on Power (Little
>> Endian) according to "Power Architecture 64-Bit ELF V2 ABI Specification".
>>
>> This PR does not include code for VaList support because it's supposed to
>> get removed by [JDK-8299736](https://bugs.openjdk.org/browse/JDK-8299736).
>> I've kept the related tests disabled for this platform and throw an
>> exception instead. Note that the ABI doesn't precisely specify variable
>> argument lists. Instead, it refers to `` (2.2.4 Variable Argument
>> Lists).
>>
>> Big Endian support is implemented to some extend, but not complete. E.g.
>> structs with size not divisible by 8 are not passed correctly (see
>> `useABIv2` in CallArranger.java). Big Endian is excluded by selecting
>> `ARCH.equals("ppc64le")` (CABI.java) only.
>>
>> There's another limitation: This PR only accepts structures with size
>> divisible by 4. (An `IllegalArgumentException` gets thrown otherwise.) I
>> think arbitrary sizes are not usable on other platforms, either, because
>> `SharedUtils.primitiveCarrierForSize` only accepts powers of 2. Update:
>> Resolved after merging of
>> [JDK-8303017](https://bugs.openjdk.org/browse/JDK-8303017)
>>
>> The ABI has some tricky corner cases related to HFA (Homogeneous Float
>> Aggregate). The same argument may need to get passed in both, a FP reg and a
>> GP reg or stack slot (see "no partial DW rule"). This cases are not covered
>> by the existing tests.
>>
>> I had to make changes to shared code and code for other platforms:
>> 1. Pass type information when creating `VMStorage` objects from `VMReg`.
>> This is needed for the following reasons:
>> - PPC64 ABI requires integer types to get extended to 64 bit (also see
>> CCallingConventionRequiresIntsAsLongs in existing hotspot code). We need to
>> know the type or at least the bit width for that.
>> - Floating point load / store instructions need the correct width to select
>> between the correct IEEE 754 formats. The register representation in single
>> FP registers is always IEEE 754 double precision on PPC64.
>> - Big Endian also needs usage of the precise size. Storing 8 Bytes and
>> loading 4 Bytes yields different values than on Little Endian!
>> 2. It happens that a `NativeMemorySegmentImpl` is used as a raw pointer
>> (with byteSize() == 0) while running TestUpcallScope. Hence, existing size
>> checks don't work (see MemorySegment.java). As a workaround, I'm just
>> skipping the check in this particular case. Please check if this makes sense
>> or if there's a better fix (possibly as separat...
>
> Martin Doerr has updated the pull request with a new target base due to a
> merge or a rebase. The pull request now contains 41 commits:
>
> - Adaptation for JDK-8308276.
> - Merge remote-tracking branch 'origin' into PPC64_Panama
> - Add comment about Register Save Area.
> - Replace abstract method useABIv2().
> - Cleanup imports, improve comments, updates from other platforms.
> - Add NONZERO check for downcall_stub_address_offset_in_bytes().
> - Replace NULL by nullptr.
> - libTestHFA: Add explicit type conversion to avoid build warning.
> - Add test case for passing a double value in a GP register. Use better
> instructions for moving between FP and GP reg. Improve comments.
> - Merge remote-tracking branch 'origin' into PPC64_Panama
> - ... and 31 more: https://git.openjdk.org/jdk/compare/939344b8...08a5c143
Hi Martin,
there seems to be a mismatch between this pr and the [64-bit ELF ABI V2 for
PPC](https://openpowerfoundation.org/specifications/64bitelfabi/). In fact all
dynamically generated calls that have to conform to ABI V2 are affected. I'm
giving a short summary of our discussion this afternoon.
Very briefly: ABI V2 states that a Parameter Save Area (PSA) shall be allocated
_unless_ all parameters can be passed in registers as indicated by the caller's
prototype, whereas the port always allocates a PSA of 8 double words.
(Details under "Parameter Save Area" in "2.2.3.3. Optional Save Areas" of ELF
ABI V2)
It is not wrong what we're doing. It is like we didn't know the prototype of
the call targets. But for most calls [1] we are wasting stack space (and
confusing everybody that tries to match the implementation with the spec).
Interestingly ABI V1 states that a PSA of at least 8 double words is always
needed. Looks like we've missed that change.
I have conducted a little experiment and compiled the following test program
using Compiler Explorer [2]
#include
int64_t test_callee(int64_t p1, int64_t p2, int64_t p3, int64_t p4);
int64_t test_call(int64_t p1, int64_t p2, int64_t p3, int64_t p4) {
return test_callee(p1, p2, p3, p4);
}
This is the -O2 output for ELF ABI V2 (little endian)
Note: the stdu allocates just the minimal frame of 4 double words without PSA.
test_call: # @test_call
.Lfunc_gep0:
Re: RFR: 8303040: linux PPC64le: Implementation of Foreign Function & Memory API (Preview) [v34]
> Implementation of "Foreign Function & Memory API" for linux on Power (Little
> Endian) according to "Power Architecture 64-Bit ELF V2 ABI Specification".
>
> This PR does not include code for VaList support because it's supposed to get
> removed by [JDK-8299736](https://bugs.openjdk.org/browse/JDK-8299736). I've
> kept the related tests disabled for this platform and throw an exception
> instead. Note that the ABI doesn't precisely specify variable argument lists.
> Instead, it refers to `` (2.2.4 Variable Argument Lists).
>
> Big Endian support is implemented to some extend, but not complete. E.g.
> structs with size not divisible by 8 are not passed correctly (see `useABIv2`
> in CallArranger.java). Big Endian is excluded by selecting
> `ARCH.equals("ppc64le")` (CABI.java) only.
>
> There's another limitation: This PR only accepts structures with size
> divisible by 4. (An `IllegalArgumentException` gets thrown otherwise.) I
> think arbitrary sizes are not usable on other platforms, either, because
> `SharedUtils.primitiveCarrierForSize` only accepts powers of 2. Update:
> Resolved after merging of
> [JDK-8303017](https://bugs.openjdk.org/browse/JDK-8303017)
>
> The ABI has some tricky corner cases related to HFA (Homogeneous Float
> Aggregate). The same argument may need to get passed in both, a FP reg and a
> GP reg or stack slot (see "no partial DW rule"). This cases are not covered
> by the existing tests.
>
> I had to make changes to shared code and code for other platforms:
> 1. Pass type information when creating `VMStorage` objects from `VMReg`. This
> is needed for the following reasons:
> - PPC64 ABI requires integer types to get extended to 64 bit (also see
> CCallingConventionRequiresIntsAsLongs in existing hotspot code). We need to
> know the type or at least the bit width for that.
> - Floating point load / store instructions need the correct width to select
> between the correct IEEE 754 formats. The register representation in single
> FP registers is always IEEE 754 double precision on PPC64.
> - Big Endian also needs usage of the precise size. Storing 8 Bytes and
> loading 4 Bytes yields different values than on Little Endian!
> 2. It happens that a `NativeMemorySegmentImpl` is used as a raw pointer (with
> byteSize() == 0) while running TestUpcallScope. Hence, existing size checks
> don't work (see MemorySegment.java). As a workaround, I'm just skipping the
> check in this particular case. Please check if this makes sense or if there's
> a better fix (possibly as separate RFE). Update: T...
Martin Doerr has updated the pull request with a new target base due to a merge
or a rebase. The pull request now contains 41 commits:
- Adaptation for JDK-8308276.
- Merge remote-tracking branch 'origin' into PPC64_Panama
- Add comment about Register Save Area.
- Replace abstract method useABIv2().
- Cleanup imports, improve comments, updates from other platforms.
- Add NONZERO check for downcall_stub_address_offset_in_bytes().
- Replace NULL by nullptr.
- libTestHFA: Add explicit type conversion to avoid build warning.
- Add test case for passing a double value in a GP register. Use better
instructions for moving between FP and GP reg. Improve comments.
- Merge remote-tracking branch 'origin' into PPC64_Panama
- ... and 31 more: https://git.openjdk.org/jdk/compare/939344b8...08a5c143
-
Changes: https://git.openjdk.org/jdk/pull/12708/files
Webrev: https://webrevs.openjdk.org/?repo=jdk=12708=33
Stats: 2479 lines in 27 files changed: 2432 ins; 0 del; 47 mod
Patch: https://git.openjdk.org/jdk/pull/12708.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/12708/head:pull/12708
PR: https://git.openjdk.org/jdk/pull/12708
Re: RFR: 8307858: [REDO] JDK-8307194 Add make target for optionally building a complete set of all JDK and hotspot libjvm static libraries [v4]
> Original description for JDK-8307194 change: > - > This PR is branched from the makefile changes for > https://bugs.openjdk.org/browse/JDK-8303796 and contains the following for > handling the JDK/hotspot static libraries: > > - Build hotspot libjvm.a and JDK static libraries for > static-libs-image/static-libs-bundles targets; This change does not affect > the graal-builder-image target > > - For libjvm.a specifically, exclude operator_new.o > > - Filter out "external" .o files (those are the .o files included from a > different JDK library and needed when creating the .so shared library only) > from JDK .a libraries; That's to avoid linker failures caused by duplicate > symbols > - For libjli.a: Not include inflate.o inftrees.o inffast.o zadler32.o > zcrc32.o zutil.o (compiled from zlib sources) if zlib is built as JDK bundled > - For libawt_xawt.a and libawt_head.a: Not include systemScale.o, since > it's provided in libawt.a > > - Handle long arguments case for static build in > make/common/NativeCompilation.gmk > > - Address @erikj79's comment in > https://github.com/openjdk/jdk/pull/13709#discussion_r1180750185 for > LIBJLI_STATIC_EXCLUDE_OBJS > - > > Updates to address build failures reported on macosx- platforms: > > - For gcc/clang, when building a static library first partially link (using > the `-r` linking option) all object files into one object. The output object > file from the partial linking is then passed to `ar` to create the static > library. > > The original change for JDK-8307194 used @argument_file for all platforms > when dealing with long arguments to `ar`, which caused failures on > macosx- builds. On darwin (https://www.unix.com/man-page/osx/1/ar/), > `ar` does not support @argument_file. The updated change avoids using > @argument_file for `ar`. > > The partial linking change is done in make/common/NativeCompilation.gmk. The > flag related change is done in make/autoconf/flags-ldflags.m4 mainly. Jiangli Zhou has updated the pull request incrementally with one additional commit since the last revision: - Add $$($1_LD) $$($1_SYSROOT_LDFLAGS) to $1_VARDEPS if $(TOOLCHAIN_TYPE) is gcc or clang, as suggested by @erikj79. - Changes: - all: https://git.openjdk.org/jdk/pull/14064/files - new: https://git.openjdk.org/jdk/pull/14064/files/6a321bd1..695bda5e Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=14064=03 - incr: https://webrevs.openjdk.org/?repo=jdk=14064=02-03 Stats: 3 lines in 1 file changed: 3 ins; 0 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/14064.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14064/head:pull/14064 PR: https://git.openjdk.org/jdk/pull/14064
Re: RFR: 8307858: [REDO] JDK-8307194 Add make target for optionally building a complete set of all JDK and hotspot libjvm static libraries [v2]
On Mon, 22 May 2023 19:52:42 GMT, Erik Joelsson wrote: >> Jiangli Zhou has updated the pull request with a new target base due to a >> merge or a rebase. The pull request now contains 12 commits: >> >> - Merge branch 'master' into JDK-8307858 >> - Merge branch 'master' into JDK-8307858 >> - Clean up. >> - In clude $MACHINE_FLAG in partial linking flag. >> - Use '-m32' instead of '-m elf_i386'. >> - Use '-m elf_i386' for partial linking with gcc for linux 32-bit platform. >> >>It's based on the post on >> https://www.linuxquestions.org/questions/linux-software-2/relocatable-linking-on-x86-64-for-i386-872812/. >> - Only do partial linking step with gcc/clang on 64-bit platform. >> >>There is a linking failure with linux-x86 build: >> >>/usr/bin/ld: relocatable linking with relocations from format elf32-i386 >> (/home/runner/work/jdk/jdk/build/linux-x86/hotspot/variant-server/libjvm/libgtest/objs/gmock-all.o) >> to format elf64-x86-64 >> (/home/runner/work/jdk/jdk/build/linux-x86/hotspot/variant-server/libjvm/libgtest/objs/libgtest_relocatable.o) >> is not supported >> - Need to set $1_AR_OBJ_ARG to $$($1_LD_OBJ_ARG) instead of $1_LD_OBJ_ARG. >> - Merge branch 'master' into JDK-8307858 >> - Revert >> src/java.desktop/linux/native/libjsound/PLATFORM_API_LinuxOS_ALSA_CommonUtils.c >> change. >> - ... and 2 more: https://git.openjdk.org/jdk/compare/8474e693...fb945210 > > make/common/NativeCompilation.gmk line 1175: > >> 1173: >> 1174: ifeq ($$($1_TYPE), STATIC_LIBRARY) >> 1175: $1_VARDEPS := $$($1_AR) $$(ARFLAGS) $$($1_ARFLAGS) $$($1_LIBS) \ > > We also need to add some things to VARDEPS. Looks like at least `$$($1_LD)` > and `$$($1_SYSROOT_LDFLAGS)` are needed. Maybe conditionally. Fix, thanks. - PR Review Comment: https://git.openjdk.org/jdk/pull/14064#discussion_r1201128665
Re: RFR: 8307858: [REDO] JDK-8307194 Add make target for optionally building a complete set of all JDK and hotspot libjvm static libraries [v3]
> Original description for JDK-8307194 change: > - > This PR is branched from the makefile changes for > https://bugs.openjdk.org/browse/JDK-8303796 and contains the following for > handling the JDK/hotspot static libraries: > > - Build hotspot libjvm.a and JDK static libraries for > static-libs-image/static-libs-bundles targets; This change does not affect > the graal-builder-image target > > - For libjvm.a specifically, exclude operator_new.o > > - Filter out "external" .o files (those are the .o files included from a > different JDK library and needed when creating the .so shared library only) > from JDK .a libraries; That's to avoid linker failures caused by duplicate > symbols > - For libjli.a: Not include inflate.o inftrees.o inffast.o zadler32.o > zcrc32.o zutil.o (compiled from zlib sources) if zlib is built as JDK bundled > - For libawt_xawt.a and libawt_head.a: Not include systemScale.o, since > it's provided in libawt.a > > - Handle long arguments case for static build in > make/common/NativeCompilation.gmk > > - Address @erikj79's comment in > https://github.com/openjdk/jdk/pull/13709#discussion_r1180750185 for > LIBJLI_STATIC_EXCLUDE_OBJS > - > > Updates to address build failures reported on macosx- platforms: > > - For gcc/clang, when building a static library first partially link (using > the `-r` linking option) all object files into one object. The output object > file from the partial linking is then passed to `ar` to create the static > library. > > The original change for JDK-8307194 used @argument_file for all platforms > when dealing with long arguments to `ar`, which caused failures on > macosx- builds. On darwin (https://www.unix.com/man-page/osx/1/ar/), > `ar` does not support @argument_file. The updated change avoids using > @argument_file for `ar`. > > The partial linking change is done in make/common/NativeCompilation.gmk. The > flag related change is done in make/autoconf/flags-ldflags.m4 mainly. Jiangli Zhou has updated the pull request incrementally with one additional commit since the last revision: Update make/common/NativeCompilation.gmk Co-authored-by: Erik Joelsson <37597443+erik...@users.noreply.github.com> - Changes: - all: https://git.openjdk.org/jdk/pull/14064/files - new: https://git.openjdk.org/jdk/pull/14064/files/fb945210..6a321bd1 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=14064=02 - incr: https://webrevs.openjdk.org/?repo=jdk=14064=01-02 Stats: 2 lines in 1 file changed: 1 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/14064.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14064/head:pull/14064 PR: https://git.openjdk.org/jdk/pull/14064
Re: RFR: 8307858: [REDO] JDK-8307194 Add make target for optionally building a complete set of all JDK and hotspot libjvm static libraries [v2]
On Mon, 22 May 2023 19:54:59 GMT, Erik Joelsson wrote: >> Jiangli Zhou has updated the pull request with a new target base due to a >> merge or a rebase. The pull request now contains 12 commits: >> >> - Merge branch 'master' into JDK-8307858 >> - Merge branch 'master' into JDK-8307858 >> - Clean up. >> - In clude $MACHINE_FLAG in partial linking flag. >> - Use '-m32' instead of '-m elf_i386'. >> - Use '-m elf_i386' for partial linking with gcc for linux 32-bit platform. >> >>It's based on the post on >> https://www.linuxquestions.org/questions/linux-software-2/relocatable-linking-on-x86-64-for-i386-872812/. >> - Only do partial linking step with gcc/clang on 64-bit platform. >> >>There is a linking failure with linux-x86 build: >> >>/usr/bin/ld: relocatable linking with relocations from format elf32-i386 >> (/home/runner/work/jdk/jdk/build/linux-x86/hotspot/variant-server/libjvm/libgtest/objs/gmock-all.o) >> to format elf64-x86-64 >> (/home/runner/work/jdk/jdk/build/linux-x86/hotspot/variant-server/libjvm/libgtest/objs/libgtest_relocatable.o) >> is not supported >> - Need to set $1_AR_OBJ_ARG to $$($1_LD_OBJ_ARG) instead of $1_LD_OBJ_ARG. >> - Merge branch 'master' into JDK-8307858 >> - Revert >> src/java.desktop/linux/native/libjsound/PLATFORM_API_LinuxOS_ALSA_CommonUtils.c >> change. >> - ... and 2 more: https://git.openjdk.org/jdk/compare/8474e693...fb945210 > > make/common/NativeCompilation.gmk line 1208: > >> 1206: $$(call ExecuteWithLog, >> $$($1_OBJECT_DIR)/$$($1_SAFE_NAME)_partial_link, \ >> 1207: $$($1_LD) $(LDFLAGS_CXX_PARTIAL_LINKING) >> $(LD_OUT_OPTION)$$($1_TARGET_RELOCATABLE) \ >> 1208: $$($1_LD_OBJ_ARG)) > > This makes my build pass. > Suggestion: > >$$($1_LD) $(LDFLAGS_CXX_PARTIAL_LINKING) $$($1_SYSROOT_LDFLAGS) \ >$(LD_OUT_OPTION)$$($1_TARGET_RELOCATABLE) \ > $$($1_LD_OBJ_ARG)) Thanks! Tested with a macosx-x86_64 build and also with our prototype on JDK 11 for linux-x64. They still build okay with the change. Looks like it affects/needed for cross build. Committing your fix. - PR Review Comment: https://git.openjdk.org/jdk/pull/14064#discussion_r1201077691
Re: RFR: 8159023: Engineering notation of DecimalFormat does not work as documented
On Sat, 20 May 2023 00:00:42 GMT, Justin Lu wrote:
> Please review this PR which updates the Scientific Notation section of
> Decimal Format. It aims to resolve
> [JDK-8159023](https://bugs.openjdk.org/browse/JDK-8159023) as well as
> [JDK-6282188](https://bugs.openjdk.org/browse/JDK-6282188).
>
> **Scientific Notation** in Decimal Format contains the definition for a
> scientific notation formatted number's mantissa as such: _The number of
> significant digits in the mantissa is the sum of the minimum integer and
> maximum fraction digits, and is unaffected by the maximum integer digits. For
> example, 12345 formatted with "##0.##E0" is "12.3E3"._
>
> Both the definition and example are incorrect, as the actual result is
> "12.E345".
>
> The following example data show that scientific notation formatted numbers do
> not adhere to that definition. As, according to the definition, the following
> numbers should have 3 significant digits, but in reality, they have up to 5.
>
> 123 formatted by ##0.#E0 is 123E0
> 1234 formatted by ##0.#E0 is 1.234E3
> 12345 formatted by ##0.#E0 is 12.34E3
> 123456 formatted by ##0.#E0 is 123.5E3
> 1234567 formatted by ##0.#E0 is 1.235E6
> 12345678 formatted by ##0.#E0 is 12.35E6
> 123456789 formatted by ##0.#E0 is 123.5E6
>
>
> The actual definition of the mantissa, as well as examples and further
> context are included in this change. In addition, a test has been added that
> tests various patterns to numbers and ensures the format follows the new
> definition's mathematical expression.
(a bit of background to others) We did some archaeological work to dig into the
history of this spec, and found that the spec was retrofitted to the
implementation. (https://bugs.openjdk.org/browse/JDK-4241670). We suspect that
this wording was incorrectly interpreted from the impl at that time.
test/jdk/java/text/Format/DecimalFormat/MantissaDigits.java line 56:
> 54: public void testMantissaDefinition(String pattern) {
> 55: Locale.setDefault(Locale.ENGLISH);
> 56: DecimalFormat df = new DecimalFormat(pattern);
Instead of setting the default locale, you could get the explicit format by
new DecimalFormat(pattern, new DecimalFormatSymbols(Locale.US)); // DFS can be
a static instance
test/jdk/java/text/Format/DecimalFormat/MantissaDigits.java line 89:
> 87: // Test the new definition of the Mantissa
> 88: Integer calculatedDigits = Math
> 89: .min(Math.max(zeroes, originalNumDigits),
> (hashes+zeroes));
Could we make those expected numbers into the `Arguments` so that there is no
need to calculate in the test case at runtime? I think that would be more
readable.
-
PR Review: https://git.openjdk.org/jdk/pull/14066#pullrequestreview-1437505357
PR Review Comment: https://git.openjdk.org/jdk/pull/14066#discussion_r1201067500
PR Review Comment: https://git.openjdk.org/jdk/pull/14066#discussion_r1201062733
Re: RFR: 8307205: Downcall to clibrary printf fails on OS X AArch64
On Mon, 22 May 2023 15:21:37 GMT, Per Minborg wrote: > This PR adds a test that shows the documented way of calling `printf` works. I'm not sure I get this: StdLibTest already tests variadic calls, and there's a brand new javadoc section about this topic: https://download.java.net/java/early_access/jdk21/docs/api/java.base/java/lang/foreign/Linker.html#variadic-funcs Note: I'm not opposed to the test per se, but I note that (a) test-wise it is redundant, and (b) using tests as placeholder for documentation is a bit odd. (Also, note that the bug this PR refers to is closed) - PR Comment: https://git.openjdk.org/jdk/pull/14088#issuecomment-1557943328
Re: RFR: JDK-8298066: java/util/concurrent/locks/Lock/OOMEInAQS.java timed out
On Mon, 22 May 2023 14:46:02 GMT, Alan Bateman wrote: >> Should the test be removed from ProblemList-generational-zgc.txt as part of >> this? > >> @AlanBateman My thinking was to integrate this and make sure that the >> problem doesn't resurface before removing it from the problem list. Does >> that make sense? 樂 > > No objection, it's just it would leave ProblemList-generational-zgc.txt > listing a test against as issue marked as fixed. @AlanBateman I wonder, perhaps it makes more sense, as you say, to clear the problem list together with this, and resurrect it in case it spuriously resurfaces. It's always a challenge to prove a negative. :) - PR Comment: https://git.openjdk.org/jdk/pull/14082#issuecomment-1557920226
Re: RFR: 8307858: [REDO] JDK-8307194 Add make target for optionally building a complete set of all JDK and hotspot libjvm static libraries [v2]
On Mon, 22 May 2023 19:40:00 GMT, Jiangli Zhou wrote: >> Original description for JDK-8307194 change: >> - >> This PR is branched from the makefile changes for >> https://bugs.openjdk.org/browse/JDK-8303796 and contains the following for >> handling the JDK/hotspot static libraries: >> >> - Build hotspot libjvm.a and JDK static libraries for >> static-libs-image/static-libs-bundles targets; This change does not affect >> the graal-builder-image target >> >> - For libjvm.a specifically, exclude operator_new.o >> >> - Filter out "external" .o files (those are the .o files included from a >> different JDK library and needed when creating the .so shared library only) >> from JDK .a libraries; That's to avoid linker failures caused by duplicate >> symbols >> - For libjli.a: Not include inflate.o inftrees.o inffast.o zadler32.o >> zcrc32.o zutil.o (compiled from zlib sources) if zlib is built as JDK bundled >> - For libawt_xawt.a and libawt_head.a: Not include systemScale.o, since >> it's provided in libawt.a >> >> - Handle long arguments case for static build in >> make/common/NativeCompilation.gmk >> >> - Address @erikj79's comment in >> https://github.com/openjdk/jdk/pull/13709#discussion_r1180750185 for >> LIBJLI_STATIC_EXCLUDE_OBJS >> - >> >> Updates to address build failures reported on macosx- platforms: >> >> - For gcc/clang, when building a static library first partially link (using >> the `-r` linking option) all object files into one object. The output object >> file from the partial linking is then passed to `ar` to create the static >> library. >> >> The original change for JDK-8307194 used @argument_file for all platforms >> when dealing with long arguments to `ar`, which caused failures on >> macosx- builds. On darwin (https://www.unix.com/man-page/osx/1/ar/), >> `ar` does not support @argument_file. The updated change avoids using >> @argument_file for `ar`. >> >> The partial linking change is done in make/common/NativeCompilation.gmk. The >> flag related change is done in make/autoconf/flags-ldflags.m4 mainly. > > Jiangli Zhou has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains 12 commits: > > - Merge branch 'master' into JDK-8307858 > - Merge branch 'master' into JDK-8307858 > - Clean up. > - In clude $MACHINE_FLAG in partial linking flag. > - Use '-m32' instead of '-m elf_i386'. > - Use '-m elf_i386' for partial linking with gcc for linux 32-bit platform. > >It's based on the post on > https://www.linuxquestions.org/questions/linux-software-2/relocatable-linking-on-x86-64-for-i386-872812/. > - Only do partial linking step with gcc/clang on 64-bit platform. > >There is a linking failure with linux-x86 build: > >/usr/bin/ld: relocatable linking with relocations from format elf32-i386 > (/home/runner/work/jdk/jdk/build/linux-x86/hotspot/variant-server/libjvm/libgtest/objs/gmock-all.o) > to format elf64-x86-64 > (/home/runner/work/jdk/jdk/build/linux-x86/hotspot/variant-server/libjvm/libgtest/objs/libgtest_relocatable.o) > is not supported > - Need to set $1_AR_OBJ_ARG to $$($1_LD_OBJ_ARG) instead of $1_LD_OBJ_ARG. > - Merge branch 'master' into JDK-8307858 > - Revert > src/java.desktop/linux/native/libjsound/PLATFORM_API_LinuxOS_ALSA_CommonUtils.c > change. > - ... and 2 more: https://git.openjdk.org/jdk/compare/8474e693...fb945210 make/common/NativeCompilation.gmk line 1175: > 1173: > 1174: ifeq ($$($1_TYPE), STATIC_LIBRARY) > 1175: $1_VARDEPS := $$($1_AR) $$(ARFLAGS) $$($1_ARFLAGS) $$($1_LIBS) \ We also need to add some things to VARDEPS. Looks like at least `$$($1_LD)` and `$$($1_SYSROOT_LDFLAGS)` are needed. Maybe conditionally. make/common/NativeCompilation.gmk line 1208: > 1206: $$(call ExecuteWithLog, > $$($1_OBJECT_DIR)/$$($1_SAFE_NAME)_partial_link, \ > 1207: $$($1_LD) $(LDFLAGS_CXX_PARTIAL_LINKING) > $(LD_OUT_OPTION)$$($1_TARGET_RELOCATABLE) \ > 1208: $$($1_LD_OBJ_ARG)) This makes my build pass. Suggestion: $$($1_LD) $(LDFLAGS_CXX_PARTIAL_LINKING) $$($1_SYSROOT_LDFLAGS) \ $(LD_OUT_OPTION)$$($1_TARGET_RELOCATABLE) \ $$($1_LD_OBJ_ARG)) - PR Review Comment: https://git.openjdk.org/jdk/pull/14064#discussion_r1200980626 PR Review Comment: https://git.openjdk.org/jdk/pull/14064#discussion_r1200982365
Re: RFR: 8308016: Use snippets in java.io package [v8]
On Mon, 22 May 2023 19:28:26 GMT, Roger Riggs wrote: > Thanks for the updates. Thanks for all the comments (and the approval). - PR Comment: https://git.openjdk.org/jdk/pull/13957#issuecomment-1557863535
Re: RFR: 8307858: [REDO] JDK-8307194 Add make target for optionally building a complete set of all JDK and hotspot libjvm static libraries [v2]
On Mon, 22 May 2023 19:21:52 GMT, Jiangli Zhou wrote: >> Thanks @erikj79. Could you please help provide some more info on the build >> failure: >> >> Which macOs version ran into the build issue? My mac is on Ventura 13.3.1 >> (a). It builds successfully for the `static-libs-image` target. >> >> Does it fail when partially linking `libjvm_relocatable.o` only, or it fails >> when partially linking other native libraries as well on macosx? Could you >> please share the partial linking command, e.g. >> hotspot/variant-server/libjvm/gtest/objs/static/BUILD_GTEST_LIBJVM_partial_link.cmdline? > >> Thanks @erikj79. Could you please help provide some more info on the build >> failure: >> >> Which macOs version ran into the build issue? My mac is on Ventura 13.3.1 >> (a). It builds successfully for the `static-libs-image` target. >> >> Does it fail when partially linking `libjvm_relocatable.o` only, or it fails >> when partially linking other native libraries as well on macosx? Could you >> please share the partial linking command, e.g. >> hotspot/variant-server/libjvm/gtest/objs/static/BUILD_GTEST_LIBJVM_partial_link.cmdline? > > Additional info, following is the partial linking command from my > `macosx-x86_64` build: > > > /usr/bin/g++ -m64 -r -o > /.../github/JDK-8307858/build/linux-x86_64-server-release/hotspot/variant-server/libjvm/gtest/objs/static/libjvm_relocatable.o > > @/.../github/JDK-8307858/build/linux-x86_64-server-release/hotspot/variant-server/libjvm/gtest/objs/static/_BUILD_GTEST_LIBJVM_objectfilenames.txt It fails because we are using a "devkit" and not an installed Xcode. The SYSROOT flags must always be added to any compiler or link command lines. - PR Review Comment: https://git.openjdk.org/jdk/pull/14064#discussion_r1200973154
Re: RFR: 8307858: [REDO] JDK-8307194 Add make target for optionally building a complete set of all JDK and hotspot libjvm static libraries [v2]
> Original description for JDK-8307194 change: > - > This PR is branched from the makefile changes for > https://bugs.openjdk.org/browse/JDK-8303796 and contains the following for > handling the JDK/hotspot static libraries: > > - Build hotspot libjvm.a and JDK static libraries for > static-libs-image/static-libs-bundles targets; This change does not affect > the graal-builder-image target > > - For libjvm.a specifically, exclude operator_new.o > > - Filter out "external" .o files (those are the .o files included from a > different JDK library and needed when creating the .so shared library only) > from JDK .a libraries; That's to avoid linker failures caused by duplicate > symbols > - For libjli.a: Not include inflate.o inftrees.o inffast.o zadler32.o > zcrc32.o zutil.o (compiled from zlib sources) if zlib is built as JDK bundled > - For libawt_xawt.a and libawt_head.a: Not include systemScale.o, since > it's provided in libawt.a > > - Handle long arguments case for static build in > make/common/NativeCompilation.gmk > > - Address @erikj79's comment in > https://github.com/openjdk/jdk/pull/13709#discussion_r1180750185 for > LIBJLI_STATIC_EXCLUDE_OBJS > - > > Updates to address build failures reported on macosx- platforms: > > - For gcc/clang, when building a static library first partially link (using > the `-r` linking option) all object files into one object. The output object > file from the partial linking is then passed to `ar` to create the static > library. > > The original change for JDK-8307194 used @argument_file for all platforms > when dealing with long arguments to `ar`, which caused failures on > macosx- builds. On darwin (https://www.unix.com/man-page/osx/1/ar/), > `ar` does not support @argument_file. The updated change avoids using > @argument_file for `ar`. > > The partial linking change is done in make/common/NativeCompilation.gmk. The > flag related change is done in make/autoconf/flags-ldflags.m4 mainly. Jiangli Zhou has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 12 commits: - Merge branch 'master' into JDK-8307858 - Merge branch 'master' into JDK-8307858 - Clean up. - In clude $MACHINE_FLAG in partial linking flag. - Use '-m32' instead of '-m elf_i386'. - Use '-m elf_i386' for partial linking with gcc for linux 32-bit platform. It's based on the post on https://www.linuxquestions.org/questions/linux-software-2/relocatable-linking-on-x86-64-for-i386-872812/. - Only do partial linking step with gcc/clang on 64-bit platform. There is a linking failure with linux-x86 build: /usr/bin/ld: relocatable linking with relocations from format elf32-i386 (/home/runner/work/jdk/jdk/build/linux-x86/hotspot/variant-server/libjvm/libgtest/objs/gmock-all.o) to format elf64-x86-64 (/home/runner/work/jdk/jdk/build/linux-x86/hotspot/variant-server/libjvm/libgtest/objs/libgtest_relocatable.o) is not supported - Need to set $1_AR_OBJ_ARG to $$($1_LD_OBJ_ARG) instead of $1_LD_OBJ_ARG. - Merge branch 'master' into JDK-8307858 - Revert src/java.desktop/linux/native/libjsound/PLATFORM_API_LinuxOS_ALSA_CommonUtils.c change. - ... and 2 more: https://git.openjdk.org/jdk/compare/8474e693...fb945210 - Changes: https://git.openjdk.org/jdk/pull/14064/files Webrev: https://webrevs.openjdk.org/?repo=jdk=14064=01 Stats: 201 lines in 10 files changed: 150 ins; 34 del; 17 mod Patch: https://git.openjdk.org/jdk/pull/14064.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14064/head:pull/14064 PR: https://git.openjdk.org/jdk/pull/14064
Re: RFR: 8308016: Use snippets in java.io package [v8]
On Thu, 18 May 2023 19:14:02 GMT, Brian Burkhalter wrote:
>> Replace `{@code ...}` patterns and the like with `{@snippet
>> lang=java : ...}`.
>
> Brian Burkhalter has updated the pull request incrementally with one
> additional commit since the last revision:
>
> 8308016: Address reviewer comments since previous commit
Thanks for the updates.
-
Marked as reviewed by rriggs (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/13957#pullrequestreview-1437322626
Re: RFR: 8307858: [REDO] JDK-8307194 Add make target for optionally building a complete set of all JDK and hotspot libjvm static libraries
On Mon, 22 May 2023 19:10:57 GMT, Jiangli Zhou wrote: > Thanks @erikj79. Could you please help provide some more info on the build > failure: > > Which macOs version ran into the build issue? My mac is on Ventura 13.3.1 > (a). It builds successfully for the `static-libs-image` target. > > Does it fail when partially linking `libjvm_relocatable.o` only, or it fails > when partially linking other native libraries as well on macosx? Could you > please share the partial linking command, e.g. > hotspot/variant-server/libjvm/gtest/objs/static/BUILD_GTEST_LIBJVM_partial_link.cmdline? Additional info, following is the partial linking command from my `macosx-x86_64` build: /usr/bin/g++ -m64 -r -o /.../github/JDK-8307858/build/linux-x86_64-server-release/hotspot/variant-server/libjvm/gtest/objs/static/libjvm_relocatable.o @/.../github/JDK-8307858/build/linux-x86_64-server-release/hotspot/variant-server/libjvm/gtest/objs/static/_BUILD_GTEST_LIBJVM_objectfilenames.txt - PR Review Comment: https://git.openjdk.org/jdk/pull/14064#discussion_r1200946838
Re: RFR: 8307858: [REDO] JDK-8307194 Add make target for optionally building a complete set of all JDK and hotspot libjvm static libraries
On Fri, 19 May 2023 22:45:16 GMT, Erik Joelsson wrote: >> make/common/NativeCompilation.gmk line 1208: >> >>> 1206: $$(call ExecuteWithLog, >>> $$($1_OBJECT_DIR)/$$($1_SAFE_NAME)_partial_link, \ >>> 1207:$$($1_LD) $(LDFLAGS_CXX_PARTIAL_LINKING) >>> $(LD_OUT_OPTION)$$($1_TARGET_RELOCATABLE) \ >>> 1208: $$($1_LD_OBJ_ARG)) >> >> This command line is missing `$$($1_SYSROOT_LDFLAGS)` which is causing it to >> fail in our builds with: >> >> ld: library not found for -lSystem > > This is the mac failure, sorry if that wasn't clear. Thanks @erikj79. Could you please help provide some more info on the build failure: Which macOs version ran into the build issue? My mac is on Ventura 13.3.1 (a). It builds successfully for the `static-libs-image` target. Does it fail when partially linking `libjvm_relocatable.o` only, or it fails when partially linking other native libraries as well on macosx? Could you please share the partial linking command, e.g. hotspot/variant-server/libjvm/gtest/objs/static/BUILD_GTEST_LIBJVM_partial_link.cmdline? - PR Review Comment: https://git.openjdk.org/jdk/pull/14064#discussion_r1200929473
Re: javac to emit static flag for local and anonymous classes in static contexts
Hi Liang, On Sun, May 21, 2023 at 11:29 PM - wrote: > Thus, I suggest emitting static flags for the InnerClasses attribute > of anonymous/local classes in static/pre-initialization contexts, > Related to your question on PR#13656 ... local/anonymous classes in a pre-initialization context (which we're now calling a "pre-construction" context) are not static, they're just not instantiable until after superclass construction. -Archie -- Archie L. Cobbs
Re: RFR: 8305538: Avoid redundant HashMap.containsKey call in ModuleDescriptor.Builder [v2]
On Tue, 4 Apr 2023 20:50:00 GMT, Andrey Turbanov wrote: >> `Map.containsKey` call is sometimes unnecessary, when it's known that `Map` >> doesn't contain `null` values. >> Instead of pair containsKey+put we can use putIfAbsent and compare result >> with null. >> Result code is shorter and a bit faster. >> Same approach is used with `HashSet uses` in >> `java.lang.module.ModuleDescriptor.Builder#uses`. Instead of separate >> `contains`+`add` - we can just call `add` then check what it returns. >> >> Testing: Linux x64 `java/lang` > > Andrey Turbanov has updated the pull request incrementally with one > additional commit since the last revision: > > 8305538: Avoid redundant HashMap.containsKey call in > ModuleDescriptor.Builder > > update java.lang.module.ModuleDescriptor.Builder.uses too let's wait for review a bit more - PR Comment: https://git.openjdk.org/jdk/pull/13288#issuecomment-1557703281
Re: Exposing checked exceptions that a MethodHandle can throw
Thanks for the good response Remi. This is part of a larger FAQ, “why are MHs hard to use?” Part of the answer is “they model bytecode behavior”. (Perhaps they should have been called BytecodeBehaviorHandles.) Lifting such raw VM-level behaviors up into Java source code is necessary, but it will never be pleasant, at least not until Java has enough capabilities in its syntax and type system to model such beasties without extra layers of object wrapping. That would include fully incorporating exception checking into the generic type system, a hard problem. It would also involve making some way to name a Java method (perhaps as `Foo::bar` or `myFoo::bar` or some lambda) but get a MH out of the expression. Also some kinds of varargs processing might be needed to “add suger” to varargs-related MH transforms. The amount of Java language engineering necessary for such things is so large it will never be done, if MHs are the only use case. There are far too many important improvements to make. (Thought experiment: Should we drop some part of the pattern matching work in order to make room for method handle exception checks? I thought not.) Perhaps in the future there will come a time when exception checking is tracked and/or `Foo::bar` syntax is made more generic, and that will benefit MHs, but if it happens it will be for a list of weighty reasons, apart from MHs. For now, MH code has to be written in a low-level style in Java source code. (But it works beautifully at the assembly code level, if you are spinning bytecodes.) For example, Java source methods which exist to process MHs should just declare `throws Throwable`. Then you catch the non-existent throwables at the boundary. You can make this a little easier on yourself, sometimes, if you write a higher-order helper function that takes a Throwable-throwing lambda and sanitizes it down to the exceptions you expect. Then there’s just one clunky `catch`, and your low-level Throwable-throwing code goes inside lambda bodies. On 21 May 2023, at 6:47, Remi Forax wrote: - Original Message - From: "-" To: "core-libs-dev" Sent: Sunday, May 21, 2023 6:52:44 AM Subject: Exposing checked exceptions that a MethodHandle can throw Hello, I am eliciting a discussion on the feasibility of tracking checked exceptions thrown by a MethodHandle. It is already requested in https://bugs.openjdk.org/browse/JDK-8268116 as it appears useful in the development of Foreign Function Interface API. At the bytecode level, checked exceptions are stored in an attribute associated to a method. https://docs.oracle.com/javase/specs/jvms/se20/html/jvms-4.html#jvms-4.7.5 If you have a direct MethodHandle, you can already get the checked exceptions using Lookup.revealDirect() + MethodHandleInfo.reflectAs(). Currently, explicit MethodHandle usages are hampered by the catch block as the invoke methods are declared to throw any Throwable. Could it be possible that we specify the types of possible exceptions at MethodHandle invocation, so that: 1. Javac accepts code that just call the handle without ugly try-catch block 2. If the exceptions anticipated at invocation site are incompatible with (i.e. more specific than) those declared by the invoked handle, it can throw an exception like the existing `WrongMethodTypeException` eagerly. The bug you reference seems to be about runtime information, but the paragraph above is about type-checking information. The question here is "is the Java type system good enough to track checked exception in a backward compatible way ?" Practically, I believe the answer is no, you can not compose function with different type parameters representing exception, there is no varargs of type parameters, there is no default type argument for a type parameter, etc. It's also significant departure of the way the method handle API is created, the idea behind is that each combiner has the semantics of an existing bytecode. But all invoke* bytecodes are oblivious to exception. Said differently, the method handle API represent how the JVM works, not how Java the language works. Is such a plan feasible? Tracking of exceptions should be easy from Direct MH and the MethodHandles combinators already, while the invocation semantics update might be more complex, maybe in the form of compiler-inserted stubs before the actual invoke calls. I wish such improvements can make MethodHandle more friendly to direct usages in code, so users don't need to wrap MH invocations in try-catch blocks everywhere. see above. Chen Liang Rémi
Re: RFR: 8308286 Fix clang warnings in linux code
On Wed, 17 May 2023 12:28:47 GMT, Artem Semenov wrote: > When using the clang compiler to build OpenJDk on Linux, we encounter various > "warnings as errors". > They can be fixed with small changes. I don't like this approach at all. if github had a "reject" button I'd be pushing it now. updating the makefiles is the normal way to do this but I don't know if we even want that. Until clang is the supported compiler for Linux then I see no reason for this to be in JDK at all. Code changes which make it so there's no warning would be the preferred way but I'd expect that to be tested properly. Also if the warning is spurious - sometimes they are - then that would NOT be appropriate. - Changes requested by prr (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/14033#pullrequestreview-1437172390
Re: RFR: 8308108: Support Unicode extension for collation settings [v5]
On Mon, 22 May 2023 17:59:18 GMT, Naoto Sato wrote: >> This change intends to interpret the BCP47 U extension wrt collation >> settings in the given `Locale`, then applies them to the created instances >> in the 1-arg factory method in `Collator`. A corresponding CSR has also been >> drafted. > > Naoto Sato has updated the pull request incrementally with one additional > commit since the last revision: > > Further clarifying the method description Thanks, Alan. Clarified the method doc based on your suggestion. - PR Comment: https://git.openjdk.org/jdk/pull/14040#issuecomment-1557663088
Re: RFR: 8308108: Support Unicode extension for collation settings [v5]
> This change intends to interpret the BCP47 U extension wrt collation settings > in the given `Locale`, then applies them to the created instances in the > 1-arg factory method in `Collator`. A corresponding CSR has also been drafted. Naoto Sato has updated the pull request incrementally with one additional commit since the last revision: Further clarifying the method description - Changes: - all: https://git.openjdk.org/jdk/pull/14040/files - new: https://git.openjdk.org/jdk/pull/14040/files/1cfc09b6..83cfe984 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=14040=04 - incr: https://webrevs.openjdk.org/?repo=jdk=14040=03-04 Stats: 4 lines in 1 file changed: 0 ins; 1 del; 3 mod Patch: https://git.openjdk.org/jdk/pull/14040.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14040/head:pull/14040 PR: https://git.openjdk.org/jdk/pull/14040
Re: RFR: 8308293: A linker should expose the layouts it supports [v4]
On Mon, 22 May 2023 14:34:32 GMT, Per Minborg wrote: > LGTM. Are there any additional types we might consider apart from the basic > ones and `size_t`? Maybe one for an address pointing at `errno`? I think there is a certain gravitational pull towards keeping the set of guaranteed canonical layouts as minimal as possible. In that sense pointer to errno seems not to meet the bar IMHO. (also note that one could always ask the captureStateLayout, and figure out how to express the errno type from there). - PR Comment: https://git.openjdk.org/jdk/pull/14037#issuecomment-1557585319
RFR: 8308090: Add container tests for on-the-fly resource quota updates
Please review these test changes which implement automatic testing of container resource updates without JVM restart. Note that this merely tests container detection code handling this case. It doesn't do anything special for the JVM itself, though it might make sense to add some sanity checks should we detect certain limits changing. In another PR, though. As to the test design, it works similar to the shared temp tests: Interact between the two containers by virtue of a shared filesystem `/tmp` and creating marker files there in order to make them cooperate. Note that the new test needs `podman` version `4.3.0` and better (`4.5` is current). Testing: - [ ] GHA (still running) - [x] Linux x86_64 container tests on cg v1 and cg v2 system - [x] Newly added tests on Linux x86_64 cg v1 and cg v2 (`podman` and `docker`) - Commit messages: - Fix whitespace - 8308090: Add container tests for on-the-fly resource quota updates Changes: https://git.openjdk.org/jdk/pull/14090/files Webrev: https://webrevs.openjdk.org/?repo=jdk=14090=00 Issue: https://bugs.openjdk.org/browse/JDK-8308090 Stats: 434 lines in 5 files changed: 431 ins; 2 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/14090.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14090/head:pull/14090 PR: https://git.openjdk.org/jdk/pull/14090
Re: RFR: 8308093: Disable language preview features use in JDK
On Mon, 22 May 2023 10:01:55 GMT, Adam Sotona wrote: > This patch disables temporary use of language preview features in JDK. > Temporary enabled language preview features (to allow Pattern Matching for > switch use in the Classfile API library) are no more necessary. > All redundant use of --enable-preview in the Classfile API tests are also > removed. > > Please review. > > Thanks, > Adam Marked as reviewed by darcy (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/14076#pullrequestreview-1437004246
Re: RFR: 8303040: linux PPC64le: Implementation of Foreign Function & Memory API (Preview) [v33]
> Implementation of "Foreign Function & Memory API" for linux on Power (Little
> Endian) according to "Power Architecture 64-Bit ELF V2 ABI Specification".
>
> This PR does not include code for VaList support because it's supposed to get
> removed by [JDK-8299736](https://bugs.openjdk.org/browse/JDK-8299736). I've
> kept the related tests disabled for this platform and throw an exception
> instead. Note that the ABI doesn't precisely specify variable argument lists.
> Instead, it refers to `` (2.2.4 Variable Argument Lists).
>
> Big Endian support is implemented to some extend, but not complete. E.g.
> structs with size not divisible by 8 are not passed correctly (see `useABIv2`
> in CallArranger.java). Big Endian is excluded by selecting
> `ARCH.equals("ppc64le")` (CABI.java) only.
>
> There's another limitation: This PR only accepts structures with size
> divisible by 4. (An `IllegalArgumentException` gets thrown otherwise.) I
> think arbitrary sizes are not usable on other platforms, either, because
> `SharedUtils.primitiveCarrierForSize` only accepts powers of 2. Update:
> Resolved after merging of
> [JDK-8303017](https://bugs.openjdk.org/browse/JDK-8303017)
>
> The ABI has some tricky corner cases related to HFA (Homogeneous Float
> Aggregate). The same argument may need to get passed in both, a FP reg and a
> GP reg or stack slot (see "no partial DW rule"). This cases are not covered
> by the existing tests.
>
> I had to make changes to shared code and code for other platforms:
> 1. Pass type information when creating `VMStorage` objects from `VMReg`. This
> is needed for the following reasons:
> - PPC64 ABI requires integer types to get extended to 64 bit (also see
> CCallingConventionRequiresIntsAsLongs in existing hotspot code). We need to
> know the type or at least the bit width for that.
> - Floating point load / store instructions need the correct width to select
> between the correct IEEE 754 formats. The register representation in single
> FP registers is always IEEE 754 double precision on PPC64.
> - Big Endian also needs usage of the precise size. Storing 8 Bytes and
> loading 4 Bytes yields different values than on Little Endian!
> 2. It happens that a `NativeMemorySegmentImpl` is used as a raw pointer (with
> byteSize() == 0) while running TestUpcallScope. Hence, existing size checks
> don't work (see MemorySegment.java). As a workaround, I'm just skipping the
> check in this particular case. Please check if this makes sense or if there's
> a better fix (possibly as separate RFE). Update: T...
Martin Doerr has updated the pull request incrementally with one additional
commit since the last revision:
Add comment about Register Save Area.
-
Changes:
- all: https://git.openjdk.org/jdk/pull/12708/files
- new: https://git.openjdk.org/jdk/pull/12708/files/70736be6..ac5c5dcc
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk=12708=32
- incr: https://webrevs.openjdk.org/?repo=jdk=12708=31-32
Stats: 3 lines in 1 file changed: 2 ins; 0 del; 1 mod
Patch: https://git.openjdk.org/jdk/pull/12708.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/12708/head:pull/12708
PR: https://git.openjdk.org/jdk/pull/12708
Re: RFR: 8287834: Add SymbolLookup::or method
On Mon, 22 May 2023 11:51:33 GMT, Maurizio Cimadamore wrote: > So, not only the new method results in more succinct code (compared to the > alternatives), but it also combines symbol lookups in the "right way", so > that the chain of lookups is defined, once and for all, when the composite > lookup is first created. > Yes, it's composition of *lookups* not composition of their results. Paul. - PR Comment: https://git.openjdk.org/jdk/pull/13954#issuecomment-1557478950
RFR: 8307205: Downcall to clibrary printf fails on OS X AArch64
This PR adds a test that shows the documented way of calling `printf` works. - Commit messages: - Add test for issue Changes: https://git.openjdk.org/jdk/pull/14088/files Webrev: https://webrevs.openjdk.org/?repo=jdk=14088=00 Issue: https://bugs.openjdk.org/browse/JDK-8307205 Stats: 22 lines in 1 file changed: 22 ins; 0 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/14088.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14088/head:pull/14088 PR: https://git.openjdk.org/jdk/pull/14088
Integrated: 8308276: Change layout API to work with bytes, not bits
On Tue, 16 May 2023 13:53:32 GMT, Maurizio Cimadamore wrote: > As explained in [1], memory layouts and memory segments describe sizes using > different units. Memory layouts use bits, whereas memory segments use bytes. > This is historical: memory layouts were designed after the Minimal LDL [2], > which allowed layout strings to be used to describe both memory *and* > register. In practice that ship has sailed, and the FFM API uses layouts to > firmly model the contents of regions of memory. While it would be possible, > in principle, to tweak segments to work on bits, changing that would have > implications on how easily code that is currently using ByteBuffer can > migrate to use MemorySegment. > > For these reasons, this patch fixes the asymmetry so that layouts also model > sizes in term of bytes. > > The patch is straightforward, although a bit tedious (as you can imagine), as > various uses of bit sizes had to be turned in byte sizes. In practice, the > migration had not been too hard, for the following reasons: > > * the `withBitAlignment` and `bitSize` methods are no longer in the API, it > is easy to fix any code (mainly tests) using it; > * most code already uses ready-made constants such as `JAVA_INT` - such code > continues to work unchanged; > * the layout API de facto already required clients to work with bit sizes > that are a multiple of 8. > > The only problematic case is the presence of the > `MemoryLayout::paddingLayout(long size)` factory. As this factory is changed > to deal in bytes instead of bits, all constants passed to this factory will > need to be updated. This is not a problem for code using jextract (as > jextract will take care of generating padding) but will be an issue for code > using the layout API directly. > > [1] - https://mail.openjdk.org/pipermail/panama-dev/2023-May/019059.html This pull request has now been integrated. Changeset: 5fc9b578 Author:Maurizio Cimadamore URL: https://git.openjdk.org/jdk/commit/5fc9b5787dc4d7f00d2c59288bc8d840fdf5b495 Stats: 724 lines in 93 files changed: 3 ins; 197 del; 524 mod 8308276: Change layout API to work with bytes, not bits Reviewed-by: psandoz, pminborg - PR: https://git.openjdk.org/jdk/pull/14013
Integrated: 8287834: Add SymbolLookup::or method
On Fri, 12 May 2023 12:11:23 GMT, Maurizio Cimadamore wrote: > This patch adds a simpler method for composing symbol lookups. It is common > for clients to chain multiple symbol lookups together, e.g. to find a symbol > in multiple libraries. > > A new instance method, namely `SymbolLookup::or` is added, which first > searches a symbol in the first lookup, and, if that fails, proceeds to search > the symbol in the provided lookup. > > We have considered alternatives to express this, such as a static factory > `SymbolLookup::ofComposite` but settled on this because of the similarity > with `Optional::or`. This pull request has now been integrated. Changeset: 91aeb5de Author:Maurizio Cimadamore URL: https://git.openjdk.org/jdk/commit/91aeb5de580633dfc361957051cd00545aa883c7 Stats: 167 lines in 2 files changed: 167 ins; 0 del; 0 mod 8287834: Add SymbolLookup::or method Reviewed-by: psandoz - PR: https://git.openjdk.org/jdk/pull/13954
Re: RFR: JDK-8298066: java/util/concurrent/locks/Lock/OOMEInAQS.java timed out
On Mon, 22 May 2023 13:28:24 GMT, Alan Bateman wrote: >> Doubling the max heap size since not all GCs might have compressed OOPs, so >> this change should make this test more robust. >> >> Cleared with @DougLea > > Should the test be removed from ProblemList-generational-zgc.txt as part of > this? > @AlanBateman My thinking was to integrate this and make sure that the problem > doesn't resurface before removing it from the problem list. Does that make > sense? 樂 No objection, it's just it would leave ProblemList-generational-zgc.txt listing a test against as issue marked as fixed. - PR Comment: https://git.openjdk.org/jdk/pull/14082#issuecomment-1557352028
Re: RFR: 8308293: A linker should expose the layouts it supports [v4]
On Mon, 22 May 2023 09:34:53 GMT, Maurizio Cimadamore
wrote:
>> This patch adds an instance method on `Linker`, namely
>> `Linker::canonicalLayouts` which returns all the layouts known by the linker
>> as implementing some ABI type. For instance, if I call this on my machine
>> (Linux/x64) I get this:
>>
>>
>> jshell> import java.lang.foreign.*;
>>
>> jshell> Linker.nativeLinker().canonicalLayouts()
>> $2 ==> {char16_t=c16, int8_t=b8, long=j64, size_t=j64, bool=z8, int=i32,
>> long long=j64, int64_t=j64, void*=a64, float=f32, char=b8, int16_t=s16,
>> int32_t=i32, short=s16, double=d64}
>>
>>
>> This can be useful to discover the ABI types supported by a linker
>> implementation, as well as for, in the future, add support for more exotic
>> (and platform-dependent) linker types, such as `long double` or `complex
>> long`.
>
> Maurizio Cimadamore has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Address further review comments
LGTM. Are there any additional types we might consider apart from the basic
ones and `size_t`? Maybe one for an address pointing at `errno`?
-
Marked as reviewed by pminborg (Committer).
PR Review: https://git.openjdk.org/jdk/pull/14037#pullrequestreview-1436779774
Re: RFR: 8308276: Change layout API to work with bytes, not bits [v4]
On Mon, 22 May 2023 10:27:04 GMT, Maurizio Cimadamore wrote: >> As explained in [1], memory layouts and memory segments describe sizes using >> different units. Memory layouts use bits, whereas memory segments use bytes. >> This is historical: memory layouts were designed after the Minimal LDL [2], >> which allowed layout strings to be used to describe both memory *and* >> register. In practice that ship has sailed, and the FFM API uses layouts to >> firmly model the contents of regions of memory. While it would be possible, >> in principle, to tweak segments to work on bits, changing that would have >> implications on how easily code that is currently using ByteBuffer can >> migrate to use MemorySegment. >> >> For these reasons, this patch fixes the asymmetry so that layouts also model >> sizes in term of bytes. >> >> The patch is straightforward, although a bit tedious (as you can imagine), >> as various uses of bit sizes had to be turned in byte sizes. In practice, >> the migration had not been too hard, for the following reasons: >> >> * the `withBitAlignment` and `bitSize` methods are no longer in the API, it >> is easy to fix any code (mainly tests) using it; >> * most code already uses ready-made constants such as `JAVA_INT` - such code >> continues to work unchanged; >> * the layout API de facto already required clients to work with bit sizes >> that are a multiple of 8. >> >> The only problematic case is the presence of the >> `MemoryLayout::paddingLayout(long size)` factory. As this factory is changed >> to deal in bytes instead of bits, all constants passed to this factory will >> need to be updated. This is not a problem for code using jextract (as >> jextract will take care of generating padding) but will be an issue for code >> using the layout API directly. >> >> [1] - https://mail.openjdk.org/pipermail/panama-dev/2023-May/019059.html > > Maurizio Cimadamore has updated the pull request with a new target base due > to a merge or a rebase. The pull request now contains 11 commits: > > - Merge branch 'master' into bits_to_bytes_layouts > - Address review comments > - Update test/jdk/java/foreign/TestLayouts.java > >Co-authored-by: Paul Sandoz > - Update test/jdk/java/foreign/TestLayoutPaths.java > >Co-authored-by: Paul Sandoz > - Update test/jdk/java/foreign/TestLayoutPaths.java > >Co-authored-by: Paul Sandoz > - Update test/jdk/java/foreign/TestLayoutPaths.java > >Co-authored-by: Paul Sandoz > - Update > src/java.base/share/classes/jdk/internal/foreign/layout/ValueLayouts.java > >Co-authored-by: Paul Sandoz > - Drop spurious reference to "bit"/"bits" in both javadoc and impl > - Fix tests > - Fix bad assertion in AbstractValueLayouts >Fix vector tests to use withByteAlignment > - ... and 1 more: https://git.openjdk.org/jdk/compare/8aa50288...145fb32d LGTM. - Marked as reviewed by pminborg (Committer). PR Review: https://git.openjdk.org/jdk/pull/14013#pullrequestreview-1436768257
Re: RFR: 8308286 Fix clang warnings in linux code
On Wed, 17 May 2023 12:28:47 GMT, Artem Semenov wrote: > When using the clang compiler to build OpenJDk on Linux, we encounter various > "warnings as errors". > They can be fixed with small changes. I would suggest either disable warnings on per file basis or rewrite problematic code. Disabling warnings per fragment of code makes it less readable. - PR Comment: https://git.openjdk.org/jdk/pull/14033#issuecomment-1557306893
Re: RFR: 8308046: Move Solaris related charsets from java.base to jdk.charsets module [v2]
On Sat, 20 May 2023 17:26:53 GMT, Naoto Sato wrote: >> Hello @naotoj . >> I'd like to confirm about DoubleByte-X.java.template and >> EUC_JP.java.template. >> I think the values are package-private. >> Even if class is changed to `public`, the classes in`sun.nio.cs.ext` package >> could not access to these values in `sun.nio.cs` package... >> I may be misunderstanding your suggestion, could you tell me more ? > >> I think the values are package-private. Even if class is changed to >> `public`, the classes in`sun.nio.cs.ext` package could not access to these >> values in `sun.nio.cs` package... > > I meant making those package-private fields public. I believe it's OK because > java.base/sun.nio.cs package is only exported to jdk.charsets module. Thanks @naotoj . I changed related fields to `public`. - PR Comment: https://git.openjdk.org/jdk/pull/13973#issuecomment-1557308396
Re: RFR: 8303040: linux PPC64le: Implementation of Foreign Function & Memory API (Preview) [v31]
On Mon, 22 May 2023 14:09:22 GMT, Martin Doerr wrote: >> Yes, good idea. > > Please take a look at > https://github.com/openjdk/jdk/pull/12708/commits/70736be631e4f1bf3fd3c0d45ddfc076b74ef9dd It looks good. - PR Review Comment: https://git.openjdk.org/jdk/pull/12708#discussion_r1200576857
Re: RFR: 8303040: linux PPC64le: Implementation of Foreign Function & Memory API (Preview) [v31]
On Mon, 22 May 2023 13:59:05 GMT, Richard Reingruber wrote: >> Or better `final boolean useABIv2 = (this instanceof ABIv2CallArranger);` > > Yes, good idea. Please take a look at https://github.com/openjdk/jdk/pull/12708/commits/70736be631e4f1bf3fd3c0d45ddfc076b74ef9dd - PR Review Comment: https://git.openjdk.org/jdk/pull/12708#discussion_r1200571704
Re: RFR: 8303040: linux PPC64le: Implementation of Foreign Function & Memory API (Preview) [v32]
> Implementation of "Foreign Function & Memory API" for linux on Power (Little
> Endian) according to "Power Architecture 64-Bit ELF V2 ABI Specification".
>
> This PR does not include code for VaList support because it's supposed to get
> removed by [JDK-8299736](https://bugs.openjdk.org/browse/JDK-8299736). I've
> kept the related tests disabled for this platform and throw an exception
> instead. Note that the ABI doesn't precisely specify variable argument lists.
> Instead, it refers to `` (2.2.4 Variable Argument Lists).
>
> Big Endian support is implemented to some extend, but not complete. E.g.
> structs with size not divisible by 8 are not passed correctly (see `useABIv2`
> in CallArranger.java). Big Endian is excluded by selecting
> `ARCH.equals("ppc64le")` (CABI.java) only.
>
> There's another limitation: This PR only accepts structures with size
> divisible by 4. (An `IllegalArgumentException` gets thrown otherwise.) I
> think arbitrary sizes are not usable on other platforms, either, because
> `SharedUtils.primitiveCarrierForSize` only accepts powers of 2. Update:
> Resolved after merging of
> [JDK-8303017](https://bugs.openjdk.org/browse/JDK-8303017)
>
> The ABI has some tricky corner cases related to HFA (Homogeneous Float
> Aggregate). The same argument may need to get passed in both, a FP reg and a
> GP reg or stack slot (see "no partial DW rule"). This cases are not covered
> by the existing tests.
>
> I had to make changes to shared code and code for other platforms:
> 1. Pass type information when creating `VMStorage` objects from `VMReg`. This
> is needed for the following reasons:
> - PPC64 ABI requires integer types to get extended to 64 bit (also see
> CCallingConventionRequiresIntsAsLongs in existing hotspot code). We need to
> know the type or at least the bit width for that.
> - Floating point load / store instructions need the correct width to select
> between the correct IEEE 754 formats. The register representation in single
> FP registers is always IEEE 754 double precision on PPC64.
> - Big Endian also needs usage of the precise size. Storing 8 Bytes and
> loading 4 Bytes yields different values than on Little Endian!
> 2. It happens that a `NativeMemorySegmentImpl` is used as a raw pointer (with
> byteSize() == 0) while running TestUpcallScope. Hence, existing size checks
> don't work (see MemorySegment.java). As a workaround, I'm just skipping the
> check in this particular case. Please check if this makes sense or if there's
> a better fix (possibly as separate RFE). Update: T...
Martin Doerr has updated the pull request incrementally with one additional
commit since the last revision:
Replace abstract method useABIv2().
-
Changes:
- all: https://git.openjdk.org/jdk/pull/12708/files
- new: https://git.openjdk.org/jdk/pull/12708/files/b1f04382..70736be6
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk=12708=31
- incr: https://webrevs.openjdk.org/?repo=jdk=12708=30-31
Stats: 13 lines in 2 files changed: 0 ins; 5 del; 8 mod
Patch: https://git.openjdk.org/jdk/pull/12708.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/12708/head:pull/12708
PR: https://git.openjdk.org/jdk/pull/12708
Re: RFR: 8303040: linux PPC64le: Implementation of Foreign Function & Memory API (Preview) [v31]
On Mon, 22 May 2023 13:42:27 GMT, Martin Doerr wrote: >> Probably, yes. I didn't find time for figuring out what would be useful >> tests. We could still add some in the future or with the big endian port. >> Another idea: Would the following be better? >> `final boolean useABIv2 = (this.getClass() == ABIv2CallArranger.class);` >> That would also allow getting rid of the method `useABIv2()`. > > Or better `final boolean useABIv2 = (this instanceof ABIv2CallArranger);` Yes, good idea. - PR Review Comment: https://git.openjdk.org/jdk/pull/12708#discussion_r1200558827
Re: RFR: 8303040: linux PPC64le: Implementation of Foreign Function & Memory API (Preview) [v31]
On Mon, 22 May 2023 13:34:49 GMT, Martin Doerr wrote: >>> That would be better to read, but would make the PPC64 CallArranger >>> dependent on the current CABI. Note that there are tests which use >>> >>> ``` >>> import jdk.internal.foreign.abi.aarch64.CallArranger; >>> ... >>> CallArranger.LINUX.getBindings(mt, fd, false); >>> ``` >>> >>> for example. The tests are designed to run on all platforms. >> >> I see, thanks. Would be nice to have some for PPC64 too :) > > Probably, yes. I didn't find time for figuring out what would be useful > tests. We could still add some in the future or with the big endian port. > Another idea: Would the following be better? > `final boolean useABIv2 = (this.getClass() == ABIv2CallArranger.class);` > That would also allow getting rid of the method `useABIv2()`. Or better `final boolean useABIv2 = (this instanceof ABIv2CallArranger);` - PR Review Comment: https://git.openjdk.org/jdk/pull/12708#discussion_r1200537482
Re: RFR: 8303040: linux PPC64le: Implementation of Foreign Function & Memory API (Preview) [v31]
On Mon, 22 May 2023 13:29:14 GMT, Richard Reingruber wrote: >> That would be better to read, but would make the PPC64 CallArranger >> dependent on the current CABI. >> Note that there are tests which use >> >> import jdk.internal.foreign.abi.aarch64.CallArranger; >> ... >> CallArranger.LINUX.getBindings(mt, fd, false); >> >> for example. The tests are designed to run on all platforms. > >> That would be better to read, but would make the PPC64 CallArranger >> dependent on the current CABI. Note that there are tests which use >> >> ``` >> import jdk.internal.foreign.abi.aarch64.CallArranger; >> ... >> CallArranger.LINUX.getBindings(mt, fd, false); >> ``` >> >> for example. The tests are designed to run on all platforms. > > I see, thanks. Would be nice to have some for PPC64 too :) Probably, yes. I didn't find time for figuring out what would be useful tests. We could still add some in the future or with the big endian port. Another idea: Would the following be better? `final boolean useABIv2 = (this.getClass() == ABIv2CallArranger.class);` That would also allow getting rid of the method `useABIv2()`. - PR Review Comment: https://git.openjdk.org/jdk/pull/12708#discussion_r1200527513
Re: RFR: JDK-8298066: java/util/concurrent/locks/Lock/OOMEInAQS.java timed out
On Mon, 22 May 2023 13:28:24 GMT, Alan Bateman wrote: >> Doubling the max heap size since not all GCs might have compressed OOPs, so >> this change should make this test more robust. >> >> Cleared with @DougLea > > Should the test be removed from ProblemList-generational-zgc.txt as part of > this? @AlanBateman My thinking was to integrate this and make sure that the problem doesn't resurface before removing it from the problem list. Does that make sense? 樂 - PR Comment: https://git.openjdk.org/jdk/pull/14082#issuecomment-1557232300
Re: RFR: JDK-8298066: java/util/concurrent/locks/Lock/OOMEInAQS.java timed out
On Mon, 22 May 2023 13:12:02 GMT, Viktor Klang wrote: > Doubling the max heap size since not all GCs might have compressed OOPs, so > this change should make this test more robust. > > Cleared with @DougLea Should the test be removed from ProblemList-generational-zgc.txt as part of this? - PR Comment: https://git.openjdk.org/jdk/pull/14082#issuecomment-1557224363
Re: RFR: 8303040: linux PPC64le: Implementation of Foreign Function & Memory API (Preview) [v31]
On Mon, 22 May 2023 12:38:23 GMT, Martin Doerr wrote: > That would be better to read, but would make the PPC64 CallArranger dependent > on the current CABI. Note that there are tests which use > > ``` > import jdk.internal.foreign.abi.aarch64.CallArranger; > ... > CallArranger.LINUX.getBindings(mt, fd, false); > ``` > > for example. The tests are designed to run on all platforms. I see, thanks. Would be nice to have some for PPC64 too :) - PR Review Comment: https://git.openjdk.org/jdk/pull/12708#discussion_r1200520623
RFR: JDK-8298066: java/util/concurrent/locks/Lock/OOMEInAQS.java timed out
Doubling the max heap size since not all GCs might have compressed OOPs, so this change should make this test more robust. Cleared with @DougLea - Commit messages: - JDK-8298066: Increasing max heap size for OOMEinAQS since not all GCs might have compressed oops Changes: https://git.openjdk.org/jdk/pull/14082/files Webrev: https://webrevs.openjdk.org/?repo=jdk=14082=00 Issue: https://bugs.openjdk.org/browse/JDK-8298066 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/14082.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14082/head:pull/14082 PR: https://git.openjdk.org/jdk/pull/14082
Re: RFR: 8308093: Disable language preview features use in JDK
On Mon, 22 May 2023 10:01:55 GMT, Adam Sotona wrote: > This patch disables temporary use of language preview features in JDK. > Temporary enabled language preview features (to allow Pattern Matching for > switch use in the Classfile API library) are no more necessary. > All redundant use of --enable-preview in the Classfile API tests are also > removed. > > Please review. > > Thanks, > Adam Marked as reviewed by alanb (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/14076#pullrequestreview-1436580302
Re: RFR: 8308093: Disable language preview features use in JDK
On Mon, 22 May 2023 10:01:55 GMT, Adam Sotona wrote: > This patch disables temporary use of language preview features in JDK. > Temporary enabled language preview features (to allow Pattern Matching for > switch use in the Classfile API library) are no more necessary. > All redundant use of --enable-preview in the Classfile API tests are also > removed. > > Please review. > > Thanks, > Adam Marked as reviewed by erikj (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/14076#pullrequestreview-1436562362
Re: RFR: 8308281: Java snippets in the FFM API need to be updated [v3]
> As the API has improved over the recent releases, not all `{@snippet ..}`
> sections have been kept in sync.
>
> This PR suggests all snippets used should be verified against real code that
> is placed in a new `snippet-files` folder and erroneous snippets are updated.
>
> In this PR, it is suggested duplicating code in the `Snippets.java` class and
> in the JavaDocs. The benefit of this is that code is directly visible in the
> code and not only in the generated javadoc.
>
> Another thing to think about is if there should be on single `Snippets.java`
> class or separate ones for each FFM class.
Per Minborg has updated the pull request incrementally with one additional
commit since the last revision:
Use hybrid snippets for Arena
-
Changes:
- all: https://git.openjdk.org/jdk/pull/14030/files
- new: https://git.openjdk.org/jdk/pull/14030/files/2cbff9c3..df59d9fa
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk=14030=02
- incr: https://webrevs.openjdk.org/?repo=jdk=14030=01-02
Stats: 75 lines in 2 files changed: 20 ins; 13 del; 42 mod
Patch: https://git.openjdk.org/jdk/pull/14030.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/14030/head:pull/14030
PR: https://git.openjdk.org/jdk/pull/14030
Re: RFR: 8303040: linux PPC64le: Implementation of Foreign Function & Memory API (Preview) [v31]
On Mon, 22 May 2023 12:14:48 GMT, Richard Reingruber wrote:
>> src/java.base/share/classes/jdk/internal/foreign/abi/ppc64/CallArranger.java
>> line 65:
>>
>>> 63: */
>>> 64: public abstract class CallArranger {
>>> 65: protected abstract boolean useABIv2();
>>
>> This could also be refactored into a static method with the same trick that
>> is used in `LinuxPPC64leLinker::getInstance`. Callers could be static then
>> and you could delete `CallArranger::ABIv2` and `ABIv2CallArranger`.
>
> Maybe something like?
>
> protected static final boolean useABIv2 = CABI.current() ==
> CABI.LINUX_PPC_64_LE;
That would be better to read, but would make the PPC64 CallArranger dependent
on the current CABI.
Note that there are tests which use
import jdk.internal.foreign.abi.aarch64.CallArranger;
...
CallArranger.LINUX.getBindings(mt, fd, false);
for example. The tests are designed to run on all platforms.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/12708#discussion_r1200459227
Re: RFR: 8303040: linux PPC64le: Implementation of Foreign Function & Memory API (Preview) [v31]
On Mon, 22 May 2023 08:53:21 GMT, Richard Reingruber wrote:
>> Martin Doerr has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Cleanup imports, improve comments, updates from other platforms.
>
> src/java.base/share/classes/jdk/internal/foreign/abi/ppc64/CallArranger.java
> line 65:
>
>> 63: */
>> 64: public abstract class CallArranger {
>> 65: protected abstract boolean useABIv2();
>
> This could also be refactored into a static method with the same trick that
> is used in `LinuxPPC64leLinker::getInstance`. Callers could be static then
> and you could delete `CallArranger::ABIv2` and `ABIv2CallArranger`.
Maybe something like?
protected static final boolean useABIv2 = CABI.current() ==
CABI.LINUX_PPC_64_LE;
-
PR Review Comment: https://git.openjdk.org/jdk/pull/12708#discussion_r1200432599
Re: RFR: 8308093: Disable language preview features use in JDK
On Mon, 22 May 2023 10:01:55 GMT, Adam Sotona wrote: > This patch disables temporary use of language preview features in JDK. > Temporary enabled language preview features (to allow Pattern Matching for > switch use in the Classfile API library) are no more necessary. > All redundant use of --enable-preview in the Classfile API tests are also > removed. > > Please review. > > Thanks, > Adam The enable-preview for jmh benchmark compilation exists before Classfile API integration, so this should be it. - Marked as reviewed by liach (Author). PR Review: https://git.openjdk.org/jdk/pull/14076#pullrequestreview-1436460682
Re: RFR: 8307795: AArch64: Optimize VectorMask.truecount() on Neon [v3]
On Thu, 18 May 2023 09:50:13 GMT, Chang Peng wrote: >> In Vector API Java level, vector mask is represented as a boolean array with >> 0x00/0x01 (8 bits of each element) as values, aka in-memory format. When it >> is loaded into vector register, e.g. Neon, the in-memory format will be >> converted to in-register format with 0/-1 value for each lane (lane width >> aligned to its type) by VectorLoadMask [1] operation, and convert back to >> in-memory format by VectorStoreMask[2]. In Neon, a typical VectorStoreMask >> operation will first narrow given vector registers by xtn insn [3] into byte >> element type, and then do a vector negate to convert to 0x00/0x01 value for >> each element. >> >> For most of the vector mask operations, the input mask is in-register >> format. And a vector mask also works in-register format all through the >> compilation. But for some operations like VectorMask.trueCount()[4] which >> counts the elements of true value, the expected input mask is in-memory >> format. So a VectorStoreMask is generated to convert the mask from >> in-register format to in-memory format before those operations. >> >> However, for trueCount() these xtn instructions in VectorStoreMask can be >> saved, since the narrowing operations will not influence the number of >> active lane (value of 0x01) of its input. >> >> This patch adds an optimized rule `VectorMaskTrueCount (VectorStoreMask >> mask)` to save the unnecessary narrowing operations. >> >> For example, >> >> >> var m = VectorMask.fromArray(IntVector.SPECIES_PREFERRED, ba, 0); >> m.not().trueCount(); >> >> >> will produce following assembly on a Neon machine before this patch: >> >> >> ... >> mvn v16.16b, v16.16b // VectorMask.not() >> xtn v16.4h, v16.4s >> xtn v16.8b, v16.8h >> neg v16.8b, v16.8b // VectorStoreMask >> addvb17, v16.8b >> umovw0, v17.b[0] // VectorMask.trueCount() >> ... >> >> >> After this patch: >> >> >> ... >> mvn v16.16b, v16.16b // VectorMask.not() >> addvs17, v16.4s >> smovx0, v17.b[0] >> neg x0, x0 // Optimized VectorMask.trueCount() >> ... >> >> >> In this case, we can save two xtn insns. >> >> Performance: >> >> Benchmark Before After Unit >> testInt 723.822 ± 1.029 1182.375 ± 12.363ops/ms >> testLong 632.154 ± 0.197 1382.74 ± 2.188ops/ms >> testShort 788.665 ± 1.852 1152.38 ± 3.77 ops/ms >> >> [1]: >> https://github.com/openjdk/jdk/blob/e1e758a7b43c29840296d337bd2f0213ab0ca3c9/src/hotspot/cpu/aarch64/aarch64_vect... > > Chang Peng has updated the pull request incrementally with one additional > commit since the last revision: > > Update benchmark to avoid potential optimization LGTM. - Marked as reviewed by eliu (Committer). PR Review: https://git.openjdk.org/jdk/pull/13974#pullrequestreview-1436444533
Re: RFR: 8308038: java/util/concurrent/ThreadPerTaskExecutor/ThreadPerTaskExecutorTest.java timed out
On Mon, 22 May 2023 10:30:27 GMT, Jaikiran Pai wrote: >> I don't mind moving this but it would require changing them everywhere (as >> you point out). > > I think it's fine to leave it in the current form then. If/when any of these > tests fail due to unexpected exception type in these asserts, we can then > move them all to the other proposed (or some other) construct. Yes, as we really want an instanceof check here, the exception could be a subclass. - PR Review Comment: https://git.openjdk.org/jdk/pull/14072#discussion_r1200372861
Re: RFR: 8308038: java/util/concurrent/ThreadPerTaskExecutor/ThreadPerTaskExecutorTest.java timed out
On Mon, 22 May 2023 10:09:27 GMT, Alan Bateman wrote:
>> test/jdk/java/util/concurrent/ThreadPerTaskExecutor/ThreadPerTaskExecutorTest.java
>> line 578:
>>
>>> 576: try {
>>> 577:
>>> scheduleInterruptAt("java.util.concurrent.ThreadPerTaskExecutor.invokeAny");
>>> 578: executor.invokeAny(Set.of(task));
>>
>> Seems strange to test invokeAny when there is only one task?
>
>> Seems strange to test invokeAny when there is only one task?
>
> There are several tests further up that exercise invokeAny with more than one
> task. This one is testing interrupt of invokeAny, we could expect it to test
> interrupting with many tasks if needed.
OK - looks fine to me then.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/14072#discussion_r1200349759
Re: RFR: 8308038: java/util/concurrent/ThreadPerTaskExecutor/ThreadPerTaskExecutorTest.java timed out
On Sun, 21 May 2023 13:52:06 GMT, Alan Bateman wrote: > This is a test only change to the unit test for the ExecutorService returned > by Executors.newThreadPerTaskExecutor. The tests for interrupting invokeAll > assume the threads started to execute the tasks do actually execute the task > code. The refresh in JEP 444 changed the implementation to use FutureTask, > and FutureTask checks the interrupt status before it executes the task code. > So some intermittent timeouts of the tests for interrupting invokeAll as > those tests were waiting for the task to complete. > > The main change is that the tests for interrupting invokeAll are changed to > interrupt when the main thread blocks in invokeAll. They are also changed to > check if the task started or not. The tests for interrupting invokeAny > already did this, but these are changed to use the same infrastructure to > avoid having two styles of tests in the same source file. Marked as reviewed by dfuchs (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/14072#pullrequestreview-1436367855
Integrated: 7065228: To interpret case-insensitive string locale independently
On Tue, 16 May 2023 10:38:52 GMT, Darragh Clarke wrote: > Updated instances of `toLowerCase` and `toUpperCase` in several net and io > files to specify `Locale.ROOT` to ensure that case conversion issues don't > occur, > > I didn't add any new tests but ran tier 1-3 with no issues This pull request has now been integrated. Changeset: 05e99db4 Author:Darragh Clarke Committer: Jaikiran Pai URL: https://git.openjdk.org/jdk/commit/05e99db466e7ef5c26f089db772a21cb2ca62e93 Stats: 74 lines in 20 files changed: 14 ins; 0 del; 60 mod 7065228: To interpret case-insensitive string locale independently Reviewed-by: dfuchs, naoto, djelinski, jpai, michaelm - PR: https://git.openjdk.org/jdk/pull/14006
Re: RFR: 8307990: jspawnhelper must close its writing side of a pipe before reading from it
On Wed, 17 May 2023 20:03:37 GMT, Joe Darcy wrote: > > > Should this issue have a CSR for any behavioral changes? > > > > > > Well, you can certainly argue that every bug fix is a behavioral changes, > > right :) > > But seriously, I don't see how this PR could require a CSR. The only > > behavioral change is really that `jspawnhelper` can now no longer block and > > deadlock. But that's exactly how it should have behaved in the first place. > > Yes, some judgement is needed on behavioral changes; this was broached in > portions of my recent talk to the JCP EC: > > "Contributing to OpenJDK: Participating in stewardship for the long-term" > https://jcp.org/aboutJava/communityprocess/ec-public/materials/2023-04-12/Contributing_to_OpenJDK_2023_04_12.pdf > > The process spawning code is apparently tricky and subtle, so much so that > this will be apparently the third attempt at it since JDK 12. (I still have > memories of various platform-specific issues in process handling from my time > maintaining the launcher.) > > So I don't think the applicability of a CSR for this change should be > dismissed I've created a [Release Note](https://bugs.openjdk.org/browse/JDK-8308297) (please feel to review it :) which outlines the "*behavioral change*". But this change is really just a simple bug fix for the case where the JVM leaves a hanging process (`jspawnhelper`) behind when it crashes. I don't really see see how a CSR will fit in here unless we start to treat crashes as "defined behavior" and start tracking them across releases. If you still think a CSR will be needed, please advice how it should look like? > and personally I would prefer a change like this be made early in JDK 22 > rather than a few weeks before JDK 21 rampdown; it is easier to address any > bug tail early in 22 rather than late in 21. First, this is a bug fix for an issue which impacts production usage of the JDK. Second, tt's a "first-day" bug introduced by [JDK-5049299](https://bugs.openjdk.org/browse/JDK-5049299) for JDK 8 (initially only active on Solaris). It started to manifest on Linux in JDK 13 when the `POSIX_SPAWN` launching mechanism was ported to Linux as well, **but** it was only detected some time after JDK 17 was released when a higher rate of production services started to migrate to JDK 17. I really don't want to go into a bike-shedding discussion about "*LTS releases are not special*", but I think this is a serious enough bug with a simple enough fix to resolve it as quick as possible (and downport it to JDK 17 as fast as possible). We are still more than two weeks before RDP 1 and we still have more than three month before GA so I don't see a reason not to fix this now. - PR Comment: https://git.openjdk.org/jdk/pull/13956#issuecomment-1556989917
Re: RFR: 8308038: java/util/concurrent/ThreadPerTaskExecutor/ThreadPerTaskExecutorTest.java timed out
On Mon, 22 May 2023 10:11:25 GMT, Alan Bateman wrote: >> test/jdk/java/util/concurrent/ThreadPerTaskExecutor/ThreadPerTaskExecutorTest.java >> line 269: >> >>> 267: assertTrue(executor.awaitTermination(10, >>> TimeUnit.MILLISECONDS)); >>> 268: Throwable e = assertThrows(ExecutionException.class, >>> future::get); >>> 269: assertTrue(e.getCause() instanceof InterruptedException); >> >> Would using `assertEquals(InterruptedException.class, >> e.getCause().getClass())` be better? That way if/when the test fails, it >> even prints the unexpected exception type? >> >> But I then see that this test already uses `assertTrue` for similar cases in >> some other place, so maybe it's fine in the current form? > > I don't mind moving this but it would require changing them everywhere (as > you point out). I think it's fine to leave it in the current form then. If/when any of these tests fail due to unexpected exception type in these asserts, we can then move them all to the other proposed (or some other) construct. - PR Review Comment: https://git.openjdk.org/jdk/pull/14072#discussion_r1200321915
Re: RFR: 8308038: java/util/concurrent/ThreadPerTaskExecutor/ThreadPerTaskExecutorTest.java timed out
On Sun, 21 May 2023 13:52:06 GMT, Alan Bateman wrote: > This is a test only change to the unit test for the ExecutorService returned > by Executors.newThreadPerTaskExecutor. The tests for interrupting invokeAll > assume the threads started to execute the tasks do actually execute the task > code. The refresh in JEP 444 changed the implementation to use FutureTask, > and FutureTask checks the interrupt status before it executes the task code. > So some intermittent timeouts of the tests for interrupting invokeAll as > those tests were waiting for the task to complete. > > The main change is that the tests for interrupting invokeAll are changed to > interrupt when the main thread blocks in invokeAll. They are also changed to > check if the task started or not. The tests for interrupting invokeAny > already did this, but these are changed to use the same infrastructure to > avoid having two styles of tests in the same source file. The change now waits for the current thread to reach a particular code location before interrupting the thread and then also verifies that the submitted task was indeed started, before checking that it is done. This I believe should solve the intermittent failures observed in this test. - Marked as reviewed by jpai (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/14072#pullrequestreview-1436322155
Re: RFR: 8305785: Avoid redundant HashMap.containsKey call in java.util.regex
On Mon, 3 Apr 2023 16:58:15 GMT, Andrey Turbanov wrote: > `Pattern.namedGroups` and `Matcher.namedGroups` contains only non-null > values. It means instead of separate `containsKey`+`get` calls, we can use > single `HashMap.get` call and then compare result with null. > Result code is a bit simpler and faster. Thank you for review! - PR Comment: https://git.openjdk.org/jdk/pull/13303#issuecomment-1556966620
Integrated: 8305785: Avoid redundant HashMap.containsKey call in java.util.regex
On Mon, 3 Apr 2023 16:58:15 GMT, Andrey Turbanov wrote: > `Pattern.namedGroups` and `Matcher.namedGroups` contains only non-null > values. It means instead of separate `containsKey`+`get` calls, we can use > single `HashMap.get` call and then compare result with null. > Result code is a bit simpler and faster. This pull request has now been integrated. Changeset: 6b65e575 Author:Andrey Turbanov URL: https://git.openjdk.org/jdk/commit/6b65e5754cc96c812892077881fc069e02fedc62 Stats: 10 lines in 2 files changed: 3 ins; 0 del; 7 mod 8305785: Avoid redundant HashMap.containsKey call in java.util.regex Reviewed-by: stsypanov, jpai - PR: https://git.openjdk.org/jdk/pull/13303
Re: RFR: 8308276: Change layout API to work with bytes, not bits [v4]
> As explained in [1], memory layouts and memory segments describe sizes using > different units. Memory layouts use bits, whereas memory segments use bytes. > This is historical: memory layouts were designed after the Minimal LDL [2], > which allowed layout strings to be used to describe both memory *and* > register. In practice that ship has sailed, and the FFM API uses layouts to > firmly model the contents of regions of memory. While it would be possible, > in principle, to tweak segments to work on bits, changing that would have > implications on how easily code that is currently using ByteBuffer can > migrate to use MemorySegment. > > For these reasons, this patch fixes the asymmetry so that layouts also model > sizes in term of bytes. > > The patch is straightforward, although a bit tedious (as you can imagine), as > various uses of bit sizes had to be turned in byte sizes. In practice, the > migration had not been too hard, for the following reasons: > > * the `withBitAlignment` and `bitSize` methods are no longer in the API, it > is easy to fix any code (mainly tests) using it; > * most code already uses ready-made constants such as `JAVA_INT` - such code > continues to work unchanged; > * the layout API de facto already required clients to work with bit sizes > that are a multiple of 8. > > The only problematic case is the presence of the > `MemoryLayout::paddingLayout(long size)` factory. As this factory is changed > to deal in bytes instead of bits, all constants passed to this factory will > need to be updated. This is not a problem for code using jextract (as > jextract will take care of generating padding) but will be an issue for code > using the layout API directly. > > [1] - https://mail.openjdk.org/pipermail/panama-dev/2023-May/019059.html Maurizio Cimadamore has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 11 commits: - Merge branch 'master' into bits_to_bytes_layouts - Address review comments - Update test/jdk/java/foreign/TestLayouts.java Co-authored-by: Paul Sandoz - Update test/jdk/java/foreign/TestLayoutPaths.java Co-authored-by: Paul Sandoz - Update test/jdk/java/foreign/TestLayoutPaths.java Co-authored-by: Paul Sandoz - Update test/jdk/java/foreign/TestLayoutPaths.java Co-authored-by: Paul Sandoz - Update src/java.base/share/classes/jdk/internal/foreign/layout/ValueLayouts.java Co-authored-by: Paul Sandoz - Drop spurious reference to "bit"/"bits" in both javadoc and impl - Fix tests - Fix bad assertion in AbstractValueLayouts Fix vector tests to use withByteAlignment - ... and 1 more: https://git.openjdk.org/jdk/compare/8aa50288...145fb32d - Changes: https://git.openjdk.org/jdk/pull/14013/files Webrev: https://webrevs.openjdk.org/?repo=jdk=14013=03 Stats: 724 lines in 93 files changed: 3 ins; 197 del; 524 mod Patch: https://git.openjdk.org/jdk/pull/14013.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14013/head:pull/14013 PR: https://git.openjdk.org/jdk/pull/14013
Re: RFR: 7065228: To interpret case-insensitive string locale independently [v2]
On Fri, 19 May 2023 11:24:30 GMT, Michael McMahon wrote: > Seems like a useful change and I can see how issues could arise if strings > were stored somewhere after being upper/lower cased and then reused in a > different locale. > > Is it correct to say that the assumption is these strings are all supposed to > be US ASCII (eg protocol defined identifiers, or hostnames etc) rather than > user generated text strings? That seems to be the case as far as I can see. Yes - PR Comment: https://git.openjdk.org/jdk/pull/14006#issuecomment-1556955595
Re: RFR: 8307990: jspawnhelper must close its writing side of a pipe before reading from it [v4]
> Since JDK13, executing commands in a sub-process defaults to the so called
> `POSIX_SPAWN` launching mechanism (i.e.
> `-Djdk.lang.Process.launchMechanism=POSIX_SPAWN`) on Linux. This works by
> using `posix_spawn(3)` to firstly start a tiny helper program called
> `jspawnhelper` in a subprocess. In a second step, `jspawnhelper` reads the
> command data from the parent Java process over a Unix pipe and finally
> executes (i.e. `execvp(3)`) the requested command.
>
> In cases where the parent process terminates abnormally before `jspawnhelper`
> has read all the expected data from the pipe, `jspawnhelper` will block
> indefinitely on the reading end of the pipe. This is especially harmful if
> the parent process had open sockets, because in that case, the forked
> `jspawnhelper` process will inherit them and keep all the corresponding ports
> open effectively preventing other processes to bind to them. Notice that this
> is not an abstract scenario. We've observed this regularly in production with
> services which couldn't be restarted after a crash after migrating to JDK 17.
>
> The fix of the issue is rather trivial. `jspawnhelper` has to close its
> writing end of the pipe which connects it with the parent Java process
> *before* starting to read from that pipe such that reads from the pipe can
> immediately return with EOF if the parent process terminates abnormally.
>
> Also did some cleanup:
> - in `jspawnhelper.c` correctly chek the result of `readFully()`
> - in `ProcessImpl_md.c` use `restartableWrite()` instead of `write()` to
> write the command data from the parent process to `jspawnhelper`
> - in `childproc.{c,h}` remove remaining traces of `MODE_CLONE` because
> that's long gone.
Volker Simonis has updated the pull request with a new target base due to a
merge or a rebase. The pull request now contains five commits:
- Merge branch 'master' into JDK-8307990
- Address comments from tstuefe and RogerRiggs
- REALLY adding the test :)
- Added test case
- 8307990: jspawnhelper must close its writing side of a pipe before reading
from it
-
Changes: https://git.openjdk.org/jdk/pull/13956/files
Webrev: https://webrevs.openjdk.org/?repo=jdk=13956=03
Stats: 260 lines in 5 files changed: 239 ins; 1 del; 20 mod
Patch: https://git.openjdk.org/jdk/pull/13956.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/13956/head:pull/13956
PR: https://git.openjdk.org/jdk/pull/13956
Re: RFR: 8307990: jspawnhelper must close its writing side of a pipe before reading from it [v3]
On Thu, 18 May 2023 05:58:26 GMT, Thomas Stuefe wrote:
>> Volker Simonis has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> REALLY adding the test :)
>
> src/java.base/unix/native/libjava/childproc.c line 413:
>
>> 411: const char* env = getenv("JTREG_JSPAWNHELPER_PROTOCOL_TEST");
>> 412: if (env != NULL && atoi(env) == stage) {
>> 413: printf("posix_spawn:%d\n", child);
>
> I would do an `_exit()` here, not `exit()`, to prevent exit handlers from
> running. You want to simulate a sudden death. Or a `kill(getpid(), 9);`
>
> Combine this with an explicit `fflush(stdout)` before to get your test output
> out.
Done.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13956#discussion_r1200309154
Re: RFR: 8308046: Move Solaris related charsets from java.base to jdk.charsets module [v3]
> According to "JDK 20 Internationalization Guide" > https://docs.oracle.com/en/java/javase/20/intl/supported-encodings.html > Following Solaris related charsets are in "contained in jdk.charsets module" > list. > > - PCK (x-PCK) > - EUC_JP_Solaris (x-eucJP-Open) > - Big5_Solaris (x-Big5-Solaris) > > These are not supported by Linux platform, so they should not be in java.base > module. > > Note: > GHA Linux x86 builds were failed. > I think it's not related by my modified code. > I opened [JDK-8308051](https://bugs.openjdk.org/browse/JDK-8308051) GHA: > Linux x86 builds failure Ichiroh Takiguchi has updated the pull request incrementally with one additional commit since the last revision: 8308046: Move Solaris related charsets from java.base to jdk.charsets module - Changes: - all: https://git.openjdk.org/jdk/pull/13973/files - new: https://git.openjdk.org/jdk/pull/13973/files/6fd12fcd..1c10b107 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=13973=02 - incr: https://webrevs.openjdk.org/?repo=jdk=13973=01-02 Stats: 43 lines in 4 files changed: 0 ins; 29 del; 14 mod Patch: https://git.openjdk.org/jdk/pull/13973.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13973/head:pull/13973 PR: https://git.openjdk.org/jdk/pull/13973
Re: RFR: 8308038: java/util/concurrent/ThreadPerTaskExecutor/ThreadPerTaskExecutorTest.java timed out
On Mon, 22 May 2023 09:48:15 GMT, Daniel Fuchs wrote:
> Seems strange to test invokeAny when there is only one task?
There are several tests further up that exercise invokeAny with more than one
task. This one is testing interrupt of invokeAny, we could expect it to test
interrupting with many tasks if needed.
> test/jdk/java/util/concurrent/ThreadPerTaskExecutor/ThreadPerTaskExecutorTest.java
> line 835:
>
>> 833: try {
>> 834:
>> scheduleInterruptAt("java.util.concurrent.ThreadPerTaskExecutor.invokeAll");
>> 835: executor.invokeAll(Set.of(task));
>
> same remark for invokeAll?
There are several tests further up that exercise invokeAll with more than one
task.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/14072#discussion_r1200295889
PR Review Comment: https://git.openjdk.org/jdk/pull/14072#discussion_r1200296492
Re: RFR: 8308038: java/util/concurrent/ThreadPerTaskExecutor/ThreadPerTaskExecutorTest.java timed out
On Mon, 22 May 2023 10:05:49 GMT, Jaikiran Pai wrote: >> This is a test only change to the unit test for the ExecutorService returned >> by Executors.newThreadPerTaskExecutor. The tests for interrupting invokeAll >> assume the threads started to execute the tasks do actually execute the task >> code. The refresh in JEP 444 changed the implementation to use FutureTask, >> and FutureTask checks the interrupt status before it executes the task code. >> So some intermittent timeouts of the tests for interrupting invokeAll as >> those tests were waiting for the task to complete. >> >> The main change is that the tests for interrupting invokeAll are changed to >> interrupt when the main thread blocks in invokeAll. They are also changed to >> check if the task started or not. The tests for interrupting invokeAny >> already did this, but these are changed to use the same infrastructure to >> avoid having two styles of tests in the same source file. > > test/jdk/java/util/concurrent/ThreadPerTaskExecutor/ThreadPerTaskExecutorTest.java > line 269: > >> 267: assertTrue(executor.awaitTermination(10, >> TimeUnit.MILLISECONDS)); >> 268: Throwable e = assertThrows(ExecutionException.class, >> future::get); >> 269: assertTrue(e.getCause() instanceof InterruptedException); > > Would using `assertEquals(InterruptedException.class, > e.getCause().getClass())` be better? That way if/when the test fails, it even > prints the unexpected exception type? > > But I then see that this test already uses `assertTrue` for similar cases in > some other place, so maybe it's fine in the current form? I don't mind moving this but it would require changing them everywhere (as you point out). - PR Review Comment: https://git.openjdk.org/jdk/pull/14072#discussion_r1200299321
RFR: 8308093: Disable language preview features use in JDK
This patch disables temporary use of language preview features in JDK. Temporary enabled language preview features (to allow Pattern Matching for switch use in the Classfile API library) are no more necessary. All redundant use of --enable-preview in the Classfile API tests are also removed. Please review. Thanks, Adam - Commit messages: - 8308093: Disable language preview features use in JDK Changes: https://git.openjdk.org/jdk/pull/14076/files Webrev: https://webrevs.openjdk.org/?repo=jdk=14076=00 Issue: https://bugs.openjdk.org/browse/JDK-8308093 Stats: 24 lines in 12 files changed: 0 ins; 22 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/14076.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14076/head:pull/14076 PR: https://git.openjdk.org/jdk/pull/14076
Re: RFR: 8308038: java/util/concurrent/ThreadPerTaskExecutor/ThreadPerTaskExecutorTest.java timed out
On Sun, 21 May 2023 13:52:06 GMT, Alan Bateman wrote: > This is a test only change to the unit test for the ExecutorService returned > by Executors.newThreadPerTaskExecutor. The tests for interrupting invokeAll > assume the threads started to execute the tasks do actually execute the task > code. The refresh in JEP 444 changed the implementation to use FutureTask, > and FutureTask checks the interrupt status before it executes the task code. > So some intermittent timeouts of the tests for interrupting invokeAll as > those tests were waiting for the task to complete. > > The main change is that the tests for interrupting invokeAll are changed to > interrupt when the main thread blocks in invokeAll. They are also changed to > check if the task started or not. The tests for interrupting invokeAny > already did this, but these are changed to use the same infrastructure to > avoid having two styles of tests in the same source file. test/jdk/java/util/concurrent/ThreadPerTaskExecutor/ThreadPerTaskExecutorTest.java line 269: > 267: assertTrue(executor.awaitTermination(10, TimeUnit.MILLISECONDS)); > 268: Throwable e = assertThrows(ExecutionException.class, > future::get); > 269: assertTrue(e.getCause() instanceof InterruptedException); Would using `assertEquals(InterruptedException.class, e.getCause().getClass())` be better? That way if/when the test fails, it even prints the unexpected exception type? But I then see that this test already uses `assertTrue` for similar cases in some other place, so maybe it's fine in the current form? - PR Review Comment: https://git.openjdk.org/jdk/pull/14072#discussion_r1200291899
Re: RFR: 8308038: java/util/concurrent/ThreadPerTaskExecutor/ThreadPerTaskExecutorTest.java timed out
On Sun, 21 May 2023 13:52:06 GMT, Alan Bateman wrote:
> This is a test only change to the unit test for the ExecutorService returned
> by Executors.newThreadPerTaskExecutor. The tests for interrupting invokeAll
> assume the threads started to execute the tasks do actually execute the task
> code. The refresh in JEP 444 changed the implementation to use FutureTask,
> and FutureTask checks the interrupt status before it executes the task code.
> So some intermittent timeouts of the tests for interrupting invokeAll as
> those tests were waiting for the task to complete.
>
> The main change is that the tests for interrupting invokeAll are changed to
> interrupt when the main thread blocks in invokeAll. They are also changed to
> check if the task started or not. The tests for interrupting invokeAny
> already did this, but these are changed to use the same infrastructure to
> avoid having two styles of tests in the same source file.
Looks reasonable. Just some question about testing invokeAll/invokeAny when
there is only one task.
test/jdk/java/util/concurrent/ThreadPerTaskExecutor/ThreadPerTaskExecutorTest.java
line 578:
> 576: try {
> 577:
> scheduleInterruptAt("java.util.concurrent.ThreadPerTaskExecutor.invokeAny");
> 578: executor.invokeAny(Set.of(task));
Seems strange to test invokeAny when there is only one task?
test/jdk/java/util/concurrent/ThreadPerTaskExecutor/ThreadPerTaskExecutorTest.java
line 835:
> 833: try {
> 834:
> scheduleInterruptAt("java.util.concurrent.ThreadPerTaskExecutor.invokeAll");
> 835: executor.invokeAll(Set.of(task));
same remark for invokeAll?
-
PR Review: https://git.openjdk.org/jdk/pull/14072#pullrequestreview-1436244152
PR Review Comment: https://git.openjdk.org/jdk/pull/14072#discussion_r1200271736
PR Review Comment: https://git.openjdk.org/jdk/pull/14072#discussion_r1200272685
Re: RFR: 8287834: Add SymbolLookup::or method [v3]
On 17/05/2023 02:33, John Hendrikx wrote:
SymbolLookup lookUp = name -> library1.find(name)
.or(() -> library2.find(name))
.or(() -> loader.find(name));
What you say is true - e.g. the fact that a lookup returns an optional
can be used to chain multiple lookups as you describe.
There are, however some difference - note that your example only uses
"captures" symbol lookup variable names - so the full code would be more
something like:
```java
SymbolLookup library1 = SymbolLookup.libraryLookup("lib1", arena);
SymbolLookup library2 = SymbolLookup.libraryLookup("lib2", arena);
SymbolLookup loader = SymbolLookup.loaderLookup();
SymbolLookup lookUp = name -> library1.find(name)
.or(() -> library2.find(name))
.or(() -> loader.find(name));
```
Without the initial setup, we could try to write something like this:
```java
SymbolLookup lookUp = name -> SymbolLookup.libraryLookup("lib1",
arena).find(name)
.or(() -> SymbolLookup.libraryLookup("lib2", arena).find(name))
.or(() -> SymbolLookup.loaderLookup().find(name));
```
But this more compact version has several issues. First, it creates a
new library lookup on each call to "find" (possibly more). Second, the
loader in which the lookup is retrieved depends on who calls the "find"
method.
So, the more compact version is not "like for like" for the more verbose
option above. And this is due to the fact that, in most cases,
retrieving a symbol lookup has some side-effects, such as loading a
library - it is *not* a purely functional process.
With the new method, it becomes like this:
```java
SymbolLookup lookUp =SymbolLookup.libraryLookup("lib1", arena)
.or(SymbolLookup.libraryLookup("lib2", arena))
.or(SymbolLookup.loaderLookup());
```
So, not only the new method results in more succinct code (compared to
the alternatives), but it also combines symbol lookups in the "right
way", so that the chain of lookups is defined, once and for all, when
the composite lookup is first created.
Cheers
Maurizio
Re: RFR: 8308293: A linker should expose the layouts it supports [v4]
> This patch adds an instance method on `Linker`, namely
> `Linker::canonicalLayouts` which returns all the layouts known by the linker
> as implementing some ABI type. For instance, if I call this on my machine
> (Linux/x64) I get this:
>
>
> jshell> import java.lang.foreign.*;
>
> jshell> Linker.nativeLinker().canonicalLayouts()
> $2 ==> {char16_t=c16, int8_t=b8, long=j64, size_t=j64, bool=z8, int=i32, long
> long=j64, int64_t=j64, void*=a64, float=f32, char=b8, int16_t=s16,
> int32_t=i32, short=s16, double=d64}
>
>
> This can be useful to discover the ABI types supported by a linker
> implementation, as well as for, in the future, add support for more exotic
> (and platform-dependent) linker types, such as `long double` or `complex
> long`.
Maurizio Cimadamore has updated the pull request incrementally with one
additional commit since the last revision:
Address further review comments
-
Changes:
- all: https://git.openjdk.org/jdk/pull/14037/files
- new: https://git.openjdk.org/jdk/pull/14037/files/df92467c..dbc8049a
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk=14037=03
- incr: https://webrevs.openjdk.org/?repo=jdk=14037=02-03
Stats: 5 lines in 1 file changed: 3 ins; 0 del; 2 mod
Patch: https://git.openjdk.org/jdk/pull/14037.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/14037/head:pull/14037
PR: https://git.openjdk.org/jdk/pull/14037
Re: RFR: 8303040: linux PPC64le: Implementation of Foreign Function & Memory API (Preview) [v31]
On Fri, 19 May 2023 18:54:19 GMT, Martin Doerr wrote:
>> Implementation of "Foreign Function & Memory API" for linux on Power (Little
>> Endian) according to "Power Architecture 64-Bit ELF V2 ABI Specification".
>>
>> This PR does not include code for VaList support because it's supposed to
>> get removed by [JDK-8299736](https://bugs.openjdk.org/browse/JDK-8299736).
>> I've kept the related tests disabled for this platform and throw an
>> exception instead. Note that the ABI doesn't precisely specify variable
>> argument lists. Instead, it refers to `` (2.2.4 Variable Argument
>> Lists).
>>
>> Big Endian support is implemented to some extend, but not complete. E.g.
>> structs with size not divisible by 8 are not passed correctly (see
>> `useABIv2` in CallArranger.java). Big Endian is excluded by selecting
>> `ARCH.equals("ppc64le")` (CABI.java) only.
>>
>> There's another limitation: This PR only accepts structures with size
>> divisible by 4. (An `IllegalArgumentException` gets thrown otherwise.) I
>> think arbitrary sizes are not usable on other platforms, either, because
>> `SharedUtils.primitiveCarrierForSize` only accepts powers of 2. Update:
>> Resolved after merging of
>> [JDK-8303017](https://bugs.openjdk.org/browse/JDK-8303017)
>>
>> The ABI has some tricky corner cases related to HFA (Homogeneous Float
>> Aggregate). The same argument may need to get passed in both, a FP reg and a
>> GP reg or stack slot (see "no partial DW rule"). This cases are not covered
>> by the existing tests.
>>
>> I had to make changes to shared code and code for other platforms:
>> 1. Pass type information when creating `VMStorage` objects from `VMReg`.
>> This is needed for the following reasons:
>> - PPC64 ABI requires integer types to get extended to 64 bit (also see
>> CCallingConventionRequiresIntsAsLongs in existing hotspot code). We need to
>> know the type or at least the bit width for that.
>> - Floating point load / store instructions need the correct width to select
>> between the correct IEEE 754 formats. The register representation in single
>> FP registers is always IEEE 754 double precision on PPC64.
>> - Big Endian also needs usage of the precise size. Storing 8 Bytes and
>> loading 4 Bytes yields different values than on Little Endian!
>> 2. It happens that a `NativeMemorySegmentImpl` is used as a raw pointer
>> (with byteSize() == 0) while running TestUpcallScope. Hence, existing size
>> checks don't work (see MemorySegment.java). As a workaround, I'm just
>> skipping the check in this particular case. Please check if this makes sense
>> or if there's a better fix (possibly as separat...
>
> Martin Doerr has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Cleanup imports, improve comments, updates from other platforms.
src/java.base/share/classes/jdk/internal/foreign/abi/ppc64/CallArranger.java
line 65:
> 63: */
> 64: public abstract class CallArranger {
> 65: protected abstract boolean useABIv2();
This could also be refactored into a static method with the same trick that is
used in `LinuxPPC64leLinker::getInstance`. Callers could be static then and you
could delete `CallArranger::ABIv2` and `ABIv2CallArranger`.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/12708#discussion_r1200197510
RFR: 8308038: java/util/concurrent/ThreadPerTaskExecutor/ThreadPerTaskExecutorTest.java timed out
This is a test only change to the unit test for the ExecutorService returned by Executors.newThreadPerTaskExecutor. The tests for interrupting invokeAll assume the threads started to execute the tasks do actually execute the task code. The refresh in JEP 444 changed the implementation to use FutureTask, and FutureTask checks the interrupt status before it executes the task code. So some intermittent timeouts of the tests for interrupting invokeAll as those tests were waiting for the task to complete. The main change is that the tests for interrupting invokeAll are changed to interrupt when the main thread blocks in invokeAll. They are also changed to check if the task started or not. The tests for interrupting invokeAny already did this, but these are changed to use the same infrastructure to avoid having two styles of tests in the same source file. - Commit messages: - Update - Merge - Initial commit Changes: https://git.openjdk.org/jdk/pull/14072/files Webrev: https://webrevs.openjdk.org/?repo=jdk=14072=00 Issue: https://bugs.openjdk.org/browse/JDK-8308038 Stats: 206 lines in 1 file changed: 87 ins; 57 del; 62 mod Patch: https://git.openjdk.org/jdk/pull/14072.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14072/head:pull/14072 PR: https://git.openjdk.org/jdk/pull/14072
Re: RFR: 8303040: linux PPC64le: Implementation of Foreign Function & Memory API (Preview) [v30]
On Fri, 19 May 2023 18:47:49 GMT, Martin Doerr wrote:
>> src/java.base/share/classes/jdk/internal/foreign/abi/ppc64/CallArranger.java
>> line 158:
>>
>>> 156: class StorageCalculator {
>>> 157: private final boolean forArguments;
>>> 158: private boolean forVarArgs = false;
>>
>> Seems to be not used.
>
> I had kept it in case another PPC64 OS would need it, but I guess it's
> unlikely. So, I just removed it. Could get added back easily if needed.
I see. There are other examples of redundant code that might serve a purpose in
the future. I honestly don't like that.
In the case of `forVarArgs` porters can still find it in the aarch64 version :)
-
PR Review Comment: https://git.openjdk.org/jdk/pull/12708#discussion_r1200139200
Re: RFR: 8307990: jspawnhelper must close its writing side of a pipe before reading from it
On Fri, 19 May 2023 15:43:30 GMT, Roger Riggs wrote: > Given the purpose and implementation of the readFully function, I don't see > how it can return anything other than an error or the full requested read > length. @RogerRiggs it will return < requested read length if it encounters EOF - i.e. a short read. - PR Comment: https://git.openjdk.org/jdk/pull/13956#issuecomment-1556727447
Re: RFR: 8307990: jspawnhelper must close its writing side of a pipe before reading from it [v3]
On Wed, 17 May 2023 17:07:13 GMT, Roger Riggs wrote: >> Volker Simonis has updated the pull request incrementally with one >> additional commit since the last revision: >> >> REALLY adding the test :) > > test/jdk/java/lang/ProcessBuilder/JspawnhelperProtocol.java line 106: > >> 104: } >> 105: line = br.readLine(); >> 106: } > > Try-with-resources works well in cases like this. (Moving foundCrashInfo out > of the t-w-r). Done. - PR Review Comment: https://git.openjdk.org/jdk/pull/13956#discussion_r1200111600
Re: RFR: 8307990: jspawnhelper must close its writing side of a pipe before reading from it [v3]
On Wed, 17 May 2023 17:05:54 GMT, Roger Riggs wrote:
>> Volker Simonis has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> REALLY adding the test :)
>
> test/jdk/java/lang/ProcessBuilder/JspawnhelperProtocol.java line 78:
>
>> 76: pb =
>> ProcessTools.createJavaProcessBuilder("-Djdk.lang.Process.launchMechanism=posix_spawn",
>> 77:
>> "JspawnhelperProtocol",
>> 78:"normalExec");
>
> I would just redirect the output to the parent.
> `pb.inheritIO()` and avoid the extra code at line 88.
Good suggestion. Done.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13956#discussion_r1200086821
Re: RFR: 8307990: jspawnhelper must close its writing side of a pipe before reading from it [v3]
On Wed, 17 May 2023 17:01:53 GMT, Roger Riggs wrote:
>> Volker Simonis has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> REALLY adding the test :)
>
> test/jdk/java/lang/ProcessBuilder/JspawnhelperProtocol.java line 63:
>
>> 61: if (p.exitValue() == 0) {
>> 62: String pwd = p.inputReader().readLine();
>> 63: if (!Path.of("").toAbsolutePath().toString().equals(pwd)) {
>
> It would be useful to print the unexpected string; it might help isolate what
> went wrong.
> And make it easier to understand what ERROR+2 means.
Done.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13956#discussion_r1200082310
Re: RFR: 8301552: Use AtomicReferenceArray for caching instead of CHM in ZoneOffset [v8]
On Thu, 9 Feb 2023 13:46:14 GMT, Per Minborg wrote: >> `ZoneOffset` instances are cached by the `ZoneOffset` class itself for >> values in the range [-18h, 18h] for each second that is on an even quarter >> of an hour (i.e. at most 2*18*4+1 = 145 values). >> >> Instead of using a `ConcurrentHashMap` for caching instanced, we could >> instead use an `AtomicReferenceArray` with direct slot value access for said >> even seconds. This will improve performance and reduce the number of object >> even though the backing array will go from an initial 32 in the CHM to an >> initial/final 145 in the ARA. The CHM will contain much more objects and >> array slots for typical numbers of entries in the cache and will compute >> hash/bucket/collision on the hot code path for each cache access. > > Per Minborg has updated the pull request incrementally with three additional > commits since the last revision: > > - Remove unused setup method > - Rename method in test > - Add copyright header Keep alive. We might use another construct. - PR Comment: https://git.openjdk.org/jdk/pull/12346#issuecomment-1556683570
Re: RFR: 8306431: File.listRoots method description should be re-examined [v6]
On Mon, 22 May 2023 07:04:41 GMT, Alan Bateman wrote: >> I think following two paragraphs belongs to `Implementation Note` too, >> because of UNC pathname and SecurityException. >> Is it wrong? > > The paragraph about that the result is filtered when checkRead is denied by > the SecurityException is normative. Thank you for your advice. I moved this paragraph upper. - PR Review Comment: https://git.openjdk.org/jdk/pull/13526#discussion_r1200068557
Re: RFR: 8306431: File.listRoots method description should be re-examined [v7]
> I fixed File.listRoots description. > * remove "the insertion or ejection of removable media" > * change "available" to "existing" > > Please review this change. Nagata-Haruhito has updated the pull request incrementally with one additional commit since the last revision: Move SecurityException paragraph - Changes: - all: https://git.openjdk.org/jdk/pull/13526/files - new: https://git.openjdk.org/jdk/pull/13526/files/7ba2460a..3d4aed98 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=13526=06 - incr: https://webrevs.openjdk.org/?repo=jdk=13526=05-06 Stats: 12 lines in 1 file changed: 6 ins; 6 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/13526.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13526/head:pull/13526 PR: https://git.openjdk.org/jdk/pull/13526
Re: RFR: 8307990: jspawnhelper must close its writing side of a pipe before reading from it [v3]
On Wed, 17 May 2023 17:08:34 GMT, Roger Riggs wrote:
>> Volker Simonis has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> REALLY adding the test :)
>
> test/jdk/java/lang/ProcessBuilder/JspawnhelperProtocol.java line 116:
>
>> 114: int ret = p.exitValue();
>> 115: if (ret == 0) {
>> 116: throw new Exception("Expected error in child execution");
>
> Mixing the two styles of error reporting seems a bit odd, some throw
> exceptions and others just exit.
> Being consistent throwing an exception with a message would be better.
Good point. Changed the exits to exceptions except in `childCode()` where we
need `System.exit()` in order to communicate different error states back to the
parent process.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13956#discussion_r1200069183
Re: RFR: 8306431: File.listRoots method description should be re-examined [v6]
On Mon, 22 May 2023 07:00:27 GMT, Nagata-Haruhito wrote:
>> src/java.base/share/classes/java/io/File.java line 1828:
>>
>>> 1826: * namely {@code "/"}. The set of filesystem roots is affected
>>> 1827: * by various system-level operations such as the disconnecting or
>>> 1828: * unmounting of physical or virtual disk drives.
>>
>> I think you'll have to move this to the end of the method description,
>> otherwise the paragraphs that follow will show up as "Implementation Note"
>> too.
>
> I think following two paragraphs belongs to `Implementation Note` too,
> because of UNC pathname and SecurityException.
> Is it wrong?
The paragraph about that the result is filtered when checkRead is denied by the
SecurityException is normative.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13526#discussion_r1200052457
Re: RFR: 8306431: File.listRoots method description should be re-examined [v6]
On Fri, 19 May 2023 09:32:03 GMT, Alan Bateman wrote:
>> Nagata-Haruhito has updated the pull request incrementally with three
>> additional commits since the last revision:
>>
>> - Merge branch 'File_javadoc' of https://github.com/Nagata-Haruhito/jdk
>> into File_javadoc
>> - Drop exist to available
>> - Merge branch 'master' into File_javadoc
>
> src/java.base/share/classes/java/io/File.java line 1828:
>
>> 1826: * namely {@code "/"}. The set of filesystem roots is affected
>> 1827: * by various system-level operations such as the disconnecting or
>> 1828: * unmounting of physical or virtual disk drives.
>
> I think you'll have to move this to the end of the method description,
> otherwise the paragraphs that follow will show up as "Implementation Note"
> too.
I think following two paragraphs belongs to `Implementation Note` too, because
of UNC pathname and SecurityException.
Is it wrong?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13526#discussion_r1200048699
