March 2020 - core-libs-dev

Re: RFR: 8232846: ProcessHandle.Info command with non-English shows question marks
by Yasumasa Suenaga 27 Mar '20

27 Mar '20

Ok, your change looks good. Thanks, Yasumasa On 2020/03/27 18:10, Toshio 5 Nakamura wrote: > Hi Suenaga-san, > Thank you for the comment. I updated the limit to 32768. > webrev.02: http://cr.openjdk.java.net/~tnakamura/8232846/webrev.02 > Well, I believe the new logic works as same as the current one. > I added NULL initialization to commandObj, and the value will be changed only if QueryFullProcessImageNameW() was success. > Then, CHECK_NULL(commandObj) works as "return" if commandObj is NULL. > src/java.base/share/native/libjava/jni_util.h > > #define CHECK_NULL(x) \ > > do { \ > > if ((x) == NULL) { \ > > return; \ > > } \ > > } while (0) \ > I hope this solves your concern. > Best regards, > Toshio Nakamura > > ----- Original message ----- > Nakamura-san, > > I think location of CHECK_NULL and SetObjectField() are incorrect. > Currently they are called when QueryFullProcessImageName() succeed only. > > In addition, you need to allocate 32768 chars (32767 + 1 ('\0')) via malloc. > I understand "32767" is max length, not includes null-terminator. > > > Thanks, > > Yasumasa > > > On 2020/03/27 14:06, Toshio 5 Nakamura wrote: > > > > Hi Roger, Suenaga-san, > > > > Thank you for your comments and discussion for the issue. > > I've updated webrev. Could you review it again? > > tier1 tests passed. > > > > webrev.01: http://cr.openjdk.java.net/~tnakamura/8232846/webrev.01 > > > > Best regards, > > Toshio Nakamura > > > > Yasumasa Suenaga <suenaga(a)oss.nttdata.com> wrote on 2020/03/27 09:10:15: > > > >> On 2020/03/27 9:01, Roger Riggs wrote: > >>> Hi, > >>> > >>> Please don't throw an exception. > >>> It would abort being able to provide any information. > >>> And who would expect or handle such an exception? > >>> > >>> Degrading the info or omitting it provides better service overall. > >>> There is no point to throwing an exception. > >> > >> hProcess in QueryFullProcessImageNameW needs > >> PROCESS_QUERY_INFORMATION or PROCESS_QUERY_LIMITED_INFORMATION > >> access right, so I thought it is reasonable to throw runtime > >> exception if the call failed. > >> > >> Anyway, I agree with you if throwing exception is not comfortable in > >> this case for Java API. > >> > >> > >> Thanks, > >> > >> Yasumasa > >> > >> > >>> Regards, Roger > >>> > >>> > >>> On 3/26/20 7:55 PM, Yasumasa Suenaga wrote: > >>>> On 2020/03/27 0:35, Roger Riggs wrote: > >>>>> Hi, > >>>>> > >>>>> Reading further down the reference to the section: > >>>>> "Enable Long Paths in Windows 10, Version 1607, and Later" > >>>>> might suggest the code be more resilient to long paths. > >>>>> > >>>>> The stack allocated buffer (1024) is fine, but I'd suggest > >> adding code to retry in the case > >>>>> of INSUFFICIENT BUFFER with a malloc'd buffer to make it more > >> robust since the enabling > >>>>> of long paths is environment and application specific. > >>>> > >>>> In addition, it's better to throw an exception if the call failed > >> due to other error code. > >>>> > >>>> Thanks, > >>>> > >>>> Yasumasa > >>>> > >>>> > >>>>> Thanks, Roger > >>>>> > >>>>> > >>>>> > >>>>> On 3/26/20 10:40 AM, Toshio 5 Nakamura wrote: > >>>>>> Hi Thomas, Suenaga-san, Roger, > >>>>>> > >>>>>> Thank you for reviewing and comments. > >>>>>> > >>>>>> I checked behaviors by a small program and debugger. > >>>>>> If QueryFullProcessImageNameW failed by not enough buffer, > >>>>>> the API didn't change the buffer. > >>>>>> It just set ERROR_INSUFFICIENT_BUFFER(0x7a) to LastError. > >>>>>> > >>>>>> The buffer size becomes 1024 characters (2048 bytes) by this patch. > >>>>>> Should it use bigger size with malloc? 32,767 characters can belimit > > [2]. > >>>>>> I feel 1024 is reasonable value for command's location. > >>>>>> > >>>>>> [2] > >>>>>> > > https://urldefense.com/v3/__https://docs.microsoft.com/en-us/windows/win32/… > > $ > >>>>>> Best regards, > >>>>>> Toshio Nakamura > >>>>>> > >>>>>>> Hi, > >>>>>>> > >>>>>>> If the call fails, the command field in the info will not be set (and > >>>>>>> therefore null). > >>>>>>> > >>>>>>> I agree that a length of 512 (characters) is probably too small. > >>>>>>> > >>>>>>> Roger > >>>>>>> > >>>>>>> > >>>>>>> On 3/26/20 6:37 AM, Yasumasa Suenaga wrote: > >>>>>>>> Hi Nakamura-san, > >>>>>>>> > >>>>>>>> Your change almost looks good, but I have one question. > >>>>>>>> Length of exeName (1024) is enough? > >>>>>>>> > >>>>>>>> According to Microsoft Doc [1], exeName might not be null-terminated > >>>>>>>> if it failed. > >>>>>>>> I concern buffer overrun might occur when QueryFullProcessImageNameW > >>>>>>>> failed. > >>>>>>>> > >>>>>>>> > >>>>>>>> Thanks, > >>>>>>>> > >>>>>>>> Yasumasa > >>>>>>>> > >>>>>>>> > >>>>>>>> [1] > >>>>>> > > https://docs.microsoft.com/en-us/windows/win32/api/winbase/nf-winbase-query… > > > >>>>>> > >>>>>>>> > >>>>>>>> On 2020/03/26 17:58, Toshio 5 Nakamura wrote: > >>>>>>>>> Hi All, > >>>>>>>>> > >>>>>>>>> Could you review this change? Additionally, I'd like to ask a > > sponsor > >>>>>>>>> for > >>>>>>>>> it, since I'm not a committer. > >>>>>>>>> > >>>>>>>>> Bug: > >>>>>> > > https://bugs.openjdk.java.net/browse/JDK-8232846 > > > >>>>>> > >>>>>>>>> Webrev: > >>>>>> > > http://cr.openjdk.java.net/~tnakamura/8232846/webrev.00 > > >>>>>>>>> Under Windows environments, ProcessHandle.Info.command shows question marks > >>>>>>>>> if the command has non-English characters. I'd like to change the > >>>>>>>>> underlying API to Unicode version. > >>>>>>>>> Tier1 tests passed. > >>>>>>>>> > >>>>>>>>> Best regards, > >>>>>>>>> Toshio Nakamura > >>>>>>>>> > >>>>> > >>> > >> > >

