On Fri, 11 Apr 2025 23:23:52 GMT, Chris Plummer <cjplum...@openjdk.org> wrote:
> This is just a preliminary review. I'd like to get some approval for the > approach I'm taking. There are over 300 tests that need to be fixed. I've > just fixed a handful in this PR to give a feel for the changes I plan on > making. If they look ok to you, then I'll update this PR with the needed > changes to the rest of the tests. > > What this PR is fixing is the issue with all of our nsk/jdi testing being > done with includevirtualthreads=y even though debuggers typically use the > default includevirtualthreads=n. As a result we have a testing gap with > includevirtualthreads=n. There are nearly 1200 nsk/jdi tests. Only about 350 > actually need includevirtualthreads=y. I plan making includevirtualthreads=n > the default for nsk/jdi tests unless the test does something to override the > default and request includevirtualthreads=y. > > includevirtualthreads=y forces the debug agent to track all virtual threads > so they are returned by vm.allThreads(). Some tests need this since they use > vm.allThreads() to find the debuggee threads. Without > includevirtualthreads=y, vm.allThreads() usually won't return any virtual > threads (although it might return some for which events have been triggered). Regarding the SerialExecutionDebugger change, some background might help. The following tests all use SerialExecutionDebugger: vmTestbase/nsk/jdi/stress/serial/forceEarlyReturn001/TestDescription.java vmTestbase/nsk/jdi/stress/serial/forceEarlyReturn002/TestDescription.java vmTestbase/nsk/jdi/stress/serial/heapwalking001/TestDescription.java vmTestbase/nsk/jdi/stress/serial/heapwalking002/TestDescription.java vmTestbase/nsk/jdi/stress/serial/mixed001/TestDescription.java vmTestbase/nsk/jdi/stress/serial/mixed002/TestDescription.java vmTestbase/nsk/jdi/stress/serial/monitorEvents001/TestDescription.java vmTestbase/nsk/jdi/stress/serial/monitorEvents002/TestDescription.java vmTestbase/nsk/jdi/stress/serial/ownedMonitorsAndFrames001/TestDescription.java vmTestbase/nsk/jdi/stress/serial/ownedMonitorsAndFrames002/TestDescription.java Each of these tests has a set of existing tests (subtests) that will be executed using the same debugger and debuggee processes. They are executed either in a random order (shuffle) or for a large number of iterations, both with a goal of being somewhat stressful. There is a .tests file that specifies the subtests to run and how to run them. For example, for vmTestbase/nsk/jdi/stress/serial/forceEarlyReturn002 we have: OPTIONS:shuffle nsk.jdi.ThreadReference.forceEarlyReturn.forceEarlyReturn001.forceEarlyReturn001 nsk.jdi.ThreadReference.forceEarlyReturn.forceEarlyReturn002.forceEarlyReturn002 nsk.jdi.ThreadReference.forceEarlyReturn.forceEarlyReturn003.forceEarlyReturn003 nsk.jdi.ThreadReference.forceEarlyReturn.forceEarlyReturn004.forceEarlyReturn004 nsk.jdi.ThreadReference.forceEarlyReturn.forceEarlyReturn005.forceEarlyReturn005 nsk.jdi.ThreadReference.forceEarlyReturn.forceEarlyReturn008.forceEarlyReturn008 nsk.jdi.ThreadReference.forceEarlyReturn.forceEarlyReturn013.forceEarlyReturn013 nsk.jdi.ThreadReference.forceEarlyReturn.forceEarlyReturn014.forceEarlyReturn014 Some of the subtests requiring launching the debug agent with includevirtualthreads=y. Usually the subtest would be managing this and making sure the debug agent is launched with includevirtualthreads=y. However, for subtests run by the SerialExecutionDebugger, it is the SerialExecutionDebugger class that is responsible launching the debug agent, not the test, and therefore the subtest has no way to enforce its requirement for includevirtualthreads=y. It turns out there is only subtest that needs includevirtualthreads=y: nsk.jdi.ThreadReference.forceEarlyReturn.forceEarlyReturn001.forceEarlyReturn002 You can see the fix for this test in this PR, but as pointed out above, this will only work when these test is run individually, not when run by SerialExecutionDebugger. Since SerialExecutionDebugger launches the debuggee, it needs to know whether or not to specify includevirtualthreads=y. At first I had it just always do that, but it was unnecessary for most of the tests, and reduced test coverage with includevirtualthreads=n. I then opted to add support for passing -includevirtualthreads to SerialExecutionDebugger. That way it's only necessary for the tests that include forceEarlyReturn002. If I want to take this further, I could add an additional @run command that does not use -includevirtualthreads, and also has a separate .test list that excludes forceEarlyReturn002, but I don't think this adds enough benefit to be worth while. ------------- PR Comment: https://git.openjdk.org/jdk/pull/24606#issuecomment-2798204267
