RFR(S): 8134103: JVMTI_ERROR_WRONG_PHASE(112): on checking for an interface

[David Holmes]

+1 from me.

David

On 3/03/2017 9:07 AM, serguei.spitsyn at oracle.com wrote:
> On 3/2/17 14:55, Daniel D. Daugherty wrote:
>> On 3/2/17 3:19 PM, serguei.spitsyn at oracle.com wrote:
>>> On 3/2/17 14:06, Daniel D. Daugherty wrote:
>>>> On 3/2/17 2:24 PM, serguei.spitsyn at oracle.com wrote:
>>>>> The updated webrev:
>>>>> http://cr.openjdk.java.net/~sspitsyn/webrevs/2017/hotspot/8134103-jdi-wrong-phase.2/
>>>>>
>>>>>
>>>>> The change at L152-L153 has been reverted.
>>>>> Just one sanity check is needed.
>>>>
>>>> This comment from earlier review is still unresolved:
>>>>
>>>> src/jdk.jdwp.agent/share/native/libjdwp/eventHelper.c
>>>>     L243:          * Immediately close out any commands enqueued from a
>>>>     L244:          * previously attached debugger.
>>>>
>>>>         Perhaps L244 can be change to:
>>>>
>>>>                    * dead VM or a previously attached debugger.
>>>>
>>>>         to match the changed line of code:
>>>>
>>>>             L246         if (gdata->vmDead || command->sessionID !=
>>>> currentSessionID) {
>>>>
>>>> Sorry I didn't notice that you didn't reply to it earlier...
>>>
>>> Sorry, I missed it - fixed now.
>>> I guess, it can be pushed now.
>>>
>>>
>>>>
>>>> I presume you are retesting... How often does
>>>> hotspot/test/closed/compiler/c1/6507107/HeapwalkingTest.java reproduce
>>>> the problem?
>>>
>>> The repro-rate is pretty high it is about 1/3 - 1/2.
>>> It is non-reproducible with the fix.
>>
>> So just to be clear, with just these code changes in place:
>>
>> - grab the vmDeathLock for all JDWP command sets instead of just
>>   the VirtualMachine command set in the debugLoop thread
>> - ignore/close out cmds when gdata->vmDead is true in addition
>>   to the existing old session check in the CommandLoop thread
>
> Exactly.
>
>
>>
>> the HeapwalkingTest goes from failing 1/3 -> 1/2 the time to
>> not failing in 200 runs so far...
>>
>> I'd say you nailed this one nicely!
>
> Thanks.
> The fix should cover all the symptoms described in the bug dups.
> At least, I tried to analyze and cover all theoretically possible cases. :)
> My initial fix had more guards.
> But then I proved to myself some of the guards are not necessary.
>
>
> Thanks,
> Serguei
>
>
>>
>> Dan
>>
>>
>>> The test is pretty slow.
>>> At this point I've got about 200 clean runs.
>>> All the jdi/jdwp tests are clean too.
>>>
>>> Thanks!
>>> Serguei
>>>
>>>>
>>>> Dan
>>>>
>>>>>
>>>>> Thanks,
>>>>> Serguei
>>>>>
>>>>>
>>>>> On 3/2/17 11:21, serguei.spitsyn at oracle.com wrote:
>>>>>> Dan,
>>>>>>
>>>>>> Thank you for reviewing!
>>>>>> I was waiting for your comments.
>>>>>>
>>>>>>
>>>>>> On 3/2/17 06:59, Daniel D. Daugherty wrote:
>>>>>>> On 3/1/17 8:49 PM, serguei.spitsyn at oracle.com wrote:
>>>>>>>> Please, review the JDK 10 fix for:
>>>>>>>>   https://bugs.openjdk.java.net/browse/JDK-8134103
>>>>>>>>
>>>>>>>>
>>>>>>>> Webrev:
>>>>>>>> http://cr.openjdk.java.net/~sspitsyn/webrevs/2017/hotspot/8134103-jdi-wrong-phase.1/
>>>>>>>>
>>>>>>>
>>>>>>> src/jdk.jdwp.agent/share/native/libjdwp/debugLoop.c
>>>>>>>     old L152:             } else if (gdata->vmDead &&
>>>>>>>     old L153:              ((cmd->cmdSet) !=
>>>>>>> JDWP_COMMAND_SET(VirtualMachine))) {
>>>>>>>
>>>>>>>         The old code used to set the error condition when the VM
>>>>>>>         is dead and the command was not in the VirtualMachine
>>>>>>>         command set. With the new code:
>>>>>>>
>>>>>>>             L150:             } else if (gdata->vmDead) {
>>>>>>>
>>>>>>>         The error condition is now set for all command sets
>>>>>>>         including the VirtualMachine command set. Minimally
>>>>>>>         that means that this comment needs work:
>>>>>>>
>>>>>>>             L152:                  * VirtualMachine cmdSet
>>>>>>> quietly ignores some cmds
>>>>>>>             L153:                  * after VM death, so, it sends
>>>>>>> it's own errors.
>>>>>>>
>>>>>>>         since you are no longer letting the VirtualMachine cmdSet
>>>>>>> send
>>>>>>>         its own errors.
>>>>>>
>>>>>> Agreed, good catch, thanks!
>>>>>>
>>>>>>
>>>>>>>
>>>>>>>         It's not clear to me why you are now setting the error
>>>>>>>         condition for VirtualMachine cmds instead of letting those
>>>>>>>         cmds send their own errors.
>>>>>>>
>>>>>>>         See more comments below for your summary.
>>>>>>
>>>>>> Most likely, you are right here.
>>>>>> I specifically looked at the VM commands but overlooked the ones
>>>>>> that are silently ignored.
>>>>>> For instance: Resume (9) or exit (10).
>>>>>>
>>>>>> So, I will need to restore the L152+L153.
>>>>>> Please, let me re-test this update and then I'll send another webrev.
>>>>>> Interesting that no test has caught this as there is a very tiny
>>>>>> gap for such an intermittent issue to appear.
>>>>>>
>>>>>>
>>>>>>>
>>>>>>> src/jdk.jdwp.agent/share/native/libjdwp/eventHelper.c
>>>>>>>     L243:          * Immediately close out any commands enqueued
>>>>>>> from a
>>>>>>>     L244:          * previously attached debugger.
>>>>>>>
>>>>>>>         Perhaps L244 can be change to:
>>>>>>>
>>>>>>>                    * dead VM or a previously attached debugger.
>>>>>>>
>>>>>>>         to match the changed line of code:
>>>>>>>
>>>>>>>             L246         if (gdata->vmDead || command->sessionID
>>>>>>> != currentSessionID) {
>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> Summary:
>>>>>>>>   This is an intermittent issue in the debugger back-end (JDWP
>>>>>>>> agent)
>>>>>>>>   that impacts the nightly testing stability.
>>>>>>>
>>>>>>> Congrats on tracking down this elusive bug!
>>>>>>
>>>>>> Thanks!
>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>>   The fix adds check guards of gdata->vmDead condition to the:
>>>>>>>>    - debugLoop (JDWP Event Helper thread) and
>>>>>>>>    - commandLoop (JDWP Transport Listener thread)
>>>>>>>>   The commands are ignored in the DEAD phase.
>>>>>>>
>>>>>>> In the debugLoop, you aren't ignoring the commands, you
>>>>>>> are setting an error condition.
>>>>>>
>>>>>> Right, I meant: ignored == not executed. :)
>>>>>>
>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>>   The check guard in the debugLoop already existed but only for
>>>>>>>> VirtualMachine
>>>>>>>>   command set, so it has been extended to commands from all JDWP
>>>>>>>> command sets.
>>>>>>>
>>>>>>> I'm reading the existing code exactly opposite of what you
>>>>>>> say here. The gdata->vmDead check applied to all command
>>>>>>> sets except for the VirtualMachine command set. I agree
>>>>>>> that you've extended it to all command sets, but we're
>>>>>>> not ignoring the commands. We are now setting the error
>>>>>>> condition for all command sets.
>>>>>>
>>>>>> Agreed.
>>>>>>
>>>>>>
>>>>>>>
>>>>>>> Maybe I'm missing something here. Perhaps I've been away
>>>>>>> from this code for too long... :-)
>>>>>>
>>>>>> No, it is that I'm still learning this code with your help. :-)
>>>>>>
>>>>>>
>>>>>> Thanks a lot!
>>>>>> Serguei
>>>>>>
>>>>>>>
>>>>>>> Dan
>>>>>>>
>>>>>>>
>>>>>>>>
>>>>>>>>   I suspect, this bug could also cause some of the timeout and
>>>>>>>> socket related issues.
>>>>>>>>
>>>>>>>>
>>>>>>>> Testing:
>>>>>>>>   The fix was tested with the nsk.jdi, jtreg com/sun/jdi and 100
>>>>>>>> runs
>>>>>>>>   of the
>>>>>>>> hotspot/test/closed/compiler/c1/6507107/HeapwalkingTest.java.
>>>>>>>>   The test results are very clean.
>>>>>>>>
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>> Serguei
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>