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

[serguei.spitsyn at oracle.com]

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.
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
>>>>
>>>
>>
>