RFR JDK-8170089: nsk/jdi/EventSet/resume/resume008: ERROR: suspendCounts don't match for : Common-Cleaner
serguei.spitsyn at oracle.com
serguei.spitsyn at oracle.com
Mon Aug 6 22:23:46 UTC 2018
Okay then.
Thanks, David for reminding this!
Gary,
I guess, you still have a discussion with Chris, so it is not ready for
push yet.
Thanks,
Serguei
On 8/6/18 15:15, David Holmes wrote:
> 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