Since the vmTestbase/nsk tests are in need of
reformatting and refactoring, I've tried to isolate
changes to just the leaf test source files. The fix
has been duplicated in the 10 resume tests that
shared the same issue.
I'd prefer to get this fix in as is and leave any test library
refactoring to a future issue.
On 8/28/18, 5:23 PM, serguei.spit...@oracle.com wrote:
Hi Gary,
I'd suggest to put the informDebuggeeTestCase(int testCase)
and waitForTestCase(int t) into the test library so that they
are implemented just once.
Thanks,
Serguei
On 8/28/18 05:20, Gary Adams wrote:
I went back and confirmed that the debuggeeClass initialization in
TestDebuggerType1 RunThis() method happens very early on in the
test setup. If it was not initialized, the very first attempts to set
the
breakpoint for communication would have failed.
So this usage after a first test case is completed would never be null.
I've removed that check and attached a patch that should be ready to
push.
On 8/27/18, 4:26 PM, Chris Plummer wrote:
Hi Gary,
Just getting caught up again. To answer your earlier question, yes,
I think removing the isDisconnected() check is an improvement since
it's use was at best inconsistent, and leads the reader to think
that this is something that is expected to happen. If it does
happen, the test will still fail in an appropriate way, and adding
the check can actually hide the failure.
And looking at this again, I'm now wondering about the debuggeeClass
!= null check. Is it possible for it to ever be null? That kind of
seems along the lines of the isDisconnected() check.
Other than that the changes look fine.
thanks,
Chris
On 8/24/18 5:32 AM, Gary Adams wrote:
Here's an updated webrev with the isDisconnected checks removed
in the setValue handling.
http://cr.openjdk.java.net/~gadams/8170089/webrev.02/index.html
Testing is in progress, but no failed tests have shown up so far
with the extra check removed.
On 8/22/18, 1:05 PM, Gary Adams wrote:
On 8/6/18, 3:16 PM, Chris Plummer wrote:
On 8/6/18 11:41 AM, Gary Adams wrote:
On 8/6/18, 1:56 PM, Chris Plummer wrote:
On 8/6/18 4:16 AM, Gary Adams wrote:
On 8/3/18, 6:38 PM, Chris Plummer wrote:
Hi Gary,
Overall it looks good.
Is the EventHandler.isDisconnected() check needed?
This just follows the pattern used in other calls to setValue.
I'm not seeing any other examples of this. Can you point me to
them? Isn't it expected that you will always be connected, and
it will only be disconnected if there is something very wrong
with the execution of the test? Not producing an error in that
case could actually be misleading, causing the test to fail
with some sort of state related error rather than some sort of
exception indicating it was disconnected.
The best examples of checking EventHandler.isDisconnected()
can be seen in the implementation of shouldRunAfterBreakPoint()
See
test/hotspot/jtreg/vmTestbase/nsk/share/jdi/TestDebuggerType1.java
It's used in the loop waiting for the breakpoint event to be
observed,
and in the getValue() fetching of the next "instruction" indicating
testing is completed.
Well, that's just 2 uses of isDisconnected() out of the 200+
get/setValue() calls. I can see its use in the loop, since it is
used to force the exit of the loop when disconnected (rather than
waiting for timeout). The one before the getValue() call is more
like your use, and I don't see the need in this case either.
What's to prevent becoming disconnected between the
isDisconnected() and the get/setValue() call?
Just following up on this loose end after vacation ...
I agree that there is nothing preventing the connection being
terminated
between the time isDisconnected() is checked and the call to
setValue()
being made. I also don't see any harm in including the
isDisconnected()
check here. If you think the test is improved by removing the check,
I'll make those changes, post a fresh webrev and repeat the testing.
Chris
Chris
No point in attempting the operation, if you know the
connection was lost. An exception at this point could
be misleading, if some other error has already occurred.
In resume008a.java you removed a lot of empty lines. In some
places it's fine, but the lines at 132 and 134 should have
remained. Also, for the ones that were ok to remove, I don't
see you doing the same thing in the other files. I think
probably it's best to be consistent, and for this webrev
probably best not to do them since it distracts too much from
the important changes.
The original bug was reported against resume008, so more time
was spent in that
particular test, including some line wrapping changes. I will
restore the blank lines
you mentioned before producing a final patch. The other tests
had observed failures
also during testing. Applying the same change fixed those
failures as well.
Seems like there is a lot of abstraction that could have been
done with these tests to share a lot of code, but since so
far that hasn't been done, probably not a good idea to start
doing that with this fix. Do you think it's worth filing an
enhancement request for?
Reformatting or refactoring these older tests would be at best
a P5.
I don't think it's worth filing a bug, but as we fix bugs in
these test it's
worth some minimal amount of cleanup while we are in the code.
thanks,
Chris
On 8/3/18 11:04 AM, Gary Adams wrote:
Here is an updated webrev with the alternate solution
implemented for
resume 1 to 10. The debugger sets testCase variable in the
debuggee
when each test case completes in the debugger. By having the
debuggee
wait for the debugger to complete with test case 0, it
avoids the interference
that occurs by proceeding to the breakpoint set in
MethodForCommunication
before the debugger has compared expected suspend counts.
Webrev:
http://cr.openjdk.java.net/~gadams/8170089/webrev.01/index.html
On 7/17/18, 11:33 AM, Gary Adams wrote:
A race condition exists between the debugger and the debuggee.
The first test thread is started with SUSPEND_NONE policy set.
While processing the thread start event the debugger captures
an initial set of thread suspend counts and resumes the
debuggee vm. If the debuggee advances quickly it reaches
the breakpoint set for methodForCommunication. Since the
breakpoint
carries with it SUSPEND_ALL policy, when the debugger
captures a second
set of suspend counts, it will not match the expected
counts for
a SUSPEND_NONE scenario.
The proposed fix introduces a yield in the debuggee test
thread run method
to allow the debugger to get the expected sampled values.
Issue: https://bugs.openjdk.java.net/browse/JDK-8170089
Webrev:
http://cr.openjdk.java.net/~gadams/8170089/webrev.00/
test/hotspot/jtreg/vmTestbase/nsk/share/jdi/TestDebuggerType1.java:
...
186 private void
setCommunicationBreakpoint(ReferenceType refType, String
methodName) {
187 Method method =
debuggee.methodByName(refType, methodName);
188 Location location = null;
189 try {
190 location =
method.allLineLocations().get(0);
191 } catch (AbsentInformationException e) {
192 throw new Failure(e);
193 }
194 bpRequest =
debuggee.makeBreakpoint(location);
195
196 bpRequest.setSuspendPolicy(EventRequest.SUSPEND_ALL);
197 bpRequest.putProperty("number", "zero");
198 bpRequest.enable();
199
200 eventHandler.addListener(
201 new EventHandler.EventListener() {
202 public boolean
eventReceived(Event event) {
203 if (event instanceof
BreakpointEvent && bpRequest.equals(event.request())) {
204 synchronized(eventHandler) {
205 display("Received communication breakpoint event.");
206 bpCount++;
207 eventHandler.notifyAll();
208 }
209 return true;
210 }
211 return false;
212 }
213 }
214 );
215 }
test/hotspot/jtreg/vmTestbase/nsk/jdi/EventSet/resume/resume008.java:
...
140 display("......--> vm.suspend();");
141 vm.suspend();
142
143 display(" getting : Map<String,
Integer> suspendsCounts1");
144
145 Map<String, Integer>
suspendsCounts1 = new HashMap<String, Integer>();
146 for (ThreadReference
threadReference : vm.allThreads()) {
147 suspendsCounts1.put(threadReference.name(),
threadReference.suspendCount());
148 }
149 display(suspendsCounts1.toString());
150
151 display(" eventSet.resume;");
152 eventSet.resume();
153
154 display(" getting : Map<String,
Integer> suspendsCounts2");
This is where the breakpoint is encountered before the
second set of suspend counts is acquired.
155 Map<String, Integer>
suspendsCounts2 = new HashMap<String, Integer>();
156 for (ThreadReference
threadReference : vm.allThreads()) {
157 suspendsCounts2.put(threadReference.name(),
threadReference.suspendCount());
158 }
159 display(suspendsCounts2.toString());