3 3

RFR: 8232846: ProcessHandle.Info command with non-English shows question marks
by Toshio 5 Nakamura 27 Mar '20

27 Mar '20

Hi All, Could you review this change? Additionally, I'd like to ask a sponsor for it, since I'm not a committer. Bug: https://bugs.openjdk.java.net/browse/JDK-8232846 Webrev: http://cr.openjdk.java.net/~tnakamura/8232846/webrev.00 Under Windows environments, ProcessHandle.Info.command shows question marks if the command has non-English characters. I'd like to change the underlying API to Unicode version. Tier1 tests passed. Best regards, Toshio Nakamura

4 11

Re: RFR: 8232846: ProcessHandle.Info command with non-English shows question marks
by Thomas Stüfe 27 Mar '20

27 Mar '20

Looks good to me in its current form, thanks. On Fri, Mar 27, 2020 at 8:02 AM Toshio 5 Nakamura <TOSHIONA(a)jp.ibm.com> wrote: > Hi Thomas, > > Yes, I tested it locally with small buffer size, 50 for example. But, it's > hard to create a jtreg test. > > I confirmed ERROR_INSUFFICIENT_BUFFER returned from GetLastError(), if the > provided buffer was not enough. > Furthermore, "lpdwSize" parameter didn't set a new value in that case. > I hope the manual has more specific description... > > Best regards, > Toshio Nakamura > > > ----- Original message ----- > > Hi Toshio, > > did you test the malloc path? Since > https://docs.microsoft.com/en-us/windows/win32/api/winbase/nf-winbase-query… does > not mention ERROR_INSUFFICIENT_BUFFER as return code, would like to see > this working. It does however mention that it returns the number of > returned characters, so if the return code does not work out we may use > that. > > If you tested the truncation path and it works I am fine with the change > as it is. > > Cheers, Thomas > > > On Fri, Mar 27, 2020 at 6:08 AM Toshio 5 Nakamura <TOSHIONA(a)jp.ibm.com> > wrote: > > > Hi Roger, Suenaga-san, > > Thank you for your comments and discussion for the issue. > I've updated webrev. Could you review it again? > tier1 tests passed. > > webrev.01: http://cr.openjdk.java.net/~tnakamura/8232846/webrev.01 > > Best regards, > Toshio Nakamura > > Yasumasa Suenaga <suenaga(a)oss.nttdata.com> wrote on 2020/03/27 09:10:15: > > > On 2020/03/27 9:01, Roger Riggs wrote: > > > Hi, > > > > > > Please don't throw an exception. > > > It would abort being able to provide any information. > > > And who would expect or handle such an exception? > > > > > > Degrading the info or omitting it provides better service overall. > > > There is no point to throwing an exception. > > > > hProcess in QueryFullProcessImageNameW needs > > PROCESS_QUERY_INFORMATION or PROCESS_QUERY_LIMITED_INFORMATION > > access right, so I thought it is reasonable to throw runtime > > exception if the call failed. > > > > Anyway, I agree with you if throwing exception is not comfortable in > > this case for Java API. > > > > > > Thanks, > > > > Yasumasa > > > > > > > Regards, Roger > > > > > > > > > On 3/26/20 7:55 PM, Yasumasa Suenaga wrote: > > >> On 2020/03/27 0:35, Roger Riggs wrote: > > >>> Hi, > > >>> > > >>> Reading further down the reference to the section: > > >>> "Enable Long Paths in Windows 10, Version 1607, and Later" > > >>> might suggest the code be more resilient to long paths. > > >>> > > >>> The stack allocated buffer (1024) is fine, but I'd suggest > > adding code to retry in the case > > >>> of INSUFFICIENT BUFFER with a malloc'd buffer to make it more > > robust since the enabling > > >>> of long paths is environment and application specific. > > >> > > >> In addition, it's better to throw an exception if the call failed > > due to other error code. > > >> > > >> Thanks, > > >> > > >> Yasumasa > > >> > > >> > > >>> Thanks, Roger > > >>> > > >>> > > >>> > > >>> On 3/26/20 10:40 AM, Toshio 5 Nakamura wrote: > > >>>> Hi Thomas, Suenaga-san, Roger, > > >>>> > > >>>> Thank you for reviewing and comments. > > >>>> > > >>>> I checked behaviors by a small program and debugger. > > >>>> If QueryFullProcessImageNameW failed by not enough buffer, > > >>>> the API didn't change the buffer. > > >>>> It just set ERROR_INSUFFICIENT_BUFFER(0x7a) to LastError. > > >>>> > > >>>> The buffer size becomes 1024 characters (2048 bytes) by this patch. > > >>>> Should it use bigger size with malloc? 32,767 characters can belimit > [2]. > > >>>> I feel 1024 is reasonable value for command's location. > > >>>> > > >>>> [2] > > >>>> > > https://urldefense.com/v3/__https://docs.microsoft.com/en-us/windows/win32/… > $ > > >>>> Best regards, > > >>>> Toshio Nakamura > > >>>> > > >>>>> Hi, > > >>>>> > > >>>>> If the call fails, the command field in the info will not be set > (and > > >>>>> therefore null). > > >>>>> > > >>>>> I agree that a length of 512 (characters) is probably too small. > > >>>>> > > >>>>> Roger > > >>>>> > > >>>>> > > >>>>> On 3/26/20 6:37 AM, Yasumasa Suenaga wrote: > > >>>>>> Hi Nakamura-san, > > >>>>>> > > >>>>>> Your change almost looks good, but I have one question. > > >>>>>> Length of exeName (1024) is enough? > > >>>>>> > > >>>>>> According to Microsoft Doc [1], exeName might not be > null-terminated > > >>>>>> if it failed. > > >>>>>> I concern buffer overrun might occur when > QueryFullProcessImageNameW > > >>>>>> failed. > > >>>>>> > > >>>>>> > > >>>>>> Thanks, > > >>>>>> > > >>>>>> Yasumasa > > >>>>>> > > >>>>>> > > >>>>>> [1] > > >>>> > > https://docs.microsoft.com/en-us/windows/win32/api/winbase/nf-winbase-query… > > > >>>> > > >>>>>> > > >>>>>> On 2020/03/26 17:58, Toshio 5 Nakamura wrote: > > >>>>>>> Hi All, > > >>>>>>> > > >>>>>>> Could you review this change? Additionally, I'd like to ask a > sponsor > > >>>>>>> for > > >>>>>>> it, since I'm not a committer. > > >>>>>>> > > >>>>>>> Bug: > > >>>> > https://bugs.openjdk.java.net/browse/JDK-8232846 > > > >>>> > > >>>>>>> Webrev: > > >>>> > http://cr.openjdk.java.net/~tnakamura/8232846/webrev.00 > > > >>>> > > >>>>>>> Under Windows environments, ProcessHandle.Info.command shows > question > > >>>>>>> marks > > >>>>>>> if the command has non-English characters. I'd like to change the > > >>>>>>> underlying API to Unicode version. > > >>>>>>> Tier1 tests passed. > > >>>>>>> > > >>>>>>> Best regards, > > >>>>>>> Toshio Nakamura > > >>>>>>> > > >>> > > > > > > > > >

