RFR: Better jdb support for vthreads. Improved jdb vthread testing support. [v2]
Chris Plummer
cjplummer at openjdk.java.net
Tue Mar 8 06:13:18 UTC 2022
> This PR contains a set of some what disparate but related set of changes to JDB, JDI, and nsk jdb tests and test support (Jdb.java).
>
> jdb: Made the “kill” command support `ThreadReference.stop()` throwing `IllegalThreadStateException` for vthreads, because this is how `ThreadReference.stop()` is now spec’d. This change is in `Commands.java`. The rest of the changes are just cleanup. One of the changes in the `kill001` test is to test for this (see below)
>
> jdb: Use `PlatformThreadOnlyFilters` to prevent `ThreadStart/Death` event for vthreads. Instead detect vthreads when events are received on them. The goal here is to prevent jdb from getting a flood of `ThreadStart/Death` events, and getting bogged down with them. With the `PlatformThreadOnlyFilters` in place, this means jdb is not notified when a vthread dies, even one that it learned about later when an event came in on it. For these each of these “learned” vthreads a separate `ThreadDeathRequest` is needed that uses the thread itself as a `ThreadFilter`. This way jdb can tell when these vthreads die. This part is handled in `EventHandler.java`. `VMConnection.java` is where `PlatformThreadOnlyFilters` are added.
>
> jdb: Add `-trackallvthreads` option. This prevents the use of the `PlatformThreadsOnlyFilters` described above, and instead relies on jdb getting the initial set of vthreads from the debug agent via `VM.allThreads()` (by turning on the `enumeratevthreads` option), and by getting a `ThreadStart/Death` event for every vthread created/destroyed after connecting. This is basially how jdb used to work by default before any changes in this PR. There are 3 tests that rely on this behaviour and need this flag. They are `kill001`, `threads002`, and `trace001` (see below). Many more would rely on it if they were converted to use vthreads as these tests were. The bulk of the changes for this support comes from having to pass `trackAllVthreads` through all sorts of constuctors/inits. I wish there were a better way, but I couldn’t find a place to store this value as a static so it is easy to access. If you see a better way, let me know.
>
> jdb: Track vthreads separately. jdb keeps track of every thread by creating a `ThreadInfo` for each and storing it in a `List<ThreadInfo>` called `threads`. I decided to store vthreads in a separate list. The reason is the list is needed by the `threads` command. It doesn’t use the `threads` list, but instead walks the `ThreadGroup` hierarchy and prints out the threads within each `ThreadGroup`. This doesn’t capture vthreads since they are not returned by `ThreadGroupReference.threads()`, even for the `VirtualThreads` group. So after the `threads` command has done its usual `ThreadGroup` traversal, it now also dumps all the vthreads. This is pretty much all hidden in `ThreadIterator.next()`, which automically transitions to getting threads from the vthread list after traversing all the `ThreadGroups`.
>
> `nsk/share/jdb/Jdb.java`: Added support for `getThreadIdsByName()`: There are many jdb tests that use `Jdb.getThreadIds(<classname>)` to get all threads in the `threads` output that are an instance of `<classname>`. The problem is that after converting a test to support a vthread mode for all debuggee threads, the name of the class in the output will always be `java.lang.VirtualThread` (or `java.lang.Thread` when not in vthread mode), and not the test specific class name, which is usually something like `MyThread`. Making `getThreadIds()` search for `java.lang.VirtualThreads` usually ends up including threads not wanted (ones that are not `MyThread`) such as the main test thread. So `getThreadIdsByName()` was added to allow searching by the thread name (actually just the prefix of the name) rather than its class. As an example, the `threads` command output used to contain a line such as:
>
> ` (nsk.jdb.kill.kill001.MyThread)693 Thread-0 waiting in a monitor`
>
> Now it contains:
>
> ` (java.lang.Thread)693 MyThread-0 waiting in a monitor`
>
> So instead of searching for “(MyThread)” and grabbing the threadId that comes right after it, `getThreadIdsByName()` first finds all the lines of output that contain “nsk.jdb.kill.kill001.MyThread”, and then for those lines only gets the threadID that is right after “(java.lang.Thread)” or “(java.lang.VirtualThread)”.
>
> jdb `kill001` test: Convert to support all debuggee threads being vthreads by using `JDIThreadFactory`. Enable new jdb `-trackallvthreads` option. Use new `getThreadIdsByName()` support instead of `getThreadIds()`. Allow/expect the `kill` command to fail with "Illegal thread state” when in vthread mode.
>
> jdb `threads002` test: Convert to support all debuggee threads being vthreads by using `JDIThreadFactory`. Enable new jdb `-trackallvthreads` option. I forget why I decided to convert this test. Seems it was useful for testing something I was working on for this PR, but I can’t recall. Maybe it was so I could clone it for the new `threads003` test below.
>
> jdb `trace001` test: Convert to support all debuggee threads being vthreads by using `JDIThreadFactory`. Enable new jdb `-trackallvthreads` option. This is a test I had converted long ago when I first experimented with the `getThreadIdsByName()` support, so I included it with this PR.
>
> jdb `threads003` test: This is a new thread test that tests jdb’s ability to learn about a thread when an event arrives on it. It creates 5 threads, two of which receive a breakpoint. It looks at the `threads` output and checks that only those two vthreads are present. I also used this test to help confirm that the `ThreadDeath` events are only received for these two vthreads. This was done manually by looking for the “TDR removed” output generated by `EventHandler.threadDeathEvent()`. I didn’t put this into the test itself since that println is just for debugging and is now commented out.
Chris Plummer has updated the pull request incrementally with one additional commit since the last revision:
Minor cleanup. Fix assert.
-------------
Changes:
- all: https://git.openjdk.java.net/loom/pull/97/files
- new: https://git.openjdk.java.net/loom/pull/97/files/174483ea..f3f8acf9
Webrevs:
- full: https://webrevs.openjdk.java.net/?repo=loom&pr=97&range=01
- incr: https://webrevs.openjdk.java.net/?repo=loom&pr=97&range=00-01
Stats: 12 lines in 2 files changed: 6 ins; 5 del; 1 mod
Patch: https://git.openjdk.java.net/loom/pull/97.diff
Fetch: git fetch https://git.openjdk.java.net/loom pull/97/head:pull/97
PR: https://git.openjdk.java.net/loom/pull/97
More information about the loom-dev
mailing list