Community
Participate
Working Groups
Build ID: I20080617-2000 Currently with org.eclipse.dd.gdb, the way GDB process is started is kind of hard coded, making it hard to accommodate some use cases. For instance, to start GDB process on a remote machine via SSH, or to start GDB process in a chroot'ed file system like Scratchbox. In those cases, the user settings in launch configuration won't usually change, but the underlying command line to start GDB and the parameters for GDB such as working directory and share library paths need special handling.
Created attachment 106904 [details] proposed fix This is my proposed fix to the issue. The main idea is to wrap all info about how GDB is launched (including how the process is launched and all the parameters from launch configuration) in an IGDBLauncher class, where a base implementation handles the common case currently supported by DSF whereas debugger implementors can override the base implementation for their special needs. Then an object of IGDBLauncher is passed to GDBControl which will launch the GDB process and set all those gdb parameters via MI commands.
(In reply to comment #0) > Currently with org.eclipse.dd.gdb, the way GDB process is started is kind of > hard coded, making it hard to accommodate some use cases. For instance, to > start GDB process on a remote machine via SSH, or to start GDB process in a > chroot'ed file system like Scratchbox. In those cases, the user settings in > launch configuration won't usually change, but the underlying command line to > start GDB and the parameters for GDB such as working directory and share > library paths need special handling. The current way to do this is to subclass GdbLaunchDelegate and provide your own FinalLaunchSequence. This would give you everything you need except for changing to the actual command to launch GDB. To change the command to launch GDB you would need to provide your own ServicesLaunchSequence and a new GDBControl service. Providing a new ServicesLaunch sequence would require a minor change to the GdbLaunchDelegate, as you have done in your patch, while the rest should be possible right now. A better solution would be to add the GDBControl service to the serviceFactory and then one could easily replace that service. That being said, I agree that this is heavy and cumbersome. I never liked the fact that changing a single thing in the FinalLaunchSequece requires an entire new one (although I'm the one that coded it that way :-( ). And then, it is even worse to have to replace the GDBControl service. (In reply to comment #1) > Created an attachment (id=106904) [details] > proposed fix > > This is my proposed fix to the issue. > > The main idea is to wrap all info about how GDB is launched (including how the > process is launched and all the parameters from launch configuration) in an > IGDBLauncher class, where a base implementation handles the common case > currently supported by DSF whereas debugger implementors can override the base > implementation for their special needs. Then an object of IGDBLauncher is > passed to GDBControl which will launch the GDB process and set all those gdb > parameters via MI commands. This idea is interesting. It almost provides a GDBLaunch service. Maybe we can do something like this but provided it as a new service IStartDebugger or something. We would have to rework the GDBControl service, but it may give us a simpler solution. I think this deserves some discussion and some input from Pawel, to make sure it stays in line with the DSF philosophy. P.S. A somewhat related issue was mentioned by Pawel a little while ago in bug 226931 comment #10.
(In reply to comment #2) > The current way to do this is to subclass GdbLaunchDelegate and provide your > own FinalLaunchSequence. This would give you everything you need except for > changing to the actual command to launch GDB. > > To change the command to launch GDB you would need to provide your own > ServicesLaunchSequence and a new GDBControl service. Providing a new > ServicesLaunch sequence would require a minor change to the GdbLaunchDelegate, > as you have done in your patch, while the rest should be possible right now. I tried the above at the beginning but it was really cumbersome. Yes, to subclass GDBLaunchDelegate is surely needed. I tried providing my own FinalLaunchSequence which just needed to convert the results from fLaunch.getLaunchConfiguration().getAttribute() (e.g. convert the working directory from host file system to a path in chroot'ed file system of Scratchbox). But FinalLaunchSequence is not originally designed for easy subclassing in such aspect, meaning I have to copy & paste most of the code for my FinalLaunchSequence which defies principle of maximum code reuse and easy maintenance. As to providing a new GDBControl, the main blocker is such code fTracker.getService(GDBControl.class) is scattered around, making it almost impossible to have a subclassed GDBControl service. Maybe as you said, "A better solution would be to add the GDBControl service to the serviceFactory and then one could easily replace that service.". Anyway, I see you are ware of those issues. My proposed solution make it much easier for different debuggers to provide their special way of launching GDB. I'm looking forward to more comment from you, Pawel and others.
It seems to me that the main problem is how to extend FinalLaunchSequence, or a launch sequence at all? If so, a good example may be in GDBControl itself. When I implemented it, I defined the different initialization/shutdown steps as proper inner classes, which could then be re-used by a subclass to customize the sequence. This example also uses the same steps for the startup and shutdown and properly implements handling of errors found during the sequence, which is something we probably want in the launch sequences as well. I.e. we could combine the launch and shutdown sequences and save the duplication of some logic.
(In reply to comment #4) > It seems to me that the main problem is how to extend FinalLaunchSequence, or a No. The problem I tried to solve is make these tasks customizable by debugger implementions. (1) How to start the gdb process. It's the task performed by the step "GDBProcessStep" in GDBControl. (2) How to set parameters for gdb. Those are the tasks performed by steps in FinalLaunchSequence such as "specify the executable file to be debugged..." and "specify GDB's working directory". Let's look at a simplified example. When debugging a CPP project located at /workspace/myproject, we normally need do these: (1) invoke: gdb --interpreter mi --nx (2) tell GDB that the executable/symbol file is : /workspace/myproject/helloworld But in one of our environments, we run eclipse on a Windows machine but do all the build and debug on a linux machine via SSH (the reason of doing that is beyond the scope here). The workspace folder on Windows is shared and mapped to /shared/workspace on the Linux mahcine. Then we need to start gdb this way: (1) invoke gdb by command line like ssh user@linuxpc gdb --interpreter mi --nx (2) tell gdb the exe file is: /shared/workspace/myproject/helloworld So you see, we need to adapt/convert the command line and path parameters for gdb. Note all those conversion is transparent to our user, who just thinks he's building and debugging on the Windows machine. BTW, I think what you suggested is good way to make FinalLaunchSequence easier to subclass.
I like the idea of separating the management of the gdb process from the rest of the work that the command control does, but there's not just the task of launching the process, this manager also needs to monitor this process and notify client when it exits. Maybe it would be better to create a separate service for this purpose, which GDB would then depend on. (In reply to comment #3) > (In reply to comment #2) > As to providing a new GDBControl, the main blocker is such code > fTracker.getService(GDBControl.class) > is scattered around, making it almost impossible to have a subclassed > GDBControl service. Maybe as you said, "A better solution would be to add the > GDBControl service to the serviceFactory and then one could easily replace that > service.". This is an indication that the GDBControl class is used too much as an interface. I hope that the IProcess service implementation will help with this somewhat, but maybe there's a need for an official IGDBCommandControl interface?
(In reply to comment #6) > I like the idea of separating the management of the gdb process from the rest > of the work that the command control does, but there's not just the task of > launching the process, this manager also needs to monitor this process and > notify client when it exits. Maybe it would be better to create a separate > service for this purpose, which GDB would then depend on. What's in my patch is just abstract out the launching of gdb process and conversion of its parameters from launch configuration (e.g. converting executable path in the example I mentioned). Management of the gdb process after it's started remains unchanged, still done by GDBControl. Pawel and Marc, you may want to take a close look at my patch (especially the added files IGDBLauncher and BaseGDBLauncher) and then decide if it's good to commit to the code base. I'm not sure if it's necessary to promote it to a service, but I'm sure my work is a step forward to make the DSF framework more accommodating.
(In reply to comment #7) Since Marc is maintaining and developing new features in this code, it's really his call. For me the only reservation I have about the patch is that it adds a new type of extension mechanism to the architecture. So besides the fact that implementing this separation using a new service could allow for "modularizing" the managment of the GDB process, it would also be more consistent with the DSF architecture.
(In reply to comment #8) > (In reply to comment #7) > Since Marc is maintaining and developing new features in this code, it's really > his call. For me the only reservation I have about the patch is that it adds a > new type of extension mechanism to the architecture. So besides the fact that > implementing this separation using a new service could allow for "modularizing" > the managment of the GDB process, it would also be more consistent with the DSF > architecture. Thanks Pawel. I tend to agree with the idea of having a new service, as it fits better in the overall picture. I will however, look in more detail at the issue as soon as I have some time. These few weeks have been extremely busy, so Ling, you'll have to be patient with me a little more. Thanks.
(In reply to comment #9) > Ling, you'll have to be patient with me a little more. Thanks. No problem. Meanwhile I'll do more study of DSF code when time permits and see if I could create the service myself. Thanks !
Hi, Marc and Pawel, I've reimplemented my solution by creating a new service. IBackendDebuggerLauncher -> IGDBLauncher -> GDBLauncher And in GDBLaunchDelegate, this method newServiceFactory() is changed from "private" to "protected" so that a debugger implementation like mine can provide its own serviceFactory and thus its own service implementations including the new IGDBLauncher service. Note the IGDBLauncher is a light-weight serivce which just handles launching the GDB process and some option conversion. I thought about making it a broader one like IGDBProcessManager that could manage the life cycle of the GDB process (which is currently done by GDBControl), but I find it requires quite some change across the DSF code, which I think is not worth the effort and risk. My new patch is attached. Please review and comment. Thanks !
Created attachment 112230 [details] Implement IGDBLauncher as a DSF service
(In reply to comment #12) > Created an attachment (id=112230) [details] > Implement IGDBLauncher as a DSF service This looks interesting. I'll have a more detailed look. Thanks!
Ling, Thank you for posting the patch :-) Architectually I really like where this is going, I just think it needs to go a little further: - Rather than returning the Process object directly from the back end service, it would be more generic if the service would return input and output streams. Returning the process itself should be optional as some back ends may not be able to have access to it (e.g. back end running on a remote system). - I do think it would be better to include the management of the process, i.e. monitoring for when it terminates and sending out the appropriate events. These changes should be confined to GDB, which is still all provisional, so we have the flexibility of breaking the API backward compatibility. BTW, making broad changes is not necessarily bad as long as it's towards a better architecture :-) - The back end launcher should be capable of launching multiple sessions. Data model contexts could be used to track the different launched instances. - An implementation note: part of the service interface requirement is that service methods should be non-blocking and are called on the executor thread. So method such as GDBLauncher.launchBackendDebugger() should be asynchronous (take a request monitor arg.) and it should internally spin off a job to launch the process in background. - It would be ideal to have another implementation, in addition to GDB, as an example in order to demonstrate flexibility of the API. The PDA example also uses a process, but it uses multiple input streams. We should make sure that the new interface accommodates that use case and implement it as proof. Once again, I think this is an excellent contribution and I hope we can get it to a state to be committed soon. BTW, it will probably require IP approval so we should try to fit it in the M3 milestone.
(In reply to comment #14) Pawel, thanks for the detailed comment. I'll try to fix it in the following two weeks. BTW, I just added "configure GDB" in the summary to make it more accurate.
(In reply to comment #14) Hi Pawel, I created a generic service like this: public interface IBackendDebuggerService extends IDsfService { ... /* IDMContext & IDMEvent declarations */ /** * This is to get back end debugger process, if any. <br> * How the back end process is launched is transparent and is up to implementers. * Note it's not required that back end returns a valid process. * * @return Process - the Process representation of the underlying back end debugger, or null. */ public Process getBackendProcess(); } As to this > - Rather than returning the Process object directly from the back end service, > it would be more generic if the service would return input and output streams. > Returning the process itself should be optional as some back ends may not be > able to have access to it (e.g. back end running on a remote system). > I think we should still require Process object because: 1. Process offers not only interface to get inputstream & outputstream, but also other necessary interfaces such as destroy() that allows Eclipse to initiate killing of backend, and getErrorStream() for possible error check. 2. In the case of back end running remotely, implementor should be able to create its own Process implementation. For instance, for a process invoked remotely via SSH, we implemented "SSHProcess extends Process" to represent it. 3. As to the PDA example > The PDA example also > uses a process, but it uses multiple input streams. We should make sure that > the new interface accommodates that use case and implement it as proof. The "multiple input streams" you mentioned are those from sockets, right ? I really don't see how a generic service API that returns some streams could cover a special case like that. Instead it should be up to the PDA implementation of IBackendDebuggerService to return the two special input streams that are only understandable by PDA debugger. Comment ?
(In reply to comment #16) Just a note to say that I also really like this solution. Hopefully we can iron things out quickly to get it in. I'll let Pawel address the architectural questions. However, maybe I can be of some help about some other points Pawel brought up: > - I do think it would be better to include the management of the process, I think this can be done be moving the entire GDBControl.GDBProcessStep and GDBControl.MonitorJobStep to the new GDBLauncher service. > - An implementation note: part of the service interface requirement is that > service methods should be non-blocking So, for example the method Process launchBackendDebugger() throws CoreException; would be changed to void launchBackendDebugger(DataRequestMonitor<Process> rm); I believe the other method of IGDBLauncher can remain as is, since they only read information from the launch config. But if any of them may need to query the backend, they will need to use a DataRequestMonitor.
(In reply to comment #17) > > - I do think it would be better to include the management of the process, > > I think this can be done be moving the entire GDBControl.GDBProcessStep and > GDBControl.MonitorJobStep to the new GDBLauncher service. Yes, that's what I've done. > > > - An implementation note: part of the service interface requirement is that > > service methods should be non-blocking > > So, for example the method > Process launchBackendDebugger() throws CoreException; > would be changed to > void launchBackendDebugger(DataRequestMonitor<Process> rm); > As launching GDB is done in GDBProcessStep which carries out the launching in a job, it's not needed to change the launchBackendDebugger(). Actually my new design is like this. The generic IBackendDebuggerService (which manages back end debugger) no longer expose/requires API "launchBackendDebugger()". How to launch the backend and make sure it's ready (e.g. for GDB, we need to wait for "(gdb)" prompt) is totally up to the backend implementer. E.g., BackendGDBService, which replaces old GDBLauncher, has "protected Process launchGDBProcess()" which is specific to GDB. Anyway, I'll post my new patch soon for your review. Thanks.
(In reply to comment #18) > > So, for example the method > > Process launchBackendDebugger() throws CoreException; > > would be changed to > > void launchBackendDebugger(DataRequestMonitor<Process> rm); > > > As launching GDB is done in GDBProcessStep which carries out the launching > in a job, it's not needed to change the launchBackendDebugger(). The job is asynchronous and will make launchBackendDebugger return right away, before the job is completed. The requestMonitor is used by the job when the actual 'return' value is known. > The generic IBackendDebuggerService (which manages back end debugger) > no longer expose/requires API "launchBackendDebugger()". So I guess there is not need for an IBackendDebuggerService interface in the DSF plugins, but this new service can be entirely in MI and GDB plugins. But I guess I'll understand more clearly when I see your new patch. > Anyway, I'll post my new patch soon for your review. Looking forward to it.
(In reply to comment #16) > ... > I think we should still require Process object because:... I agree that it's useful to have the process returned by the API, but making it a requirement places a large burden on the implementation. That's why I suggested making it optional. > The "multiple input streams" you mentioned are those from sockets, right ? I > really don't see how a generic service API that returns some streams could > cover a special case like that. Instead it should be up to the PDA > implementation of IBackendDebuggerService to return the two special input > streams that are only understandable by PDA debugger. I guess this begs the question what are the clients of this interface? If PDA should use internal methods to get at the IO streams, why shouldn't GDB? IMO, there is room for a generic client that creates process objects (org.eclipse.debug.core.model.IProcess) representing the back end entities. In case of PDA there is a single process, but multiple IO streams. So the API I imagined would have a a method such as: IChannelDMContext[] getChannels(); Process getProcess(IChannelDMContext); public InputStream getErrorStream(IChannelDMContext); public InputStream getInputStream(IChannelDMContext); public OutputStream getOutputStream(IChannelDMContext); I'll wait for the next patch to comment further. Cheers, Pawel
(In reply to comment #20) > (In reply to comment #16) > > ... > > I think we should still require Process object because:... > I agree that it's useful to have the process returned by the API, but making it > a requirement places a large burden on the implementation. That's why I > suggested making it optional. Yes, that's optional. > > > The "multiple input streams" you mentioned are those from sockets, right ? I > > really don't see how a generic service API that returns some streams could > > cover a special case like that. Instead it should be up to the PDA > > implementation of IBackendDebuggerService to return the two special input > > streams that are only understandable by PDA debugger. > I guess this begs the question what are the clients of this interface? If PDA > should use internal methods to get at the IO streams, why shouldn't GDB? IMO, > there is room for a generic client that creates process objects > (org.eclipse.debug.core.model.IProcess) representing the back end entities. You mean having an IProcess object in the Debug View for the backend ? If yes, I would disagree. There is case in Nokia debugger that we don't want backend process to show up in Debug View. > > In case of PDA there is a single process, but multiple IO streams. So the API > I imagined would have a a method such as: > > IChannelDMContext[] getChannels(); > Process getProcess(IChannelDMContext); > public InputStream getErrorStream(IChannelDMContext); > public InputStream getInputStream(IChannelDMContext); > public OutputStream getOutputStream(IChannelDMContext); I don't get what the "channel" is. Is the IChannelDMContext supposed for case like multi-backend-in-one-session ? > > > I'll wait for the next patch to comment further. Ok, it's here.
Created attachment 113909 [details] full backend service 1. I implemented the backend service for both GDB and PDA. For PDA, it surely makes the code including JUnit test code cleaner. 2. I haven't made changes to classes related to gdb 7.0 such as GDBControl_7_0, as (1) I'm really wondering why we cann't derive those classes from non-7.0 ones. They contain mostly duplicated code. (2) It's better for me not change them until you approve my change to non-7.0 ones :)
Oh, one more note about the DMContext for GDBBackendService. I don't see real need for it right now because 1) Currently we only support one backend per session. To support multi-backend-in-one-sesion for case like multi-core debug, quite some change is necessary all over DSF, IMHO, which is beyond scope of this bugzilla entry. 2) There is no UI component that needs to access the DMContext of the backend service, as I see. correct me if I'm wrong.
Created attachment 114029 [details] Updated patch. I've been experimenting with the patch for a little while. I also started refactoring it a bit (simplifying some names etc). But I'm actually have some other high-priority tasks I need to attend to and I won't be able to get back to this till next week or so. Overall I think this patch is getting close to being ready to commit it, but I still have a couple of reservations: - My main hang up is having the java.lang.Process object being returned directly by the service interface. This object does not fit the DSF threading model very cleanly (as it can block and throw exceptions all over the place), it is no easily extended to represent things other local processes, and is not even required by the standard debug model. If I had the time right now, I would try to hide the use of the Process object inside of the respective backend service implementations. Instead I would have the backend service return things which could be used to implement IProcess directly (stream monitors, etc.). As it is, I think a quick alternative is to hide the getProcess() object in the sub-classes of the IBackend interface. - Since ICommandControlService is designed to deal with only a single instance of a back end, I think it make sense to make the same restriction on the IBackend interface. We can plan for the future of having multiple backends in a single session in the same way as we did with ICommandControlService. In fact, I think for most cases, the command control DM context and the back end DM context could be the same. - In the PDA implementaiton, I removed the reference to the launch. As much as possible, I would like to keep references to standard debug model objects out of the service implementations. - Backend started/exited events are defined but not used in PDA. Actually PDA doesn't implement ICommandControlService interface yet either, so I think this will need to be done for completeness as well. - There's still many comments missing and some comments need cleanup (we don't use the @author tag since we use the copyright statement for the same purpose). Some copyright statement also need updating. The patch I'm attaching is a work in progress, I haven't even tried to run it, but it starts to cover the issues I described above.
(In reply to comment #22) > Created an attachment (id=113909) [details] > full backend service Let me first say that I was impressed by the quality of the path. > (1) I'm really wondering why we can't derive those classes from non-7.0 ones. > They contain mostly duplicated code. We made a decision to duplicate services instead of adapting them or deriving them. The idea was to keep a service class very close to the GDB version. We didn't like the code duplication, but we felt it was still better as it will help keep the logic cleaner. At least that is our hope :-) The more I think about the patch, the more I wonder about the difference between IBackendService and ICommandControlService. 1- With Pawel's change, the base IBackendService does not provide any functionality anymore. It is in fact, almost a copy of ICommandControlService. 2- IBackendDMContext should be implemented by the same class as for ICommandControlDMContext 3- IBackend*DMEvent will pretty much trigger ICommandControl*DMEvent 4- All the functions Ling needs are done in IGDBBackendService 5- IBackendService is meant to control the life-cycle of the backend. That is how we imagined this new service. IGDBBackendService does much more than that with methods like getProgramPath(), getGDBInitFile(), etc Reading this, I can't help to wonder if we really need to separate the two services? Ling's real need is to be able to override certain parts of the launch and configuration of GDB. I think we can try to do this with GDBControl only. a) GDBControl is now part of the ServiceFactory (which was not the case when this bug was written), and IGDBControl is now available b) Ling has well defined the methods needed to be overriden (in IGDBBackendService) So, I suggest not to have IBackendService and simply merge IGDBBackendService into IGDBControl. Then have FinalLaunchSequence use IGDBControl instead of IGDBBackend. I think this will re-use most of the patch and yet make it much simpler. And everything should be customizable as is needed by this bug. What do you think?
My hope was that by using the backend service we could make a clean separation between the logic of managing the back end process and managing the protocol. This way a completely different back end could be used (such as one running one a remote machine and communicating through sockets), while reusing the command control as is. This is why I suggested that the generic IBackend interface contain a generic API for describing communication channels that would then be consumed by the command control. Unfortunately this is kind of a major architectural change and it does necessarily add more complexity to the overall system. On the upside, it could somewhat break up the rather large and monolithic MI Control we have now. i would still like to take a shot at it sometime next week, even though we're rather running out of time.
(In reply to comment #25) > The more I think about the patch, the more I wonder about the difference > between IBackendService and ICommandControlService. > 1- With Pawel's change, the base IBackendService does not provide any > functionality anymore. It is in fact, almost a copy of ICommandControlService. But the concept of having a separate IBackend service does offer obvious benefit, making the logic and code cleaner and more modular. ICommandControl handles communication to backend while IBackend handles the starting/terminating backend and making sure backend is ready to accept commands. For instance, my original request of the bug entry is better implemented by providing my GDBBackend service instead of providing a new CommandControl service. Actually my implementation of IBackend for PDA example better proves that, where at least a big chunk of duplicated code in JUnit test is removed and backend details like "RequestPort" and "EventPort" are hidden by the service. IMHO, renaming GDBControl to GDBCommandControl could better reflect the design:) > 2- IBackendDMContext should be implemented by the same class as for > ICommandControlDMContext True, the IBackendDMContext currently is of no real value. > 3- IBackend*DMEvent will pretty much trigger ICommandControl*DMEvent I thought about replacing ICommandControl*Events with IBackend*Events. The ICommandControlShutdownEvent can be replaced by IBackendTerminatedEvent, but ICommandControlInitializedEvent has more meaning that IBackendStartedEvent in that it's fired after more preparation steps are done in addition to starting of backend. Anyway, I do agree with Pawel on this: > ...by using the backend service we could make a clean separation > between the logic of managing the back end process and managing the protocol. > This way a completely different back end could be used (such as one running one > a remote machine and communicating through sockets), while reusing the command > control as is. T However, I kinda disagree on this point of Pawel. > This is why I suggested that the generic IBackend interface > contain a generic API for describing communication channels that would then be > consumed by the command control. For instance, PDA example use three special streams RequestInputStream, RequestOutputStream, and EventInputStream instead of standard input/output/err streams of a process, which indicates that it's hard (if not impossible) to describing the communication channels in a generic API.
Created attachment 114856 [details] Update patch. I played around with this patch today and I think i simplified it a bit. I removed the common IBackend and replaced it with IMIBackend, which can be more meaningful I think. Also I removed the DM context from the backend, because it seemed redundant. Left to do: - PDA still needs to be updated, - GDBControl_7_0 needs to be converted - Consider adding the streams proxy monitor to IMIBackend.
Created attachment 114958 [details] Updated patch. Further refactoring: I got rid of GDBCLIProcess and replaced it with a generic MIBackendCLIProcess which uses only IMIBackend interface. I also tried to clean up the code style and comments. The only major thing that's left is the GDBControl_7_0. Marc, would you be able to take a look at it. I could refactor it, but I'm not familiar with the detailed differences and I can't test it either...
(In reply to comment #29) > Created an attachment (id=114958) [details] > Updated patch. The latch patch seems to be missing some files, IGDBBackend, for example. > The only major thing that's left is the GDBControl_7_0. Marc, would you be > able to take a look at it. I could refactor it, but I'm not familiar with the > detailed differences and I can't test it either... It was a day off in Canada today, which is why I haven't looked at it yet. But to get it in M3, I'll have a look right now. I'm hoping you can post the complete patch...
Couple of preliminary questions: 1- did you mean to keep IGDBBackend.getProcess() commented out? 2- did you consider having a method in IMIBackend to start the backend, instead of starting it immediately when the service is initialized?
Created attachment 114993 [details] Updated patch. Here's the patch again (hopefully complete). - Yes I got rid of the getProcess() because I realized I could implement a generic version of the CLI process object without it. - I'm not really sure about a "start()" method. I do think it would create a more clear connection between the startup parameters (currently in the constructor) and the action which uses them (currently the initialize() method). But besides that I don't know if there are other benefits.
Created attachment 115048 [details] Updated patch For convenience, the attached patch includes the comments that I have below. (Note that I didn't review the PDA changes.) I had a look a the patch and I think it is great. I want to make some changes to IGDBBackend, but since it will not be blocked by M3 and I wanted to post something as soon as possible, I didn't waste time on that yet, I'll look at it this afternoon. Here are comments. When the bullet is a '?' it means it is a question but I didn't do anything about it in my patch. When the bullet is a '-' it means I changed it in the attached patch. IMIBackend: ? You don't need to put the public keyword in interfaces (but it does not hurt :-)) ? for getExitCode(), the javadoc says that it returns 0 if the backend exit code is not available. I'm not sure when this can happen, but 0 is a valid exitCode I believe. In fact, -1 (which I was going to suggest) is also a valid code... Maybe getExitCode() should return an Integer type, and return null if the exitCode is not available? ? do we want a method to return the error stream? We don't use it now, but maybe another backend would? MIBackendCLIProcess.java: - BackedExitedEventListener should make sure the event received is a TERMINATED event ? the old method waitFor() would check if isDisposed(). It is still done for exitValue() but no longer for waitFor(). Is that on purpose? ? In waitFor(), you wait for BackendStateChangedEvent event rather than call process.waitFor() as we used to. I'm just trying to understand why the change? - Should replace the string "GDB Process has not exited" with "Backend Process has not exited" - in exitValue(), I think the last return be -1 instead of 0 ShutdownSequence.java, ServicesLaunchSequence.java and GdbDebugServicesFactory.java: - I prefer to use IMIBackend.class instead of IGDBBackend.class to create the service. I find it more future proof, in case we get rid of IGDBBackend. Not a big deal in this particular case, but it follows the pattern used in the rest of the service factory. You got my vote to commit if you want.
Created attachment 115066 [details] Final patch. I did some more cleanup and committed this patch.
(In reply to comment #33) > ... I want to make some changes to IGDBBackend... Yes, I also think this needs some more cleanup: things like isAttach() and getSessionType() seem to belong in IGDBBackend. > IMIBackend: > ? You don't need to put the public keyword in interfaces... I added it where I didn't see it yet. > ? for getExitCode(), the javadoc says that it returns 0 ... The main client is IProcess.exitCode() and it returns an "int" so I don't think it makes sense to return a null. But I did change it to -1. > ? do we want a method to return the error stream? We don't use it now, but > maybe another backend would? MI protocol doesn't define an error stream, so I don't think it makes sense in IMIBackend. But I do want to add an IStreamsProxy to be returned by IMIBackend eventually... to use in the console, and that does include the error stream. > MIBackendCLIProcess.java: I reworked the whole exit status logic. It was broken anyway because it wasn't able to retrieve the exit value after GDB exited and the services were shut down... which is kind of the point of it. > ? In waitFor(), you wait for BackendStateChangedEvent event rather than call > process.waitFor() as we used to. I'm just trying to understand why the > change? I think including java.lang.Process object in the interface is not good because it's not very flexible, so I'm glad to get rid of it. > You got my vote to commit if you want. I did, thanks for the quick review, I'm glad we were able to get this in without breaking our API deadline :-)
Committed, Marc please review.
Created attachment 115083 [details] Cleanup for some GDB stuff I'm quite happy with the results of this bug. This patch does some cleanup to GDB parts, of which the most important ones are: - fix the JUnit tests - remove get getGDBCommandLine() from IGDBBackend, which was not needed. This may need a confirmation from Ling. Committed. I still want to look at a couple of things before I mark as verified.
I verified it with our debugger. It worked fine.
Created attachment 115298 [details] More cleanup of IGDBControl vs IGDBBackend This patch moves getSessionType() and getIsAttachSession() from IGDBControl to IGDBBackend. Also, it removes IGDBControl.getExecutablePath() since IGDBBackend.getProgramPath() did the same thing. I believe IGDBControl.terminate() should be in IGDBBackend, since it request that the entire GDB exit. However, this calls sends the -gdb-exit command which requires the use of the CommandControl; moving terminate() would introduce a dependency from IGDBBackend to IGDBControl, while we already have the reverse. Therfore, I didn't move the terminate() call. Waiting for status on M3 before committing.
(In reply to comment #39) > Created an attachment (id=115298) [details] > More cleanup of IGDBControl vs IGDBBackend Committed this patch. I believe these are all the changes I wanted to make. Marking as Verified.
