Thank you for review and noticing issues. Still waiting for a second review.
Leonid > On Jul 29, 2020, at 12:08 AM, [email protected] wrote: > > It looks good. > Thank you for the update! > > Thanks, > Serguei > > > On 7/28/20 23:26, Leonid Mesnik wrote: >> ok, let change it to following >> >> diff -r f489d5d13a51 >> test/hotspot/jtreg/vmTestbase/nsk/jdi/EventRequest/setEnabled/setenabled002.java >> --- >> a/test/hotspot/jtreg/vmTestbase/nsk/jdi/EventRequest/setEnabled/setenabled002.java >> Thu Jul 23 16:36:44 2020 -0400 >> +++ >> b/test/hotspot/jtreg/vmTestbase/nsk/jdi/EventRequest/setEnabled/setenabled002.java >> Tue Jul 28 23:17:22 2020 -0700 >> @@ -368,6 +368,7 @@ >> throw new JDITestRuntimeException("** default case 2 >> **"); >> } >> >> + vm.suspend(); >> if (eventRequest1 instanceof StepRequest) { >> try { >> log2("......eventRequest1.setEnabled(true); >> IllegalThreadStateException is expected"); >> @@ -405,6 +406,7 @@ >> } catch ( InvalidRequestStateException e ) { >> log2(" InvalidRequestStateException"); >> } >> + vm.resume(); >> >> //~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ >> } >> >> webrev: http://cr.openjdk.java.net/~lmesnik/8244537/webrev.04/ >> <http://cr.openjdk.java.net/~lmesnik/8244537/webrev.04/> >> Leonid >> >>> On Jul 28, 2020, at 11:06 PM, [email protected] >>> <mailto:[email protected]> wrote: >>> >>> http://cr.openjdk.java.net/~lmesnik/8244537/webrev.03/test/hotspot/jtreg/vmTestbase/nsk/jdi/EventRequest/setEnabled/setenabled002.java.frames.html >>> >>> <http://cr.openjdk.java.net/~lmesnik/8244537/webrev.03/test/hotspot/jtreg/vmTestbase/nsk/jdi/EventRequest/setEnabled/setenabled002.java.frames.html> >>> >>> I'd suggest to simplify it: >>> >>> - insert suspend before the line: >>> 371 if (eventRequest1 instanceof StepRequest) { >>> - keep resume at the line: >>> 411 vm.resume(); >>> - these lines are not needed: >>> 389 vm.resume(); >>> 394 vm.suspend(); >>> >>> Thanks, >>> Serguei >>> >>> >>> On 7/28/20 21:15, Leonid Mesnik wrote: >>>> Included in webrev.03 >>>> http://cr.openjdk.java.net/~lmesnik/8244537/webrev.03 >>>> <http://cr.openjdk.java.net/~lmesnik/8244537/webrev.03> >>>> >>>> Leonid >>>> >>>>> On Jul 28, 2020, at 6:44 PM, [email protected] >>>>> <mailto:[email protected]> wrote: >>>>> >>>>> http://cr.openjdk.java.net/~lmesnik/8244537/webrev.01/test/hotspot/jtreg/vmTestbase/nsk/jdi/EventRequest/setEnabled/setenabled002.java.frames.html >>>>> >>>>> <http://cr.openjdk.java.net/~lmesnik/8244537/webrev.01/test/hotspot/jtreg/vmTestbase/nsk/jdi/EventRequest/setEnabled/setenabled002.java.frames.html> >>>>> 371 if (eventRequest1 instanceof StepRequest) { >>>>> 372 try { >>>>> 373 log2("......eventRequest1.setEnabled(true); >>>>> IllegalThreadStateException is expected"); >>>>> 374 eventRequest1.setEnabled(true); >>>>> 375 testExitCode = FAILED; >>>>> 376 log3("ERROR: NO IllegalThreadStateException >>>>> for StepRequest"); >>>>> 377 } catch ( IllegalThreadStateException e ) { >>>>> 378 log2(" IllegalThreadStateException"); >>>>> 379 } >>>>> 380 try { >>>>> 381 log2("......eventRequest1.setEnabled(false); >>>>> IllegalThreadStateException is not expected"); >>>>> 382 eventRequest1.setEnabled(false); >>>>> 383 log2(" no IllegalThreadStateException for >>>>> StepRequest"); >>>>> 384 } catch ( IllegalThreadStateException e ) { >>>>> 385 testExitCode = FAILED; >>>>> 386 log3("ERROR: IllegalThreadStateException"); >>>>> 387 } >>>>> 388 } >>>>> Above is one more case where suspend/resume is needed, I guess. >>>>> >>>>> Thanks, >>>>> Serguei >>>>> >>>>> >>>>> On 7/28/20 17:45, [email protected] >>>>> <mailto:[email protected]> wrote: >>>>>> http://cr.openjdk.java.net/~lmesnik/8244537/webrev.01/test/hotspot/jtreg/vmTestbase/nsk/jdi/EventRequest/setEnabled/setenabled003.java.frames.html >>>>>> >>>>>> <http://cr.openjdk.java.net/~lmesnik/8244537/webrev.01/test/hotspot/jtreg/vmTestbase/nsk/jdi/EventRequest/setEnabled/setenabled003.java.frames.html> >>>>>> 288 case 0: >>>>>> 289 thread1 = (ThreadReference) >>>>>> 290 >>>>>> debuggeeClass.getValue(debuggeeClass.fieldByName(threadName1)); >>>>>> 291 >>>>>> 292 log2("......setting up StepRequest"); >>>>>> 293 eventRequest1 = eventRManager.createStepRequest >>>>>> 294 (thread1, >>>>>> StepRequest.STEP_MIN, StepRequest.STEP_INTO); >>>>>> 295 >>>>>> 296 vm.suspend(); >>>>>> ... >>>>>> 360 default: >>>>>> 361 throw new JDITestRuntimeException("** default >>>>>> case 2 **"); >>>>>> 362 } >>>>>> 363 vm.resume(); >>>>>> >>>>>> Sorry, the fix is not going to work correctly. >>>>>> The first vm.suspend() has to be before the switch statement to work for >>>>>> all 3 cases. >>>>>> >>>>>> Thanks, >>>>>> Serguei >>>>>> >>>>>> >>>>>> On 7/28/20 16:49, Leonid Mesnik wrote: >>>>>>> I've update to suspend/resume in all cases. >>>>>>> >>>>>>> new webrev: http://cr.openjdk.java.net/~lmesnik/8244537/webrev.01/ >>>>>>> <http://cr.openjdk.java.net/~lmesnik/8244537/webrev.01/> >>>>>>> >>>>>>> Leonid >>>>>>> >>>>>>>> On Jul 28, 2020, at 2:06 PM, [email protected] >>>>>>>> <mailto:[email protected]> wrote: >>>>>>>> >>>>>>>> I prefer to suspend/resume in all cases, so we avoid all these >>>>>>>> unexpected failures. >>>>>>>> >>>>>>>> Thanks, >>>>>>>> Serguei >>>>>>>> >>>>>>>> >>>>>>>> On 7/28/20 13:59, Leonid Mesnik wrote: >>>>>>>>> It should be failure anyway if we managed to enable events, so we >>>>>>>>> don't expect to really enable anything in these cases. >>>>>>>>> However I agree that adding suspend/resume shouldn't make it worse, >>>>>>>>> just possible cleaner log (in very rare cases also). If you feel it >>>>>>>>> is need I will just add suspension for all cases. >>>>>>>>> >>>>>>>>> Leonid >>>>>>>>> >>>>>>>>>> On Jul 28, 2020, at 1:54 PM, [email protected] >>>>>>>>>> <mailto:[email protected]> wrote: >>>>>>>>>> >>>>>>>>>> Does it mean, you did not fix cases 0 and 2 because the related >>>>>>>>>> failures have never been observed? >>>>>>>>>> >>>>>>>>>> Thanks, >>>>>>>>>> Serguei >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> On 7/28/20 13:51, Leonid Mesnik wrote: >>>>>>>>>>> Test should fail in cases 0 and 2 with IllegalThreadStateException >>>>>>>>>>> if we can enable events. Such failures should be easily identified >>>>>>>>>>> by reading logs. >>>>>>>>>>> >>>>>>>>>>> Leonid >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>>> On Jul 27, 2020, at 10:28 PM, [email protected] >>>>>>>>>>>> <mailto:[email protected]> wrote: >>>>>>>>>>>> >>>>>>>>>>>> Hi Leonid, >>>>>>>>>>>> >>>>>>>>>>>> The fix looks good in general. >>>>>>>>>>>> You missed to explain that the suspend/resume are added to avoid >>>>>>>>>>>> actual generation of event that cause this issue. >>>>>>>>>>>> The reason is that these events are not actually required. >>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> http://cr.openjdk.java.net/~lmesnik/8244537/webrev.00/test/hotspot/jtreg/vmTestbase/nsk/jdi/EventRequest/setEnabled/setenabled003.java.frames.html >>>>>>>>>>>> >>>>>>>>>>>> <http://cr.openjdk.java.net/~lmesnik/8244537/webrev.00/test/hotspot/jtreg/vmTestbase/nsk/jdi/EventRequest/setEnabled/setenabled003.java.frames.html> >>>>>>>>>>>> 316 case 1: >>>>>>>>>>>> 317 vm.suspend(); >>>>>>>>>>>> ... >>>>>>>>>>>> 336 vm.resume(); >>>>>>>>>>>> >>>>>>>>>>>> Q: Why is only in case 1 suspend/resume used? >>>>>>>>>>>> What about cases 0 and 2? >>>>>>>>>>>> >>>>>>>>>>>> Thanks, >>>>>>>>>>>> Serguei >>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> On 7/27/20 18:08, Leonid Mesnik wrote: >>>>>>>>>>>>> Hi >>>>>>>>>>>>> >>>>>>>>>>>>> Could you please review following fix which suspends debugger VM >>>>>>>>>>>>> while enabling/disabling events. >>>>>>>>>>>>> >>>>>>>>>>>>> All changed tests fail intermittently getting unexpected events >>>>>>>>>>>>> instead of breakpoint used for communication between >>>>>>>>>>>>> debugger/debuggee VM. The tests request different events and >>>>>>>>>>>>> verify request's properties but don't process/verify events >>>>>>>>>>>>> themselves. Test doesn't aware if events are generated or not. >>>>>>>>>>>>> The vm suspension doesn't affect JDWP native agent and it still >>>>>>>>>>>>> should get and verify JDWP commands. >>>>>>>>>>>>> >>>>>>>>>>>>> webrev: http://cr.openjdk.java.net/~lmesnik/8244537/webrev.00/ >>>>>>>>>>>>> <http://cr.openjdk.java.net/~lmesnik/8244537/webrev.00/> >>>>>>>>>>>>> bug: https://bugs.openjdk.java.net/browse/JDK-8244537 >>>>>>>>>>>>> <https://bugs.openjdk.java.net/browse/JDK-8244537> >>>>>>>>>>>>> >>>>>>>>>>>>> >>>>>>>>>>>>> Leonid >>>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>> >>>>>>>>> >>>>>>>> >>>>>>> >>>>>> >>>>> >>>> >>> >> >
