On Thu, 30 Jun 2022 14:54:47 GMT, Zhengyu Gu <[email protected]> wrote:
>> I just noticed the test is being added to a newly created
>> hotspot/jtreg/serviciability/jdi directory. It should be placed in one of
>> the existing jdi test locations, either nsk/jdi or jdk/com/sun/jdi. I think
>> the latter would be better and it can leverage TestScaffold.
>>
>> Another reason to use TestScaffold is that it will automatically add any
>> specified VM options to the debuggee:
>>
>>
>> String vmOpts = System.getProperty("test.vm.opts");
>> System.out.println("vmOpts: '" + vmOpts + "'");
>>
>>
>> This way when we do our testing with various GC configurations, it should
>> end up testing the debuggee with each GC configuration also. Same goes with
>> other VM options that get tested like -Xcomp.
>
>> I just noticed the test is being added to a newly created
>> hotspot/jtreg/serviciability/jdi directory. It should be placed in one of
>> the existing jdi test locations, either nsk/jdi or jdk/com/sun/jdi. I think
>> the latter would be better and it can leverage TestScaffold.
>>
>> Another reason to use TestScaffold is that it will automatically add any
>> specified VM options to the debuggee:
>>
>> ```
>> String vmOpts = System.getProperty("test.vm.opts");
>> System.out.println("vmOpts: '" + vmOpts + "'");
>> ```
>>
>> This way when we do our testing with various GC configurations, it should
>> end up testing the debuggee with each GC configuration also. Same goes with
>> other VM options that get tested like -Xcomp.
>
> Moved new test to jdk/com/sun/jdi.
> @zhengyu123 I uploaded a patch to the test. It now tests EventSets with two
> ClassUnloadEvents. See http://cr.openjdk.java.net/~cjplummer/8256811/. It
> still needs some cleaning up, but look it over first and let me know what you
> think.
>
> One issue is you have a big try block around the event handling code that is
> catching and not rethrowing exceptions. I'm not sure why that is needed. It
> is preventing exceptions for some new checks I added from propagating.
>
> The change I made was two split the class names into two groups. The first
> has the class name prefix you setup, and the second has the same prefixe, but
> with __ALT added to it. Half of the classes are created with the regular
> prefix and half with the ALT prefix. The ClassUnloadRequest you setup should
> match all the unloaded classes, but the 2nd ClassUnloadRequest I setup should
> only match the __ALT classes. That means the EventSet for a ClassUnloadEvent
> WITH the ALT name should contain two ClassUnloadEvents, one for each
> ClassUnloadRequest, whereas the EventSet for a ClassUnloadEvent WITHOUT the
> ALT name should contain one ClassUnloadEvent since it won't match the ALT
> ClassUnloadRequest.
>
> I added a println of the EventSet that should help you understand this a bit
> better. Here's an example:
>
> ```
> Running debugger
> EventSet: event set, policy:2, count:1 = {VMStartEvent in thread main}
> EventSet: event set, policy:0, count:1 = {ClassUnloadEvent}
> EventSet: event set, policy:0, count:2 = {ClassUnloadEvent, ClassUnloadEvent}
> EventSet: event set, policy:0, count:2 = {ClassUnloadEvent, ClassUnloadEvent}
> EventSet: event set, policy:0, count:1 = {ClassUnloadEvent}
> EventSet: event set, policy:0, count:2 = {ClassUnloadEvent, ClassUnloadEvent}
> EventSet: event set, policy:0, count:1 = {ClassUnloadEvent}
> EventSet: event set, policy:0, count:2 = {ClassUnloadEvent, ClassUnloadEvent}
> EventSet: event set, policy:0, count:1 = {ClassUnloadEvent}
> EventSet: event set, policy:0, count:2 = {ClassUnloadEvent, ClassUnloadEvent}
> EventSet: event set, policy:0, count:1 = {ClassUnloadEvent}
> EventSet: event set, policy:0, count:1 = {VMDeathEvent}
> ```
>
> So you can see how half of the EventSets ended up with two ClassUnloadEvents,
> one for each ClassUnloadRequest, and the other half only one ClassUnloadEvent.
Thanks @plummercj
I incorporated your patch.
> One issue is you have a big try block around the event handling code that is
> catching and not rethrowing exceptions. I'm not sure why that is needed. It
> is preventing exceptions for some new checks I added from propagating.
>
This catch block intended to catch infrastructure failure, I changed `catch`
statement to catch specific exceptions, that will allow your `RuntimeException`
to propagate.
-------------
PR: https://git.openjdk.org/jdk/pull/9168