1 0

jpackage Are the RPM Requires necessary?
by Sverre Moe 26 Mar '20

26 Mar '20

Running jpackage to build an RPM will add some Requires to the package. It comes from the Template file: https://github.com/openjdk/jdk/blob/jdk-14%2B36/src/jdk.incubator.jpackage/… Autoprov: 0 Autoreq: 0 %if "xPACKAGE_DEFAULT_DEPENDENCIES" != x || "xPACKAGE_CUSTOM_DEPENDENCIES" != x Requires: PACKAGE_DEFAULT_DEPENDENCIES PACKAGE_CUSTOM_DEPENDENCIES %endif Building on OpenSUSE will give us these Requires: +%if "xglibc, libX11-6, libXau6, libXext6, libXi6, libXrender1, libXtst6, libasound2, libxcb1, libz1" != x || "x" != x +Requires: glibc, libX11-6, libXau6, libXext6, libXi6, libXrender1, libXtst6, libasound2, libxcb1, libz1 +%endif + +#avoid ARCH subfolder We override the RPM spec file with resource directory. I copied the RPM spec file content from the one jpackage generated and added something extra to it. Unfortunately this RPM will not install on Fedora, but installs fine on OpenSUSE where I built it. Installing on Fedora will spew out this line for all these Requires. nothing provides libX11-6 Are these Requires needed by Java? What is the consequences if these are not available on the system installing the RPM? It is an JavaFX application we are packaging. /Sverre

