Bug 248734 - [model] Simplify context hierarchy
Summary: [model] Simplify context hierarchy
Status: RESOLVED WONTFIX
Alias: None
Product: CDT
Classification: Tools
Component: cdt-debug-dsf-gdb (show other bugs)
Version: 0 DD 1.1   Edit
Hardware: PC Linux
: P3 enhancement (vote)
Target Milestone: ---   Edit
Assignee: cdt-debug-inbox@eclipse.org CLA
QA Contact: Marc Khouzam CLA
URL:
Whiteboard:
Keywords: api
Depends on:
Blocks:
 
Reported: 2008-09-26 11:23 EDT by Marc Khouzam CLA
Modified: 2009-01-07 18:38 EST (History)
2 users (show)

See Also:


Attachments
Proposed fix (68.51 KB, patch)
2008-09-26 11:23 EDT, Marc Khouzam CLA
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Marc Khouzam CLA 2008-09-26 11:23:28 EDT
Created attachment 113601 [details]
Proposed fix

After thinking some more about how we deal with contexts as part of Bug 244749, the relationship between IContainerDMContext and IProcessDMContext, and the one between IExecutionDMContext and IThreadDMContext was bothering me.   The context hierarchy is not simple and requires a bit of juggling is some places to get to the context.

This bug is a proposal for a new hierarchy.

The old hierarchy was:

//                           MIControlDMContext
//                                |
//                           MIProcessDMC (IProcess)
//     MIContainerDMC _____/      |
//     (IContainer)               |
//          |                MIThreadDMC (IThread)
//    MIExecutionDMC  _____/
//     (IExecution)

How about if we did this instead:

//   Process not being debugged               Process being debugged
//   --------------------------               ----------------------
//
//       MIControlDMContext                   MIControlDMContext
//              |                                      |
//     MIProcessDMC (IProcess)       MIContainerDMC (IProcess and IContainer)
//              |                                      |
//      MIThreadDMC (IThread)         MIExecutionDMC (IThread and IExecution)

The hierarchy is linear, creating context becomes simpler, with an MIContainerDMC one can easily use both IProcess or IContainer, and the same for MIExecutionDMC.

Further, we currently use the same id for the OSThread as for the ExecutionThread, and the same id for the OSProcess as for the Container; so it makes this joint context quite simple.  If the ids were different, it would not be a problem as we would simply store two ids in the class.

Also, I'm hoping it may open the door to having more specific contexts use in IProcesses, although I'm not sure about that yet.

The attached patch contains the necessary changes which seems to work well.  The majority of changes are simplifications in the creation of the contexts.

I would appreciate opinions.

Thanks.
Comment 1 Pawel Piech CLA 2008-09-29 17:45:11 EDT
Yes, the complex hierarchy is pretty difficult to manage.  Which makes me wonder if it's even worth it to support it in the IDMContext interface.

The one potential breaking problem with a single hierarchy solution is that a process context which is not being debugged (which would be used in a process list view for example), should be equal to a process which is being debugged.  So I think the simplified solution should be to have the IProcess and IThread interfaces always implemented by the same context classes, which also implement IContainer and IExecution (DMContexts).  I.e. the hierarchies for processes being debugged and not being debugged should be the same.
Comment 2 Marc Khouzam CLA 2008-09-29 20:50:40 EDT
(In reply to comment #1)
> Yes, the complex hierarchy is pretty difficult to manage.  Which makes me
> wonder if it's even worth it to support it in the IDMContext interface.
[...] 
> I.e. the hierarchies for processes
> being debugged and not being debugged should be the same.

So ultimately, IContainer and IProcess would be different names for the same thing (same for IThread and IExecution); and this would allow for the use of a specialized context within IProcesses and IRunControl without using each others context.

So, you suggest that I get rid of MIProcessDMC and MIThreadDMC and always use MIContainerDMC and MIExecutionDMC.  That is even easier than my suggestion.  

I do have a concern that if the GDB threadGroupId is no longer the pid, we would have to maintain two ids in the MIContainerDMC, which would imply that the context with the debugger attached would not be the same as with the debugger not attached.  I'm not convinced that GDB will always use the pid...
 

Comment 3 Pawel Piech CLA 2008-09-30 10:38:14 EDT
(In reply to comment #2)
> I do have a concern that if the GDB threadGroupId is no longer the pid, we
> would have to maintain two ids in the MIContainerDMC, which would imply that
> the context with the debugger attached would not be the same as with the
> debugger not attached.  I'm not convinced that GDB will always use the pid...

This is definitely a big risk :-(  Another alternative is what I had in the original IProcesses service interface: conversion method for debug and non-debug contexts

IProcessDMContext getProcessForDebugContext(IDMContext dmc);
IThreadDMContext getThreadForDebugContext(IDMContext dmc);
(the reverse mappings were obtainable through DM data)

This interface basically allowed the two context hierarchies to be independent, but it also complicated the UI quite a bit.  We could go back to this and still use a single hierarchy, but we would have the option of changing that in the future if needed.
Comment 4 Marc Khouzam CLA 2008-09-30 10:59:25 EDT
In reply to comment #3)
> IProcessDMContext getProcessForDebugContext(IDMContext dmc);
> IThreadDMContext getThreadForDebugContext(IDMContext dmc);
> (the reverse mappings were obtainable through DM data)
> 
> This interface basically allowed the two context hierarchies to be independent,
> but it also complicated the UI quite a bit.  
> We could go back to this and still
> use a single hierarchy, but we would have the option of changing that in the
> future if needed.

I wonder if this risk of complexity is worth making the change...  Let's hash it out at the meeting.

Another idea would be to take your solution of a single hierarchy of comment #1
"the hierarchies for processes being debugged and not being debugged should be the same", but to ignore the threadGroupId in the equals() method.  Two context would be equal if the pid was the same; so if a process did not have the debugger attached it would have groupId of null, while if it was attached, it would have a real groupId, but the equal() would ignore that.

I wonder if that is a solution or a hack? :-)


Comment 5 Pawel Piech CLA 2008-09-30 14:26:32 EDT
In the past I've been using "model" to refer to data model related bugs.
Comment 6 Marc Khouzam CLA 2008-10-02 14:14:29 EDT
After our weekly meeting we decided that since the current solution works, and the new proposal is not as clean-cut as we had hoped, we would leave things as is.