Re: RFR: 8254102: use ProcessHandle::pid instead of ManagementFactory::getRuntimeMXBean to get pid in tests
On Tue, 6 Oct 2020 23:08:40 GMT, Igor Ignatyev wrote:
> Hi all,
>
> could you please review this small cleanup which replaces
> `ManagementFactory.getRuntimeMXBean().getName().split("@")[0]` w/
> `ProcessHandle.current().pid()` to get current
> process pid? Thanks,
> -- Igor
All of these changes can call `ProcessHandle.current().toString()` to return
pid of the current process.
test/failure_handler/test/sanity/Suicide.java line 36:
> 34: String osName = System.getProperty("os.name");
> 35: if (osName.contains("Windows")) {
> 36: cmd = "taskkill.exe /F /PID " + pidStr;
This can be simplified to ProcessHandle.current().toString(). It returns the
pid of the process as a string.
Explicitly converting it to a string is not necessary. The "+" concatenation
would convert the number to a string.
-
Changes requested by rriggs (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/534
Re: RFR: 8253757: Add LLVM-based backend for hsdis
On Fri, 2 Oct 2020 11:44:51 GMT, Magnus Ihse Bursie wrote: >> @navyxliu I've merged the sources into `src/utils/hsdis` and added support >> to build it in the Makefile. > > This is an interesting suggestion. There is a similar attempt at replacing > binutils with capstone in > https://bugs.openjdk.java.net/browse/JDK-8188073, which unfortunately has not > seen much progress due to lack of > resources; I don't know if you are aware of that? There is also a (extremely > low priority) effort to rewrite the hsdis > makefile to be part of the normal build system, see e.g. > https://bugs.openjdk.java.net/browse/JDK-8208495. Neither of > these should be any blocker for your change, but I think it might be good if > you know about them. I have couple of > concerns with your patch. One is the method in which LLVM is selected instead > of binutils; afaict this depends on > having the `LLVM` variable set when executing the makefile. At the very > least, this should be documented in the README. > I don't think any more complicated configuration is really necessary at this > point. With full integration with the > build system, a more user-friendly way of selecting hsdis backend should be > implemented, though. Second, and I don't > know if this is an artifact of git/github/the new skara tooling, but if you > renamed hsdis.c to hsdis.cpp, this > relationship does not show up, not even in the generated webrevs. Instead > they are considered a new + a deleted file. > This makes it hard to see what code changes you have done in that file. And > third; have you tested that your changes > (both changing the main file from C to C++, and any code changes in it) does > not break the old binutils functionality? > Afaic there are no test suites for exercising hsdis :-( so manual ad-hoc > testing is likely needed. Can you separate LLVM and binutils from hsdis.cpp? I guess you say that the problem is both GCC and binutils are not available on Windows AArch64. Is it right? 1 question: binutils seems to support Windows AArch64. Did you try recently binutils? If we can use binutils on Windows AArch64, you can fix makefile only. https://sourceware.org/git/?p=binutils-gdb.git;a=blob;f=binutils/dlltool.c;h=ed016b97dc38cdb1b85d2f6df676b9c9750f0d41;hb=HEAD#l248 - PR: https://git.openjdk.java.net/jdk/pull/392
RFR: 8254102: use ProcessHandle::pid instead of ManagementFactory::getRuntimeMXBean to get pid in tests
Hi all,
could you please review this small cleanup which replaces
`ManagementFactory.getRuntimeMXBean().getName().split("@")[0]` w/
`ProcessHandle.current().pid()` to get current
process pid?
Thanks,
-- Igor
-
Commit messages:
- update copyright
- use ProcessHandle::pid instead of ManagementFactory::getRuntimeMXBean to get
pid
Changes: https://git.openjdk.java.net/jdk/pull/534/files
Webrev: https://webrevs.openjdk.java.net/?repo=jdk=534=00
Issue: https://bugs.openjdk.java.net/browse/JDK-8254102
Stats: 55 lines in 8 files changed: 0 ins; 41 del; 14 mod
Patch: https://git.openjdk.java.net/jdk/pull/534.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/534/head:pull/534
PR: https://git.openjdk.java.net/jdk/pull/534
Re: RFR: 8247536: Support for pre-generated java.lang.invoke classes in CDS static archive [v12]
> This patch is reorganized after 8252725, which is separated from this patch > to refactor jlink glugin code. The previous > webrev with hg can be found at: > http://cr.openjdk.java.net/~minqi/2020/8247536/webrev-05. With 8252725 > integrated, the > regeneration of holder classes is simply to call the new added > GenerateJLIClassesHelper.cdsGenerateHolderClasses > function. Tests: tier1-4 Yumin Qi has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 23 commits: - Added new separate function to CDS for logging species and modified the existing function to log lambda form invokers. Changed isDumpLoadedClassList to a reasonable name isDumpingClassList as read only in CDS. - Merge branch 'master' of https://github.com/openjdk/jdk into jdk-8247536 - Removed unused imports. - Fixed comments with correct class and method name in CDS, removed unused variables after last change. - Moved and renamed cdsGenerateHolderClasses from GenerateJLIClassesHelp to CDS as generateLambdaFormHolderClasses. Added input verification function in CDS before class generation. Added more test scenarios. Removed trailing unused ending words for output of lambda form trace line in case of DumpLoadedClassList. - Move the check work to java, restore code in VM. Modified test code according to the changes. The invoke name verififcation is not implemented since not all the holder class are processed, not all the functions of processed holder classes are added. For holder class with DirectMethodHandle in its name, only the name in the DMH_METHOD_TYPE_MAP keyset is added, ithe line with other names just gets skipped silently. This makes the verification on invoke names difficul, a name not in the keyset should not fail the test. Also add a boolean to cdsGenerateHolderClasses to indicate call path. - Remove trailing word of line which is not used in holder class regeneration. There is a trailing LF (Line Feed) so trim white spaces from both front and end of the line or it will fail method type validation. - In case of exception happens during reloading class, CHECK will return without free the allocated buffer for class bytes so moved the buffer allocation and freeing to caller. Also removed test 6 since there is not guarantee that we can give a signature which will always fail. Additional changes to GenerateJLIClassesHelper according to review suggestion. - Merge branch 'master' of https://github.com/openjdk/jdk into jdk-8247536 - Merge branch 'master' of https://git.openjdk.java.net/jdk into jdk-8247536 - ... and 13 more: https://git.openjdk.java.net/jdk/compare/82fe023b...f5584dcf - Changes: https://git.openjdk.java.net/jdk/pull/193/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=193=11 Stats: 567 lines in 21 files changed: 545 ins; 14 del; 8 mod Patch: https://git.openjdk.java.net/jdk/pull/193.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/193/head:pull/193 PR: https://git.openjdk.java.net/jdk/pull/193
Re: RFR: 8247536: Support for pre-generated java.lang.invoke classes in CDS static archive [v11]
On Tue, 6 Oct 2020 17:35:18 GMT, Yumin Qi wrote:
>> This patch is reorganized after 8252725, which is separated from this patch
>> to refactor jlink glugin code. The previous
>> webrev with hg can be found at:
>> http://cr.openjdk.java.net/~minqi/2020/8247536/webrev-05. With 8252725
>> integrated, the
>> regeneration of holder classes is simply to call the new added
>> GenerateJLIClassesHelper.cdsGenerateHolderClasses
>> function. Tests: tier1-4
>
> Yumin Qi has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Removed unused imports.
src/java.base/share/classes/java/lang/invoke/GenerateJLIClassesHelper.java line
63:
> 61: if (TRACE_RESOLVE) {
> 62: System.out.println(traceLF + (resolvedMember != null ? "
> (success)" : " (fail)"));
> 63: }
I suggest not to change the existing code. Instead, have
`CDS::traceLambdaFormInvoker`
to take individual parameters `Class holder, String name, String
shortenSignature`
(rather than the formatted string). Something like:
if (CDS.isDumpLoadedClassList()) {
CDS.traceLambdaFormInvoker(holder, name,
shortenSignature(basicTypeSignature(type));
}
This also gives flexibility to CDS to decide on what format to write to the
class list (like this case, you drop the
text "success/fail")
In addition, the conditional check on `CDS.isDumpLoadedClassList()` is hard to
relate to why CDS traces these events.
I see Ioi's comment on this method name too. I agree with Ioi that
`isDumpingClassList` makes more sense.
src/java.base/share/classes/java/lang/invoke/GenerateJLIClassesHelper.java line
74:
> 72: System.out.println(traceSP + (salvage != null ? "
> (salvaged)" : " (generated)"));
> 73: }
> 74: CDS.traceLambdaFormInvoker(traceSP);
I suggest leaving the existing code unchanged. Instead, add the following:
if (CDS.isDumpingClassList()) {
CDS.traceSpeciesType(cn);
}
The above uses Ioi's suggested method name which reads better.
src/java.base/share/classes/jdk/internal/misc/CDS.java line 83:
> 81: * check if -XX:+DumpLoadedClassList and given file is open
> 82: */
> 83: public static boolean isDumpLoadedClassList() {
I agree with Ioi's suggestion to rename this to `isDumpingClassList` which
describes what the VM is doing.
-
PR: https://git.openjdk.java.net/jdk/pull/193
Re: RFR: 8247536: Support for pre-generated java.lang.invoke classes in CDS static archive [v11]
> This patch is reorganized after 8252725, which is separated from this patch > to refactor jlink glugin code. The previous > webrev with hg can be found at: > http://cr.openjdk.java.net/~minqi/2020/8247536/webrev-05. With 8252725 > integrated, the > regeneration of holder classes is simply to call the new added > GenerateJLIClassesHelper.cdsGenerateHolderClasses > function. Tests: tier1-4 Yumin Qi has updated the pull request incrementally with one additional commit since the last revision: Removed unused imports. - Changes: - all: https://git.openjdk.java.net/jdk/pull/193/files - new: https://git.openjdk.java.net/jdk/pull/193/files/686e211b..5d32a547 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=193=10 - incr: https://webrevs.openjdk.java.net/?repo=jdk=193=09-10 Stats: 2 lines in 1 file changed: 0 ins; 2 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/193.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/193/head:pull/193 PR: https://git.openjdk.java.net/jdk/pull/193
Re: RFR: 8247536: Support for pre-generated java.lang.invoke classes in CDS static archive [v10]
> This patch is reorganized after 8252725, which is separated from this patch > to refactor jlink glugin code. The previous > webrev with hg can be found at: > http://cr.openjdk.java.net/~minqi/2020/8247536/webrev-05. With 8252725 > integrated, the > regeneration of holder classes is simply to call the new added > GenerateJLIClassesHelper.cdsGenerateHolderClasses > function. Tests: tier1-4 Yumin Qi has updated the pull request incrementally with one additional commit since the last revision: Fixed comments with correct class and method name in CDS, removed unused variables after last change. - Changes: - all: https://git.openjdk.java.net/jdk/pull/193/files - new: https://git.openjdk.java.net/jdk/pull/193/files/52764a6e..686e211b Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=193=09 - incr: https://webrevs.openjdk.java.net/?repo=jdk=193=08-09 Stats: 4 lines in 2 files changed: 0 ins; 3 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/193.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/193/head:pull/193 PR: https://git.openjdk.java.net/jdk/pull/193
Integrated: 8254054: Pre-submit testing using GitHub Actions should not use the deprecated set-env command
On Tue, 6 Oct 2020 06:39:55 GMT, Robin Westberg wrote: > The `set-env` command was recently deprecated in favor of a different method > of setting environment variables, due to a > security vulnerability [1]. The vulnerability does not affect our usage of > GitHub Actions, but we should nevertheless > update to avoid the associated deprecation warnings. [1] > https://github.blog/changelog/2020-10-01-github-actions-deprecating-set-env-and-add-path-commands/ This pull request has now been integrated. Changeset: d2b1dc6d Author:Robin Westberg URL: https://git.openjdk.java.net/jdk/commit/d2b1dc6d Stats: 53 lines in 1 file changed: 1 ins; 21 del; 31 mod 8254054: Pre-submit testing using GitHub Actions should not use the deprecated set-env command Reviewed-by: ehelin, erikj - PR: https://git.openjdk.java.net/jdk/pull/518
Re: RFR: 8254054: Pre-submit testing using GitHub Actions should not use the deprecated set-env command
On Tue, 6 Oct 2020 06:39:55 GMT, Robin Westberg wrote: > The `set-env` command was recently deprecated in favor of a different method > of setting environment variables, due to a > security vulnerability [1]. The vulnerability does not affect our usage of > GitHub Actions, but we should nevertheless > update to avoid the associated deprecation warnings. [1] > https://github.blog/changelog/2020-10-01-github-actions-deprecating-set-env-and-add-path-commands/ Marked as reviewed by erikj (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/518
Re: RFR: 8245194: Unix domain socket channel implementation [v16]
> Continuing this review as a PR on github with the comments below > incorporated. I expect there will be a few more > iterations before integrating. > On 06/09/2020 19:47, Alan Bateman wrote: >> On 26/08/2020 15:24, Michael McMahon wrote: >>> >>> As I mentioned the other day, I wasn't able to use the suggested method on >>> Windows which returns an absolute path. So, >>> I added a method to WindowsPath which returns the path in the expected >>> UTF-8 encoding as a byte[]. Let me know what you >>> think of that. >>> >>> There is a fair bit of other refactoring and simplification done also. Next >>> version is at: >>> >>> http://cr.openjdk.java.net/~michaelm/8245194/impl.webrev/webrev.9/ >>> >> Adding a method to WindowsPath to encode the path using UTF-8 is okay but I >> don't think we should be caching it as the >> encoding for sun_path is an outlier on Windows. I'm also a bit dubious about >> encoding a relative path when the resolved >> path (before encoding) is > 247 chars. The documentation on the MS site >> isn't very completely and I think there are a >> number points that require clarification to fully understand how this will >> work with relative, directly relative and >> drive relative paths. >> > > Maybe it would be better to just use the path returned from toString() and do > the conversion to UTF-8 externally. That > would leave WindowsPath unchanged. >> In the same area, the new PathUtil is a bit inconsistent with the existing >> provider code. One suggestion is to add a >> method to AbstractFileSystemProvider instead. That is the base class the >> platform providers and would be a better place >> to get the file path in bytes. >> > > Okay, I gave the method a name that is specific to Unix domain sockets > because this UTF-8 strangeness is not likely to > be useful by other components. >> One other comment on the changes to the file system provider it should be >> okay to change UnixUserPrinipals ad its >> fromUid and fromGid method to be public. This would mean that the patch >> shouldn't need to add UnixUserGroup (the main >> issue is that class is that it means we end up with two classes with static >> factory methods doing the same thing). > > Okay, that does simplify it a bit. > > Thanks, > Michael. > >> -Alan. >> >> >> >> >> >> Michael McMahon has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains 18 additional commits since the last revision: - Merge branch 'master' into unixdomainchannels - - update after Alan's review on Oct 4 - includes API change required by JDK-8251952 - original CSR for the overall change will be resubmitted with all api changes consolidated based on this update - - simplified Copy.gmk to CAT source files directly - renamed net.properties source files to all be net.properties - unixdomainchannels: error in the last commit in make/modules/java.base/Copy.gmk - unixdomainchannels: (1) rename UnixDomainHelper to UnixDomainSocketsUtil (2) remove hardcoded /tmp and /var/tmp paths from UnixDomainSocketsUtil (3) replace (2) with documented system/networking properties (4) Small update to UnixDomainSocketAddress API (5) CSR for (3) and (4) submitted at JDK-8253930 (6) Update build to generate net.properties from shared net.properties.common plus platform specific additions. - Merge branch 'master' into unixdomainchannels - unixdomainchannels: remove more cruft from Windows Net.c - unixdomainchannels: change to Net.c was missed after all - unixdomainchannels: typo in WindowsFileSystemProvider.java - unixdomainchannels: Update after Alan's review of Sept 29 - ... and 8 more: https://git.openjdk.java.net/jdk/compare/96b742dd...36efb46c - Changes: - all: https://git.openjdk.java.net/jdk/pull/52/files - new: https://git.openjdk.java.net/jdk/pull/52/files/17acf10a..36efb46c Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=52=15 - incr: https://webrevs.openjdk.java.net/?repo=jdk=52=14-15 Stats: 15436 lines in 1715 files changed: 5751 ins; 2807 del; 6878 mod Patch: https://git.openjdk.java.net/jdk/pull/52.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/52/head:pull/52 PR: https://git.openjdk.java.net/jdk/pull/52
Re: RFR: 8245194: Unix domain socket channel implementation [v15]
> Continuing this review as a PR on github with the comments below > incorporated. I expect there will be a few more > iterations before integrating. > On 06/09/2020 19:47, Alan Bateman wrote: >> On 26/08/2020 15:24, Michael McMahon wrote: >>> >>> As I mentioned the other day, I wasn't able to use the suggested method on >>> Windows which returns an absolute path. So, >>> I added a method to WindowsPath which returns the path in the expected >>> UTF-8 encoding as a byte[]. Let me know what you >>> think of that. >>> >>> There is a fair bit of other refactoring and simplification done also. Next >>> version is at: >>> >>> http://cr.openjdk.java.net/~michaelm/8245194/impl.webrev/webrev.9/ >>> >> Adding a method to WindowsPath to encode the path using UTF-8 is okay but I >> don't think we should be caching it as the >> encoding for sun_path is an outlier on Windows. I'm also a bit dubious about >> encoding a relative path when the resolved >> path (before encoding) is > 247 chars. The documentation on the MS site >> isn't very completely and I think there are a >> number points that require clarification to fully understand how this will >> work with relative, directly relative and >> drive relative paths. >> > > Maybe it would be better to just use the path returned from toString() and do > the conversion to UTF-8 externally. That > would leave WindowsPath unchanged. >> In the same area, the new PathUtil is a bit inconsistent with the existing >> provider code. One suggestion is to add a >> method to AbstractFileSystemProvider instead. That is the base class the >> platform providers and would be a better place >> to get the file path in bytes. >> > > Okay, I gave the method a name that is specific to Unix domain sockets > because this UTF-8 strangeness is not likely to > be useful by other components. >> One other comment on the changes to the file system provider it should be >> okay to change UnixUserPrinipals ad its >> fromUid and fromGid method to be public. This would mean that the patch >> shouldn't need to add UnixUserGroup (the main >> issue is that class is that it means we end up with two classes with static >> factory methods doing the same thing). > > Okay, that does simplify it a bit. > > Thanks, > Michael. > >> -Alan. >> >> >> >> >> >> Michael McMahon has updated the pull request incrementally with one additional commit since the last revision: - update after Alan's review on Oct 4 - includes API change required by JDK-8251952 - original CSR for the overall change will be resubmitted with all api changes consolidated based on this update - Changes: - all: https://git.openjdk.java.net/jdk/pull/52/files - new: https://git.openjdk.java.net/jdk/pull/52/files/0096645e..17acf10a Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=52=14 - incr: https://webrevs.openjdk.java.net/?repo=jdk=52=13-14 Stats: 327 lines in 35 files changed: 108 ins; 125 del; 94 mod Patch: https://git.openjdk.java.net/jdk/pull/52.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/52/head:pull/52 PR: https://git.openjdk.java.net/jdk/pull/52
Re: RFR: 8254054: Pre-submit testing using GitHub Actions should not use the deprecated set-env command
On Tue, 6 Oct 2020 06:39:55 GMT, Robin Westberg wrote: > The `set-env` command was recently deprecated in favor of a different method > of setting environment variables, due to a > security vulnerability [1]. The vulnerability does not affect our usage of > GitHub Actions, but we should nevertheless > update to avoid the associated deprecation warnings. [1] > https://github.blog/changelog/2020-10-01-github-actions-deprecating-set-env-and-add-path-commands/ Looks good, thanks for fixing! - Marked as reviewed by ehelin (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/518
RFR: 8254054: Pre-submit testing using GitHub Actions should not use the deprecated set-env command
The `set-env` command was recently deprecated in favor of a different method of setting environment variables, due to a security vulnerability [1]. The vulnerability does not affect our usage of GitHub Actions, but we should nevertheless update to avoid the associated deprecation warnings. [1] https://github.blog/changelog/2020-10-01-github-actions-deprecating-set-env-and-add-path-commands/ - Commit messages: - Remove usage of deprecated ::set-env Changes: https://git.openjdk.java.net/jdk/pull/518/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=518=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8254054 Stats: 53 lines in 1 file changed: 1 ins; 21 del; 31 mod Patch: https://git.openjdk.java.net/jdk/pull/518.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/518/head:pull/518 PR: https://git.openjdk.java.net/jdk/pull/518
