On Mon, 15 May 2023 20:22:53 GMT, Chris Plummer <cjplum...@openjdk.org> wrote:
>>> 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.
Looks good to me.
Please update "@summary" statement about "notKilled" (now "killed") field
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/13967#discussion_r1194369025
