Re: RFR: 8237521: Memory Access API fixes for 32-bit
Hi Nick :) On 23/01/2020 5:25 pm, David Holmes wrote: On 23/01/2020 5:08 pm, Nick Gasson wrote: Hi Maurizio, Yes, for jdk/jdk. Do I need another Reviewer? Yes - specifically you need a reviewer from Hotspot! cc'ing hotspot-runtime-dev :) So on 32-bit size_t is 32-bit and so may have ~half the capacity allowed for by the jlong size variable. On 64-bit overflow is not possible because jlong is signed and size_t is not. So we only need an overflow check on 32-bit. But your checks don't look correct to me. If size is already aligned to HeapWordSize then "sz < (size_t)size" won't be true but you already have a completely bogus value for sz when you truncated size to 32-bits. Cheers, David Cheers, David Thanks, Nick On 01/22/20 20:46 pm, Maurizio Cimadamore wrote: Fixes look good to me - I assume this is for JDK 15, right? Maurizio On 22/01/2020 08:43, Nick Gasson wrote: Hi, Bug: https://bugs.openjdk.java.net/browse/JDK-8237521 Webrev: http://cr.openjdk.java.net/~ngasson/8237521/webrev.01/ This is a follow-up to JDK-8236634 which just contained changes to the tests to make them build/pass on 32-bit systems for JDK 14. This patch removes the test workarounds and fixes two things to make them pass: * In Unsafe_AllocateMemory0 handle the case where the aligned-up size overflows a 32-bit size_t and throw an OutOfMemoryError instead of calling os::malloc(0). * Change the value of Utils.MAX_ALIGN from 16 to 8 on 32-bit systems, as malloc can return addresses that are only 8-byte aligned. Tested jdk_foreign on x86_32, arm32, aarch64, x86_64. Thanks, Nick
Re: RFR: 8237521: Memory Access API fixes for 32-bit
Hi Maurizio, Yes, for jdk/jdk. Do I need another Reviewer? Thanks, Nick On 01/22/20 20:46 pm, Maurizio Cimadamore wrote: > Fixes look good to me - I assume this is for JDK 15, right? > > Maurizio > > On 22/01/2020 08:43, Nick Gasson wrote: >> Hi, >> >> Bug: https://bugs.openjdk.java.net/browse/JDK-8237521 >> Webrev: http://cr.openjdk.java.net/~ngasson/8237521/webrev.01/ >> >> This is a follow-up to JDK-8236634 which just contained changes to the >> tests to make them build/pass on 32-bit systems for JDK 14. This patch >> removes the test workarounds and fixes two things to make them pass: >> >> * In Unsafe_AllocateMemory0 handle the case where the aligned-up size >> overflows a 32-bit size_t and throw an OutOfMemoryError instead of >> calling os::malloc(0). >> >> * Change the value of Utils.MAX_ALIGN from 16 to 8 on 32-bit systems, >> as malloc can return addresses that are only 8-byte aligned. >> >> Tested jdk_foreign on x86_32, arm32, aarch64, x86_64. >> >> >> Thanks, >> Nick
RFR: JDK-8237607: [macos] Signing app bundle with jpackage fails if runtime is already signed
Please review the jpackage fix for bug [1] at [2]. - Fixed by forcing signing even if runtime already signed. - Original implementation was not taken in consideration that runtime can be signed (JDK itself is signed from which jpackage is used or runtime provided via --runtime-image). - Signature of binaries files in runtime will not be change, with this fix we will generate new _CodeSignature/CodeResources file which contains signatures of all files inside runtime folder signed with user provided certificate. [1] https://bugs.openjdk.java.net/browse/JDK-8237607 [2] http://cr.openjdk.java.net/~almatvee/8237607/webrev.00/ Thanks, Alexander
RFR 8237599 : Greedy matching against supplementary chars fails to respect the region
Hello everyone! When the input of a j.u.regex.Matcher is restricted with .region() method, it can possibly cut off a half of a surrogate pair. It turns out that greedy matching implemented in the Pattern.CharPropertyGreedy class fails to recognize this edge case in two scenarios: 1) When it greedily consumes the input and meets a higher half of a surrogate pair that was cut off at the end of input, and 2) When it backs off and meets a lower half of a surrogate pair at the very beginning of input. In both cases, the engine reads the entire codepoint, crossing the boundaries of the set region. Instead, it should only read the half of the surrogate pair that lies inside the region and ignore the other half. Would you please help review the fix? BUGURL: https://bugs.openjdk.java.net/browse/JDK-8237599 WEBREV: http://cr.openjdk.java.net/~igerasim/8237599/00/webrev/ Thanks in advance! -- With kind regards, Ivan Gerasimov
Re: RFR [15] 8236825: Reading output from ... using ProcessBuilder/Process might hang
My sense is it is fixing a marginal case for the less dominant platform where
this is less likely arise, whereas for the dominant platform there is still an
issue.
Waiting for a non-blocking native read is a reasonable approach (IIUC that is
required for the desired proper fix). It would be useful to assess any
time-frame of such support to plan ahead?
—
ProcessImpl
—
665 void processExited() {
666 synchronized (closeLock) {
667 try {
668 InputStream in = this.in;
669 // this stream is closed if and only if: in == null
670 if (in != null) {
671 boolean noCompete = false;
672 try {
673 // Briefly, wait for competing read to complete
674 noCompete = readLock.tryAcquire(500L,
TimeUnit.MILLISECONDS);
675 if (noCompete) {
676 // no competing read, buffer any pending
input
677 this.in = drainInputStream(in);
678 }
679 } catch (InterruptedException ie) {
680 // Ignore interrupt and release and close
always
681 } finally {
682 readAborted = !noCompete;
683 in.close(); // close the original
underlying input stream
684 if (noCompete)
685 readLock.release();
686 }
687 }
688 } catch (IOException ignored) {}
689 }
690 }
Do you need to move the code at lines 684-685 to occur before line 683? since
in.close() could throw.
Paul.
> On Jan 21, 2020, at 12:36 PM, Roger Riggs wrote:
>
> Please review a potential way to address two issues of java.lang.Process
> deadlocks during process exit. [1] [2]
> (Linxu OS process expertise appreciated).
>
> The deadlock is between some thread reading process output from a process
> that has exited
> and the processExited thread that is attempting to buffer any remaining
> output so
> the file descriptor for the pipe can be closed. The methods involved are
> synchronized
> on a ProcessPipeInputStream instance.
>
> The hard case arises infrequently since the pipe streams are closed by the OS
> normally (or within a few seconds) and the readXXX completes.
> However, the case causing trouble is when the subprocess has spawned
> another process and both processes are using the same file descriptor/stream
> for output.
> Though the process that exits doesn't have the fd open anymore the extra
> subprocess does.
> And if that subprocess does not exit, then the read and deadlock does not get
> resolved.
>
> The approach proposed is to use a semaphore to guard the readXXX and
> providing some non-blocking logic in processExited to forcibly close
> the pipe if it detects that there is a conflicting read in progress.
> (And remove the synchronized on processExited).
>
> This solution works ok on MacOSX, where one of the issues occurred frequently.
> Closing the pipe unblocks the reading thread.
>
> On Linux, it appears that the blocking read (in native code) does not unblock
> unless a signal occurs; so the solution does not fix the problem
> adqurated/completely.
>
> Having a non-blocking native read would be the next step of complexity.
> The problem has been around for a while so it may be an option to wait
> for additional work on non-blocking pipe reads, the direction that Loom is
> moving.
>
> Suggestions welcome,
>
> Thanks, Roger
>
> Webrev: http://cr.openjdk.java.net/~rriggs/webrev-hdiutil-8236825/
>
> Issues:
> [1] https://bugs.openjdk.java.net/browse/JDK-8236825
> [2] https://bugs.openjdk.java.net/browse/JDK-8169565
Re: [PING] RFR: 8231111: Cgroups v2: Rework Metrics in java.base so as to recognize unified hierarchy
> On Jan 22, 2020, at 11:06 AM, Mandy Chung wrote: > > > > On 1/22/20 1:53 AM, Severin Gehwolf wrote: >> Hi Mandy, >> >> Thanks again for the review! I'll be sure to fix things. Some of the >> issues you've pointed out probably pre-existed in old code. Some became >> more complicated since unlimited in cgroupv1 is a large long value in >> files. Anyway, I'll update it. > > Thank you. >> On Tue, 2020-01-21 at 16:09 -0800, Mandy Chung wrote: >>> MetricsCgroupV1 is linux-only. It will fail the compilation when >>> building on non-linux. >> I don't think so. MetricsCgroupV1 is a new interface in shared code >> just like Metrics itself: >> src/java.base/share/classes/jdk/internal/platform/MetricsCgroupV1.java > > Ah, I mis-read that was placed under src/java.base/linux/classes. OK. >> I've put it there for the exact reason to NOT break compilation on non- >> Linux. We could call it ExtendedMetrics or something, though. Pondered >> that for a bit, but it wasn't important enough so I've kept it. >>> One option is to move this code to >>>src/java.base/linux/share/sun/launcher/CgroupMetrics.java >> Not sure if that would help. MetricsCgropuV1 is being used in >> LauncherHelper.java which itself is available on all platforms, no? > > -XshowSettings:system is a linux-only option. >>> Are they continued to be interesting metrics to be output from >>> -XshowSetting? I wonder if they can simply be dropped from the output. >>> Bob will have an opinion. >> I think they are. At least it prints the same metrics as were printed >> before this patch on cgroupv1. What's the rationale of not printing >> them any longer? My premise was, they were useful before and they >> continue to be useful. For cgroupv2 they're not configurable so they're >> not printed (and not useful as they'd show up as N/A anyway). > > Bob can advice on the usefulness of these metrics.I have no objection to > print them on cgroup v1. On v2, they are not shown (not even N/A, right?) These metrics are not that useful in the -XshowSettings context. I’d just drop them from the output. Bob. > > Mandy
Re: [PING] RFR: 8231111: Cgroups v2: Rework Metrics in java.base so as to recognize unified hierarchy
On 1/22/20 1:53 AM, Severin Gehwolf wrote: Hi Mandy, Thanks again for the review! I'll be sure to fix things. Some of the issues you've pointed out probably pre-existed in old code. Some became more complicated since unlimited in cgroupv1 is a large long value in files. Anyway, I'll update it. Thank you. On Tue, 2020-01-21 at 16:09 -0800, Mandy Chung wrote: MetricsCgroupV1 is linux-only. It will fail the compilation when building on non-linux. I don't think so. MetricsCgroupV1 is a new interface in shared code just like Metrics itself: src/java.base/share/classes/jdk/internal/platform/MetricsCgroupV1.java Ah, I mis-read that was placed under src/java.base/linux/classes. OK. I've put it there for the exact reason to NOT break compilation on non- Linux. We could call it ExtendedMetrics or something, though. Pondered that for a bit, but it wasn't important enough so I've kept it. One option is to move this code to src/java.base/linux/share/sun/launcher/CgroupMetrics.java Not sure if that would help. MetricsCgropuV1 is being used in LauncherHelper.java which itself is available on all platforms, no? -XshowSettings:system is a linux-only option. Are they continued to be interesting metrics to be output from -XshowSetting? I wonder if they can simply be dropped from the output. Bob will have an opinion. I think they are. At least it prints the same metrics as were printed before this patch on cgroupv1. What's the rationale of not printing them any longer? My premise was, they were useful before and they continue to be useful. For cgroupv2 they're not configurable so they're not printed (and not useful as they'd show up as N/A anyway). Bob can advice on the usefulness of these metrics. I have no objection to print them on cgroup v1. On v2, they are not shown (not even N/A, right?) Mandy
Re: [15] RFR: 8236903: ZoneRules#getOffset throws DateTimeException for rules with last rules
+1, looks good On 1/22/20 4:55 AM, Stephen Colebourne wrote: Looks good Stephen On Tue, 21 Jan 2020 at 21:53, wrote: Please review the fix to the following issue: https://bugs.openjdk.java.net/browse/JDK-8236903 The proposed changeset is located at: http://cr.openjdk.java.net/~naoto/8236903/webrev.00/ This edge case throws an out-of-bounds DateTimeException in ZoneRules.findYear() on calculating the year. Instead of relying on LocalDate.ofEpochDay(), inlining the year caluculation from that method, and cap to the MAX value if needed, will solve the issue. Naoto
Re: RFR: 8237521: Memory Access API fixes for 32-bit
Fixes look good to me - I assume this is for JDK 15, right? Maurizio On 22/01/2020 08:43, Nick Gasson wrote: Hi, Bug: https://bugs.openjdk.java.net/browse/JDK-8237521 Webrev: http://cr.openjdk.java.net/~ngasson/8237521/webrev.01/ This is a follow-up to JDK-8236634 which just contained changes to the tests to make them build/pass on 32-bit systems for JDK 14. This patch removes the test workarounds and fixes two things to make them pass: * In Unsafe_AllocateMemory0 handle the case where the aligned-up size overflows a 32-bit size_t and throw an OutOfMemoryError instead of calling os::malloc(0). * Change the value of Utils.MAX_ALIGN from 16 to 8 on 32-bit systems, as malloc can return addresses that are only 8-byte aligned. Tested jdk_foreign on x86_32, arm32, aarch64, x86_64. Thanks, Nick
Re: RFR 8236034 : Use optimized Ques node for curly {0,1} quantifier
Thank you Roger for review! Pushed. -- With kind regards, Ivan Gerasimov
Re: [15] RFR: 8236903: ZoneRules#getOffset throws DateTimeException for rules with last rules
Looks good Stephen On Tue, 21 Jan 2020 at 21:53, wrote: > Please review the fix to the following issue: > > https://bugs.openjdk.java.net/browse/JDK-8236903 > > The proposed changeset is located at: > > http://cr.openjdk.java.net/~naoto/8236903/webrev.00/ > > This edge case throws an out-of-bounds DateTimeException in > ZoneRules.findYear() on calculating the year. Instead of relying on > LocalDate.ofEpochDay(), inlining the year caluculation from that method, > and cap to the MAX value if needed, will solve the issue. > > Naoto
Re: [PING] RFR: 8231111: Cgroups v2: Rework Metrics in java.base so as to recognize unified hierarchy
Hi Mandy, Thanks again for the review! I'll be sure to fix things. Some of the issues you've pointed out probably pre-existed in old code. Some became more complicated since unlimited in cgroupv1 is a large long value in files. Anyway, I'll update it. On Tue, 2020-01-21 at 16:09 -0800, Mandy Chung wrote: > MetricsCgroupV1 is linux-only. It will fail the compilation when > building on non-linux. I don't think so. MetricsCgroupV1 is a new interface in shared code just like Metrics itself: src/java.base/share/classes/jdk/internal/platform/MetricsCgroupV1.java I've put it there for the exact reason to NOT break compilation on non- Linux. We could call it ExtendedMetrics or something, though. Pondered that for a bit, but it wasn't important enough so I've kept it. > One option is to move this code to >src/java.base/linux/share/sun/launcher/CgroupMetrics.java Not sure if that would help. MetricsCgropuV1 is being used in LauncherHelper.java which itself is available on all platforms, no? > Are they continued to be interesting metrics to be output from > -XshowSetting? I wonder if they can simply be dropped from the output. > Bob will have an opinion. I think they are. At least it prints the same metrics as were printed before this patch on cgroupv1. What's the rationale of not printing them any longer? My premise was, they were useful before and they continue to be useful. For cgroupv2 they're not configurable so they're not printed (and not useful as they'd show up as N/A anyway). Thanks, Severin
RFR: 8237521: Memory Access API fixes for 32-bit
Hi, Bug: https://bugs.openjdk.java.net/browse/JDK-8237521 Webrev: http://cr.openjdk.java.net/~ngasson/8237521/webrev.01/ This is a follow-up to JDK-8236634 which just contained changes to the tests to make them build/pass on 32-bit systems for JDK 14. This patch removes the test workarounds and fixes two things to make them pass: * In Unsafe_AllocateMemory0 handle the case where the aligned-up size overflows a 32-bit size_t and throw an OutOfMemoryError instead of calling os::malloc(0). * Change the value of Utils.MAX_ALIGN from 16 to 8 on 32-bit systems, as malloc can return addresses that are only 8-byte aligned. Tested jdk_foreign on x86_32, arm32, aarch64, x86_64. Thanks, Nick
