Bug 434558 - Disconnect actually terminates the session if launch element is selected
Summary: Disconnect actually terminates the session if launch element is selected
Status: RESOLVED FIXED
Alias: None
Product: CDT
Classification: Tools
Component: cdt-debug-dsf-gdb (show other bugs)
Version: 8.3.0   Edit
Hardware: PC Windows 7
: P3 normal (vote)
Target Milestone: 8.6.0   Edit
Assignee: Nobody - feel free to take it CLA
QA Contact: Marc Khouzam CLA
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-05-09 18:52 EDT by Nobody - feel free to take it CLA
Modified: 2014-12-05 10:33 EST (History)
3 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Nobody - feel free to take it CLA 2014-05-09 18:52:25 EDT
Using "Disconnect" on a launch element terminates the session instead of disconnecting from the processes being debugged.
Currently GdbDisconnectCommand works only for a single element even if there are multiple elements selected in the view. To support "Disconnect" for GdbLaunch this    restriction needs to be removed to handle the case when multiple processes are being debugged.
Comment 1 Nobody - feel free to take it CLA 2014-05-09 21:11:48 EDT
Pushed a patch to Gerrit: https://git.eclipse.org/r/26318. Marc, can you take a look, please?
Comment 2 Marc Khouzam CLA 2014-05-15 11:54:42 EDT
Looking at this solution, it reminded me of when we added support for multi-selection for suspend/resume operations.

What we are doing here is similar for Disconnect and could also be done for Terminate.

For suspend/resume, as part of Bug 330974, we took the approach to have the service handle operations on multiple-selections.  I think it made the implementation more elegant and more versatile.  IMultiRunControl.java is a good description of this?