2 6

RFR: JDK-8241400: [macos] jpackageapplauncher/main.m built using CXXFLAGS_JDKEXE
by Andy Herrick 26 Mar '20

26 Mar '20

Please review the fix to issue [1] at [2]. Simple makefile fix. /Andy [1]: https://bugs.openjdk.java.net/browse/JDK-8241400 [2]: http://cr.openjdk.java.net/~herrick/8241400/webrev.01/

2 1

RFR 8237599 : Greedy matching against supplementary chars fails to respect the region
by Ivan Gerasimov 25 Mar '20

25 Mar '20

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

2 3

RFR: (T) 8241144 Javadoc is not generated for new module jdk.nio.mapmode
by Andrew Dinn 25 Mar '20

25 Mar '20

Could I please have a review for this trivial fix to the module make file which ensures that javadoc is generated for new module jdk.nio.mapmode created as part of the implementation of JEP 352. The original patch added the module to the BOOT_MODULES list but was not to the DOCS_MODULES list. JIRA: https://bugs.openjdk.java.net/browse/JDK-8241144 webrev: http://cr.openjdk.java.net/~adinn/8241144/webrev Testing: Built image and compiled + ran Hello World. Built make target docs-jdk-api-javadoc and checked module jdk.nio.mapmode was included in output regards, Andrew Dinn ----------- Senior Principal Software Engineer Red Hat UK Ltd Registered in England and Wales under Company Registration No. 03798903 Directors: Michael Cunningham, Michael ("Mike") O'Neill

