Re: RFR: 8287200: Test java/lang/management/ThreadMXBean/VirtualThreadDeadlocks.java timed out after JDK-8287103
On Tue, 24 May 2022 19:52:57 GMT, Leonid Mesnik wrote: > Need to use proper synchronization. > > The CyclicBarriers might move the thread to WAITING state but not BLOCKED. So > it should not confuse existing checks. Yes I think that's nice - the two competing Threads own their first lock and wait until the other thread has owned its first lock, before continuing. 8-) - Marked as reviewed by kevinw (Committer). PR: https://git.openjdk.java.net/jdk/pull/8874
Re: RFR: 8287103: java/lang/management/ThreadMXBean/VirtualThreadDeadlocks.java fails with Xcomp [v2]
On Sat, 21 May 2022 16:34:32 GMT, Leonid Mesnik wrote: >> Sync improved in test > > Leonid Mesnik has updated the pull request incrementally with one additional > commit since the last revision: > > fix Looks good! - Marked as reviewed by kevinw (Committer). PR: https://git.openjdk.java.net/jdk/pull/8821
Re: RFR: 8284191: Replace usages of 'a the' in hotspot and java.base
On Wed, 18 May 2022 13:27:24 GMT, Alexey Ivanov wrote: > Replaces usages of articles that follow each other in all combinations: > a/the, an?/an?, the/the… > > Also, I fixed a couple of spelling mistakes. OK. I started with serviceability but then went through everything as it's hard to record what you have/haven't seen in these long reviews. test/jdk/java/net/InterfaceAddress/Equals.java line 81: > 79: /** > 80: * Returns an InterfaceAddress instance with its fields set the values > 81: * specificed. probably a typo for "set to the values specified." - Marked as reviewed by kevinw (Committer). PR: https://git.openjdk.java.net/jdk/pull/8768
Re: RFR: 8284191: Replace usages of 'a the' in hotspot and java.base
On Wed, 18 May 2022 13:27:24 GMT, Alexey Ivanov wrote: > Replaces usages of articles that follow each other in all combinations: > a/the, an?/an?, the/the… > > Also, I fixed a couple of spelling mistakes. test/jdk/jdk/nio/zipfs/TestLocOffsetFromZip64EF.java line 84: > 82: > 83: /** > 84: * Create a Zip file that will result in an Zip64 Extra (EXT) header "result in a Zip64..." test/jdk/sun/security/tools/jarsigner/OldSig.java line 32: > 30: /* > 31: * See also PreserveRawManifestEntryAndDigest.java for tests with > arbitrarily > 32: * formatted individual sections in addition the main attributes tested "in addition to the..." - PR: https://git.openjdk.java.net/jdk/pull/8768
Re: RFR: 8284191: Replace usages of 'a the' in hotspot and java.base
On Wed, 18 May 2022 13:27:24 GMT, Alexey Ivanov wrote: > Replaces usages of articles that follow each other in all combinations: > a/the, an?/an?, the/the… > > Also, I fixed a couple of spelling mistakes. src/hotspot/share/cds/filemap.cpp line 1914: > 1912: > 1913: // the current value of the pointers to be patched must be within > this > 1914: // range (i.e., must be between the requested base address, and the > of the current archive). "the of the" ? Maybe "..and the address of the current archive", or maybe it was a typo for "and that of the current archive". - PR: https://git.openjdk.java.net/jdk/pull/8768
Re: RFR: 8284191: Replace usages of 'a the' in hotspot and java.base
On Wed, 18 May 2022 13:27:24 GMT, Alexey Ivanov wrote: > Replaces usages of articles that follow each other in all combinations: > a/the, an?/an?, the/the… > > Also, I fixed a couple of spelling mistakes. src/hotspot/share/interpreter/bytecodeUtils.cpp line 186: > 184: static const int _max_cause_detail = 5; > 185: > 186: // Merges the stack the given bci with the given stack. If there "...the stack at the given bci..." - PR: https://git.openjdk.java.net/jdk/pull/8768
Re: RFR: 8284191: Replace usages of 'a the' in hotspot and java.base
On Wed, 18 May 2022 13:27:24 GMT, Alexey Ivanov wrote: > Replaces usages of articles that follow each other in all combinations: > a/the, an?/an?, the/the… > > Also, I fixed a couple of spelling mistakes. src/hotspot/share/opto/graphKit.cpp line 3626: > 3624: // The optional arguments are for specialized use by intrinsics: > 3625: // - If 'extra_slow_test' if not null is an extra condition for the > slow-path. > 3626: // - If 'return_size_val', report the total object size to the caller. "reports the total" - PR: https://git.openjdk.java.net/jdk/pull/8768
Re: RFR: 8284191: Replace usages of 'a the' in hotspot and java.base
On Wed, 18 May 2022 13:27:24 GMT, Alexey Ivanov wrote:
> Replaces usages of articles that follow each other in all combinations:
> a/the, an?/an?, the/the…
>
> Also, I fixed a couple of spelling mistakes.
src/jdk.jdi/share/classes/com/sun/jdi/ClassType.java line 348:
> 346:
> 347: /**
> 348: * Returns the single non-abstract {@link Method} visible from
I would think "Returns a single ..." because the implementation returns the
first match it finds, from possibly many.
-
PR: https://git.openjdk.java.net/jdk/pull/8768
Re: RFR: 8284191: Replace usages of 'a the' in hotspot and java.base
On Wed, 18 May 2022 13:27:24 GMT, Alexey Ivanov wrote: > Replaces usages of articles that follow each other in all combinations: > a/the, an?/an?, the/the… > > Also, I fixed a couple of spelling mistakes. src/jdk.sctp/share/classes/com/sun/nio/sctp/ShutdownNotification.java line 28: > 26: > 27: /** > 28: * Notification emitted when a peers shutdowns the association. Maybe: "...when a peer shuts down an association." (could be "shuts down the association" if there is only ever one, but using "an" looks safe) - PR: https://git.openjdk.java.net/jdk/pull/8768
Re: RFR: 8281268: Resolve duplication of test ClassTransformer class
On Fri, 13 May 2022 22:50:11 GMT, Alex Menkov wrote: > The fix deletes ClassTransformer class from jdi, switches all the test to > jdk.test.lib copy. Looks good. Those are the only imports I can see for this, and the ClassTransformer.java text is the same in both places. - Marked as reviewed by kevinw (Committer). PR: https://git.openjdk.java.net/jdk/pull/8710
Integrated: 8284331: Add sanity check for signal handler modification warning.
On Tue, 5 Apr 2022 10:37:26 GMT, Kevin Walls wrote: > A sanity check using "jcmd VM.info" to catch the signal handler modification > warning: it should never trigger during this test. This pull request has now been integrated. Changeset: 116763cb Author:Kevin Walls URL: https://git.openjdk.java.net/jdk/commit/116763cb5d58a7316b7bada689a0fa34a7250ee7 Stats: 20 lines in 1 file changed: 19 ins; 0 del; 1 mod 8284331: Add sanity check for signal handler modification warning. Reviewed-by: dholmes, amenkov - PR: https://git.openjdk.java.net/jdk/pull/8106
Re: RFR: 8284331: Add sanity check for signal handler modification warning.
On Tue, 5 Apr 2022 10:37:26 GMT, Kevin Walls wrote: > A sanity check using "jcmd VM.info" to catch the signal handler modification > warning: it should never trigger during this test. thanks David and Alex and Chris 8-) - PR: https://git.openjdk.java.net/jdk/pull/8106
Re: RFR: 8284331: Add sanity check for signal handler modification warning.
On Fri, 29 Apr 2022 03:15:09 GMT, Chris Plummer wrote: > How does this relate the failure in JDK-8285647? Is this just meant to detect > that failure, but a proper fix is still needed for it? ..it's not directly related - I had this test addition in progress already, as an addition to JDK-8283337 which fixes the modification handler being broken by a previous change (the warning was firing all the time, after JDK-8279124 and before JDK-8283337). So we should have a sanity check (this PR) which fails if the warning starts firing unnecessarily. Logged JDK-8285792 to signal that we need some more cleanup here, including the news that I think JDK-8285647 shows that we don't consistently ignore the crash_handler. - PR: https://git.openjdk.java.net/jdk/pull/8106
Re: RFR: 8284161: Implementation of Virtual Threads (Preview) [v8]
On Wed, 27 Apr 2022 14:24:20 GMT, Alan Bateman wrote: >> This is the implementation of JEP 425: Virtual Threads (Preview); TBD which >> JDK version to target. >> >> We will refresh this PR periodically to pick up changes and fixes from the >> loom repo. >> >> Most of the new mechanisms in the HotSpot VM are disabled by default and >> require running with `--enable-preview` to enable. >> >> The patch has support for x64 and aarch64 on the usual operating systems >> (Linux, macOS, and Windows). There are stubs (calling Unimplemented) for >> zero and some of the other ports. Additional ports can be contributed via >> PRs against the fibers branch in the loom repo. >> >> There are changes in many areas. To reduce notifications/mails, the labels >> have been trimmed down for now to hotspot, serviceability and core-libs. >> We'll add the complete set of labels when the PR is further along. >> >> The changes include a refresh of java.util.concurrent and ForkJoinPool from >> Doug Lea's CVS. These changes will probably be proposed and integrated in >> advance of this PR. >> >> The changes include some non-exposed and low-level infrastructure to support >> the (in draft) JEPs for Structured Concurrency and Scope Locals. This is to >> make life a bit easier and avoid having to separate VM changes and juggle >> branches at this time. > > Alan Bateman has updated the pull request incrementally with one additional > commit since the last revision: > > Refresh 7965cc6b168e567ac2596f2fbc3b00a7d99b7e1e Been through the JDI changes and jdb/example code. Built and manually tested some operations with jdb observing virtual threads. Been through jdk.management/share/classes/com/sun/management and also manually tested jconsole attaching (e.g. dumpThreads operation in both TEXT_PLAIN and JSON). - Marked as reviewed by kevinw (Committer). PR: https://git.openjdk.java.net/jdk/pull/8166
Re: RFR: 8285366: Fix typos in serviceability
On Thu, 21 Apr 2022 17:22:04 GMT, Magnus Ihse Bursie wrote: >> src/jdk.jdwp.agent/share/native/libjdwp/invoker.h line 38: >> >>> 36: jboolean pending; /* Is an invoke requested? */ >>> 37: jboolean started; /* Is an invoke happening? */ >>> 38: jboolean available;/* Is the thread in an invocable state? */ >> >> invocable could perhaps stay as invokable ? >> Elsewhere we have a filename com/sun/tools/jdi/InvokableTypeImpl.java which >> we clearly don't want to change, and I see Internet dictionary evidence of >> invokable being acceptable. > > But on the other hand we have `javax.script.Invocable`. :-) > > Codespell suggested this change, and I based my decision to keep it based on > [Merriam-Webster](https://www.merriam-webster.com/dictionary/invocable) not > even listing "invokable" as an alternate spelling, and that "invocable" has > about 3x the number of Google hits than "invokable". > > But sure, there is perhaps reason to consider "invokable" an acceptable > alternative and keep it. I guess it depends on if you consider the word to be > based on "invoke" with a suffix, or a latinized form, like "invocation". > > I'll wait a while for others to chime in, otherwise I'll revert the > "invokable" changes. Sure, I just thought there was enough evidence that invokable is not definitely wrong, even if invocable is more correct and popular, so it's not obvious it should be changed. Don't lose sleep over it. 8-) More importantly, on the tests -- I see the changes in exception messages searched for the wrong text in the test directories, and didn't find any matches that looked like checks. - PR: https://git.openjdk.java.net/jdk/pull/8334
Re: RFR: 8285366: Fix typos in serviceability
On Thu, 21 Apr 2022 11:22:48 GMT, Magnus Ihse Bursie wrote: > I ran `codespell` on modules owned by the serviceability team > (`java.instrument java.management.rmi java.management jdk.attach > jdk.hotspot.agent jdk.internal.jvmstat jdk.jcmd jdk.jconsole jdk.jdi > jdk.jdwp.agent jdk.jstatd jdk.management.agent jdk.management`), and accepted > those changes where it indeed discovered real typos. > > > I will update copyright years using a script before pushing (otherwise like > every second change would be a copyright update, making reviewing much > harder). > > The long term goal here is to make tooling support for running `codespell`. > The trouble with automating this is of course all false positives. But before > even trying to solve that issue, all true positives must be fixed. Hence this > PR. All looks good to me, just the invokable which you might want to leave as is, unless there are other strong feelings. 8-) src/jdk.jdwp.agent/share/native/libjdwp/invoker.h line 38: > 36: jboolean pending; /* Is an invoke requested? */ > 37: jboolean started; /* Is an invoke happening? */ > 38: jboolean available;/* Is the thread in an invocable state? */ invocable could perhaps stay as invokable ? Elsewhere we have a filename com/sun/tools/jdi/InvokableTypeImpl.java which we clearly don't want to change, and I see Internet dictionary evidence of invokable being acceptable. - Marked as reviewed by kevinw (Committer). PR: https://git.openjdk.java.net/jdk/pull/8334
Re: RFR: 8284853: Fix various 'expected' typo [v2]
On Thu, 14 Apr 2022 18:04:16 GMT, Andrey Turbanov wrote: > I found [yet another > typo](https://github.com/kelthuzadx/jdk/commit/acb9e15bc0bf5395d1c0875f36992f692734f948) > ... I didn't think "JVMInvokeMethodSlack" was a typo. I think it's the idea of "slack space" meaning leftover space. We require a certain amount of this space. We need some slack on the stack, in order to invoke. - PR: https://git.openjdk.java.net/jdk/pull/8231
Re: RFR: 8284330: jcmd may not be able to find processes in the container [v2]
On Wed, 6 Apr 2022 12:44:35 GMT, Yasumasa Suenaga wrote: >> jcmd uses >> src/jdk.internal.jvmstat/linux/classes/sun/jvmstat/PlatformSupportImpl.java >> to scan temporary directories to find out processes in the container. It >> checks inode to ensure the temp directory is not conflicted. However inode >> maybe same value between the container and others. Thus we should check >> device id for that case. >> >> For example I saw following case on [distroless >> cc-debian11](https://github.com/GoogleContainerTools/distroless/blob/main/cc/README.md) >> container. I started rescue:jdk19 container with sharing PID namespace. >> `/proc/1/root/tmp` is different from `/tmp` on rescue:jdk19, but they are >> same inode value. However we can see the differense in device id. >> >> >> $ podman run -it --rm --entrypoint=sh --pid=container:fa39662f7352 >> rescue:jdk19 >> / # >> / # stat /tmp >> File: /tmp >> Size: 29 Blocks: 0 IO Block: 4096 directory >> Device: efh/239dInode: 135674931 Links: 1 >> Access: (1777/drwxrwxrwt) Uid: (0/root) Gid: (0/root) >> Access: 2022-04-05 08:51:37.0 >> Modify: 2022-04-05 08:51:37.0 >> Change: 2022-04-05 08:51:37.0 >> >> / # stat /proc/1/root/tmp >> File: /proc/1/root/tmp >> Size: 29 Blocks: 0 IO Block: 4096 directory >> Device: e1h/225dInode: 135674931 Links: 1 >> Access: (1777/drwxrwxrwt) Uid: (0/root) Gid: (0/root) >> Access: 2022-04-05 08:51:37.0 >> Modify: 2022-04-05 08:50:42.0 >> Change: 2022-04-05 08:50:42.0 > > Yasumasa Suenaga has updated the pull request incrementally with one > additional commit since the last revision: > > Fix comments Marked as reviewed by kevinw (Committer). Great, thanks. - PR: https://git.openjdk.java.net/jdk/pull/8103
Re: RFR: 8284330: jcmd may not be able to find processes in the container
On Tue, 5 Apr 2022 09:04:56 GMT, Yasumasa Suenaga wrote: > jcmd uses > src/jdk.internal.jvmstat/linux/classes/sun/jvmstat/PlatformSupportImpl.java > to scan temporary directories to find out processes in the container. It > checks inode to ensure the temp directory is not conflicted. However inode > maybe same value between the container and others. Thus we should check > device id for that case. > > For example I saw following case on [distroless > cc-debian11](https://github.com/GoogleContainerTools/distroless/blob/main/cc/README.md) > container. I started rescue:jdk19 container with sharing PID namespace. > `/proc/1/root/tmp` is different from `/tmp` on rescue:jdk19, but they are > same inode value. However we can see the differense in device id. > > > $ podman run -it --rm --entrypoint=sh --pid=container:fa39662f7352 > rescue:jdk19 > / # > / # stat /tmp > File: /tmp > Size: 29 Blocks: 0 IO Block: 4096 directory > Device: efh/239dInode: 135674931 Links: 1 > Access: (1777/drwxrwxrwt) Uid: (0/root) Gid: (0/root) > Access: 2022-04-05 08:51:37.0 > Modify: 2022-04-05 08:51:37.0 > Change: 2022-04-05 08:51:37.0 > > / # stat /proc/1/root/tmp > File: /proc/1/root/tmp > Size: 29 Blocks: 0 IO Block: 4096 directory > Device: e1h/225dInode: 135674931 Links: 1 > Access: (1777/drwxrwxrwt) Uid: (0/root) Gid: (0/root) > Access: 2022-04-05 08:51:37.0 > Modify: 2022-04-05 08:50:42.0 > Change: 2022-04-05 08:50:42.0 Hi, So it's that the /tmp inode can be the same, between the host and a container. You might like some text for the bug description as that could be clearer: --- jcmd uses src/jdk.internal.jvmstat/linux/classes/sun/jvmstat/PlatformSupportImpl.java to scan for all temporary directories: the host temp dir, and (through the /proc filesystem) the temp dirs of running containers. It checks the inode value to ensure a temp directory under /proc is not the same as the host temp directory. However the inode can have the same value in the container and host. Thus we should check device id additionally. --- I think the changes look good. VM's temp directory (from os::get_temp_directory()) is a constant, i.e. /tmp, in current implementations, so keeping the inode looks safe. Just being picky with the English, "isSameWithTemporaryDirectory" is odd (we would say something is "the same as" rather than "with"). It could be called "tempDirectoryEquals" ? 8-) Maybe the comment could also be clearer: 116 * Host and container devices could have the same inode value, 117 * so we also need to check the device id. - PR: https://git.openjdk.java.net/jdk/pull/8103
Re: RFR: 8284331: Add sanity check for signal handler modification warning.
On Tue, 5 Apr 2022 10:37:26 GMT, Kevin Walls wrote: > A sanity check using "jcmd VM.info" to catch the signal handler modification > warning: it should never trigger during this test. (adding a note to trigger email notification, as that appears lost...) - PR: https://git.openjdk.java.net/jdk/pull/8106
Re: RFR: 8283903: GetContainerCpuLoad does not return the correct result in share mode [v4]
On Wed, 30 Mar 2022 11:02:18 GMT, xpbob wrote: >> ``` >>long hostTicks = getHostTotalCpuTicks0(); >> int totalCPUs = getHostOnlineCpuCount0(); >> int containerCPUs = getAvailableProcessors(); >> // scale the total host load to the actual container cpus >> hostTicks = hostTicks * containerCPUs / totalCPUs; >> >> hostTicks=17547615556000 >> totalCPUs=96 >> containerCPUs=90 >> >> Calculate the overflow > > xpbob has updated the pull request incrementally with one additional commit > since the last revision: > > Change the expression Marked as reviewed by kevinw (Committer). OK thanks for the description update, and I think the updated code is better also. I guess the app has not been running with the fix long enough to test it fully yet? But if it's looking good so far and reporting good information, and any other tests are unaffected, then looks good. - PR: https://git.openjdk.java.net/jdk/pull/8028
Re: RFR: 8283903: GetContainerCpuLoad does not return the correct result in share mode [v3]
On Wed, 30 Mar 2022 06:20:19 GMT, xpbob wrote: >> ``` >>long hostTicks = getHostTotalCpuTicks0(); >> int totalCPUs = getHostOnlineCpuCount0(); >> int containerCPUs = getAvailableProcessors(); >> // scale the total host load to the actual container cpus >> hostTicks = hostTicks * containerCPUs / totalCPUs; >> >> hostTicks=17547615556000 >> totalCPUs=96 >> containerCPUs=90 >> >> Calculate the overflow > > xpbob has updated the pull request incrementally with one additional commit > since the last revision: > > Change the expression Hi, Could the bug description be updated to state what the problem is, and under what circumstances is there a problem? - PR: https://git.openjdk.java.net/jdk/pull/8028
Re: RFR: 8283254: Remove redundant class jdk/internal/agent/spi/AgentProvider
On Tue, 22 Mar 2022 21:19:20 GMT, Kevin Walls wrote: > There are no uses of jdk/internal/agent/spi/AgentProvider, since the SNMP > agent was removed ( 8071367 ): this class should be removed. It is not a > public interface. > > Remove > src/jdk.management.agent/share/classes/jdk/internal/agent/spi/AgentProvider.java > Remove import from > src/jdk.management.agent/share/classes/jdk/internal/agent/Agent.java > Remove uses from src/jdk.management.agent/share/classes/module-info.java > > Testing with the test/jdk/sun/management tests looks good. Thanks for the reviews! - PR: https://git.openjdk.java.net/jdk/pull/7912
Integrated: 8283254: Remove redundant class jdk/internal/agent/spi/AgentProvider
On Tue, 22 Mar 2022 21:19:20 GMT, Kevin Walls wrote: > There are no uses of jdk/internal/agent/spi/AgentProvider, since the SNMP > agent was removed ( 8071367 ): this class should be removed. It is not a > public interface. > > Remove > src/jdk.management.agent/share/classes/jdk/internal/agent/spi/AgentProvider.java > Remove import from > src/jdk.management.agent/share/classes/jdk/internal/agent/Agent.java > Remove uses from src/jdk.management.agent/share/classes/module-info.java > > Testing with the test/jdk/sun/management tests looks good. This pull request has now been integrated. Changeset: 61d7d868 Author:Kevin Walls URL: https://git.openjdk.java.net/jdk/commit/61d7d868db030d878f4a1c4467075e8d4e116a6e Stats: 80 lines in 3 files changed: 0 ins; 79 del; 1 mod 8283254: Remove redundant class jdk/internal/agent/spi/AgentProvider Reviewed-by: mchung, redestad, dfuchs - PR: https://git.openjdk.java.net/jdk/pull/7912
RFR: 8283254: Remove redundant class jdk/internal/agent/spi/AgentProvider
There are no uses of jdk/internal/agent/spi/AgentProvider, since the SNMP agent was removed ( 8071367 ): this class should be removed. It is not a public interface. Remove src/jdk.management.agent/share/classes/jdk/internal/agent/spi/AgentProvider.java Remove import from src/jdk.management.agent/share/classes/jdk/internal/agent/Agent.java Remove uses from src/jdk.management.agent/share/classes/module-info.java Testing with the test/jdk/sun/management tests looks good. - Commit messages: - 8283254: Remove redundant class jdk/internal/agent/spi/AgentProvider Changes: https://git.openjdk.java.net/jdk/pull/7912/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=7912=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8283254 Stats: 80 lines in 3 files changed: 0 ins; 79 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/7912.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7912/head:pull/7912 PR: https://git.openjdk.java.net/jdk/pull/7912
Integrated: 8283092: JMX subclass permission check redundant with strong encapsulation
On Tue, 15 Mar 2022 20:22:16 GMT, Kevin Walls wrote: > Removing permission checks which, in the presence of a Security Manager, > would check for a RuntimePermission "className.subclass". This was to > prevent subclassing these classes, but is no longer necessary with strong > encapsulation from modules. This pull request has now been integrated. Changeset: 37fc77ef Author:Kevin Walls URL: https://git.openjdk.java.net/jdk/commit/37fc77ef60dd97c4acc468ecfeb6753132974720 Stats: 108 lines in 4 files changed: 28 ins; 45 del; 35 mod 8283092: JMX subclass permission check redundant with strong encapsulation Reviewed-by: dfuchs, mchung - PR: https://git.openjdk.java.net/jdk/pull/7827
Re: RFR: 8283092: JMX subclass permission check redundant with strong encapsulation [v3]
On Mon, 21 Mar 2022 20:19:03 GMT, Kevin Walls wrote: >> Removing permission checks which, in the presence of a Security Manager, >> would check for a RuntimePermission "className.subclass". This was to >> prevent subclassing these classes, but is no longer necessary with strong >> encapsulation from modules. > > Kevin Walls has updated the pull request incrementally with one additional > commit since the last revision: > > remove redundant constructors Thanks all for the comments and reviews! - PR: https://git.openjdk.java.net/jdk/pull/7827
Re: RFR: 8283092: JMX subclass permission check redundant with strong encapsulation [v2]
On Mon, 21 Mar 2022 18:40:53 GMT, Mandy Chung wrote:
>> Kevin Walls has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Test update
>
> src/java.management/share/classes/sun/management/spi/PlatformMBeanProvider.java
> line 210:
>
>> 208: }
>> 209:
>> 210: private PlatformMBeanProvider(Void unused) {
>
> This `PlatformMBeanProvider(Void)` constructor is no longer needed. Same
> for `AgentProvider(Void)` constructor.
Thanks, yes they can go... Rebuilt and tested ok after the new commit.
-
PR: https://git.openjdk.java.net/jdk/pull/7827
Re: RFR: 8283092: JMX subclass permission check redundant with strong encapsulation [v3]
> Removing permission checks which, in the presence of a Security Manager, > would check for a RuntimePermission "className.subclass". This was to > prevent subclassing these classes, but is no longer necessary with strong > encapsulation from modules. Kevin Walls has updated the pull request incrementally with one additional commit since the last revision: remove redundant constructors - Changes: - all: https://git.openjdk.java.net/jdk/pull/7827/files - new: https://git.openjdk.java.net/jdk/pull/7827/files/98f1577d..913958e9 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=7827=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7827=01-02 Stats: 6 lines in 2 files changed: 0 ins; 6 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/7827.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7827/head:pull/7827 PR: https://git.openjdk.java.net/jdk/pull/7827
Re: RFR: 8281853 serviceability/sa/ClhsdbThreadContext.java failed with NullPointerException: Cannot invoke "sun.jvm.hotspot.gc.shared.GenCollectedHeap.getGen(int)" because "this.heap" is null
On Sat, 19 Mar 2022 19:34:23 GMT, Chris Plummer wrote:
> `isInNewGen()` is throwing an NPE because "heap" is null:
>
>
> public boolean isInNewGen() {
> return ((gen != null) && (gen == ((GenCollectedHeap)heap).getGen(0)));
> }
>
>
> The call came from here:
>
>
> } else if (isInHeap()) {
> if (isInTLAB()) {
> tty.print("In thread-local allocation buffer for thread (");
> getTLABThread().printThreadInfoOn(tty);
> tty.print(") ");
> getTLAB().printOn(tty); // includes "\n"
> } else {
> if (isInNewGen()) {
> tty.print("In new generation ");
> } else if (isInOldGen()) {
> tty.print("In old generation ");
> } else {
> tty.print("In unknown section of Java heap");
> }
> if (getGeneration() != null) {
> getGeneration().printOn(tty); // does not include "\n"
> }
> tty.println();
> }
>
>
> `isInHeap()` returns true if either "heap" or "gen" is non-null. If "gen" is
> non-null and "heap" is null, it is not safe to call `isInNewGen()`. Yet you
> can see from the code above that when `isInNewGen()` is called, the only
> guarantee is that "heap" or "gen" is non-null, not both, and it turns out
> that `PointerFinder.find()` only sets "gen" when the ptr is found to be in a
> generation of the heap. It does not set "heap". So the logic in
> `PointerFinder.find()` is not in agreement with the logic in
> `PointerLocation.printOn()` w.r.t. to the setting up and meaning of the "gen"
> and "heap" fields.
>
> The solution is pretty straight forward. Always set "heap" when the ptr is in
> the heap, even if it is in a generation (in which case "gen" is also set) or
> in a tlab (in which case "tlab" is also set). `isInHeap()` no longer requires
> that "gen" also be set, just "heap".
>
> I also noticed that the printlns in the following were not being triggered:
>
>
> if (isInNewGen()) {
> tty.print("In new generation ");
> } else if (isInOldGen()) {
> tty.print("In old generation ");
> } else {
>
>
> This was true even though "gen" was set and was indeed either pointing to the
> new gen or old gen. It turns out that the following returns a newly created
> Generation object:
>
> ` ((GenCollectedHeap)heap).getGen(0)`
>
> For this reason `equals()` must be used to compare them, not ==. The
> `VMObject.equals()` method is what ends up being called, and it compares the
> address associated with the underlying `VMObject`.
Yes, all looks good to me.
-
Marked as reviewed by kevinw (Committer).
PR: https://git.openjdk.java.net/jdk/pull/7873
Re: RFR: 8283092: JMX subclass permission check redundant with strong encapsulation [v2]
> Removing permission checks which, in the presence of a Security Manager, > would check for a RuntimePermission "className.subclass". This was to > prevent subclassing these classes, but is no longer necessary with strong > encapsulation from modules. Kevin Walls has updated the pull request incrementally with one additional commit since the last revision: Test update - Changes: - all: https://git.openjdk.java.net/jdk/pull/7827/files - new: https://git.openjdk.java.net/jdk/pull/7827/files/a6d05f73..98f1577d Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=7827=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7827=00-01 Stats: 71 lines in 1 file changed: 28 ins; 11 del; 32 mod Patch: https://git.openjdk.java.net/jdk/pull/7827.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7827/head:pull/7827 PR: https://git.openjdk.java.net/jdk/pull/7827
Re: RFR: 8283092: JMX subclass permission check redundant with strong encapsulation
On Tue, 15 Mar 2022 20:22:16 GMT, Kevin Walls wrote: > Removing permission checks which, in the presence of a Security Manager, > would check for a RuntimePermission "className.subclass". This was to > prevent subclassing these classes, but is no longer necessary with strong > encapsulation from modules. Added test update. Checks that with permissive module options we can extend sun.management.spi.PlatformMBeanProvider, and that without them we cannot. Output of the new test is like this: --System.out:(11/1279)-- ---PlatformMBeanProviderConstructorCheck: ---PlatformMBeanProviderConstructorCheck: invoke MyProvider with expectedFail = false ---PlatformMBeanProviderConstructorCheck PASSED (1) (expectedFail = false) ---PlatformMBeanProviderConstructorCheck: re-invoke without --add-modules or --add-exports Command line: [/tank/kwalls/repos/personal/jdk/open/test/../../build/linux-x86_64-server-release/images/jdk/bin/java -cp /tank/kwalls/repos/personal/jdk/open/test/JTwork/classes/sun/management/PlatformMBeanProviderConstructorCheck.d:/tank/kwalls/repos/personal/jdk/open/test/jdk/sun/management:/tank/kwalls/repos/personal/jdk/open/test/JTwork/classes/test/lib:/tank/kwalls/repos/personal/jdk/open/test/lib:/opt/jtreg6.1/lib/javatest.jar:/opt/jtreg6.1/lib/jtreg.jar PlatformMBeanProviderConstructorCheck --nomoduleargs ] [2022-03-18T17:19:34.798024163Z] Gathering output for process 10627 [2022-03-18T17:19:34.902532753Z] Waiting for completion for process 10627 [2022-03-18T17:19:34.902807078Z] Waiting for completion finished for process 10627 Output and diagnostic info for process 10627 was saved into 'pid-10627-output.log' [2022-03-18T17:19:34.908368606Z] Waiting for completion for process 10627 [2022-03-18T17:19:34.908503964Z] Waiting for completion finished for process 10627 --System.err:(9/647)-- stdout: [---PlatformMBeanProviderConstructorCheck: ---PlatformMBeanProviderConstructorCheck: invoke MyProvider with expectedFail = true ---PlatformMBeanProviderConstructorCheck got exception: java.lang.IllegalAccessError: superclass access check failed: class PlatformMBeanProviderConstructorCheck$MyProvider (in unnamed module @0x4795bbf5) cannot access class sun.management.spi.PlatformMBeanProvider (in module java.management) because module java.management does not export sun.management.spi to unnamed module @0x4795bbf5 ---PlatformMBeanProviderConstructorCheck PASSED (2) (expectedFail = true) ]; stderr: [] exitValue = 0 STATUS:Passed. - PR: https://git.openjdk.java.net/jdk/pull/7827
Re: RFR: 8283092: JMX subclass permission check redundant with strong encapsulation
On Thu, 17 Mar 2022 13:55:22 GMT, Sean Mullan wrote: > test/jdk/sun/management/PlatformMBeanProviderConstructorCheck.java Thank for noticing that Sean - had run various tests but missed this. I have an update, will add it here soon. - PR: https://git.openjdk.java.net/jdk/pull/7827
Re: RFR: 8283092: JMX subclass permission check redundant with strong encapsulation
On Wed, 16 Mar 2022 15:14:53 GMT, Daniel Fuchs wrote: >> Removing permission checks which, in the presence of a Security Manager, >> would check for a RuntimePermission "className.subclass". This was to >> prevent subclassing these classes, but is no longer necessary with strong >> encapsulation from modules. > > src/java.management/share/classes/sun/management/spi/PlatformMBeanProvider.java > line 233: > >> 231: } >> 232: return null; >> 233: } > > I have verified in module-info.java that sun.management.spi is only > conditionally exported so I agree that the explicit permission check can now > be removed. thanks > src/jdk.management.agent/share/classes/jdk/internal/agent/spi/AgentProvider.java > line 35: > >> 33: >> 34: /** >> 35: * Instantiates a new AgentProvider. > > This class should probably be removed altogether. I see that you logged > [JDK-8283254](https://bugs.openjdk.java.net/browse/JDK-8283254) so this is > fine. Thanks Daniel - yes will get rid of this class soon, I'll keep these two actions in separate changes. 8-) - PR: https://git.openjdk.java.net/jdk/pull/7827
RFR: 8283092: JMX subclass permission check redundant with strong encapsulation
Removing permission checks which, in the presence of a Security Manager, would check for a RuntimePermission "className.subclass". This was to prevent subclassing these classes, but is no longer necessary with strong encapsulation from modules. - Commit messages: - 8283092: JMX subclass permission check redundant with strong encapsulation Changes: https://git.openjdk.java.net/jdk/pull/7827/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=7827=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8283092 Stats: 31 lines in 3 files changed: 0 ins; 28 del; 3 mod Patch: https://git.openjdk.java.net/jdk/pull/7827.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7827/head:pull/7827 PR: https://git.openjdk.java.net/jdk/pull/7827
Re: RFR: 8282691: add jdb "-R" option for passing any argument to the launched debuggee process [v4]
On Sat, 12 Mar 2022 05:44:21 GMT, Chris Plummer wrote: >> jdb has 3 types of arguments: >> >> 1. Those that are jdb specific, such as -attach, -launch, and -listconnectors >> 2. Those that are passed to the JVM used to run jdb. These are all prefixed >> with -J, and any valid JVM argument can be passed in this manner. >> 3. Those that are passed to the debuggee JVM when it is launched, such as >> -classpath, -D, and any option that starts with -X (including -XX) >> >> The problem with the 3rd group is that (other than for -D and -X), jdb will >> only pass through arguments that it recognizes, and that list is very >> limited. When you want to launch the debuggee with an argument that jdb >> doesn't recognize, you have no choice but to launch the debuggee yourself >> (using a separate command line and using the -agentlib:jdwp argument) and >> then tell jdb to attach to the debuggee process. It's much easier when you >> can just let jdb launch the debuggee, and our nsk/jdb testing currently >> relies on this feature. >> >> This PR adds --enable-preview and --add-modules to the list of arguments >> that jdb recognizes and passes through to the debuggee JVM. These seem to be >> the two most glaring omissions, and are two that will likely be needed soon >> in order for loom nsk/jdb testing to work properly. > > Chris Plummer has updated the pull request incrementally with one additional > commit since the last revision: > > Updated jdb man page for -R command. Marked as reviewed by kevinw (Committer). - PR: https://git.openjdk.java.net/jdk/pull/7708
Re: RFR: 8282691: add jdb "-R" option for passing any argument to the launched debuggee process [v3]
On Tue, 8 Mar 2022 19:26:41 GMT, Chris Plummer wrote: >> jdb has 3 types of arguments: >> >> 1. Those that are jdb specific, such as -attach, -launch, and -listconnectors >> 2. Those that are passed to the JVM used to run jdb. These are all prefixed >> with -J, and any valid JVM argument can be passed in this manner. >> 3. Those that are passed to the debuggee JVM when it is launched, such as >> -classpath, -D, and any option that starts with -X (including -XX) >> >> The problem with the 3rd group is that (other than for -D and -X), jdb will >> only pass through arguments that it recognizes, and that list is very >> limited. When you want to launch the debuggee with an argument that jdb >> doesn't recognize, you have no choice but to launch the debuggee yourself >> (using a separate command line and using the -agentlib:jdwp argument) and >> then tell jdb to attach to the debuggee process. It's much easier when you >> can just let jdb launch the debuggee, and our nsk/jdb testing currently >> relies on this feature. >> >> This PR adds --enable-preview and --add-modules to the list of arguments >> that jdb recognizes and passes through to the debuggee JVM. These seem to be >> the two most glaring omissions, and are two that will likely be needed soon >> in order for loom nsk/jdb testing to work properly. > > Chris Plummer has updated the pull request incrementally with one additional > commit since the last revision: > > Minor help message improvements. Marked as reviewed by kevinw (Committer). - PR: https://git.openjdk.java.net/jdk/pull/7708
Re: RFR: 8282691: add jdb support for passing --enable-preview and --add-modules to the debuggee JVM [v2]
On Tue, 8 Mar 2022 03:37:44 GMT, Chris Plummer wrote: >> jdb has 3 types of arguments: >> >> 1. Those that are jdb specific, such as -attach, -launch, and -listconnectors >> 2. Those that are passed to the JVM used to run jdb. These are all prefixed >> with -J, and any valid JVM argument can be passed in this manner. >> 3. Those that are passed to the debuggee JVM when it is launched, such as >> -classpath, -D, and any option that starts with -X (including -XX) >> >> The problem with the 3rd group is that (other than for -D and -X), jdb will >> only pass through arguments that it recognizes, and that list is very >> limited. When you want to launch the debuggee with an argument that jdb >> doesn't recognize, you have no choice but to launch the debuggee yourself >> (using a separate command line and using the -agentlib:jdwp argument) and >> then tell jdb to attach to the debuggee process. It's much easier when you >> can just let jdb launch the debuggee, and our nsk/jdb testing currently >> relies on this feature. >> >> This PR adds --enable-preview and --add-modules to the list of arguments >> that jdb recognizes and passes through to the debuggee JVM. These seem to be >> the two most glaring omissions, and are two that will likely be needed soon >> in order for loom nsk/jdb testing to work properly. > > Chris Plummer has updated the pull request incrementally with three > additional commits since the last revision: > > - Re-add copyright change. > - Remove copyright change. > - Support -R instead of --enable-preview and --add-modules. Yes, that should avoid chasing future options! There is some magic that lets us add "--add-modules", then "jdk.attach" separately, but retrieve them later as "--add-modules=jdk.attach" in the test. I don't see what is doing that, so I ran it and verified the test works. - PR: https://git.openjdk.java.net/jdk/pull/7708
Re: RFR: 8282641: Make jdb "threadgroup" command with no args reset the current threadgroup back to the default [v2]
On Fri, 4 Mar 2022 07:34:35 GMT, Chris Plummer wrote:
>> jdb has a probably very little used command called "threadgroup" which is
>> used to set the current TheadGroup. The only purpose of the current
>> ThreadGroup is as the default ThreadGroup to use for the "threads" command
>> when no ThreadGroup argument is passed to it.
>>
>> "threads" prints out every thread in the ThreadGroup specified as the first
>> argument. If none is specified, it uses the current ThreadGroup. If the
>> current ThreadGroup has not yet been specified, it automatically gets set to
>> the top level ThreadGroup.
>>
>> Once the current ThreadGroup has been set by using the threadgroup command,
>> it's not that obvious how to reset it back to the default. It turns out the
>> way to do this to set it to the "system" ThreadGroup, which is the top level
>> ThreadGroup (and therefore the initial current ThreadGroup).
>>
>> With this enhancement I've made it so if you use the "threadgroup" command
>> with no argument, it resets the current ThreadGroup back to the top level
>> ThreadGroup ("system"). Previously with no argument it produces an error for
>> not specifying the ThreadGroup argument.
>
> Chris Plummer has updated the pull request incrementally with two additional
> commits since the last revision:
>
> - Use THREADGROUP_NAME instead of "MyThreadGroup#"
> - fix minor typo in help text
Looks good.
(Choosing the null or top-level thread group means to show all threads in all
thread groups, that makes sense though I didn't find if we say it anywhere. A
niche feature like you say, but good to have a way back from choosing a thread
group!)
-
Marked as reviewed by kevinw (Committer).
PR: https://git.openjdk.java.net/jdk/pull/7687
Re: RFR: 8282691: add jdb support for passing --enable-preview and --add-modules to the debuggee JVM
On Fri, 4 Mar 2022 22:12:08 GMT, Chris Plummer wrote: > jdb has 3 types of arguments: > > 1. Those that are jdb specific, such as -attach, -launch, and -listconnectors > 2. Those that are passed to the JVM used to run jdb. These are all prefixed > with -J, and any valid JVM argument can be passed in this manner. > 3. Those that are passed to the debuggee JVM when it is launched, such as > -classpath, -D, and any option that starts with -X (including -XX) > > The problem with the 3rd group is that (other than for -D and -X), jdb will > only pass through arguments that it recognizes, and that list is very > limited. When you want to launch the debuggee with an argument that jdb > doesn't recognize, you have no choice but to launch the debuggee yourself > (using a separate command line and using the -agentlib:jdwp argument) and > then tell jdb to attach to the debuggee process. It's much easier when you > can just let jdb launch the debuggee, and our nsk/jdb testing currently > relies on this feature. > > This PR adds --enable-preview and --add-modules to the list of arguments that > jdb recognizes and passes through to the debuggee JVM. These seem to be the > two most glaring omissions, and are two that will likely be needed soon in > order for loom nsk/jdb testing to work properly. Looks good. Yes a "pass through unrecognised options" to the target might be nice in future. There are a few more module-related options (in the "java -X" help page) like --add-opens and --add-exports and a few more, which might be useful for somebody. - Marked as reviewed by kevinw (Committer). PR: https://git.openjdk.java.net/jdk/pull/7708
Re: RFR: 8267796: vmTestbase/nsk/jvmti/scenarios/hotswap/HS201/hs201t002/TestDescription.java fails with NoClassDefFoundError [v2]
On Tue, 1 Mar 2022 13:07:36 GMT, Alex Menkov wrote: >> test/hotspot/jtreg/vmTestbase/nsk/jvmti/scenarios/hotswap/HS201/hs201t002/TestDescription.java >> line 75: >> >>> 73: * @run main/othervm/native >>> 74: * -agentlib:hs201t002=pathToNewByteCode=./bin,-waittime=5,-verbose >>> 75: * nsk.jvmti.scenarios.hotswap.HS201.hs201t002 -verbose >> >> Is this second -verbose a mistake? > > No. This is the way nsk framework works. > 1st -verbose turns on verbose output for agent (native), 2nd one for java > part of the test. aha, I didn't see where it was being recognised, thanks! - PR: https://git.openjdk.java.net/jdk/pull/7607
Re: RFR: 8267796: vmTestbase/nsk/jvmti/scenarios/hotswap/HS201/hs201t002/TestDescription.java fails with NoClassDefFoundError
On Thu, 24 Feb 2022 12:44:18 GMT, Alex Menkov wrote: > The fix adds workaround in hs201t001a class like we have in hs201t001 test to > avoid class loading while the test do single stepping/pop frame. > Also fixed a number of issues in the test. Looks like a good improvement. test/hotspot/jtreg/vmTestbase/nsk/jvmti/scenarios/hotswap/HS201/hs201t002/TestDescription.java line 75: > 73: * @run main/othervm/native > 74: * -agentlib:hs201t002=pathToNewByteCode=./bin,-waittime=5,-verbose > 75: * nsk.jvmti.scenarios.hotswap.HS201.hs201t002 -verbose Is this second -verbose a mistake? - Marked as reviewed by kevinw (Committer). PR: https://git.openjdk.java.net/jdk/pull/7607
Integrated: 8206187: javax/management/remote/mandatory/connection/DefaultAgentFilterTest.java fails with Port already in use
On Wed, 23 Feb 2022 13:59:19 GMT, Kevin Walls wrote: > Test fails occasionally due to a port clash. > Presumably the port that was returned by Utils.getFreePort(), is no longer > free. > The test creates a ProcessBuilder with the parameters for JMX, including port > number, and uses that to create a new Process. > It should retry with a new port if we fail due to a port in use, for some > limited number of attempts. > > main already has some retry logic, but not working: > it checks for an InvocationTargetException to contain a BindException, but it > simply gets a BindException, thrown by TestAppRun.start(). > TestAppRun.start() runs the new process and scans for errors, but on failure > its predicate has only seen the first line of a failure, so a BindException > is never recognised and thrown. > Also main does not limit the retries, and handling the port retry in main() > is duplicated, for each run of the test method. > > So... > > Make the error-scanning predicate in TestAppRun recognise a "port in use" > message and throw a BindExeption. This is a notification to the caller that > it failed, it's not the actual BindException as that was thrown in a > different process. > Make the testDefaultAgent method (the main part of the test) handle retrying > with a new port, a limited number of times. This pull request has now been integrated. Changeset: cd36be42 Author:Kevin Walls URL: https://git.openjdk.java.net/jdk/commit/cd36be42c2eb3eacdb3625e87510eb15acac3230 Stats: 82 lines in 1 file changed: 30 ins; 30 del; 22 mod 8206187: javax/management/remote/mandatory/connection/DefaultAgentFilterTest.java fails with Port already in use Reviewed-by: msheppar, amenkov - PR: https://git.openjdk.java.net/jdk/pull/7589
Re: RFR: 8206187: javax/management/remote/mandatory/connection/DefaultAgentFilterTest.java fails with Port already in use [v2]
On Thu, 24 Feb 2022 12:17:54 GMT, Kevin Walls wrote: >> Test fails occasionally due to a port clash. >> Presumably the port that was returned by Utils.getFreePort(), is no longer >> free. >> The test creates a ProcessBuilder with the parameters for JMX, including >> port number, and uses that to create a new Process. >> It should retry with a new port if we fail due to a port in use, for some >> limited number of attempts. >> >> main already has some retry logic, but not working: >> it checks for an InvocationTargetException to contain a BindException, but >> it simply gets a BindException, thrown by TestAppRun.start(). >> TestAppRun.start() runs the new process and scans for errors, but on failure >> its predicate has only seen the first line of a failure, so a BindException >> is never recognised and thrown. >> Also main does not limit the retries, and handling the port retry in main() >> is duplicated, for each run of the test method. >> >> So... >> >> Make the error-scanning predicate in TestAppRun recognise a "port in use" >> message and throw a BindExeption. This is a notification to the caller that >> it failed, it's not the actual BindException as that was thrown in a >> different process. >> Make the testDefaultAgent method (the main part of the test) handle retrying >> with a new port, a limited number of times. > > Kevin Walls has updated the pull request incrementally with one additional > commit since the last revision: > > Clarify predicate usage. Thanks for the reviews! Apologies, one more update - I had a break in there earlier to terminate the retry loop, and I removed it accidentally when changing things to make it fail. The retry loop has to let the testDefaultAgent method complete normally (which needs the break), or by throwing an Exception. Both looking good now. - PR: https://git.openjdk.java.net/jdk/pull/7589
Re: RFR: 8206187: javax/management/remote/mandatory/connection/DefaultAgentFilterTest.java fails with Port already in use [v3]
> Test fails occasionally due to a port clash. > Presumably the port that was returned by Utils.getFreePort(), is no longer > free. > The test creates a ProcessBuilder with the parameters for JMX, including port > number, and uses that to create a new Process. > It should retry with a new port if we fail due to a port in use, for some > limited number of attempts. > > main already has some retry logic, but not working: > it checks for an InvocationTargetException to contain a BindException, but it > simply gets a BindException, thrown by TestAppRun.start(). > TestAppRun.start() runs the new process and scans for errors, but on failure > its predicate has only seen the first line of a failure, so a BindException > is never recognised and thrown. > Also main does not limit the retries, and handling the port retry in main() > is duplicated, for each run of the test method. > > So... > > Make the error-scanning predicate in TestAppRun recognise a "port in use" > message and throw a BindExeption. This is a notification to the caller that > it failed, it's not the actual BindException as that was thrown in a > different process. > Make the testDefaultAgent method (the main part of the test) handle retrying > with a new port, a limited number of times. Kevin Walls has updated the pull request incrementally with one additional commit since the last revision: break to terminate retry loop when succesful - Changes: - all: https://git.openjdk.java.net/jdk/pull/7589/files - new: https://git.openjdk.java.net/jdk/pull/7589/files/6cdcde8a..33649830 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=7589=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7589=01-02 Stats: 1 line in 1 file changed: 1 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/7589.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7589/head:pull/7589 PR: https://git.openjdk.java.net/jdk/pull/7589
Re: RFR: 8206187: javax/management/remote/mandatory/connection/DefaultAgentFilterTest.java fails with Port already in use [v2]
> Test fails occasionally due to a port clash. > Presumably the port that was returned by Utils.getFreePort(), is no longer > free. > The test creates a ProcessBuilder with the parameters for JMX, including port > number, and uses that to create a new Process. > It should retry with a new port if we fail due to a port in use, for some > limited number of attempts. > > main already has some retry logic, but not working: > it checks for an InvocationTargetException to contain a BindException, but it > simply gets a BindException, thrown by TestAppRun.start(). > TestAppRun.start() runs the new process and scans for errors, but on failure > its predicate has only seen the first line of a failure, so a BindException > is never recognised and thrown. > Also main does not limit the retries, and handling the port retry in main() > is duplicated, for each run of the test method. > > So... > > Make the error-scanning predicate in TestAppRun recognise a "port in use" > message and throw a BindExeption. This is a notification to the caller that > it failed, it's not the actual BindException as that was thrown in a > different process. > Make the testDefaultAgent method (the main part of the test) handle retrying > with a new port, a limited number of times. Kevin Walls has updated the pull request incrementally with one additional commit since the last revision: Clarify predicate usage. - Changes: - all: https://git.openjdk.java.net/jdk/pull/7589/files - new: https://git.openjdk.java.net/jdk/pull/7589/files/a1bc3d0e..6cdcde8a Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=7589=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7589=00-01 Stats: 10 lines in 1 file changed: 4 ins; 1 del; 5 mod Patch: https://git.openjdk.java.net/jdk/pull/7589.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7589/head:pull/7589 PR: https://git.openjdk.java.net/jdk/pull/7589
Re: RFR: 8206187: javax/management/remote/mandatory/connection/DefaultAgentFilterTest.java fails with Port already in use
On Wed, 23 Feb 2022 17:42:35 GMT, Alex Menkov wrote: > The test uses warm-up predicate in a strange way Thanks Alex - yes so that's why we only see one line in the predicate, it contains the word "exception" and the predicate returns true, signalling that the process is done starting up. 8-) But if you do see an exception, you're never going to see the text from the app that it has started OK, so I see why it was returning true always - we were either seeing an error, or otherwise we presume TestApp has started. But it returned true after seeing the first line, which does not contain "bindexception", so it never saw the BindException later to recognise the port being in use. Anyway... I will update shortly to hopefully clarify the predicate part. Still good to keep main() simpler and not have duplicated retry logic there. - PR: https://git.openjdk.java.net/jdk/pull/7589
Re: RFR: 8206187: javax/management/remote/mandatory/connection/DefaultAgentFilterTest.java fails with Port already in use
On Wed, 23 Feb 2022 17:19:16 GMT, Mark Sheppard wrote: > An alternative is to use a fixed port 1098 Thanks Mark - I will avoid that fixed slot as we no doubt run tests concurrently, and also in case these get backported far enough that it's not free. 8-) Utils.getFreePort() lets new ServerSocket choose a port, but there's clearly a race to use it. We could make it simply random, but I think we need still need to retry like this to avoid failures -- will monitor and see how the race goes with 10 attempts. - PR: https://git.openjdk.java.net/jdk/pull/7589
Re: RFR: 8282200: ShouldNotReachHere() reached by AsyncGetCallTrace after JDK-8280422 [v4]
On Wed, 23 Feb 2022 21:59:35 GMT, Johannes Bechberger wrote: >> Fixes the mentioned bug by replacing the check in AsyncGetCallTrace using >> the newly introduced method `JavaThread::thread_from_jni_environment`. > > Johannes Bechberger has updated the pull request incrementally with one > additional commit since the last revision: > > Remove old code Marked as reviewed by kevinw (Committer). - PR: https://git.openjdk.java.net/jdk/pull/7559
RFR: 8206187: javax/management/remote/mandatory/connection/DefaultAgentFilterTest.java fails with Port already in use
Test fails occasionally due to a port clash. Presumably the port that was returned by Utils.getFreePort(), is no longer free. The test creates a ProcessBuilder with the parameters for JMX, including port number, and uses that to create a new Process. It should retry with a new port if we fail due to a port in use, for some limited number of attempts. main already has some retry logic, but not working: it checks for an InvocationTargetException to contain a BindException, but it simply gets a BindException, thrown by TestAppRun.start(). TestAppRun.start() runs the new process and scans for errors, but on failure its predicate has only seen the first line of a failure, so a BindException is never recognised and thrown. Also main does not limit the retries, and handling the port retry in main() is duplicated, for each run of the test method. So... Make the error-scanning predicate in TestAppRun recognise a "port in use" message and throw a BindExeption. This is a notification to the caller that it failed, it's not the actual BindException as that was thrown in a different process. Make the testDefaultAgent method (the main part of the test) handle retrying with a new port, a limited number of times. - Commit messages: - Remove tab. - Throw on last retry if port clash persists. - Merge remote-tracking branch 'upstream/master' into 8206187_DefaultAgentFilterTest - 8206187: javax/management/remote/mandatory/connection/DefaultAgentFilterTest.java fails with Port already in use Changes: https://git.openjdk.java.net/jdk/pull/7589/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=7589=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8206187 Stats: 73 lines in 1 file changed: 25 ins; 29 del; 19 mod Patch: https://git.openjdk.java.net/jdk/pull/7589.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7589/head:pull/7589 PR: https://git.openjdk.java.net/jdk/pull/7589
Re: RFR: 8281377: Remove vmTestbase/nsk/monitoring/ThreadMXBean/ThreadInfo/Deadlock/JavaDeadlock001/TestDescription.java from problemlist.
On Mon, 7 Feb 2022 15:40:11 GMT, Kevin Walls wrote: > Test failed many years ago. > Cannot provoke a failure today in hundreds of runs. > Initial failure may have been specific to Solaris. Right - vmTestbase/nsk/monitoring/ThreadMXBean/ThreadInfo/Deadlock/MixedDeadlock001/TestDescription.java does not appear to fail recently except that one old mention, or the other scenarios tested. - PR: https://git.openjdk.java.net/jdk/pull/7369
Integrated: 6779701: Wrong defect ID in the code of test LocalRMIServerSocketFactoryTest.java
On Mon, 7 Feb 2022 17:28:29 GMT, Kevin Walls wrote: > Trivial comment and exception text update, correcting a bug ID to make more > sense. This pull request has now been integrated. Changeset: 8a662105 Author: Kevin Walls URL: https://git.openjdk.java.net/jdk/commit/8a662105c2da1f0fb9b7ecc5058fc85858439ed9 Stats: 6 lines in 1 file changed: 0 ins; 0 del; 6 mod 6779701: Wrong defect ID in the code of test LocalRMIServerSocketFactoryTest.java Reviewed-by: cjplummer, dfuchs - PR: https://git.openjdk.java.net/jdk/pull/7371
Re: RFR: 6779701: Wrong defect ID in the code of test LocalRMIServerSocketFactoryTest.java
On Mon, 7 Feb 2022 17:28:29 GMT, Kevin Walls wrote: > Trivial comment and exception text update, correcting a bug ID to make more > sense. Thanks Chris, thanks Daniel! - PR: https://git.openjdk.java.net/jdk/pull/7371
Integrated: 8281377: Remove vmTestbase/nsk/monitoring/ThreadMXBean/ThreadInfo/Deadlock/JavaDeadlock001/TestDescription.java from problemlist.
On Mon, 7 Feb 2022 15:40:11 GMT, Kevin Walls wrote: > Test failed many years ago. > Cannot provoke a failure today in hundreds of runs. > Initial failure may have been specific to Solaris. This pull request has now been integrated. Changeset: 1dfc94dd Author: Kevin Walls URL: https://git.openjdk.java.net/jdk/commit/1dfc94dd561f6a91ef3784fe28c83f839f8188c4 Stats: 1 line in 1 file changed: 0 ins; 1 del; 0 mod 8281377: Remove vmTestbase/nsk/monitoring/ThreadMXBean/ThreadInfo/Deadlock/JavaDeadlock001/TestDescription.java from problemlist. Reviewed-by: sspitsyn - PR: https://git.openjdk.java.net/jdk/pull/7369
Re: RFR: 8281377: Remove vmTestbase/nsk/monitoring/ThreadMXBean/ThreadInfo/Deadlock/JavaDeadlock001/TestDescription.java from problemlist.
On Mon, 7 Feb 2022 15:40:11 GMT, Kevin Walls wrote: > Test failed many years ago. > Cannot provoke a failure today in hundreds of runs. > Initial failure may have been specific to Solaris. Thanks Serguei! - PR: https://git.openjdk.java.net/jdk/pull/7369
RFR: 6779701: Wrong defect ID in the code of test LocalRMIServerSocketFactoryTest.java
Trivial comment and exception text update, correcting a bug ID to make more sense. - Commit messages: - 6779701: Wrong defect ID in the code of test LocalRMIServerSocketFactoryTest.java Changes: https://git.openjdk.java.net/jdk/pull/7371/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=7371=00 Issue: https://bugs.openjdk.java.net/browse/JDK-6779701 Stats: 6 lines in 1 file changed: 0 ins; 0 del; 6 mod Patch: https://git.openjdk.java.net/jdk/pull/7371.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7371/head:pull/7371 PR: https://git.openjdk.java.net/jdk/pull/7371
RFR: 8281377: Remove vmTestbase/nsk/monitoring/ThreadMXBean/ThreadInfo/Deadlock/JavaDeadlock001/TestDescription.java from problemlist.
Test failed many years ago. Cannot provoke a failure today in hundreds of runs. Initial failure may have been specific to Solaris. - Commit messages: - 8281377: Remove vmTestbase/nsk/monitoring/ThreadMXBean/ThreadInfo/Deadlock/JavaDeadlock001/TestDescription.java from problemlist. Changes: https://git.openjdk.java.net/jdk/pull/7369/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=7369=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8281377 Stats: 1 line in 1 file changed: 0 ins; 1 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/7369.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7369/head:pull/7369 PR: https://git.openjdk.java.net/jdk/pull/7369
RFR: 8276680: Malformed Javadoc inline tags in JDK source in javax/management
Trivial doc typo. Now I realise this javadoc for a package-private constructor isn't even in the standard generated docs. But you could choose to use different javadoc options. - Commit messages: - 8276680: Malformed Javadoc inline tags in JDK source in javax/management Changes: https://git.openjdk.java.net/jdk/pull/7366/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=7366=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8276680 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/7366.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7366/head:pull/7366 PR: https://git.openjdk.java.net/jdk/pull/7366
Integrated: 8281049: man page update for jstatd Security Manager dependency removal
On Wed, 2 Feb 2022 10:54:19 GMT, Kevin Walls wrote: > Policy settings are no longer required to run jstatd. > > This PR is the change to the source file src/jdk.jstatd/share/man/jstatd.1 > and in case that's hard to read, here is a diff of the text as rendered by > the man command: > https://cr.openjdk.java.net/~kevinw/8281049/jstatd.man.diff.txt > > And a copy of the new text rendered by man: > https://cr.openjdk.java.net/~kevinw/8281049/jstatd.man.new.txt This pull request has now been integrated. Changeset: 48523b09 Author:Kevin Walls URL: https://git.openjdk.java.net/jdk/commit/48523b090886f7b24ed4009f0c150efaa6f7b056 Stats: 16 lines in 1 file changed: 0 ins; 3 del; 13 mod 8281049: man page update for jstatd Security Manager dependency removal Reviewed-by: cjplummer - PR: https://git.openjdk.java.net/jdk/pull/7320
Re: RFR: 8281049: man page update for jstatd Security Manager dependency removal [v2]
On Thu, 3 Feb 2022 16:39:45 GMT, Chris Plummer wrote: >> Kevin Walls has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Update > > src/jdk.jstatd/share/man/jstatd.1 line 124: > >> 122: .PP >> 123: For security purposes, the jstatd server uses an RMI ObjectInputFilter >> 124: to allow only essential classes to be deserialized. > > It looks like some formatting is now missing around `jstatd`. Right, missed the quotes we use around jstatd in other places. Added them in this update - I now see the word **jstatd** in bold in the man output like the other places. - PR: https://git.openjdk.java.net/jdk/pull/7320
Re: RFR: 8281049: man page update for jstatd Security Manager dependency removal [v3]
> Policy settings are no longer required to run jstatd. > > This PR is the change to the source file src/jdk.jstatd/share/man/jstatd.1 > and in case that's hard to read, here is a diff of the text as rendered by > the man command: > https://cr.openjdk.java.net/~kevinw/8281049/jstatd.man.diff.txt > > And a copy of the new text rendered by man: > https://cr.openjdk.java.net/~kevinw/8281049/jstatd.man.new.txt Kevin Walls has updated the pull request incrementally with one additional commit since the last revision: Quotes/bold - Changes: - all: https://git.openjdk.java.net/jdk/pull/7320/files - new: https://git.openjdk.java.net/jdk/pull/7320/files/1e082f9d..45ad90a3 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=7320=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7320=01-02 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/7320.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7320/head:pull/7320 PR: https://git.openjdk.java.net/jdk/pull/7320
Integrated: 8272777: Clean up remaining AccessController warnings in test library
On Wed, 2 Feb 2022 21:35:59 GMT, Kevin Walls wrote:
> Reduce noise in test output by adding the @SuppressWarnings("removal")
> annotation (which has already been widely applied).
This pull request has now been integrated.
Changeset: 63a00a0d
Author:Kevin Walls
URL:
https://git.openjdk.java.net/jdk/commit/63a00a0df24b154ef459936dbd69bcd2f0626235
Stats: 5 lines in 3 files changed: 3 ins; 0 del; 2 mod
8272777: Clean up remaining AccessController warnings in test library
Reviewed-by: rriggs, sspitsyn
-
PR: https://git.openjdk.java.net/jdk/pull/7328
Re: RFR: 8272777: Clean up remaining AccessController warnings in test library
On Wed, 2 Feb 2022 21:35:59 GMT, Kevin Walls wrote:
> Reduce noise in test output by adding the @SuppressWarnings("removal")
> annotation (which has already been widely applied).
Thanks Roger and Serguei!
-
PR: https://git.openjdk.java.net/jdk/pull/7328
Re: RFR: 8281049: man page update for jstatd Security Manager dependency removal [v2]
On Wed, 2 Feb 2022 22:41:02 GMT, Chris Plummer wrote: >> Kevin Walls has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Update > > src/jdk.jstatd/share/man/jstatd.1 line 124: > >> 122: .PP >> 123: As RMI is in use, the jstatd server uses an ObjectInputFilter to allow >> 124: only essential classes to be deserialized. > > How about: > > For security purposes, the jstatd server uses an RMI ObjectInputFilter to > allow only essential classes to be deserialized. Yes, sounds good - updated. - PR: https://git.openjdk.java.net/jdk/pull/7320
Re: RFR: 8281049: man page update for jstatd Security Manager dependency removal [v2]
> Policy settings are no longer required to run jstatd. > > This PR is the change to the source file src/jdk.jstatd/share/man/jstatd.1 > and in case that's hard to read, here is a diff of the text as rendered by > the man command: > https://cr.openjdk.java.net/~kevinw/8281049/jstatd.man.diff.txt > > And a copy of the new text rendered by man: > https://cr.openjdk.java.net/~kevinw/8281049/jstatd.man.new.txt Kevin Walls has updated the pull request incrementally with one additional commit since the last revision: Update - Changes: - all: https://git.openjdk.java.net/jdk/pull/7320/files - new: https://git.openjdk.java.net/jdk/pull/7320/files/9becb618..1e082f9d Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=7320=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7320=00-01 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/7320.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7320/head:pull/7320 PR: https://git.openjdk.java.net/jdk/pull/7320
RFR: 8272777: Clean up remaining AccessController warnings in test library
Reduce noise in test output by adding the @SuppressWarnings("removal")
annotation (which has already been widely applied).
-
Commit messages:
- 8272777: Clean up remaining AccessController warnings in test library
Changes: https://git.openjdk.java.net/jdk/pull/7328/files
Webrev: https://webrevs.openjdk.java.net/?repo=jdk=7328=00
Issue: https://bugs.openjdk.java.net/browse/JDK-8272777
Stats: 5 lines in 3 files changed: 3 ins; 0 del; 2 mod
Patch: https://git.openjdk.java.net/jdk/pull/7328.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/7328/head:pull/7328
PR: https://git.openjdk.java.net/jdk/pull/7328
Re: RFR: 8279662: serviceability/sa/ClhsdbScanOops.java can fail do to unexpected GC
On Wed, 2 Feb 2022 16:21:05 GMT, Chris Plummer wrote: > Actually it's not, and that's why I check the Eden gen. Yes sorry, should have said it's a good bet to find a String when only checking one generation. Carry on... - PR: https://git.openjdk.java.net/jdk/pull/7295
RFR: 8281049: man page update for jstatd Security Manager dependency removal
Policy settings are no longer required to run jstatd. This PR is the change to the source file src/jdk.jstatd/share/man/jstatd.1 and in case that's hard to read, here is a diff of the text as rendered by the man command: https://cr.openjdk.java.net/~kevinw/8281049/jstatd.man.diff.txt And a copy of the new text rendered by man: https://cr.openjdk.java.net/~kevinw/8281049/jstatd.man.new.txt - Commit messages: - 8281049: man page update for jstatd Security Manager dependency removal Changes: https://git.openjdk.java.net/jdk/pull/7320/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=7320=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8281049 Stats: 16 lines in 1 file changed: 0 ins; 3 del; 13 mod Patch: https://git.openjdk.java.net/jdk/pull/7320.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7320/head:pull/7320 PR: https://git.openjdk.java.net/jdk/pull/7320
Re: RFR: 8279662: serviceability/sa/ClhsdbScanOops.java can fail do to unexpected GC
On Tue, 1 Feb 2022 05:29:36 GMT, Chris Plummer wrote: > The test is failing to find certain types in the scanoops output when run > with -Xcomp. This is happening in the loom repo. The reason it is happening > there is because loom introduced a full GC during codecache sweeping. The > test only runs scanoops on the eden gen, and the full GC is causing objects > of certain expected types to no longer be present in the eden gen. The fix is > to also check the old gen. > > The logic of the test had to be reworked a bit to accomplish checking the > output of two scanoops commands together. It was relying on the > ClhsdbLauncher class to check the output for expected strings, but in this > case we need to accumulate the output of the two scanoops commands and check > the combined output for the expected strings, so now the checking is done > directly by the test and not by ClhsdbLauncher. > > I'm choosing to fix this in the jdk repo rather than the loom repo since it > is a latent bug that theoretically could occur even without the loom changes, > and also to help reduce the amount of changes to be reviewed when loom is > integrated into jdk. Looks good. For the scanoops for a type, it seems like a reasonable assumption that there will be a String in the old gen! - Marked as reviewed by kevinw (Committer). PR: https://git.openjdk.java.net/jdk/pull/7295
Re: RFR: 8280601: ClhsdbThreadContext.java test is triggering codecache related assert in PointerFinder.find() [v2]
On Fri, 28 Jan 2022 04:07:40 GMT, Chris Plummer wrote: >> It's possible for an address to be in the codecache but not in any CodeBlob. >> Don't assert in this case. >> >> Also I ran into a couple of other asserts listed below. It looks like since >> dumping the threadcontext can result in using PointerFinder with fairly >> random addresses, it is doing a much better job of stress testing >> PointerFinder than is done by other tests. The root issue in all these >> asserts is that a random address in the codecache can either be outside of >> any CodeBlob, or can map to a CodeBlob that is not yet initialized or could >> even have been freed. PointerFinder and PointerLocation need to be prepared >> to handled these asserts, and any other exception thrown. >> >> sun.jvm.hotspot.utilities.AssertionFailure: found wrong CodeBlob >> at jdk.hotspot.agent/sun.jvm.hotspot.utilities.Assert.that(Assert.java:32) >> at >> jdk.hotspot.agent/sun.jvm.hotspot.code.CodeCache.findBlobUnsafe(CodeCache.java:140) >> at >> jdk.hotspot.agent/sun.jvm.hotspot.utilities.PointerFinder.find(PointerFinder.java:138) >> at >> jdk.hotspot.agent/sun.jvm.hotspot.runtime.JavaThread.printThreadContextOn(JavaThread.java:493) >> at >> jdk.hotspot.agent/sun.jvm.hotspot.CommandProcessor$46.doit(CommandProcessor.java:1702) >> at >> jdk.hotspot.agent/sun.jvm.hotspot.CommandProcessor.executeCommand(CommandProcessor.java:2215) >> at >> jdk.hotspot.agent/sun.jvm.hotspot.CommandProcessor.executeCommand(CommandProcessor.java:2185) >> at >> jdk.hotspot.agent/sun.jvm.hotspot.CommandProcessor.run(CommandProcessor.java:2056) >> at jdk.hotspot.agent/sun.jvm.hotspot.CLHSDB.run(CLHSDB.java:112) >> at jdk.hotspot.agent/sun.jvm.hotspot.CLHSDB.main(CLHSDB.java:44) >> at >> jdk.hotspot.agent/sun.jvm.hotspot.SALauncher.runCLHSDB(SALauncher.java:281) >> at jdk.hotspot.agent/sun.jvm.hotspot.SALauncher.main(SALauncher.java:500) >> >> sun.jvm.hotspot.utilities.AssertionFailure: Should have found CodeBlob >> at jdk.hotspot.agent/sun.jvm.hotspot.utilities.Assert.that(Assert.java:32) >> at >> jdk.hotspot.agent/sun.jvm.hotspot.utilities.PointerFinder.find(PointerFinder.java:140) >> at >> jdk.hotspot.agent/sun.jvm.hotspot.runtime.JavaThread.printThreadContextOn(JavaThread.java:493) >> at >> jdk.hotspot.agent/sun.jvm.hotspot.CommandProcessor$46.doit(CommandProcessor.java:1702) >> at >> jdk.hotspot.agent/sun.jvm.hotspot.CommandProcessor.executeCommand(CommandProcessor.java:2215) >> at >> jdk.hotspot.agent/sun.jvm.hotspot.CommandProcessor.executeCommand(CommandProcessor.java:2185) >> at >> jdk.hotspot.agent/sun.jvm.hotspot.CommandProcessor.run(CommandProcessor.java:2056) >> at jdk.hotspot.agent/sun.jvm.hotspot.CLHSDB.run(CLHSDB.java:112) >> at jdk.hotspot.agent/sun.jvm.hotspot.CLHSDB.main(CLHSDB.java:44) >> at >> jdk.hotspot.agent/sun.jvm.hotspot.SALauncher.runCLHSDB(SALauncher.java:281) >> at jdk.hotspot.agent/sun.jvm.hotspot.SALauncher.main(SALauncher.java:500) > > Chris Plummer has updated the pull request incrementally with one additional > commit since the last revision: > > Guard against invalid CodeBlobs that may throw exceptions. Marked as reviewed by kevinw (Committer). Yes, looks good -- I like less asserting or throwing from methods that are trying to make sense of data that may not make perfect sense! - PR: https://git.openjdk.java.net/jdk/pull/7217
Integrated: 8272317: jstatd has dependency on Security Manager which needs to be removed
On Wed, 22 Dec 2021 18:14:43 GMT, Kevin Walls wrote: > Remove the use of Security Manager from jstatd. > Add use of an ObjectInputFilter to restrict RMI. > > Also we can undo the property-setting Launcher.gmk change from: 8279007: > jstatd fails to start because SecurityManager is disabled > ..as that is no longer needed. > > Docs/man page update to follow (JDK-8278619). This pull request has now been integrated. Changeset: cb8a82ee Author:Kevin Walls URL: https://git.openjdk.java.net/jdk/commit/cb8a82ee24881113af4eea04d7ce5963d18e9b83 Stats: 29 lines in 4 files changed: 4 ins; 15 del; 10 mod 8272317: jstatd has dependency on Security Manager which needs to be removed Reviewed-by: cjplummer, rriggs - PR: https://git.openjdk.java.net/jdk/pull/6919
Re: RFR: 8272317: jstatd has dependency on Security Manager which needs to be removed [v4]
> Remove the use of Security Manager from jstatd. > Add use of an ObjectInputFilter to restrict RMI. > > Also we can undo the property-setting Launcher.gmk change from: 8279007: > jstatd fails to start because SecurityManager is disabled > ..as that is no longer needed. > > Docs/man page update to follow (JDK-8278619). Kevin Walls 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 eight additional commits since the last revision: - Merge remote-tracking branch 'upstream/master' into 8272317_jstatd_secmgr - Copyright update - Wildcard in object filter to permit proxies, in case other activity in this JVM changes the nameing/numbering of proxy classes. - Remove jstad launcher property setting to allow Security Manager. - Merge remote-tracking branch 'upstream/master' into 8272317_jstatd_secmgr - Add ObjectInputFilter - Merge remote-tracking branch 'upstream/master' into 8272317_jstatd_secmgr - 8272317: jstatd has dependency on Security Manager which needs to be removed - Changes: - all: https://git.openjdk.java.net/jdk/pull/6919/files - new: https://git.openjdk.java.net/jdk/pull/6919/files/443b18b4..e6f016a8 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=6919=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk=6919=02-03 Stats: 34764 lines in 1297 files changed: 23346 ins; 6357 del; 5061 mod Patch: https://git.openjdk.java.net/jdk/pull/6919.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/6919/head:pull/6919 PR: https://git.openjdk.java.net/jdk/pull/6919
Re: RFR: 8272317: jstatd has dependency on Security Manager which needs to be removed [v2]
On Thu, 27 Jan 2022 17:44:39 GMT, Roger Riggs wrote: >> Kevin Walls has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Wildcard in object filter to permit proxies, in case other activity in >> this JVM changes the nameing/numbering of proxy classes. > > test/jdk/sun/tools/jstatd/JstatdTest.java line 2: > >> 1: /* >> 2: * Copyright (c) 2013, 2021, Oracle and/or its affiliates. All rights >> reserved. > > Update copyrights. Yes, thanks, time has moved on since I started!... - PR: https://git.openjdk.java.net/jdk/pull/6919
Re: RFR: 8272317: jstatd has dependency on Security Manager which needs to be removed [v2]
On Mon, 10 Jan 2022 11:17:12 GMT, Kevin Walls wrote: >> Remove the use of Security Manager from jstatd. >> Add use of an ObjectInputFilter to restrict RMI. >> >> Also we can undo the property-setting Launcher.gmk change from: 8279007: >> jstatd fails to start because SecurityManager is disabled >> ..as that is no longer needed. >> >> Docs/man page update to follow (JDK-8278619). > > Kevin Walls has updated the pull request incrementally with one additional > commit since the last revision: > > Wildcard in object filter to permit proxies, in case other activity in this > JVM changes the nameing/numbering of proxy classes. Many thanks Chris, Roger! - PR: https://git.openjdk.java.net/jdk/pull/6919
Re: RFR: 8272317: jstatd has dependency on Security Manager which needs to be removed [v3]
> Remove the use of Security Manager from jstatd. > Add use of an ObjectInputFilter to restrict RMI. > > Also we can undo the property-setting Launcher.gmk change from: 8279007: > jstatd fails to start because SecurityManager is disabled > ..as that is no longer needed. > > Docs/man page update to follow (JDK-8278619). Kevin Walls has updated the pull request incrementally with one additional commit since the last revision: Copyright update - Changes: - all: https://git.openjdk.java.net/jdk/pull/6919/files - new: https://git.openjdk.java.net/jdk/pull/6919/files/e49b9fe2..443b18b4 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=6919=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk=6919=01-02 Stats: 3 lines in 3 files changed: 0 ins; 0 del; 3 mod Patch: https://git.openjdk.java.net/jdk/pull/6919.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/6919/head:pull/6919 PR: https://git.openjdk.java.net/jdk/pull/6919
Re: RFR: 8270199: Most SA tests are skipped on macosx-aarch64 because all executables are signed [v2]
On Wed, 19 Jan 2022 18:22:13 GMT, Chris Plummer wrote: >> For any SA test that attaches to an OSX process (this would be all SA tests >> except for those that test core file support), there is a check to make sure >> that the target jvm process is not a signed binary. If it is, >> SkippedException is thrown, and the test passes without doing anything. This >> is all we can do since being signed implied being notarized, and debuggers >> cannot attach to a notarized binary. >> >> I noticed that on macosx-aarch64, all our SA tests that attach to a process >> were being skipped because the binary was signed, even for debug builds. It >> turns out that for macosx-aarch64, the linker always ads what is known as >> ad-hoc signing. You can find some info on ad-hoc signing here: >> >> https://eclecticlight.co/2020/08/22/apple-silicon-macs-will-require-signed-code/ >> >> The tests use the codesign tool to determine if the binary is signed. >> Normally the check just relies on getting an error code of 1 when not >> signed, but since all binaries are now signed on macosx-aarch64, we need to >> modify the check to ignore ad-hoc signing. >> >> Using "codesign --display" on an an ad-hoc signed binary shows some output >> that is of interest: >> >> bash-3.2$ codesign --display --verbose=4 a.out >> CodeDirectory v=20400 size=254 flags=0x20002(adhoc,linker-signed) hashes=5+0 >> location=embedded >> >> Looking for flags=0x20002(adhoc,linker-signed) will tell us if the binary is >> adhoc signed, which means we can assume that the SA tests can still be run >> against it. >> >> Looking for flags=0x1(runtime) will tell us that the binary is hardened. >> These are binaries that we cannot use with SA. >> >> If the binary is not signed at all, which should only happen on macosx-x64, >> codesign will print the message "code object is not signed at all". We can >> also run SA against these binaries. >> >> Due to fixing the issue, we now have to problem list a few more core file >> tests on macosx-aarch64 due to 8269982. This issue was not noticed in the >> past because the tests were not being run. This was due to most of our >> macosx-aarch64 machines not being capable of producing a core file. Without >> this fix the test assumes it couldn't produce the core file because the >> binary was signed. With this fix it, for binaries that are not hardened the >> test will fail if it does not produce a core file. > > Chris Plummer has updated the pull request incrementally with one additional > commit since the last revision: > > Update copyrights Looks good. No problem with dealing with future codesign output changes as they happen, or handling more flags in future, above the cases recognised here. - Marked as reviewed by kevinw (Committer). PR: https://git.openjdk.java.net/jdk/pull/6906
Re: RFR: 8280601: ClhsdbThreadContext.java test is triggering codecache related assert in PointerFinder.find()
On Tue, 25 Jan 2022 22:07:37 GMT, Chris Plummer wrote:
> It's possible for an address to be in the codecache but not in any CodeBlob.
> Don't assert in this case.
>
> Note I couldn't reproduce this failure. Not sure why since it seemed to
> reproduce pretty readily in our CI tier7, and I ran with the same options.
Marked as reviewed by kevinw (Committer).
src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/utilities/PointerLocation.java
line 305:
> 303: } else {
> 304: if (Assert.ASSERTS_ENABLED) {
> 305: Assert.that(isInBlobUnknownLocation(), "Should have known
> location in CodeBlob");
Line 305's Assert message is hard to understand: "Should have known location in
CodeBlob", when it would accept knowing that the location is unknown.
It means that none of the isInBlobXXX flags are true, and probably impossible
to trigger at the moment.
If you ever wanted to delete it, or make it say "Should have known or unknown
location in CodeBlob" then I'd agree. 8-)
-
PR: https://git.openjdk.java.net/jdk/pull/7217
Re: RFR: 8076089: Cleanup: Inline & remove sun.management.Util.newException
On Wed, 26 Jan 2022 04:35:58 GMT, Jaikiran Pai wrote: > Can I please get a review for this cleanup that's requested in > https://bugs.openjdk.java.net/browse/JDK-8076089? > > The change here removes a package private method > `sun.management.Util.newException(Exception e)` and inlines its > implementation at the caller locations. > > Given the nature of this change no new jtreg tests have been added. Looks good, thanks. - Marked as reviewed by kevinw (Committer). PR: https://git.openjdk.java.net/jdk/pull/7226
Re: RFR: 8272317: jstatd has dependency on Security Manager which needs to be removed [v2]
On Thu, 20 Jan 2022 16:54:21 GMT, Mandy Chung wrote: > If `sun.jvmstat.monitor.remote.RemoteVm` is the only proxy interface, > `com.sun.proxy.jdk.proxy*` should adequately cover the proxy classes created > for `RemoteVm`. Thanks. With that endorsement I think there are no unresolved issues with this change. - PR: https://git.openjdk.java.net/jdk/pull/6919
Re: RFR: 8272317: jstatd has dependency on Security Manager which needs to be removed [v2]
On Wed, 19 Jan 2022 19:56:53 GMT, Mandy Chung wrote: > Are all the proxy interfaces public? sun.jvmstat.monitor.remote.RemoteVm is "public interface RemoteVm extends Remote" and methods in there only return basic types. This is in the jdk.jstatd module, where I see the module info contains "exports sun.jvmstat.monitor.remote to java.rmi;" The only other names permitted are proxy/reflect/rmi related. - PR: https://git.openjdk.java.net/jdk/pull/6919
Re: RFR: 8272317: jstatd has dependency on Security Manager which needs to be removed [v2]
On Mon, 10 Jan 2022 11:17:12 GMT, Kevin Walls wrote: >> Remove the use of Security Manager from jstatd. >> Add use of an ObjectInputFilter to restrict RMI. >> >> Also we can undo the property-setting Launcher.gmk change from: 8279007: >> jstatd fails to start because SecurityManager is disabled >> ..as that is no longer needed. >> >> Docs/man page update to follow (JDK-8278619). > > Kevin Walls has updated the pull request incrementally with one additional > commit since the last revision: > > Wildcard in object filter to permit proxies, in case other activity in this > JVM changes the nameing/numbering of proxy classes. CSR has been approved (https://bugs.openjdk.java.net/browse/JDK-8279891) - PR: https://git.openjdk.java.net/jdk/pull/6919
Re: RFR: 8279124: VM does not handle SIGQUIT during initialization [v3]
On Wed, 19 Jan 2022 09:10:56 GMT, Xin Liu wrote: >> In early stage of initialization, HotSpot doesn't handle SIGQUIT. The >> default signal preposition on Linux is to quit the process and generate >> coredump. >> >> There are 2 applications for this signal. >> 1. There's a handshake protocol between sun.tools.attach and HotSpot. >> VirtualMachineImpl sends SIGQUIT(3) if the AttachListener has not been >> initialized. It expects "Signal Dispatcher" to handle SIGQUIT and create >> AttachListener. >> 2. POSIX systems use SIGQUIT as SIGBREAK. After AttachListener is up, >> HotSpot will reinterpret the signal for thread dump. >> >> It is possible that HotSpot is still initializing in Threads::create_vm() >> when SIGQUIT arrives. We should change JVM_HANDLE_XXX_SIGNAL to catch >> SIGQUIT and ignore it. It is installed os::init_2() and should cover the >> early stage of initialization. Later on, os::initialize_jdk_signal_support() >> still overwrites the signal handler of SIGQUIT if ReduceSignalUsage is >> false(default). >> >> Testing >> >> Before, this patch, once initialization takes long time, jcmd may quit the >> java process. >> >> $java -Xms64g -XX:+AlwaysPreTouch -Xlog:gc+heap=debug:stderr >> -XX:ParallelGCThreads=1 & >> [1] 9589 >> [0.028s][debug][gc,heap] Minimum heap 68719476736 Initial heap 68719476736 >> Maximum heap 68719476736 >> [0.034s][debug][gc,heap] Running G1 PreTouch with 1 workers for 16384 work >> units pre-touching 68719476736B. >> $jcmd 9589 VM.flags >> 9589: >> [1] + 9589 quit java -Xms64g -XX:+AlwaysPreTouch >> -Xlog:gc+heap=debug:stderr >> java.io.IOException: No such process >> at jdk.attach/sun.tools.attach.VirtualMachineImpl.sendQuitTo(Native >> Method) >> at >> jdk.attach/sun.tools.attach.VirtualMachineImpl.(VirtualMachineImpl.java:100) >> at >> jdk.attach/sun.tools.attach.AttachProviderImpl.attachVirtualMachine(AttachProviderImpl.java:58) >> at >> jdk.attach/com.sun.tools.attach.VirtualMachine.attach(VirtualMachine.java:207) >> at jdk.jcmd/sun.tools.jcmd.JCmd.executeCommandForPid(JCmd.java:113) >> at jdk.jcmd/sun.tools.jcmd.JCmd.main(JCmd.java:97) >> >> >> With this patch, neither jcmd nor kill -3 will disrupt java process 45850. >> >> $java -Xms64g -XX:+AlwaysPreTouch -XX:ParallelGCThreads=1 >> -Xlog:os+init=debug:stderr -version & >> [1] 45850 >> $ kill -3 45850 >> [6.902s][info][os,init] ignore BREAK_SIGNAL in the initialization phase. >> >> >> >> $jcmd 45850 VM.flags >> 45850: >> [19.920s][info][os,init] ignore BREAK_SIGNAL in the initialization phase. >> [25.422s][info][os,init] ignore BREAK_SIGNAL in the initialization phase. >> [26.522s][info][os,init] ignore BREAK_SIGNAL in the initialization phase. >> [27.723s][info][os,init] ignore BREAK_SIGNAL in the initialization phase. >> [29.023s][info][os,init] ignore BREAK_SIGNAL in the initialization phase. >> [30.423s][info][os,init] ignore BREAK_SIGNAL in the initialization phase. >> com.sun.tools.attach.AttachNotSupportedException: Unable to open socket file >> /proc/45850/root/tmp/.java_pid45850: target process 45850 doesn't respond >> within 10500ms or HotSpot VM not loaded >> at >> jdk.attach/sun.tools.attach.VirtualMachineImpl.(VirtualMachineImpl.java:105) >> at >> jdk.attach/sun.tools.attach.AttachProviderImpl.attachVirtualMachine(AttachProviderImpl.java:58) >> at >> jdk.attach/com.sun.tools.attach.VirtualMachine.attach(VirtualMachine.java:207) >> at jdk.jcmd/sun.tools.jcmd.JCmd.executeCommandForPid(JCmd.java:113) >> at jdk.jcmd/sun.tools.jcmd.JCmd.main(JCmd.java:97) >> $openjdk version "19-internal" 2022-09-20 >> OpenJDK Runtime Environment (fastdebug build 19-internal+0-adhoc.xxinliu.jdk) >> OpenJDK 64-Bit Server VM (fastdebug build 19-internal+0-adhoc.xxinliu.jdk, >> mixed mode, sharing) >> [1] + 45850 done java -Xms64g -XX:+AlwaysPreTouch >> -XX:ParallelGCThreads=1 -version > > Xin Liu has updated the pull request incrementally with one additional commit > since the last revision: > > Only install JVM_HANDLE_XXX_SIGNAL when ReduceSignalUsage is > false(default value). > > This patch also adds a log message with 'os+init=info'. Hi -- thanks for updating the bug title and text. Yes it's much better to start with a concise problem description. I'm in favour of the signal hander change. I'm not personally concerned about printing, silently handling SIGQUIT seems fine for a VM at this stage, perhaps printing just adds risk. Still curious that I don't reproduce the problem by making heap initialization slow with options like -Xms100g -XX:+AlwaysPreTouch as you could. Startup can be so slow I can attach gdb and see it's in: Threads::create_vm / init_globals / universe_init /
Re: RFR: 8279124: VirtualMachineImpl should check signal handler has installed before sending SIGQUIT
On Mon, 17 Jan 2022 08:10:57 GMT, Thomas Stuefe wrote: > I propose a simpler and more robust way to fix it though Great, this is the kind of thing I was heading towards with the conversation in the bug text. Although not sure why I could not reproduce the problem, with various different JDK versions. - PR: https://git.openjdk.java.net/jdk/pull/7003
Re: RFR: 8279918: Fix various doc typos
On Thu, 13 Jan 2022 10:30:07 GMT, Pavel Rappo wrote: > - Most of the typos are of a trivial kind: missing whitespace. > - If any of the typos should be fixed in the upstream projects instead, > please say so; I will drop those typos from the patch. > - As I understand it, ` ` in ImageInputStream and DataInput is an irrelevant > formatting artefact and thus can be removed. > - `` is an apostrophe, which does not require to be encoded. Marked as reviewed by kevinw (Committer). - PR: https://git.openjdk.java.net/jdk/pull/7063
Re: RFR: 8279918: Fix various doc typos
On Thu, 13 Jan 2022 11:04:43 GMT, Pavel Rappo wrote:
>> src/java.sql/share/classes/java/sql/BatchUpdateException.java line 58:
>>
>>> 56: * A JDBC driver implementation should use
>>> 57: * the constructor {@code BatchUpdateException(String reason, String
>>> SQLState,
>>> 58: * int vendorCode, long []updateCounts, Throwable cause) } instead of
>>
>> While we are here, is it more normal to say "long[] updateCounts", not
>> separating the [] from the type.
>> Similarly at line 81, 118, 151, 247, 277, 318, 354.
>
> I thought about it too, but then noticed how the position of `[]` mimics that
> of the respective method signatures in code.
OK, so lines 264, 295, 329, 364, 431 are arguably wrong as well? Separating
the [] completely looks quite rare.
I'll leave it up to you. 8-)
-
PR: https://git.openjdk.java.net/jdk/pull/7063
Re: RFR: 8279918: Fix various doc typos
On Thu, 13 Jan 2022 10:30:07 GMT, Pavel Rappo wrote:
> - Most of the typos are of a trivial kind: missing whitespace.
> - If any of the typos should be fixed in the upstream projects instead,
> please say so; I will drop those typos from the patch.
> - As I understand it, ` ` in ImageInputStream and DataInput is an irrelevant
> formatting artefact and thus can be removed.
> - `` is an apostrophe, which does not require to be encoded.
src/java.sql/share/classes/java/sql/Statement.java line 784:
> 782: * statement returns a {@code ResultSet} object, the second argument
> 783: * supplied to this method is not an
> 784: * {@code int} array whose elements are valid column indexes, the
> method is called on a
Should it be "or the method is called on...", i.e. add the "or", otherwise it's
a list of problems but we don't know if they are all required, or are
alternatives. It probably does not mean that all these have to be true to
throw the exception, but it doesn't say that. We are nitpicking of course.
-
PR: https://git.openjdk.java.net/jdk/pull/7063
Re: RFR: 8279918: Fix various doc typos
On Thu, 13 Jan 2022 10:30:07 GMT, Pavel Rappo wrote:
> - Most of the typos are of a trivial kind: missing whitespace.
> - If any of the typos should be fixed in the upstream projects instead,
> please say so; I will drop those typos from the patch.
> - As I understand it, in ImageInputStream and DataInput is an irrelevant
> formatting artefact and thus can be removed.
> - is an apostrophe, which does not require to be encoded.
src/java.sql/share/classes/java/sql/BatchUpdateException.java line 58:
> 56: * A JDBC driver implementation should use
> 57: * the constructor {@code BatchUpdateException(String reason, String
> SQLState,
> 58: * int vendorCode, long []updateCounts, Throwable cause) } instead of
While we are here, is it more normal to say "long[] updateCounts", not
separating the [] from the type.
Similarly at line 81, 118, 151, 247, 277, 318, 354.
-
PR: https://git.openjdk.java.net/jdk/pull/7063
Re: RFR: 8279918: Fix various doc typos
On Thu, 13 Jan 2022 10:30:07 GMT, Pavel Rappo wrote: > - Most of the typos are of a trivial kind: missing whitespace. > - If any of the typos should be fixed in the upstream projects instead, > please say so; I will drop those typos from the patch. > - As I understand it, in ImageInputStream and DataInput is an irrelevant > formatting artefact and thus can be removed. > - is an apostrophe, which does not require to be encoded. src/java.base/share/classes/sun/text/RuleBasedBreakIterator.java line 73: > 71: * will be transparent to the BreakIterator. A sequence of > characters will break the > 72: * same way it would if any ignore characters it contains are taken > out. Break > 73: * positions never occur before ignore characters. "before ignored characters" - PR: https://git.openjdk.java.net/jdk/pull/7063
Integrated: 8278597: Remove outdated comments regarding RMISecurityManager in HotSpotAgent.java
On Wed, 12 Jan 2022 15:04:04 GMT, Kevin Walls wrote: > The HotSpotAgent.java setupDebugger method has a commmented out section > relating to possibly using RMISecurityManager. > The comment is from pre-jdk7. As RMISecurityManager has been deprecated for > a while the comments should be removed. > > This is a comment-only change, no impact on what we build, so seems trivial. > > This is slightly more relevant today as with JEP411 and plans to remove the > Security Mananger, it is natural to search the source for references to > Security Manager. Removing a false-positive from those results is a good > thing. This pull request has now been integrated. Changeset: 69339346 Author:Kevin Walls URL: https://git.openjdk.java.net/jdk/commit/693393463385a966f9bf8a4569074c185c1f2863 Stats: 12 lines in 1 file changed: 0 ins; 11 del; 1 mod 8278597: Remove outdated comments regarding RMISecurityManager in HotSpotAgent.java Reviewed-by: rriggs, sspitsyn - PR: https://git.openjdk.java.net/jdk/pull/7050
Re: RFR: 8278597: Remove outdated comments regarding RMISecurityManager in HotSpotAgent.java
On Wed, 12 Jan 2022 15:04:04 GMT, Kevin Walls wrote: > The HotSpotAgent.java setupDebugger method has a commmented out section > relating to possibly using RMISecurityManager. > The comment is from pre-jdk7. As RMISecurityManager has been deprecated for > a while the comments should be removed. > > This is a comment-only change, no impact on what we build, so seems trivial. > > This is slightly more relevant today as with JEP411 and plans to remove the > Security Mananger, it is natural to search the source for references to > Security Manager. Removing a false-positive from those results is a good > thing. Thanks! - PR: https://git.openjdk.java.net/jdk/pull/7050
RFR: 8278597: Remove outdated comments regarding RMISecurityManager in HotSpotAgent.java
The HotSpotAgent.java setupDebugger method has a commmented out section relating to possibly using RMISecurityManager. The comment is from pre-jdk7. As RMISecurityManager has been deprecated for a while the comments should be removed. This is a comment-only change, no impact on what we build, so seems trivial. This is slightly more relevant today as with JEP411 and plans to remove the Security Mananger, it is natural to search the source for references to Security Manager. Removing a false-positive from those results is a good thing. - Commit messages: - 8278597: Remove outdated comments regarding RMISecurityManager in HotSpotAgent.java Changes: https://git.openjdk.java.net/jdk/pull/7050/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=7050=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8278597 Stats: 12 lines in 1 file changed: 0 ins; 11 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/7050.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7050/head:pull/7050 PR: https://git.openjdk.java.net/jdk/pull/7050
Re: RFR: 8272317: jstatd has dependency on Security Manager which needs to be removed [v2]
> Remove the use of Security Manager from jstatd. > Add use of an ObjectInputFilter to restrict RMI. > > Also we can undo the property-setting Launcher.gmk change from: 8279007: > jstatd fails to start because SecurityManager is disabled > ..as that is no longer needed. > > Docs/man page update to follow (JDK-8278619). Kevin Walls has updated the pull request incrementally with one additional commit since the last revision: Wildcard in object filter to permit proxies, in case other activity in this JVM changes the nameing/numbering of proxy classes. - Changes: - all: https://git.openjdk.java.net/jdk/pull/6919/files - new: https://git.openjdk.java.net/jdk/pull/6919/files/25ed85e7..e49b9fe2 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=6919=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=6919=00-01 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/6919.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/6919/head:pull/6919 PR: https://git.openjdk.java.net/jdk/pull/6919
RE: [libattach] misleading error message when checking gid fails
Hi - The file is "/proc/1974261/root/tmp/.java_pid1974261", so is that a JVM in a container, and is the group of the file in the container unknown to the host? ( so -1 is"nogroup") -Original Message- From: serviceability-dev On Behalf Of Serguei Spitsyn Sent: 07 January 2022 18:25 To: Baesken, Matthias ; Alan Bateman ; stuart nelson ; serviceability-dev@openjdk.java.net Subject: Re: [libattach] misleading error message when checking gid fails Hi, Just some addition to the below comment. The man page also has this: RATIONALE top In a conforming environment, getegid() will always succeed. It is possible for implementations to provide an extension where a process in a non-conforming environment will not be associated with a user or group ID. It is recommended that such implementations return (gid_t)-1 and set errno to indicate such an environment; doing so does not violate this standard, since such an environment is already an extension. I'm not sure what kind of a better message users would like to have in case the (gid_t)-1 is returned. Current message is pretty good providing this returned value which helps to identify problem. Thanks, Serguei On 1/7/22, 4:05 AM, "serviceability-dev on behalf of Baesken, Matthias" wrote: Hi, the manpage of getegid https://man7.org/linux/man-pages/man3/getegid.3p.html says : " The getegid() function shall always be successful and no return value is reserved to indicate an error." So I am not sure what kind of additional check or error message you would expect ? > In this case, the exception message is clear that the gid is -1 so it should help with debugging the issue. Yes , the message gives already some info, that's good (or better than nothing at least). Best regards, Matthias -Original Message- From: jdk-dev On Behalf Of Alan Bateman Sent: Freitag, 7. Januar 2022 12:48 To: stuart nelson ; jdk-...@openjdk.java.net Subject: Re: [libattach] misleading error message when checking gid fails On 07/01/2022 11:33, stuart nelson wrote: > Hey, > > First, apologies if this should be directed to a different mailing list, I didn't find one that seemed correct in the mailing lists (https://mail.openjdk.java.net/mailman/listinfo). > > I was building up a syscall filters list for a java process for seccomp, when I encountered this error stack trace: > > (elided) > Caused by: java.io.IOException: well-known file /proc/1974261/root/tmp/.java_pid1974261 is not secure: file's group should be the current group (which is -1) but the group is 1000 > at jdk.attach/sun.tools.attach.VirtualMachineImpl.checkPermissions(Native Method) > at jdk.attach/sun.tools.attach.VirtualMachineImpl.(VirtualMachineImpl.java:112) > at jdk.attach/sun.tools.attach.AttachProviderImpl.attachVirtualMachine(AttachProviderImpl.java:58) > at jdk.attach/com.sun.tools.attach.VirtualMachine.attach(VirtualMachine.java:207) > ... 6 more > > The error originates from this line: > https://hg.openjdk.java.net/jdk/jdk/file/ee1d592a9f53/src/jdk.attach/linux/native/libattach/VirtualMachineImpl.c#l167 > > The value for gid is found on this line: > https://hg.openjdk.java.net/jdk/jdk/file/ee1d592a9f53/src/jdk.attach/linux/native/libattach/VirtualMachineImpl.c#l150 > > The reason getegid() returns -1 is because it wasn't in my allowed syscalls list for seccomp, so EPERM (-1) was returned instead. > > My question is: -1 is an invalid gid. Should this be checked in the code, and a more helpful error message returned? It could definitely save future developers time. > serviceability-dev is the mailing lists for the Attach API. In this case, the exception message is clear that the gid is -1 so it should help with debugging the issue. -Alan
Re: RFR: 8272317: jstatd has dependency on Security Manager which needs to be removed
On Wed, 22 Dec 2021 18:14:43 GMT, Kevin Walls wrote: > Remove the use of Security Manager from jstatd. > Add use of an ObjectInputFilter to restrict RMI. > > Also we can undo the property-setting Launcher.gmk change from: 8279007: > jstatd fails to start because SecurityManager is disabled > ..as that is no longer needed. > > Docs/man page update to follow (JDK-8278619). Thanks for the comments - The proxy objects are needed in the filter for this to work at all. The proxy names/numbers and innner class names/numbers are predictable and stable IF we are using jstatd and jstat as standalone programs. But they are unstable if there is other relevant activity in the VM process, e.g. a JMX connection comes in before they are created. We should wildcard the proxy names to work in such a situation: com.sun.proxy.jdk.proxy* I'll mention also that jstatd has always been an experimental feature. The man page warns about lack of authentication and advises usage with caution. - PR: https://git.openjdk.java.net/jdk/pull/6919
Re: RFR: 8250801: Add clhsdb "threadcontext" command [v2]
On Sun, 2 Jan 2022 00:56:49 GMT, Chris Plummer wrote:
>> SA has the ability to fetch the thread's registers via the thread context.
>> It would be nice to allow access to the registers from clhsdb. This plays in
>> well with the enhancements being done to PointerFinder as part of
>> JDK-8247514. Many of the register values will then be automatically
>> displayed as symbols, Methods, Threads, stack offsets, nmethods, interpreter
>> codelets, etc.
>>
>> During some recent debugging I did I found it useful to dump a thread's
>> registers in this manner. Although in this case I was inlining the code in
>> the part of SA where I wanted to see the registers, having it as a clhsdb
>> command would not only be useful to user, but also useful when debugging SA
>> because it would serve as a code snippet to copy-n-paste where needed.
>>
>> The syntax is:
>>
>> threadcontext [-v] { -a | id }
>>
>> Where -a displays all threads, and "id" is used to display a specific
>> thread. This is the same argument syntax as some other commands that let
>> you choose all threads or just one thread, such as the "thread" and "where"
>> commands. -v just means more verbose output, whereas without it for the most
>> part each register printed will just take up one line.
>
> Chris Plummer has updated the pull request incrementally with four additional
> commits since the last revision:
>
> - Update the copyright this time.
> - Really update the copyright this time.
> - Update copyright and only print the register name.
> - Update copyright and remove @bug reference
Marked as reviewed by kevinw (Committer).
-
PR: https://git.openjdk.java.net/jdk/pull/6925
Re: RFR: 8250801: Add clhsdb "threadcontext" command
On Thu, 23 Dec 2021 04:06:58 GMT, Chris Plummer wrote:
> SA has the ability to fetch the thread's registers via the thread context. It
> would be nice to allow access to the registers from clhsdb. This plays in
> well with the enhancements being done to PointerFinder as part of
> JDK-8247514. Many of the register values will then be automatically displayed
> as symbols, Methods, Threads, stack offsets, nmethods, interpreter codelets,
> etc.
>
> During some recent debugging I did I found it useful to dump a thread's
> registers in this manner. Although in this case I was inlining the code in
> the part of SA where I wanted to see the registers, having it as a clhsdb
> command would not only be useful to user, but also useful when debugging SA
> because it would serve as a code snippet to copy-n-paste where needed.
>
> The syntax is:
>
> threadcontext [-v] { -a | id }
>
> Where -a displays all threads, and "id" is used to display a specific thread.
> This is the same argument syntax as some other commands that let you choose
> all threads or just one thread, such as the "thread" and "where" commands. -v
> just means more verbose output, whereas without it for the most part each
> register printed will just take up one line.
Looks useful to expose this info if we have it!
-
Marked as reviewed by kevinw (Committer).
PR: https://git.openjdk.java.net/jdk/pull/6925
Re: RFR: 8250801: Add clhsdb "threadcontext" command
On Thu, 23 Dec 2021 04:06:58 GMT, Chris Plummer wrote:
> SA has the ability to fetch the thread's registers via the thread context. It
> would be nice to allow access to the registers from clhsdb. This plays in
> well with the enhancements being done to PointerFinder as part of
> JDK-8247514. Many of the register values will then be automatically displayed
> as symbols, Methods, Threads, stack offsets, nmethods, interpreter codelets,
> etc.
>
> During some recent debugging I did I found it useful to dump a thread's
> registers in this manner. Although in this case I was inlining the code in
> the part of SA where I wanted to see the registers, having it as a clhsdb
> command would not only be useful to user, but also useful when debugging SA
> because it would serve as a code snippet to copy-n-paste where needed.
>
> The syntax is:
>
> threadcontext [-v] { -a | id }
>
> Where -a displays all threads, and "id" is used to display a specific thread.
> This is the same argument syntax as some other commands that let you choose
> all threads or just one thread, such as the "thread" and "where" commands. -v
> just means more verbose output, whereas without it for the most part each
> register printed will just take up one line.
src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/runtime/JavaThread.java
line 489:
> 487: for (int r = 0; r < tc.getNumRegisters(); r++) {
> 488: Address regAddr = tc.getRegisterAsAddress(r);
> 489: System.out.format("Register(%s): %s", tc.getRegisterName(r),
> regAddr);
Would you consider just printing name = value, rather than printing the word
Register many times? 8-)
-
PR: https://git.openjdk.java.net/jdk/pull/6925
Re: RFR: 8279119: src/jdk.hotspot.agent/doc/index.html file contains references to scripts that no longer exist
On Thu, 23 Dec 2021 03:45:31 GMT, Chris Plummer wrote: > The SA src/jdk.hotspot.agent/doc/index.html file contains a table of SA > scripts and their descriptions. All of these scripts are now gone. They were > used to make it easier to launch what today we would call SA tools. Now all > these tools are launched as sub-commands of the jhsdb command, which has its > own man page. > > I also did some other minor fixes and cleanups. > > The rendered html can be found here: > https://htmlpreview.github.io/?https://raw.githubusercontent.com/openjdk/jdk/53d2753cf32ef65ddea631062ca8641c3d19f27b/src/jdk.hotspot.agent/doc/index.html Nice refresh/cleanup. src/jdk.hotspot.agent/doc/index.html line 51: > 49: > 50: When a core dump is moved from the machine where it was produced to a > 51: difference machine, it may not always be possible for SA to debug it. "difference machine" rather than "different machine " is not your typo but if you had time while we are here... 8-) - Marked as reviewed by kevinw (Committer). PR: https://git.openjdk.java.net/jdk/pull/6924
Re: RFR: 8272317: jstatd has dependency on Security Manager which needs to be removed
On Wed, 22 Dec 2021 21:41:13 GMT, Mandy Chung wrote: >> Remove the use of Security Manager from jstatd. >> Add use of an ObjectInputFilter to restrict RMI. >> >> Also we can undo the property-setting Launcher.gmk change from: 8279007: >> jstatd fails to start because SecurityManager is disabled >> ..as that is no longer needed. >> >> Docs/man page update to follow (JDK-8278619). > > src/jdk.jstatd/share/classes/sun/tools/jstatd/Jstatd.java line 51: > >> 49: private static RemoteHost remoteHost; >> 50: >> 51: private static final String rmiFilterPattern = >> "sun.jvmstat.monitor.remote.RemoteVm;com.sun.proxy.jdk.proxy1.$Proxy1;com.sun.proxy.jdk.proxy1.$Proxy2;java.lang.reflect.Proxy;java.rmi.server.RemoteObjectInvocationHandler;java.rmi.server.RemoteObject;!*"; > > The class name of the dynamic proxy is generated at runtime and can be > different. As Bernd commented, the proxy classes cannot/should not be > listed in the filter pattern. OK thanks - I was trying the minimal pattern to overcome rejections such as the following, captured in logs on different runs: ObjectInputFilter REJECTED: class com.sun.proxy.jdk.proxy1.$Proxy1, array length: -1, nRefs: 2, depth: 1, bytes: 84, ex: n/a ObjectInputFilter REJECTED: class com.sun.proxy.jdk.proxy1.$Proxy2, array length: -1, nRefs: 2, depth: 1, bytes: 84, ex: n/a - PR: https://git.openjdk.java.net/jdk/pull/6919
RFR: 8272317: jstatd has dependency on Security Manager which needs to be removed
Remove the use of Security Manager from jstatd. Add use of an ObjectInputFilter to restrict RMI. Also we can undo the property-setting Launcher.gmk change from: 8279007: jstatd fails to start because SecurityManager is disabled ..as that is no longer needed. Docs/man page update to follow (JDK-8278619). - Commit messages: - Remove jstad launcher property setting to allow Security Manager. - Merge remote-tracking branch 'upstream/master' into 8272317_jstatd_secmgr - Add ObjectInputFilter - Merge remote-tracking branch 'upstream/master' into 8272317_jstatd_secmgr - 8272317: jstatd has dependency on Security Manager which needs to be removed Changes: https://git.openjdk.java.net/jdk/pull/6919/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=6919=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8272317 Stats: 27 lines in 4 files changed: 4 ins; 15 del; 8 mod Patch: https://git.openjdk.java.net/jdk/pull/6919.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/6919/head:pull/6919 PR: https://git.openjdk.java.net/jdk/pull/6919
