On Wed, 19 Apr 2023 23:40:56 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. test/hotspot/jtreg/vmTestbase/nsk/jdi/ThreadReference/stop/stop002.java line 169: > 167: thrRef.stop(throwableRef); > 168: log.display("TEST #2 PASSED: stop() call succeeded."); > 169: } catch(Exception ue) { nit Suggestion: } catch (Exception ue) { test/hotspot/jtreg/vmTestbase/nsk/jdi/ThreadReference/stop/stop002.java line 192: > 190: log.display("TEST #3 PASSED: stop() call > succeeded."); > 191: } > 192: } catch(Exception ue) { nit Suggestion: } catch (Exception ue) { test/hotspot/jtreg/vmTestbase/nsk/jdi/ThreadReference/stop/stop002.java line 223: > 221: log.display("TEST #4 PASSED: stop() call > succeeded."); > 222: } > 223: } catch(Throwable ue) { Suggestion: } catch (Throwable ue) { test/hotspot/jtreg/vmTestbase/nsk/jdi/ThreadReference/stop/stop002.java line 255: > 253: log.display("TEST #5 PASSED: stop() call for > suspended thread succeeded"); > 254: } > 255: } catch(Throwable ue) { Suggestion: } catch (Throwable ue) { test/hotspot/jtreg/vmTestbase/nsk/jdi/ThreadReference/stop/stop002.java line 274: > 272: } finally { > 273: // Force the debuggee out of both loops > 274: if (objRef != null && stopLoop1 != null && stopLoop2 != > null) { Suggestion: if (objRef != null && stopLoop1 != null && stopLoop2 != null) { ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/13548#discussion_r1174633416 PR Review Comment: https://git.openjdk.org/jdk/pull/13548#discussion_r1174633435 PR Review Comment: https://git.openjdk.org/jdk/pull/13548#discussion_r1174633460 PR Review Comment: https://git.openjdk.org/jdk/pull/13548#discussion_r1174633464 PR Review Comment: https://git.openjdk.org/jdk/pull/13548#discussion_r1174633486
