Community
Participate
Working Groups
From bug 237556 command #22: "In MIRunControlEventProcessor, we now issue a MIRunningEvent, when getting the "running" event from GDB. However, we still issue MIRunningEvents when getting ^running (also in MIRunControlEventProcessor) and when using the console to make the program run (CLIEventProcessor). This is not a problem for the old GDBs since *running does not exist, but won't it be a problem with the new non-stop GDB? I think we have the same issue in MIRunControlEventProcessor for ThreadCreatedEvent which is now issued with the new =thread-created but also in the CLIEventProcessor when ~"[New Thread 0xb7c76ba0 (LWP 18630)]\n" is seen. ThreadExitedEvent is also affected for all-stop with the new GDB, but not when actually running in non-stop mode, I think." With the fix of Bug 241985, GDBControl is now instanciated by the service factory. It is therefore possible to create a new GDBControl version which will create new EventProcessor classes to avoid this duplication of events.
I just wanted to confirm that the agree solution to this bug is to create a new GDBControl_7_0 class, which will instantiate a new MIRunControlEventProcessor_7_0 to deal with events, and probably also a new CLIEventProcessor_7_0. If that is agreed way to proceed, I will address this as a first step towards a fix to bug 245749.
(In reply to comment #1) Yes, this make sense to me.
(In reply to comment #2) > (In reply to comment #1) > Yes, this make sense to me. I started doing this, and it also means duplicating AbstractMIControl... What do you say?
(In reply to comment #3) > I started doing this, and it also means duplicating AbstractMIControl... > What do you say? I think that's OK too. I thought we were going to have to do this to accomodate --thread anyway.
(In reply to comment #4) > (In reply to comment #3) > > I started doing this, and it also means duplicating AbstractMIControl... > > What do you say? > I think that's OK too. I thought we were going to have to do this to > accomodate --thread anyway. We had decided to change the constructor and pass in a boolean to say to use -- thread or not. It turns out that getting a GDBControl_7_0 is a little more involved than I thought. It affects AbstractMIControl_7_0.java GDBInferiorProcess_7_0.java MIInferiorProcess_7_0.java AbstractCLIProcess_7_0.java GDBCLIProcess_7_0.java CLIEventProcessor_7_0.java MIRunControlEventProcessor_7_0.java Or maybe we can re-use those, but we'd have to introduce some new interfaces because right now, some constructor types are hard-coded as GDBControl instead of GDBControl_7_0 I'm gonna think about this in more detail and try to come up with a clear suggestions. More to come...
Created attachment 111855 [details] Replacing AbstractMIControl with ICommandControlService Originally, in most places in the code, we used the ICommandControl interface to deal with the commandControl service. With changes to the cache (see bug 237325) we had to use AbstractMIControl directly to be able to access the MICommandControl context. Later on, with bug 243611, the interface ICommandControlService was introduced and allows to get the context. This patch replaces most AbstractMIControl ussage with ICommandControlService, to allow for different versions of AbstractMIControl. This patch is backwards compatible for the classes that were part of 1.0. The only part I'm uncomfortable with is the use of PROP_INSTANCE_ID from within class MIControlDMContext.java. I guess I could have left it using AbstractMIControl, but I thought it was asking for trouble. I am not committing because I need to wait for the M2 milestone to be good and done.
Created attachment 111868 [details] Also replace MIControlDMContext with ICommandControlDMContext This patch builds on top of the previous one to also replace MIControlDMContext with ICommandControlDMContext in a lot of places. Although this is not necessary to allow versioning of services, it its more proper. Not committed, waiting for M2
(In reply to comment #7) > Created an attachment (id=111868) [details] I might have a compatibility issue in this patch. Is it ok to change public static MIInferiorExitEvent parse(ICommandControlDMContext ctx, int token, MIResult[] results) to public static MIInferiorExitEvent parse(MIControlDMContext ctx, int token, MIResult[] results) For constructors, I believe this is ok, but for methods, the problem I am worried about is that if a class overrides this method, this change will break this. And if the @Override annotation was used, the compilation will fail for the overriding class. Opinions? P.S. This is also the case in MIInferiorSignalExitEvent.java
Created attachment 112110 [details] Cleanup patch to be committed This is the patch that I actually committed, after merging the latest changes coming from M2 testing. Comments are welcome, especially on: 1- creating a new PROP_INSTANCE_ID constant in MIControlDMContext and removing it from AbstractMIControl.java 2- potential backwards-compatibility issue of the changes to parse() in MIInferiorSignalExitEvent.java and in MIInferiorExitEvent.java (see comment #8)
Created attachment 112118 [details] New IGDBControl interface To be able to version the GDBControl service, we cannot make direct reference to the class GDBControl. That class contains a bunch of private methods however. What I did was to brute-force include all these methods in a new interface IGDBControl. I have then replaced every reference to GDBControl with IGDBControl. Before officially releasing IGDBControl in 1.1, we should clean it up as much as possible; for instance the terminate() method can probably be replaced by a call to IProcesses.terminate(), etc. I have opened bug 246771 to track this effort. Also, I have moved SessionType out of GDBControl and into its own class. I committed this patch.
Created attachment 112239 [details] Cleanup of GDBControl class from JUnit tests The JUnit tests made use of GDBControl in a hard-coded fashion. This patch replaces uses by ICommandControlService or IGDBControl Committed Note that the Memory tests are still not fixed, but that all others pass (except for one in Breakpoints)
Created attachment 112346 [details] Fix for events being sent twice Finally, this is the actual fix for the bug. It creates GDBControl_7_0, CLIEventProcessor_7_0 and MIRunControlEventProcessor_7_0. It also reverts CLIEventProcessor and MIRunControlEventProcessor to the logic needed for a GDB pre 7.0 (the way we released it in 1.0) MIThreadExited and MIThreadCreated events are now only issued once. Running events are also issued only once, however, it needed a little gymnastics to keep the same functionality. Before 7.0, what we did was trigger the event once a continue, step or next command was executed; this allowed to also know the type of command that caused the running event. With the new MI event "*running", we don't know the type of the command that caused it. What I did was, like before, when a continue, step, next command is issued, I store the type of the command, and once the *running event occurs, I use that type. I do this using a global MIRunControlEventProcessor_7_0.fLastRunningCmdType The problem I have, is that if the continue, etc command is given through the CLI and therefore parsed by CLIEventProcessor_7_0, it has no way of setting MIRunControlEventProcessor_7_0.fLastRunningCmdType. Before somehow giving access to this, I wanted to post the solution in case other ideas were presented. I committed this patch, which fixed the bug, but I will leave it open until this last small issue is resolved.
