RFR: 8306467: Fix nsk/jdb/kill/kill001 to work with new JVMTI StopThread support for virtual threads. [v2]

[Chris Plummer]

On Sat, 13 May 2023 03:59:12 GMT, Chris Plummer <cjplummer at openjdk.org> wrote:

>>> The debugger side checks for the proper notKilled value (it should be 0). So the test still passes even though it never confirmed that the correct async exception was thrown and caught. It just knows that the thread did not exit cleanly. Probably instead of tracking `notKilled`, the test should track `killed` and only increment when the correct exception is actually caught.
>> 
>> I think this will make the test more reliable.
>> 
>>> I'm not so sure the sleep is even necessary since we block in the synchronized indefinitely until the async exception is thrown. I think just a simple statement at the start of the try block is good enough. Maybe a println(). But this is somewhat fragile because we need to make sure there is no code between the synchronized and the try, so we probably need to move the synchronized to within the try also.
>> 
>> looks like we just need to move synchronized (or some other code to force vthread to be mounted) to inside the try and remove Thread.sleep, no other statements are needed.
>
>> looks like we just need to move synchronized (or some other code to force vthread to be mounted) to inside the try and remove Thread.sleep, no other statements are needed.
> 
> That should work. At first I was unsure because the async exception is not actually thrown until executing the next statement after the synchronized. But there should be a branch at the end of the try block, and the exception should be thrown before it is executed. Hopefully the exception table covers that bytecode as being in the try block.

This became a lot trickier to understand than I expected. The JVM does not have to throw the async exception at the next executed bytecode. The interpreter seems to do it at an invoke or after a goto (so the target of the goto is the instruction that will appear to have thrown the async exception). I think JIT'd code is similar, although maybe throws on a backwards branch and not after a forward branch. For this reason we need some sort of statement in place after the synchronized block to trigger the throwing of the exception before we leave the try block.

Another thing that messes up the test is that within the synchronized block, you can't have anything that will trigger the throw of the async exception. For example:

        try {
            synchronized (kill001a.lock) {
                kill001a.log.display("entered synchronized");
            }
            kill001a.log.display("exited synchronized");
        } catch (Throwable t) {

This messes up the test because now the exception will be thrown from within the synchronized block. This triggers a catch of the exception by an implicit catch clauses whose only purpose is to do the monitorexit and then rethrow the exception (which the explicit catch clause will then catch). The problem is that the test only expects one throw per thread, and now it gets 2. Although fixable on the debugger side, it is probably best to avoid.

I've pushed an update. Let me know if you think it is ok.

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/13967#discussion_r1194326288