Bugzilla – Full Text Bug Listing
| Summary: | [multi-process] Move startOrRestart to the Processes service | ||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | [Tools] CDT | Reporter: | Marc Khouzam <marc.khouzam> | ||||||||||||||||
| Component: | cdt-debug-dsf-gdb | Assignee: | Marc Khouzam <marc.khouzam> | ||||||||||||||||
| Status: | RESOLVED FIXED | QA Contact: | Marc Khouzam <marc.khouzam> | ||||||||||||||||
| Severity: | normal | ||||||||||||||||||
| Priority: | P3 | CC: | ajin, cdtdoug, elaskavaia.cdt, nobody, onurakdemir1, pawel.1.piech, vladimir.prus | ||||||||||||||||
| Version: | 8.0 | Flags: | nobody:
review+
|
||||||||||||||||
| Target Milestone: | 8.0 | ||||||||||||||||||
| Hardware: | PC | ||||||||||||||||||
| OS: | Linux | ||||||||||||||||||
| Whiteboard: | |||||||||||||||||||
| Bug Depends on: | |||||||||||||||||||
| Bug Blocks: | 237306 | ||||||||||||||||||
| Attachments: |
|
||||||||||||||||||
|
Description
Marc Khouzam
This change will probably have important impacts to the FinalLaunchSequence. This will affect people who are extending it. Marc, one of the steps of startOrRestart sequence sets a temporary breakpoint at the symbol specified by user. Shouldn't it be moved to a separate step in the final launch sequence? (In reply to comment #2) > Marc, one of the steps of startOrRestart sequence sets a temporary breakpoint > at the symbol specified by user. Shouldn't it be moved to a separate step in > the final launch sequence? It probably should have been. But with the changes I'm making now, the FinalLaunchSequence will instead be calling the Processes service to do all that. What I'm working on now is to create a StartOrRestartProcess sequence that will do all the steps that where originally in startOrRestart. Setting the breakpoint "on main" will be done in one step of that sequence and you should be able to override it easily (if that is what you need to do). I'm hoping to have a patch tomorrow. (In reply to comment #2) > Marc, one of the steps of startOrRestart sequence sets a temporary breakpoint > at the symbol specified by user. Shouldn't it be moved to a separate step in > the final launch sequence? I remember now that this step was part of the FLS originally. In fact, all of the IGDBControl.start() was part of FLS. But when I implemented the Restart feature, all this code was duplicated because it needed to also be in a service (so the Restart command could call it). Also, at the time, the IProcesses service did not exists. So, to avoid the duplication, I had the FLS call the new IGDBControl.start(). Created attachment 187721 [details]
First half of fix, which handles the Restart
This patch moves the Restart logic to the IGDBProcesses service.
I still need to move the Start logic, which I'm working on now. Until that is ready, I had the FLS use restart() to start the process, since the work is identical.
I am aiming at having the full patch today or tomorrow.
Created attachment 187916 [details]
First half of fix, which handles the Start/Restart
I prefer to submit the fix to this bug as two patches. One for Start/Restart, and the second for IProcesses.debugNewProcess().
This current patch handles the change for Start/Restart and it pretty straightforward. It is mostly moving code from the GDBControl services to the GDBProcesses service. In detail, the patch does the following:
General
1- Introduce a new CDebugUtils.getAttribute() method which allows to more easily extract attributes from the launchConfiguration.
Starting/Restart a process
2- Remove IGDBControl.start(). There is not case where a process is created and needs to be started as a separate step. The reason this method existed is that it allowed FLS to re-use code for the restart() action. Instead, I will create a class for the start/restart code, which can be called directly from FLS. If it turns out that we do need a start() method, we'll add it at that time.
3- Move restart() and canRestart() from IGDBControl to IGDBProcesses, and add a ContainerDmc parameter
4- Make GDBProcesses implement IGDBProcesses to provide access for these new restart() and canRestart() methods
5- Move the code for restart() and canRestart() from GDBControl to GDBProcesses
6- Create a new Sequence to start/restart a process: StartOrRestartProcessSequence_7_0
7- Move the code for restart() and canRestart() from GDBControl_7_0 to GDBProcesses_7_0 but have GDBProcesses_7_0 use StartOrRestartProcessSequence_7_0 in the implementation of restart() and start()
8- Have GdbRestartCommand use IGDBProcesses.restart() instead of IGDBControl.restart()
It has the following API breaking changes (which I will document on the CDT 8.0 New and Noteworthy page):
1- The interface org.eclipse.cdt.dsf.gdb.service.IGDBProcesses gets two new methods:
canRestart(...) and restart(...)
2- org.eclipse.cdt.dsf.gdb.service.command.GDBControl and org.eclipse.cdt.dsf.gdb.service.command.GDBControl_7_0 no longer implement the five methods: start(...), restart(...), canRestart(...), startOrRestart(...), useContinueCommand(...)
3- org.eclipse.cdt.dsf.gdb.service.command.IGDBControl no longer has the three methods: start(...), restart(...) and canRestart(...)
I will commit this first half to HEAD tomorrow or Monday.
I will then post the second half of the fix, which has a greater impact.
Mikhail, Volodya, please be aware that the GDBControl/GDBControl_7_0.useContinueCommand() will be moved which I expect will impact you. Feel free to let me know if this change does not work well for you. Created attachment 188194 [details] Fix for new JUnit tests This is an update for the tests I just added in bug 336008. I'm happy to report that all the tests still pass with the changes proposed in this bug (which is why I decided to write the tests in the first place :-)) I committed both patches to HEAD. I'll soon post the second part of the solution. *** cdt cvs genie on behalf of mkhouzam *** Bug 335528: [multi-process] Move startOrRestart to the Processes service [*] GdbRestartCommand.java 1.4 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/dsf-gdb/org.eclipse.cdt.dsf.gdb.ui/src/org/eclipse/cdt/dsf/gdb/internal/ui/actions/GdbRestartCommand.java?root=Tools_Project&r1=1.3&r2=1.4 [*] GDBControl_7_0.java 1.25 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/dsf-gdb/org.eclipse.cdt.dsf.gdb/src/org/eclipse/cdt/dsf/gdb/service/command/GDBControl_7_0.java?root=Tools_Project&r1=1.24&r2=1.25 [*] IGDBControl.java 1.7 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/dsf-gdb/org.eclipse.cdt.dsf.gdb/src/org/eclipse/cdt/dsf/gdb/service/command/IGDBControl.java?root=Tools_Project&r1=1.6&r2=1.7 [*] GDBControl.java 1.21 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/dsf-gdb/org.eclipse.cdt.dsf.gdb/src/org/eclipse/cdt/dsf/gdb/service/command/GDBControl.java?root=Tools_Project&r1=1.20&r2=1.21 [+] StartOrRestartProcessSequence_7_0.java http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/dsf-gdb/org.eclipse.cdt.dsf.gdb/src/org/eclipse/cdt/dsf/gdb/service/StartOrRestartProcessSequence_7_0.java?root=Tools_Project&revision=1.1&view=markup [*] GDBProcesses.java 1.9 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/dsf-gdb/org.eclipse.cdt.dsf.gdb/src/org/eclipse/cdt/dsf/gdb/service/GDBProcesses.java?root=Tools_Project&r1=1.8&r2=1.9 [*] GDBProcesses_7_0.java 1.31 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/dsf-gdb/org.eclipse.cdt.dsf.gdb/src/org/eclipse/cdt/dsf/gdb/service/GDBProcesses_7_0.java?root=Tools_Project&r1=1.30&r2=1.31 [*] IGDBProcesses.java 1.6 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/dsf-gdb/org.eclipse.cdt.dsf.gdb/src/org/eclipse/cdt/dsf/gdb/service/IGDBProcesses.java?root=Tools_Project&r1=1.5&r2=1.6 [*] FinalLaunchSequence.java 1.18 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/dsf-gdb/org.eclipse.cdt.dsf.gdb/src/org/eclipse/cdt/dsf/gdb/launching/FinalLaunchSequence.java?root=Tools_Project&r1=1.17&r2=1.18 [*] SyncUtil.java 1.15 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/dsf-gdb/org.eclipse.cdt.tests.dsf.gdb/src/org/eclipse/cdt/tests/dsf/gdb/framework/SyncUtil.java?root=Tools_Project&r1=1.14&r2=1.15 [*] CDebugUtils.java 1.34 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/all/org.eclipse.cdt.debug.core/src/org/eclipse/cdt/debug/core/CDebugUtils.java?root=Tools_Project&r1=1.33&r2=1.34 Created attachment 188492 [details]
Refactoring of FinalLaunchSequence
Finally, this is the final part of this effort. It moves all the process-specific logic of FinalLaunchSequence into IProcesses.attachDebuggerToProcess() and IProcesses.debugNewProcess()
This allows to properly override these methods to handle multi-process, which was not possible when using FinalLaunchSequence.
This is a pretty impacting change that affects the launching of every type of session. So I tried be thorough in my testing. I made sure things worked as before (or better :-)) for:
GDB 6.8, 7.0, 7,1 and 7.2 and for all 5 launch types:
local launch
post-mortem launch
local attach
remote attach
remote non-attach
and with reverse debugging too.
I found three issues that were also there before this changes:
1- Detaching does not work nicely for a local attach session for any GDB (same as before I believe)
2- Detaching a process does not stop tracking breakpoints (same as before)
3- After a detach with reverse enabled, if we re-attach, we see that reverse is enabled when it is not true.
What I am left to do is to make sure all the plumbing for multi-process works properly when using GDB 7.2.
Created attachment 188732 [details]
Refactoring of FinalLaunchSequence 2
Just an update to handle the change that IMIContainerDMContext is no longer an IBreakpointTargetDMContext.
(In reply to comment #12) > Created attachment 188732 [details] > Refactoring of FinalLaunchSequence 2 I committed this to HEAD. *** cdt cvs genie on behalf of mkhouzam *** Bug 335528: [multi-process] Move startOrRestart to the Processes service [+] DebugNewProcessSequence_7_2.java http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/dsf-gdb/org.eclipse.cdt.dsf.gdb/src/org/eclipse/cdt/dsf/gdb/service/DebugNewProcessSequence_7_2.java?root=Tools_Project&revision=1.1&view=markup [+] DebugNewProcessSequence.java http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/dsf-gdb/org.eclipse.cdt.dsf.gdb/src/org/eclipse/cdt/dsf/gdb/service/DebugNewProcessSequence.java?root=Tools_Project&revision=1.1&view=markup [*] GDBProcesses.java 1.10 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/dsf-gdb/org.eclipse.cdt.dsf.gdb/src/org/eclipse/cdt/dsf/gdb/service/GDBProcesses.java?root=Tools_Project&r1=1.9&r2=1.10 [*] GDBProcesses_7_2.java 1.4 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/dsf-gdb/org.eclipse.cdt.dsf.gdb/src/org/eclipse/cdt/dsf/gdb/service/GDBProcesses_7_2.java?root=Tools_Project&r1=1.3&r2=1.4 [*] GDBProcesses_7_0.java 1.33 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/dsf-gdb/org.eclipse.cdt.dsf.gdb/src/org/eclipse/cdt/dsf/gdb/service/GDBProcesses_7_0.java?root=Tools_Project&r1=1.32&r2=1.33 [*] FinalLaunchSequence.java 1.20 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/dsf-gdb/org.eclipse.cdt.dsf.gdb/src/org/eclipse/cdt/dsf/gdb/launching/FinalLaunchSequence.java?root=Tools_Project&r1=1.19&r2=1.20 (In reply to comment #11) > I found three issues that were also there before this changes: > 1- Detaching does not work nicely for a local attach session for any GDB (same > as before I believe) > 2- Detaching a process does not stop tracking breakpoints (same as before) > 3- After a detach with reverse enabled, if we re-attach, we see that reverse is > enabled when it is not true. I've opened Bug 336889, Bug 336890 and Bug 336891 for those issues. > What I am left to do is to make sure all the plumbing for multi-process works > properly when using GDB 7.2. I'll continue this work as part of general multi-process work. Mikhail, can you review when you have some time? Fixed Marc, my first comment is about extensibility. To allow other debuggers override DebugNewProcessSequence and StartOrRestartSequence I would recommend adding protected factory methods to: GDBProcess, GDBProcesses_7_0, DebugNewProcessSequence. Something like getDebugNewProcessSequence() and getStartOrRestartSequence(). (In reply to comment #18) > Marc, my first comment is about extensibility. To allow other debuggers > override DebugNewProcessSequence and StartOrRestartSequence I would recommend > adding protected factory methods to: > GDBProcess, GDBProcesses_7_0, DebugNewProcessSequence. Something like > getDebugNewProcessSequence() and getStartOrRestartSequence(). Yes, extensibility was something I wasn't very sure about. Thanks for looking into it. Do you think having getDebugNewProcessSequence() and getStartOrRestartSequence() would make things easier than overriding the methods that use those sequences? Currently, one can override GDBProcesses_7_0#restart() to change the startOrRestartSequence. Same for DebugNewProcessSequence#stepStartExecution(). What do you think? (In reply to comment #19) > Do you think having getDebugNewProcessSequence() and > getStartOrRestartSequence() would make things easier than overriding the > methods that use those sequences? Currently, one can override > GDBProcesses_7_0#restart() to change the startOrRestartSequence. Same for > DebugNewProcessSequence#stepStartExecution(). > > What do you think? The main reason is that it is easier to maintain the code. The changes in GDBProcesses_7_0#restart() and DebugNewProcessSequence#stepStartExecution() will not affect the subclasses. The implementation of DebugNewProcessSequence#stepStartExecution() uses private members of DebugNewProcessSequence. Yes, all of these classes can be duplicated in subclasses but it's not a good approach. (In reply to comment #20) > (In reply to comment #19) > > Do you think having getDebugNewProcessSequence() and > > getStartOrRestartSequence() would make things easier than overriding the > > methods that use those sequences? Currently, one can override > > GDBProcesses_7_0#restart() to change the startOrRestartSequence. Same for > > DebugNewProcessSequence#stepStartExecution(). > > > > What do you think? > > The main reason is that it is easier to maintain the code. The changes in > GDBProcesses_7_0#restart() and DebugNewProcessSequence#stepStartExecution() > will not affect the subclasses. Valid point. I thought the methods were small enough, but you are right that it would be safer. > The implementation of DebugNewProcessSequence#stepStartExecution() uses private > members of DebugNewProcessSequence. Yes, all of these classes can be duplicated > in subclasses but it's not a good approach. I tried to make available everything that was required. All other private fields can all be obtained by the subclasses. However, I agree that it would require to override the initialization step and the getExecutionOrder method, which may be more work than really necessary. Created attachment 189046 [details]
New methods to get sequences
How about this?
It wasn't totally trivial due to parameters that needed to be passed.
I still have to test it, but I wanted to see if it is what you had in mind.
(In reply to comment #22) > How about this? > It wasn't totally trivial due to parameters that needed to be passed. > I still have to test it, but I wanted to see if it is what you had in mind. It seems to be OK except this part from debugNewProcess(): boolean isInitial = getIsInitialProcess(); if (getIsInitialProcess()) { setIsInitialProcess(false); Created attachment 189089 [details] New methods to get sequences 2 (In reply to comment #23) > (In reply to comment #22) > > How about this? > > It wasn't totally trivial due to parameters that needed to be passed. > > I still have to test it, but I wanted to see if it is what you had in mind. > It seems to be OK except this part from debugNewProcess(): > > boolean isInitial = getIsInitialProcess(); > if (getIsInitialProcess()) { > setIsInitialProcess(false); I added a comment to explain: // Store the current value of the initialProcess variable because // we will use it later and we are about to change it. Here is the patch I committed to HEAD. Let me know if want me to change anything. *** cdt cvs genie on behalf of mkhouzam *** Bug 335528: Add method to allow overriding which StartOrRestart and DebugNewProcess sequences to use. [*] GDBProcesses_7_2.java 1.5 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/dsf-gdb/org.eclipse.cdt.dsf.gdb/src/org/eclipse/cdt/dsf/gdb/service/GDBProcesses_7_2.java?root=Tools_Project&r1=1.4&r2=1.5 [*] DebugNewProcessSequence_7_2.java 1.2 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/dsf-gdb/org.eclipse.cdt.dsf.gdb/src/org/eclipse/cdt/dsf/gdb/service/DebugNewProcessSequence_7_2.java?root=Tools_Project&r1=1.1&r2=1.2 [*] StartOrRestartProcessSequence_7_0.java 1.3 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/dsf-gdb/org.eclipse.cdt.dsf.gdb/src/org/eclipse/cdt/dsf/gdb/service/StartOrRestartProcessSequence_7_0.java?root=Tools_Project&r1=1.2&r2=1.3 [*] GDBProcesses.java 1.11 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/dsf-gdb/org.eclipse.cdt.dsf.gdb/src/org/eclipse/cdt/dsf/gdb/service/GDBProcesses.java?root=Tools_Project&r1=1.10&r2=1.11 [*] GDBProcesses_7_0.java 1.34 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/dsf-gdb/org.eclipse.cdt.dsf.gdb/src/org/eclipse/cdt/dsf/gdb/service/GDBProcesses_7_0.java?root=Tools_Project&r1=1.33&r2=1.34 [*] DebugNewProcessSequence.java 1.2 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/dsf-gdb/org.eclipse.cdt.dsf.gdb/src/org/eclipse/cdt/dsf/gdb/service/DebugNewProcessSequence.java?root=Tools_Project&r1=1.1&r2=1.2 Comment on attachment 189089 [details] New methods to get sequences 2 This patch was not generated properly. The commit can be seen in comment 25 which is pretty much the patch attached to this bug: "New methods to get sequences" Comment on attachment 189046 [details]
New methods to get sequences
The second version of this patch was not generated properly, so this patch is close enough for information.
|