5 7

[15] RFR: 8241311: Move some charset mapping tests from closed to open
by naoto.sato＠oracle.com 25 Mar '20

25 Mar '20

Hello, Please review the fix to the following issue: https://bugs.openjdk.java.net/browse/JDK-8241311 The proposed changeset is located at: https://cr.openjdk.java.net/~naoto/8241311/webrev.04/ This is simply to move some test cases that have been in closed repository into open repository. Naoto

4 11

RFR (XS/T) 8241445: Fix copyrights after JDK-8240629 change
by Aleksey Shipilev 24 Mar '20

24 Mar '20

This change in JDK-8240629 was not correct and slipped the review? https://hg.openjdk.java.net/jdk/jdk/rev/79371dab85c2#l2.5 Also, other file has not updated the copyright line. Fix: diff -r 35ff74547c64 src/java.base/share/native/libjli/args.c --- a/src/java.base/share/native/libjli/args.c Mon Mar 23 14:10:58 2020 +0100 +++ b/src/java.base/share/native/libjli/args.c Mon Mar 23 15:00:46 2020 +0100 @@ -1,4 +1,4 @@ /* - * Copyright (c) 2015, 2018, Oracle and/or its affiliates. All rights reserved. + * Copyright (c) 2015, 2020, Oracle and/or its affiliates. All rights reserved. * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. * diff -r 35ff74547c64 test/jdk/tools/launcher/ArgFileSyntax.java --- a/test/jdk/tools/launcher/ArgFileSyntax.java Mon Mar 23 14:10:58 2020 +0100 +++ b/test/jdk/tools/launcher/ArgFileSyntax.java Mon Mar 23 15:00:46 2020 +0100 @@ -1,4 +1,4 @@ /* - * Copyright (c) 2020, Oracle and/or its affiliates. All rights reserved. + * Copyright (c) 2015, 2020, Oracle and/or its affiliates. All rights reserved. * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. * Testing: none. -- Thanks, -Aleksey

2 4

RFR (XS) 8241462: StripNativeDebugSymbols jlink plugin allocates huge arrays
by Aleksey Shipilev 24 Mar '20

24 Mar '20

RFE: https://bugs.openjdk.java.net/browse/JDK-8241462 In some of my testing pipelines that deal with huge .so-s due to specific build configuration, tests that use the StripNativeDebugSymbols plugin fail with OOME. It can be fixed by using the method that copies the InputStream straight to destination file without instantiating the huge array. The only wrinkle is that we need to close the InputStream after we are done. diff -r c456587e7ef4 src/jdk.jlink/linux/classes/jdk/tools/jlink/internal/plugins/StripNativeDebugSymbolsPlugin.java --- a/src/jdk.jlink/linux/classes/jdk/tools/jlink/internal/plugins/StripNativeDebugSymbolsPlugin.java Mon Mar 23 19:14:01 2020 +0100 +++ b/src/jdk.jlink/linux/classes/jdk/tools/jlink/internal/plugins/StripNativeDebugSymbolsPlugin.java Mon Mar 23 20:13:15 2020 +0100 @@ -1,4 +1,4 @@ /* - * Copyright (c) 2019, Red Hat, Inc. + * Copyright (c) 2019, 2020, Red Hat, Inc. * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. * @@ -25,4 +25,5 @@ package jdk.tools.jlink.internal.plugins; +import java.io.InputStream; import java.io.IOException; import java.lang.ProcessBuilder.Redirect; @@ -314,5 +315,8 @@ String relativeDbgFileName = relativeFileName + "." + debugExt; - Files.write(resourceFileBinary, resource.contentBytes()); + try (InputStream in = resource.content()) { + Files.copy(in, resourceFileBinary); + } + Path resourceFileDebugSymbols; if (includeDebug) { Testing: tools/jlink, tools/jpackage, jdk-submit (running) -- Thanks, -Aleksey

3 4