Do you think this idea would apply well here?
Comment 3 Nobody - feel free to take it CLA 2014-05-15 12:15:14 EDT
(In reply to Marc Khouzam from comment #2)
> Looking at this solution, it reminded me of when we added support for
> multi-selection for suspend/resume operations.
> 
> What we are doing here is similar for Disconnect and could also be done for
> Terminate.
> 
> For suspend/resume, as part of Bug 330974, we took the approach to have the
> service handle operations on multiple-selections.  I think it made the
> implementation more elegant and more versatile.  IMultiRunControl.java is a
> good description of this?
> 
> Do you think this idea would apply well here?

I think it's a good idea, wasn't aware of it. I'll take a look.
Comment 4 Nobody - feel free to take it CLA 2014-05-16 14:29:49 EDT
(In reply to Marc Khouzam from comment #2)
> Looking at this solution, it reminded me of when we added support for
> multi-selection for suspend/resume operations.
> 
> What we are doing here is similar for Disconnect and could also be done for
> Terminate.
> 
> For suspend/resume, as part of Bug 330974, we took the approach to have the
> service handle operations on multiple-selections.  I think it made the
> implementation more elegant and more versatile.  IMultiRunControl.java is a
> good description of this?
> 
> Do you think this idea would apply well here?

I can define a special interface IMultiDisconnect, make GDBProcesses* implement it and register as a service. Is that what you mean?
Comment 5 Marc Khouzam CLA 2014-05-16 14:47:25 EDT
(In reply to Mikhail Khodjaiants from comment #4)

> I can define a special interface IMultiDisconnect, make GDBProcesses*
> implement it and register as a service. Is that what you mean?

Pretty much.  If you feel it is worth it.  It made things better in the case of MultiRunControl, but I haven't tried here.

I'd also include a multi-terminate API in the new interface, so we don't have to extend it later one.  It does not need to be supported with this fix.
Comment 6 Nobody - feel free to take it CLA 2014-05-23 12:56:56 EDT
The Java docs for IProcesses.detachDebuggerFromProcess() and IProcess.canDetachDebuggerFromProcess() say the contexts that are passed to the methods "should have IProcessDMContext as an ancestor". This is correct for GDBProcesses_7_0 but GDBProcesses_7_2 expects IMIContainerDMContext. I understand why it has changed but we need to fix the docs somehow.
Comment 7 Nobody - feel free to take it CLA 2014-05-23 17:59:27 EDT
I added two new interfaces IMultiTerminate and IMultiDetach and changed the patch to use both. The new patch enables "Terminate" and "Disconnect" for multiple selections regardless whether the selected elements are in the same session on not.
Comment 8 Martin Schreiber CLA 2014-06-04 01:39:19 EDT
I am not sure if it is related to that bug, but I discovered that the DsfTerminateCommand (Using the "Terminate" Button) does different things depending what is selected in the "Debug View".

If the root element is selected it calls 
execute(GdbLaunch launch, IEnabledStateRequest request),
wich is calling GDBControl.terminate which is calling the commandfactory to get the MIGDBExit command, which will end up in a 'D' (=Disconnect) package.

If the selected element is a Thread, the DsfTerminateCommand is calling 
execute(final IProcessDMContext processDmc, IDebugCommandRequest request)
which calls terminate on the process service (GDBProcesses.terminate).
And the GDBProcesses is calling the fCommandFactory.createMIInterpreterExecConsoleKill, wich will end up in a 'k' (=kill) package.

In short:
Selecting the root element ends up in a 'D' (disconnect gdb command), selecting a thread ends up in a 'k' (kill gdb command). 

Is this the expected behavior? IMHO, both should send 'k'ill.
Comment 9 Marc Khouzam CLA 2014-06-17 10:02:16 EDT
(In reply to Martin Schreiber from comment #8)
 
> In short:
> Selecting the root element ends up in a 'D' (disconnect gdb command),
> selecting a thread ends up in a 'k' (kill gdb command). 
> 
> Is this the expected behavior? IMHO, both should send 'k'ill.

I haven't looked at the code, but I think this might be either a non-stop vs all-stop behaviour.  Also, it may be different depending on the version of GDB used.

I agree we should be consistent.  We'll pay attention to that as we get Mikhail's patch in.
Comment 10 Nobody - feel free to take it CLA 2014-10-27 18:33:54 EDT
I rewrote the patch and pushed it to Gerrit - https://git.eclipse.org/r/#/c/26318/3
This patch simply modifies DsfTerminateCommand and GdbDisconnectCommand (btw, why not DsfDisconnectCommand?) to support IMultiTerminate and IMultiDisconnect. I also addressed Marc's comments relevant to the new changes.
Marc, could you please take a look? For some reason I have problems setting up the API baseline on my Linux box and had to move the patch from one workspace to another to catch the API changes. It's possible that some of API changes are not annotated correctly.
Thanks
Comment 11 Marc Khouzam CLA 2014-10-30 14:49:14 EDT
(In reply to Mikhail Khodjaiants from comment #10)
> I rewrote the patch and pushed it to Gerrit -
> https://git.eclipse.org/r/#/c/26318/3

Thanks.
I'm looking at it now.

> This patch simply modifies DsfTerminateCommand and GdbDisconnectCommand
> (btw, why not DsfDisconnectCommand?)

DsfTerminateCommand was first written and Pawel named it like that although it is part of the dsf.gdb plugin.  Later on, I created GdbDisconnectCommand and since it was in dsf.gdb I named it with the Gdb prefix.  Personally, I'm not a big fan of Dsf* names in dsf.gdb but DsfTerminateCommand and some others were already there.
Comment 12 Nobody - feel free to take it CLA 2014-12-04 21:58:20 EST
Checked the patch in. Marc, thank you for your patience. 
Doug has been talking about 9.0 which is not in the target list, so I set the target milestone to 8.6.0.
Comment 13 Marc Khouzam CLA 2014-12-05 08:11:41 EST
(In reply to Mikhail Khodjaiants from comment #12)
> Checked the patch in. Marc, thank you for your patience. 

No problem, it wasn't a simple fix.

> Doug has been talking about 9.0 which is not in the target list, so I set
> the target milestone to 8.6.0.

8.6 is for sure and is in February, it is 8.7 that could be a 9.0 in June.

When you can, could you update the N&N with this nice addition?
Comment 14 Nobody - feel free to take it CLA 2014-12-05 09:06:51 EST
(In reply to Marc Khouzam from comment #13)
> When you can, could you update the N&N with this nice addition?

I will, thanks.
Comment 15 Nobody - feel free to take it CLA 2014-12-05 10:33:51 EST
(In reply to Marc Khouzam from comment #13)
> When you can, could you update the N&N with this nice addition?

Done.