LGTM
--alex
On 07/28/2020 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/
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
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
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
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] wrote:
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/
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
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/
bug: https://bugs.openjdk.java.net/browse/JDK-8244537
Leonid
