RFR JDK-8170089: nsk/jdi/EventSet/resume/resume008: ERROR: suspendCounts don't match for : Common-Cleaner
David Holmes
david.holmes at oracle.com
Mon Aug 6 22:15:05 UTC 2018
On 7/08/2018 4:51 AM, serguei.spitsyn at oracle.com wrote:
> My understanding is that volatile is to prevent compiler optimizations.
Java volatile is purely about establishing orderings within the Java
memory model. It is needed only for lock-free access to variables shared
across threads.
Cheers,
David
> 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.spitsyn at 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());
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>
More information about the serviceability-dev
mailing list