|
My understanding is that volatile is to
prevent compiler optimizations.
This has to the exact case where it is needed.
Thanks,
Serguei
On 8/6/18 11:46, Gary Adams wrote:
I
used the same declaration used for "instruction" variable,
which was already being used for getValue() calls.
I think we would want volatile if this was thread to thread
communication, but these variables are being used
for interprocess communication between the debugger and a
debuggee process. There are no reader/writer race conditions
here.
On 8/6/18, 1:36 PM, serguei.spit...@oracle.com
wrote:
Hi Gary,
I was going to sponsor this but wanted to look at the latest
webrev first.
And found this:
44 static int testCase = -1;
Should it be declared as volatile?
Thanks,
Serguei
On 8/6/18 05:27, Gary Adams wrote:
I'll
need a sponsor for the patch attached.
On 8/6/18, 7: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.
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());
|
