Community
Participate
Working Groups
From bug 239028 comment #9: I don't know what kind of discovery mechanism GDB is going to provide to determine whether a given group context represents a process with an address space, which groups can be used to set breakpoints on, etc. But, for now I think we can make the assumption that an execution group represents a process or a core, and they both have an address space and can be used to set breakpoints, send signals, etc. So either MIExecutionGroupDMC should implement the context interfaces that GDBControlDMContext implements, or more correctly there should be a GDBExecutionGroupDMC which does. In either case, GDBControlDMContext should not implement these interfaces anymore since this context now becomes just an empty parent context which could be later used to identify the correct command control to send the commands to. In fact the GDBControlDMContext class could be eliminated and MIControlDMContext could be used instead.
I want to do this fix cleanly, so I started looking at one type of context at a time. The first one is the IMemoryDMContext. After verifying with the GDB maintainers, it is now confirmed that GDB memory commands must be performed in the context of a specific process. Therefore, as expected, our IMemoryDMContext must be moved from GDBControlDMContext to something else. However, things are not as simple as we would hope. The MI protocol does not provide a way to specify a process (thread-group) but only a specific thread within a process. So, for a -data-read-memory command (for example,) we can either do a thread-select or use the --thread option, using some thread within the process we want to select. Currently, the GDB maintainer does not want to add a --thread-group type of option (like the --thread option) because, from a GDB point-of-view, it is not clear if every platform will have a memory space per process, or have a memory space per thread. For our implementation, we could do something like this: 1- Make MIContainerDMC implement IMemoryDMContext. Then, choose some valid (random) thread within that MIContainer (process). (I won't go into the details on how to do that yet) 2- Make MIExecutionDMC implement IMemoryDMContext. This will always give us a proper threadId for a memory context. This also allows DSF to cover the case of a memory space per thread. However, if the user selects the whole process in the debug view, the memory view will not show anything since the Memory Context will be only valid for threads. 3- Make both MIContainerDMC and MIExecutionDMC implement IMemoryDMContext. This would allow us to use the specific thread chosen by the user; but in the case of the user selecting a process, we would again have to find some 'random' thread. I wanted to prototype a bit to get a feel for these solutions. But to do this, I need a bit of help with the Memory stuff. I wanted to know if the DsfMemoryBlockRetrieval was meant to only have one instance, or to have one instance per context? Also, from what I see, a DsfMemoryBlock is associated with a context. But I don't understand how the user can see this from the Memory view? If I create a memory monitor towards a context, how is this shown to the user, once the monitor is listed? For expressions, an expression is not attached to a fixed context, but to the one currently selected. This does not seem to be the case for the memory view. I was thinking we could have a discussion at our "weekly" meeting, if it is easier than going through bugzilla. BTW, Francois will not be back for another two weeks, which is why I started looking at this now.
(In reply to comment #1) > I want to do this fix cleanly, so I started looking at one type of context at a > time. The first one is the IMemoryDMContext.... IMO, this is partly the short coming of using separate name spaces for threads and thread groups on part of GDB. If --thread could be used to specify either a thread or thread-group, the protocol could be made a little less messy. But anyway, none of these options seem ideal but the least damaging to me seems a version of the first option. Rather than actually storing a thread id in the context though, I would have GDBControl be smart enough to see that a context is a memory context, and based on that perform a thread-select on one of them. This way GDBControl becomes the central place for dealing with GDB protocol inconsistencies and bugs... which is nice to do centrally. BTW, I think it's probably OK to have MIProcesses.MIContainerDMC be protected, and extend it in GDBProcesses, since they're exactly the same and that makes them a little confusing. > I wanted to know if the DsfMemoryBlockRetrieval was meant to only have one > instance, or to have one instance per context? Francois made it for one memory context, I extended it to cover multiple. So there may be some inconsistency in that. > Also, from what I see, a DsfMemoryBlock is associated with a context. But I > don't understand how the user can see this from the Memory view?... That's correct, this is a little different. The user just sees what blocks are available based on the selection in Debug view. Ted (and many of our users) think that memory blocks are unnecessary and confusing, but since they are part of platform, it will be very hard to change them (in APIs and the UI). > I was thinking we could have a discussion at our "weekly" meeting, if it is > easier than going through bugzilla. True, though this week I'm at the CDT summit, and won't be able to do our weekly meeting. > BTW, Francois will not be back for another two weeks, which is why I started > looking at this now. I'm glad to hear that he'll be back soon then :-)
Created attachment 113487 [details] Removing GDBControlDMContext (In reply to comment #0) This patch addresses the main point of this bug, which is to move the interfaces from GDBControlDMContext to MIContainerDMContext. For the old GDB, there is a single container (process) and it should be used everywhere, just like GDBControlDMC was. For GDB 7.0, the support for multi-process is still evolving and not fully available; therefore, I took the same approach of 'hard-coding' the single process that is being debugged; this will need to be enhanced, when it is available in GDB. In this path, I did the full change of itnerfaces, but I think each interface should be checked individually, as GDB evolves (like the fact that for Memory, we need to do some juggling as pointed out in comment #1 and coment #2.) Another example is that global breakpoints will be supported, and may affect our use of the IBreakpointDMC interface. So, exactly as Pawel suggested, the following was done: 1- GDBControlDMContext has been removed altogether and MIControlDMContext is used instead. Actually, I ran into an API Tooling problem here. The API Tooling reported an error when I removed the GDBControlDMContext, although it is in an internal/provisional API. Furthermore, if I tried creating a filter for that error, it would crash my eclipse (I tried 5 times). So, I left the class there, but unused, and maybe some else can try removing it? 2- GDBContainerDMC has been added to GDBProcesses and GDBProcesses_7_0 to extend MIContainerDMC, and it implements all the interfaces that GDBControlDMContext used to implement. And as Pawel mentioned with such foresight, I had to make MIContainerDMC protected. 3- In every place where GDBControlDMContext was blindly cast to another interface, I replaced the GDBControlDMC with a GDBContainerDMC (and blindly cast it to the proper context interface :-)) 4- Deprecated and stopped using InferiorStartedDMEvent since ContainerStartedDMEvent should be used instead. For the legacy classes, ContainerStartedDMEvent is issued by GDBControl where it used to issue InferiorStartedDMEvent. For the 7_0 classes, ContainerStartedDMEvent is being issued when GDB sends the =thread-group-created event. 5- Deprecated and stopped using InferiorExitedDMEvent since ContainerExitedDMEvent should be used instead. For the legacy classes, ContainerExitedDMEvent is issued by MIInferiorProcess where it used to issue InferiorExitedDMEvent. For the 7_0 classes, ContainerExitedDMEvent is being issued when GDB sends the =thread-group-exited event; I'm not 100% sure this will be enough for the shutdown case so it is also issued by MIInferiorProcess; time will tell if this should be changed. Note that getting the ContainerDMC to the MIInferiorProcess class needed some 'hacking'. I'm hoping to clean that up eventually, when we deal with multiple inferiors. 6- In LaunchVMProvider, there is a method canSkipHandlingEvent(). I don't know what this method is for, but it made sure Inferior*DMEvent are not 'skipped'. Now that I don't use them anymore, should I have the canSkipHandlingEvent() method look for IStartDMEvent or maybe ContainerStartedDMEvent or is this not needed? I need clarification on point 6. Thanks. This patch breaks the GDB JUnit tests so I won't commit it right away. I'' first try to fix the tests and commit everything at once.
Created attachment 113503 [details] Fix for tests Fixing the tests turned out to be easier than I thought. This patch finally gets all the JUnit tests working (except one in Breakpoints, which we have a bug on.) I committed both patches of this bug.
This bug is fixed. Before verifying it though: 1- we need to try to remove GDBControlDMContext.java 2- we need to confirm that LaunchVMProvider.canSkipHandlingEvent() was changed properly (see comment #3 part 6) Pawel, it's all yours :-)
I opened Bug 248636 to address comment #1 and comment #2
Sorry for the flood of comments... we can deal with this in the weekly meeting if it makes it easier. (In reply to comment #2) > Rather than actually storing a thread id in the > context though, I would have GDBControl be smart enough to see that a context > is a memory context, and based on that perform a thread-select on one of them. > This way GDBControl becomes the central place for dealing with GDB protocol > inconsistencies and bugs... which is nice to do centrally. I also like keeping this central. In fact, I was thinking that any command that applies to a context at the process level, should force GDB to select that process. So, I was thinking that when we extract the threadId in AbstractMIControl, if we don't find and IMIExecutionContext, we would then look for an IMIContainerContext, and if there is one, pick a threadId belonging to that process. This would give a general solution to making sure GDB knows which process we are interested in. The problem however, is that GDB needs to know the process for memory commands, but not for the others (yet.) For instance, setting a breakpoint is not process-specific just yet in GDB. In fact, when launching a program, the breakpoints must be set -before- the program is run, so we have no idea which threads belong that that process. So, using --thread for these contexts does not really work yet, except for the memory context. The problem is that once AbstractMIControl gets the context, it cannot know if it should treat it as a memoryCtx or a breakpointCtx, or a disassemblyCtx, etc, since all of these contexts are implemented by the same class: GDBContainerDMC. I'd have to base the decision on the command type, which is not very nice. So I'm thinking that maybe I jumped the gun on moving ISymbolDMContext, IBreakpointsTargetDMContext, ISourceLookupDMContext, ISignalsDMContext, IDisassemblyDMContext away from the old GDBControlDMContext... Until GDB supports those things per-process, I guess I should have left them global to the entire CommandControl. We can discuss in detail in the meeting as soon as we can have one.
(In reply to comment #7) > So I'm thinking that maybe I jumped the gun on moving ... I see the dilemma. I guess we could move the interface back to the command control for those operations which are not per-process yet, but I guess we can hash it out some more on Thursday.
Re-opening until we discuss the issue.
Created attachment 113920 [details] Small fix I had made a small mistake in retrieving the Processes service in the fix for this bug. This is an update. Committed
(In reply to comment #3) > 6- In LaunchVMProvider, there is a method canSkipHandlingEvent(). I don't know > what this method is for, but it made sure Inferior*DMEvent are not 'skipped'. > Now that I don't use them anymore, should I have the canSkipHandlingEvent() > method look for IStartDMEvent or maybe ContainerStartedDMEvent or is this not > needed? This method is used for optimization, and yes it needs to make an exception for the started/exited events just like it did for the inferior events. I created bug 249239 for this issue.
As we agreed in the meeting (or at least hinted to,) I have moved ISymbolDMContext, IBreakpointsTargetDMContext, ISourceLookupDMContext, ISignalsDMContext, IDisassemblyDMContext back to GDBControlDMContext. Maybe later, depending on GDB's final solution, we can move things back. So, GDBControlDMContext is no longer an IMemoryDMContext or an IContainerDMContext. Those interface are implemented by GDBContainerDMC. Committed
Created attachment 114342 [details] Partial reverting of the fix (In reply to comment #12) I forgot to post the patch that I committed.
Ok, so this bug is fixed for the current GDB. I will continue working on the specific memory changes as part of bug 248636. Pawel, can you verify?
Removing the refactoring is a bummer, but I guess it's the reality of working with a protocol that is is a moving target.
DD 1.1 reelased!
