On Fri, 28 Apr 2023 21:30:23 GMT, Chris Plummer <cjplum...@openjdk.org> wrote:
>> Note this PR depends on the #13546 PR for the following: >> >> [JDK-8306434](https://bugs.openjdk.org/browse/JDK-8306434): add support of >> virtual threads to JVMTI StopThread >> >> So it can't be finalized and push until after JDK-8306434 is pushed. There >> will also be GHA failures until then. If JDK-8306434 results in any >> additional spec changes, they will likely impact this CR also. >> >> >> [JDK-8306034](https://bugs.openjdk.org/browse/JDK-8306034) is adding some >> virtual thread support to JVMTI StopThread. This will allow JDWP >> ThreadReference.Stop and JDI ThreadReference.stop() to have the same level >> support for virtual threads. >> >> Mostly this is a spec update for JDWP and JDI, but there are also some minor >> changes needed to the ThreadReference.stop() handling of JDWP errors, and >> throwing the appropriate exceptions. Also some minor cleanup in jdb. The >> debug agent doesn't need changes since JVMTI errors are just passed through >> as the corresponding JDWP errors, and the code for this is already in place. >> These errors are not new nor need any special handling. >> >> Our existing testing for ThreadReference.stop() is fairly weak: >> >> - nsk/jdb/kill/kill001, which tests stop() when the thread is suspended at a >> breakpoint. It will get problem listed by >> [JDK-8306034](https://bugs.openjdk.org/browse/JDK-8306034). I have fixes for >> it already working and will push it separately. >> - nsk/jdi/stop/stop001, which is problem listed and only tests when the >> thread is blocked in Object.wait() >> - nsk/jdi/stop/stop002, which only tests that throwing an invalid exception >> fails properly >> >> I decided to take stop002 and make it a more thorough test of >> ThreadReference.stop(). See the comment at the top for a list of what is >> tested. As for reviewing this test, it's probably best to look at it as a >> completely new test rather than looking at diffs since so much has been >> changed and added. > > Chris Plummer has updated the pull request incrementally with one additional > commit since the last revision: > > Some test logging improvements. test/hotspot/jtreg/vmTestbase/nsk/jdi/ThreadReference/stop/stop002.java line 63: > 61: // debuggee local var used to find needed non-throwable object > 62: static final String DEBUGGEE_NON_THROWABLE_VAR= > "stop002tNonThrowable"; > 63: // debuggee local var used to find needed non-throwable object This comment is a little bit confusing. Should it be one line up or it is intentionally placed before `DEBUGGEE_THROWABLE_VAR`? test/hotspot/jtreg/vmTestbase/nsk/jdi/ThreadReference/stop/stop002.java line 300: > 298: IntegerValue ival; > 299: do { > 300: ival = > (IntegerValue)mainClass.getValue(mainClass.fieldByName("testNumReady")); Do we need a sleep at each iteration? test/hotspot/jtreg/vmTestbase/nsk/jdi/ThreadReference/stop/stop002t.java line 78: > 76: /* > 77: * TEST #2: async exception while suspended at a breakpoint. > 78: */ Where is a similar comment for TEST #1 ? Would it make sense to implement each subtest as a separate method? ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/13548#discussion_r1182993575 PR Review Comment: https://git.openjdk.org/jdk/pull/13548#discussion_r1182999880 PR Review Comment: https://git.openjdk.org/jdk/pull/13548#discussion_r1182989578
