Community
Participate
Working Groups
I am opening this bug more to start a discussion about the following idea. In the IRunControl interface IExecutionDMContext is meant to represent any unit of execution; such things as Processors, Cores, Processes and Threads. Further, the method getExecutionContexts(IContainerDMContext) can be used to find the 'children' of any level of the execution hierarchy. I think we could use this to make the View Model more generic by having a single VMNode represent any of the different execution units. Currenlty, we have ContainerVMNode and ThreadVMNode. I was thinking that those two could be merged into a single ExecutionVMNode that would be a child of itself, recursively, and use getExecutionContexts(IContainerDMContext) to fill each level of the view model. The advantage I see is that the view model would theoretically automatically support any new level of execution; only the IRunControl service would need to be adapted. A necessary change for this would be to delegate to the service the responsibility of building the Label of the node. Which, I believe is good thing in itself. Deltas are still a mystery for me, so I may be missing some other limitation.
(In reply to comment #0) +1, but this is definitely not a trivial task > A necessary change for this would be to delegate to the service the > responsibility of building the Label of the node.... Yes the building of the labels would need to be delegated, but the service is not the right place for it, since the services are in non-UI plugin and do not have access to things like fonts, colors, or images. Instead separate label providers could be instantiated by the VM node itself. > Deltas are still a mystery for me, so I may be missing some other limitation. Delta handling would be tricky, but it actually may end up being simpler than the current logic in AbstractContainerVMNode and AbstractThreadVMNode, because currently both of them need to check whether the element in the event is a container or not.
I'd like to start looking at this bug and experiment with delivering features described in: http://wiki.eclipse.org/CDT/MultiCoreDebugWorkingGroup I need some time to get familiar with the code but as an first experiment I'd like to have something like: - the user will be able to create custom groups at any level in the - "launch - group - group - process - group - thread - group". - the user will be able to step/run/halt these groups. - some groups can be part of the model, some not. I'd like to do it in a way so the feature is used by any DFS debugger - (GDB,TI's). - as part of this I want to present the process and thread in DSF an just being one an instance of a group. Any pointers are welcome.
(In reply to comment #2) > I'd like to start looking at this bug and experiment with delivering features > described in: http://wiki.eclipse.org/CDT/MultiCoreDebugWorkingGroup > > I need some time to get familiar with the code but as an first experiment I'd > like to have something like: > - the user will be able to create custom groups at any level in the - "launch - > group - group - process - group - thread - group". So a group can contain another group? > - the user will be able to step/run/halt these groups. That will be backend-specific, so should be done in the service I believe. GDB currently does not support the concepts of groups, so we'll need to loop over the content of the group. Later on, if GDB ever supports this concept, we could use whatever command will be provided by GDB. This makes me think that we somehow have to get the service involved in creating the group so that the backends that support grouping can be sent the proper command. > - some groups can be part of the model, some not. I'd like to do it in a way so > the feature is used by any DFS debugger - (GDB,TI's). > - as part of this I want to present the process and thread in DSF an just > being one an instance of a group. So you want to use IContainerDMC to represent a group or do we need a new type of DMC? Let me mention up front that my experience with the DFS view model is quite limited; I've been much more involved in the data model (backend part). Pawel is the fountain of knowledge here :-)
What I want to do is somehow merge the concept of IExecutionDMContext and IContainerDMContext. Let’s just say that: 1.everything is a IContainerDMContext 2.A IContainerDMContext can have zero or more children of IContainerDMContext 3.A IContainerDMContext can have stack that is available or not. If we do this we’ll be able to put any hierarchy in the debug view – and from the DFS view model we need to define the following in the AbstractLaunchVMProvider. Launch has Container VM child nodes . Container has Container and Stack Frame child nodes. Then everything in the debug view is either a launch node, container node or a stack frame. This will allow as to insert container nodes at any level in the debug view. I believe that is what exactly this bugzilla entry was initially opened for. Do you think I am thinking in the right direction or I am missing some pieces of the puzzle? >> So a group can contain another group? Yes, in the general case. the serivce layer will be involved and I belive we can have default implementation that doesn't involved the back end. I don't want to define the requirement yet. I just want to make sure that we can have generic hiearchy in the Debug view for now. > That will be backend-specific, so should be done in the service I believe. GDB > currently does not support the concepts of groups, so we'll need to loop over > the content of the group. Later on, if GDB ever supports this concept, we > could use whatever command will be provided by GDB. This makes me think that > we somehow have to get the service involved in creating the group so that the > backends that support grouping can be sent the proper command. That's my thinking too. > So you want to use IContainerDMC to represent a group or do we need a new type > of DMC? Couuld be, I don't know. My goal is to somehow generalize it. I perosonally envision everyting is a container, just in some cases it has zero children. > Let me mention up front that my experience with the DFS view model is quite > limited; I've been much more involved in the data model (backend part). Pawel > is the fountain of knowledge here :-) I wish I have most of the DSF and GDB-DSF code already downloded in my head too. I'm getting there :)
(In reply to comment #4) > What I want to do is somehow merge the concept of IExecutionDMContext and > IContainerDMContext. > Let’s just say that: > 1.everything is a IContainerDMContext > 2.A IContainerDMContext can have zero or more children of IContainerDMContext > 3.A IContainerDMContext can have stack that is available or not. > > If we do this we’ll be able to put any hierarchy in the debug view – and from > the DFS view model we need to define the following in the > AbstractLaunchVMProvider. > > Launch has Container VM child nodes . > Container has Container and Stack Frame child nodes. > > Then everything in the debug view is either a launch node, container node or a > stack frame. > This will allow as to insert container nodes at any level in the debug view. > > I believe that is what exactly this bugzilla entry was initially opened for. Yes, that sounds very nice. I don't know about the complexity of it, but I think it is the right way forward. However, since DSF and DSF-GDB currently sometimes have different code when we deal with a IContainerDMC and when we deal with an IExecutionDMC that is not an IContainerDMC (dealing with a process vs dealing with a thread), we'll have to figure out what to do in those cases. In the services, I'm sure we'll need additional marker interface to be able to know if an IContainerDMC is a group of threads, of cores or of processes or... I'm not sure if we'll need this information in the VModel, but I'm afraid we will. Do you think you can make the VModel generic and push all the differences down to the services? That was my original goal, but I'm not sure it is possible.
> Do you think you can make the VModel generic and push all the differences down > to the services? That was my original goal, but I'm not sure it is possible. I don't know yet. Whatever the apporach is, it should be backward compatible with existing DSF debuggers. I keep digging...
Created attachment 183048 [details] GDB user groups demo I simple modification of some of the GDB classes to show recursive containers so we can explore what are the problems with having recursive containers in DSF.
(In reply to comment #7) > Created an attachment (id=183048) [details] > GDB user groups demo > > I simple modification of some of the GDB classes to show recursive containers > so we can explore what are the problems with having recursive containers in > DSF. I finally had a chance to try this out. It was cool to see the recursion. I now think it really makes sense to merge the ContainerVMNode and the ThreadVMNode. I think it is wasteful that for every container in the DV, both the ContainerVMNode and the ThreadVMNode get called to figure out the children. It is still the services that figure this out, but we have to juggle the two different calls and have to figure out if it is the threadVMNode that made the call or the ContainerVMNode. That being said, we can leave that for later, once we got the deltas working. I assume that the problem you are referring too is the fact that the launch does not expand to select the stack frame? And that on a stopped event, the stack frame does not get selected? Are there any other issues you see?
Created attachment 183500 [details] Something that helps with deltas I don't know much about deltas, but I looked a bit into the problem of things not getting selected properly. I found that the DefaultVMModelProxyStrategy has some protection against recursive VM nodes. The attached patch should be used on top of Dobrin's patch. It comments out the checks that I found, and with that, the stack frame gets selected at launch time, while using the recursive containers. This patch is just to help us progress. I have no idea of its side-effects. I noticed that it does not fix everything either: the stack does not get re-selected on a *stopped event if it wasn't already selected. I'll try to look into that.
Thanks Mark, >>>>I assume that the problem you are referring too is the fact that the launch does not expand to select the stack frame? And that on a stopped event, the stack frame does not get selected? You are correct, I was expecting when I select a process node, suspending the target to select the top stack frame. >>> Are there any other issues you see? I think there’re were others when I was paying with it a year ago. For some analysis and discussions you can look at the other bug: https://bugs.eclipse.org/bugs/show_bug.cgi?id=306868 Both bugs are referenced on the multicore group site: http://wiki.eclipse.org/CDT/MultiCoreDebugWorkingGroup
Mark, I noticed that the small change you did in the patch: https://bugs.eclipse.org/bugs/attachment.cgi?id=183500&action=diff somehow works 60% of the time :)
(In reply to comment #11) > Mark, > I noticed that the small change you did in the patch: > https://bugs.eclipse.org/bugs/attachment.cgi?id=183500&action=diff > somehow works 60% of the time :) That is better than nothing :-) I've spend some time today on this again. I believe the problem is that the events that trigger the delta generation are not created with the proper dmcontext hierarchy. With your test patch, when we populate the DV, the context hierarchy is gdb->process->group1->child1->thread->stack, but when we get the GDB *stopped event and we issue a ContainerSuspended event, we build the event with context gdb->process->thread->stack. I think this is (one of?) the problems, because things don't match. I hacked the code to properly generate the dmcontext for the different events but then I hit an infinite recursive loop. Most probably because of the small change I did in the patch you mention above. I haven't been able to figure out where the problem was yet.
(In reply to comment #12) I'm sorry I've been silent on this bug, because I would really like to help, but I just don't have the time right now. The way I intended to solve this use case has been to have the container vm node generate all the levels of the delta that are needed in the single call to IVMNode.buildDelta(). So for example, if the desired hierarchy to be shown in the view is: launch->process->group1->child1->thread->stack frame. Then the delta would be created as follows: LaunchVMNode create the first level of the delta: "launch" ContainerVMNode creates the delta nodes for "process->group1->child1" ThreadVMNode creates "thread" StackFrameVMNode creates "stack frame" So the trick would be to have the ContainerVMNode.buildDelta() loop through the all the levels of IContainerDMContext in the hierarchy and create a delta for each level. When finished, thread VM node can build on top of it.
(In reply to comment #13) > The way I intended to solve this use case has been to have the container vm > node generate all the levels of the delta that are needed in the single call to > IVMNode.buildDelta(). Thanks! That makes sense. This would probably allow us to not use my patch "Something that helps with deltas", which would probably also remove the infinite recursion I'm seeing. Dobrin, what do you think?
Created attachment 184130 [details] Full patch which hacks the proper context hierarchy This is where I got last night. This patch is Dobrin's patch + "Something that helps with deltas" + some changes in MIProcesses.java and MIRunControlEventProcessor.java to hard-code the test context hierarchy (process->group->child). This is just to try to prove that we can get things to work. When I launch GDB 6.8, the stack frame is properly selected. Then I select the thread node, and press step: the stack frame is not properly selected and we get into an infinite recursion. With Pawel's suggestion of comment 13, I'm hoping we can get rid of the infinite recursion and see if the delta's work.
Created attachment 184314 [details] Split the Full patch in three so it can be controlled by Testing I split the Full patch so different aspects of it can be controlled by different Testing functions. We can add new experiments and introduce new function so can we easily try different things. If we set Testing { true, false, false} this is my original's patch. It doesn't expand when suspended event is received. If we set Testing( true, true, false} It doesn't expand but the source editor loses the highlighted line after stepping. If we set Testing{ true, true, true} It sometimes expands. Terminate doesn’t work most of the times. I have lots of messages RequestMonitor.isCanceled() sometimes.
Pawel, I am trying to follow you advice of building the complete deltas: >>> LaunchVMNode create the first level of the delta: "launch" >>> ContainerVMNode creates the delta nodes for "process->group1->child1" >>> ThreadVMNode creates "thread" >>> StackFrameVMNode creates "stack frame" But I believe that multiple changes are also needed inside the class DefaultVMModelProxyStrategy to handle recursive IVMNode-s. In some places single loop over the children of a IVMNode is specified instead of recursive gradually filling up the deltas. For example getChildNodesElementOffsets returns single array with offsets- I have felling that this should be more like a tree of offsets. Is my findings on the right track or I should try to override some of the DefaultVMModelProxyStrategy at a higher level. I'll continue debugging, but I'd like to know if I should focus of making DefaultVMModelProxyStrategy working with recursive VMNodes or I should be looking of an implementation if which some methods of DefaultVMModelProxyStrategy are not called. Thanks Dobrin
(In reply to comment #17) > But I believe that multiple changes are also needed inside the class > DefaultVMModelProxyStrategy to handle recursive IVMNode-s. > In some places single loop over the children of a IVMNode is specified instead > of recursive gradually filling up the deltas. Actually, I'm not sure if this is the right approach. When I was writing this class, I couldn't find a way to make the model proxy handle the recursive node hierarchies. However, that doesn't mean that it can't be done. You may need some changes in the IVMNode interface to do it though. > For example getChildNodesElementOffsets returns single array with offsets- I > have felling that this should be more like a tree of offsets. Yes, if you want to handle recursion in the proxy strategy then you will probably need to make extensive changes to it. Finally, the reason that the proxy strategy is a separate class was to encapsulate the complexity of it, but also to make it interchangeable. So free free to modify it as needed and don't worry about backward compatibility. You can also implement it from scratch and do some other simplifying assumptions: for example you could assume that each VM node has only one child. On 12/07/2010 08:14 AM, Alexiev, Dobrin wrote: > > >>> LaunchVMNode create the first level of the delta: "launch" > >>> ContainerVMNode creates the delta nodes for "process->group1->child1" > >>> ThreadVMNode creates "thread" > >>> StackFrameVMNode creates "stack frame" > > Spending lots of time debugging the code in DefaultVMModelProxyStrategy I > start to think that there is another approach to this. > > What if instead of firing the whole delta inside > ContainerVMNode.buildDelta(...) the parameter parentDelta is used to figure > out the recursive level the delta has to be returned, added. > > When the parent delta has only { launch context } top level will be returned, > when the parent delta is { launch , process } the first level group will be > added, etc. > > If we do this we can remove some of the checking inside > DefaultVMModelProxyStrategy that prevent getting inside the recursive > containers. To exist the recursion the delta will be examined to figure out > if it should keep going. > > Do you think it make sense to investigate this option too, or I am missing > something important? Honestly, it's been too long since I've worked on this component to be able to make a sensible suggestion without spending a lot of time to think about it. > I am afraid that some methods inside DefaultVMModelProxyStrategy do looping > and expect top level contexts returned. > > It look cumbersome to make that code do looping as well as recursion. I agree, it's not an ideal approach because it pushes a lot of the complexity of this problem into the VM Node implementation. > Also what will ContainerVMNode.getContextsForEvent(..) return? All contexts > at the first level or all containers at all levels? This method is also not designed for recursion. You'd probably have to enhance it so that it's aware of what level in a recursive hierarchy it is.
Created attachment 185467 [details] support for recursive VMNodes I think now I have a handle on firing the recursive delta. The trick is for recursive containers and their children to gradually build the deltas - level by level. As well as provide the right context for the event based in the specified delta. On the DefaultVMModelProxyStrategy side now I allow recursion but only if a delta node was added. Right now this patch works with my Windows Cygwin with GDB 6.8 and select the threads at the lowest level. Monday I’ll setup my Linux VM and try to use the newer GDB - 7.2. I also restructured the classes so we can start discussing the user group interfaces. Can you guys review and let me know your comments…
(In reply to comment #19) > Created an attachment (id=185467) [details] > support for recursive VMNodes I had a quick try and I'm getting an exception: !ENTRY org.eclipse.cdt.dsf 4 -1 Uncaught exception in session executor thread: java.lang.AssertionError at org.eclipse.cdt.dsf.mi.service.MIExecutionContextTranslator.getChildContainers(MIExecutionContextTranslator.java:231) at org.eclipse.cdt.dsf.gdb.internal.ui.viewmodel.launch.ContainerVMNode.updateElementsInSessionThread(ContainerVMNode.java:131) at org.eclipse.cdt.dsf.ui.viewmodel.datamodel.AbstractDMVMNode$3.run(AbstractDMVMNode.java:230) at org.eclipse.cdt.dsf.concurrent.DefaultDsfExecutor$TracingWrapperRunnable.run(DefaultDsfExecutor.java:371) at java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:441) at java.util.concurrent.FutureTask$Sync.innerRun(FutureTask.java:303) at java.util.concurrent.FutureTask.run(FutureTask.java:138) at java.util.concurrent.ScheduledThreadPoolExecutor$ScheduledFutureTask.access$301(ScheduledThreadPoolExecutor.java:98) at java.util.concurrent.ScheduledThreadPoolExecutor$ScheduledFutureTask.run(ScheduledThreadPoolExecutor.java:206) at java.util.concurrent.ThreadPoolExecutor$Worker.runTask(ThreadPoolExecutor.java:886) at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:908) at java.lang.Thread.run(Thread.java:662)
(In reply to comment #19) > Monday I値l setup my Linux VM and try to use the newer GDB - 7.2. For things to work with GDB 7.2, you will probably need to modify the services that are used for that version. Things like GDBRunControl_7_0 and maybe the IProcesses service.
I see problems with GDB 7_2. I'm looking at it now.
(In reply to comment #22) > I see problems with GDB 7_2. > I'm looking at it now. BTW, I suggest we make the first set of changes for GDB 7.2. We can then add them to older classes if we want/have time. Starting with an older GDB will only require us to add them to the newer code later.
Will do from now on. How can I have the latest GDB on Windows? I'm using Cygwin and the latest come with GDB 6.8. I have to use my VM for the latest GDB.
(In reply to comment #24) > Will do from now on. > How can I have the latest GDB on Windows? > I'm using Cygwin and the latest come with GDB 6.8. > I have to use my VM for the latest GDB. I guess you have to find a site from where you can download it, already built. I found that MinGW was easier to use. I believe this site has it: http://sourceforge.net/projects/mingw/files/MinGW/BaseSystem/GDB/
I commented out the two assert from MIExecutionContextTranslator and everything started to work! Nice job Dobrin! The deltas seem to work and I see the proper selection when I step, with a whole bunch of (fake) groups. Cool! I'll look in more detail a the patch now.
Created attachment 185599 [details] handles GDB 7.2 Thanks Mark for the MinGW link. I think this patch now work for GDB 7.2 as well as older. I just had to make the same changes on the classses you menetioned - GDBProcesses_7_0 and GDBRunControl_7_0. I remove the assertion as well. (I didn't have -enableassertions in my VM args:) BTW, I wanted to try a non stop mode in MinGW but it doesn't work. Can I test the non stop GDB mode in Windows?
BTW, I am currently expanding the patch to support GDB non stop mode.
(In reply to comment #27) > BTW, I wanted to try a non stop mode in MinGW but it doesn't work. > Can I test the non stop GDB mode in Windows? I don't actually remember if it is supported on Windows. You can start MinGw on the command-line and give the two commands: set target-async on set non-stop on show non-stop and see what happens... I'm off until the new year, so I'll look more into the details of this patch when I return. Have a great holiday!
Created attachment 185737 [details] Handles single threaded program in all GDBs - 6.8, 7.2 and 7.2 non stop I modified the patch to handle single threaded C programs: I tested four different paths: Windows (Cygwin – GDB 6.8 and MinGW – GDB 7.2 (stop mode) Linux ( Unbuntu – GDB 7.2 (both stop mode and non-stop mode) Looks like the approach I’ve taken to build the delta level by level works so far. To get everything working with the prototype I think I need to make sure proper delta are created for all events handled by the ContainerVMNode and ThreadVMNode. * ISuspendedDMEvent, IResumedDMEvent, * IContainerSuspendedDMEvent, IContainerResumedDMEvent, IStartedDMEvent ExpandStackEvent * FullStackRefreshEvent * SteppingTimedOutEvent ContainerExitedDMEvent Now I want to make sure multithread debugging also works.
Thanks for the latest patch! It looks to be working very well with GDB 7.2 and single-threaded programs. Since this is a prototype, I'll try to focus on the design ideas and not the details of the patch. - Please turn on API tooling: http://wiki.eclipse.org/CDT/policy#Using_API_Tooling - I like the Translator service. We need more javadoc to explain it better. I assume it is that service that will keep track of all the user-defined groups, right? Other services and VMNodes will go through that service to expand their context in whichever way the Translator says it should. - To keep things simple and get the in faster, I suggest you don't change DSF to add IExecutionContextTranslator just yet. Instead, let's just use IMIExecutionContextTranslator, which is all we need for now. This concept can be pushed down to DSF later if it is desired. You can immitate what I did for IGDBTraceControl, which is only in DSF-GDB. Of course if you really want to have this in DSF, you are welcome to keep it, but you'll need Pawel's separate approval before getting it in. IMIExecutionContextTranslator#isTopLevelContainer, what does top-level container mean? A user of the interface will need to clearly understand what top-level vs other containers are. IMIExecutionContextTranslator#createContainerPath, what is a container path? I assume it is a the container context that directly contains the threadDmc, and that has as parents, all other containers up until and including the top-level container. IMIExecutionContextTranslator#createContainerPath, takes IThreadDMContext as a parameter. However, a normal container, in DSF-GDB is not the parent of an IThreadDMContext. Maybe this method should take an IMIExecutionDMContext as a parameter? You can see the context hierarchy at the very top of the MIProcesses class. IMIExecutionContextTranslator#hasMultiLevelContainers, the javadoc is not clear to me. " Returns true if it contains child containers", what is 'it'? What does this method do? I'll keep looking at things, but I'm not sure how you want to move forward? I guess we need commands to group things if we want to be able to test a full version of this patch. Is this the next step?
Mark, I am close to submitting the next version of the patch ( I hope I’ll have it by the end of today): It should handle: - both stop mode and non-stop GDBs - single threaded and multithreaded I’ll list all test cases I believe are relevant in the changes in this patch. Please add to that list anything else you think is relevant to the change. My biggest concerns are: Make sure the common code handles properly both recursive containers and all legacy debuggers. Since there are changes in DSF I want to make sure these change will not cause any problem with already working debuggers. Once I submit the next patch I start testing with another DSF debugger – I think EDC is the only one available. So I think the next step is for both of us to keep testing all changes in the patch to make sure we don’t introduce any problem in already existing code. Once we are confident we haven’t introduced any bugs in already working code we go full ahead with adding the features. When we have MIExecutionContextTranslator.layoutId = NO_GROUPS we should have 100% backward behavior of the GDB debugger. When we have MIExecutionContextTranslator.layoutId = TWO_LEVELS_SECOND_INDEX we will be testing the recursive container changes. As soon as I submit the patch I’ll address your comments as well. Thanks Dobrin
(In reply to comment #32) > I am close to submitting the next version of the patch ( I hope I’ll have it by > the end of today): Awesome! > My biggest concerns are: > Make sure the common code handles properly both recursive containers and all > legacy debuggers. Yes, that is very important. > Since there are changes in DSF I want to make sure these change will not cause > any problem with already working debuggers. The only change to DSF that could have an impact, I believe, is the change to DefaultVMModelProxyStrategy.java, but with your check to allowRecursiveVMNodes(), I don't see how you could break anything in DSF. So, I'm not worried about that. > So I think the next step is for both of us to keep testing all changes in the > patch to make sure we don’t introduce any problem in already existing code. > > Once we are confident we haven’t introduced any bugs in already working code we > go full ahead with adding the features. > > When we have MIExecutionContextTranslator.layoutId = NO_GROUPS we should have > 100% backward behavior of the GDB debugger. > When we have MIExecutionContextTranslator.layoutId = TWO_LEVELS_SECOND_INDEX we > will be testing the recursive container changes. That sounds like a good plan. Let's focus on those two cases. > As soon as I submit the patch I’ll address your comments as well. Thanks
Created attachment 187345 [details] Now the patch handles multithread program Here is a new version of the patch. Here are the user case I have in mind: I tested four different paths: Windows (Cygwin – GDB 6.8 and MinGW – GDB 7.2 (stop mode only) Linux ( Unbuntu – GDB 7.2 (both stop mode and non-stop mode) I created multithread program and perform the following tests: UC1 (all GDBs): When the debugger start the top stack frame on one of the main should be selected. UC2: ( all GDBs) Have program that has sleep(5). Select the top stack frame and step over the line that has "sleep(5)". The stack frames should disappear. Select the launch node. After few seconds when the thread is suspends it should select the top stack frame. UC3 (all GDBs) Make sure terminate always works no matter which thread is halted or not. UC4 (all GDBs) Create program that creates and destroys threads. Step over the main thread over lines that create and destroy thread. Make sure that the correct threads are always shown in the right node. For non-stop mode – the new threads will be running For stop mode – the new threads will be suspended. UC5 (all GDBs) Halting a stack that has more than 10 frames will show as last stack <more frames…> Double click on <more frames..> will show more frames. I still need to rework ThreadVMNode.
(In reply to comment #31) > - Please turn on API tooling: > http://wiki.eclipse.org/CDT/policy#Using_API_Tooling Thanks. I have the API tooling on now. > - I like the Translator service. We need more javadoc to explain it better. I don’t know yet. It is an empty interface at this moment. I just started integrating the patch into our debugger. Having GDB and our debugger will allow me to define the common behavior that can be shared between debuggers. > - To keep things simple and get the in faster, I suggest you don't change DSF > to add IExecutionContextTranslator just yet. I introduced this interface at the DSF level because our debugger is not a GDB debugger. I would like to have the shared code outside the GDB plugin so the code and the use cases can be shared between these two debuggers and any other DSF debugger that that finds these features useful. > documenting IMIExecutionContextTranslator#... I’ll put more documentation once I integrate our debugger to show multilevel hierarchy. Only then I’ll have a clear idea if the code I already wrote is a shared code between debuggers or GDB specific code. > I'll keep looking at things, but I'm not sure how you want to move forward? I > guess we need commands to group things if we want to be able to test a full > version of this patch. Is this the next step? Next two steps for me: 1. Get our debugger to use the patch to show recursive containers. That will give me much better idea what is common between DSF debuggers and what is really a GDB (demo) specific code. 2. Discuss the use case of grouping, hiding and container types. Once we agree on use cases I can start extracting the code that is in our debugging and pushing it in DSF. Anyone that can help: Please, do more testing with the latest patch to see if I have missed something.
Created attachment 187477 [details] Cleaned ThreadVMNode. Applied API tooling.
A basic question: I assume that if we want to share code between GDB and our DSF debugger the proper layer is DSF. Is this a right assumption, or I should be looking for different approaches. I don’t like copy and paste much.
(In reply to comment #37) > A basic question: > > I assume that if we want to share code between GDB and our DSF debugger the > proper layer is DSF. Is the code you are talking about for DSF-based debuggers only? If yes, then it will depend on what your debugger uses. If your debugger uses only DSF and nothing from DSF-GDB, then DSF could be the right place. However, some features don't fit in a framework, in which case, we would either need to duplicate the code for each debugger, or maybe have a new plugin, or have it in DSF-GDB and have your debugger use it from there. If you are thinking of menus like "group" and "hide", then I would put that directly in CDT plugins, since it is not DSF-specific. You can look at the reverse debugging buttons for an example. But maybe I didn't understand what exactly you were asking?
I should have been more specific. Our debugger is DSF based but does not depend on GDB. Our view model adapters derive from AbstractThreadVMNode and AbstractContainerVMNode the same way the GDB’s view model adaptor derive. To get the support for recursive container I believe I have to do similar changes to the one I made to ContainerVMNode and ThreadVMNode. My question is: which option works best: 1.Do I put the common functionality inside the base classes: AbstractThreadVMNode and AbstractContainerVMNode? 2.Do I create a class that derive from AbstractThreadVMNode and AbstractContainerVMNode and have both GDB and our debugger derive from them? 3.Do I just copy and paste the code ? As you suggested I’ll add grouping and hiding functionality in the same way reverse run control is added so it can be added to CDI when needed. But I see that DSF command handler GdbReverseResumeCommand is using a GDB specific service. In my case I’d like the groping command handler to use a DFS service that is outside of GDB since our debugger will also implement the same service. Do I define the grouping and hiding DSF services inside the DSF plugin as well as the groping command handler that will use these services? Earlier you suggested not to add IExecutionContextTranslator just yet. If I don’t add it now that means for our debugger I have to define different interfaces, different handlers, etc. Should I go that way or I should see what is the common between the two and start adding the common code to DSF. Please advices.
Ironically this discussion is under the bug entry “A single VMNode should represent all execution levels” While doing the recursive containers I already see the need for Thread and Container VM nodes to share code. Can I make AbstractContainerVMNode and AbstractThreadVMNode derive from common base class to share code?
(In reply to comment #39) > I should have been more specific. > > Our debugger is DSF based but does not depend on GDB. I hadn't realized you were making this change in your debugger at the same time as for DSF-GDB. But it makes total sense that you do that, and it helps confirm that the changes are good. > Our view model adapters derive from AbstractThreadVMNode and > AbstractContainerVMNode the same way the GDB’s view model adaptor derive. > To get the support for recursive container I believe I have to do similar > changes to the one I made to ContainerVMNode and ThreadVMNode. > My question is: which option works best: > 1.Do I put the common functionality inside the base classes: > AbstractThreadVMNode and AbstractContainerVMNode? This would normally be the right place, but because Pawel has to be very careful in what goes in DSF (there are of course more extenders of DSF and of DSF-GDB), then this change may not be as easy to commit to DSF quickly. > 2.Do I create a class that derive from AbstractThreadVMNode and > AbstractContainerVMNode and have both GDB and our debugger derive from them? I like this option best. It will keep your changes well contained so that we can easily push them down to DSF if Pawel wants them. In the mean time, they can be in DSF-GDB, and your debugger can inherit from those new classes. You will have a dependency to the DSF-GDB plugins though. But that should be ok, no? > 3.Do I just copy and paste the code ? Let's avoid copy/paste :-) > > As you suggested I’ll add grouping and hiding functionality in the same way > reverse run control is added so it can be added to CDI when needed. > > But I see that DSF command handler GdbReverseResumeCommand is using a GDB > specific service. So, the generic stuff like IReverseResumeHandler is part of CDT. But the actual implementation which has GDB-specific and DSF-specific details, is part of DSF-GDB (GdbReverseResumeCommand). > In my case I’d like the groping command handler to use a DFS service that is > outside of GDB since our debugger will also implement the same service. > Do I define the grouping and hiding DSF services inside the DSF plugin as well > as the groping command handler that will use these services? Again, this may be more difficult to push in to DSF right away. You can ask Pawel directly if such a contribution can be added to DSF in the short term or not. But you can implement the service in that exact same way, but simply store the class in DSF-GDB (even if it has not GDB-specific code). We can then move the class to DSF if approved. Your debugger will simply extend the DSF-GDB class, and DSF-GDB itself will also extend that class for the GDB-specific parts. > Earlier you suggested not to add IExecutionContextTranslator just yet. > If I don’t add it now that means for our debugger I have to define different > interfaces, different handlers, etc. > Should I go that way or I should see what is the common between the two and > start adding the common code to DSF. Same issue :-) You can then create IExecutionContextTranslator in DSF-GDB for the moment. So, the way I see it is: 1- Pawel says that it is ok to go straight to DSF with these changes or 2- You code it exactly like you would have for DSF, but simply store the new classes in an internal package of DSF-GDB for now. And we'll move those classes to DSF whenever possible. (In reply to comment #40) > Ironically this discussion is under the bug entry “A single VMNode should > represent all execution levels” > > While doing the recursive containers I already see the need for Thread and > Container VM nodes to share code. > Can I make AbstractContainerVMNode and AbstractThreadVMNode derive from common > base class to share code? Will this be new code or simply a refactoring of the existing AbstractContainerVMNode and AbstractThreadVMNode? If it is a refactoring of existing code, then I think that should be able to get into DSF. If it is new code, then it may depend on the complexity... I'm not sure.
I'm sorry I haven't had time to look at the patches yet. From following the discussion though I suggest the following: - Place the common recursive classes in dsf.ui plugin. - Either: - If there are no backward compatibility issues, then simply extend existing abstract vm nodes. - If there are potential compatibility issues, then fork the abstract nodes while factoring common parts (label and property provider code) into a separate shared modules.
Created attachment 188715 [details] moved common code from GDB to DSF The commond code between any DSF debugger that requires grouping is moved from GDB to DSF.
Dobrin, the commit for thePin&Clone icon changed some of the same files you change in your patch, so the patch does not apply cleanly. When you have time, can you update it to HEAD? Thanks
Hi Dobrin, I was able to spend some time looking at the patch. The behavior looks great. Since the patch is mostly a test patch that aims at making sure we generate the proper deltas, I focused only on the delta generation. I know very little about deltas so I couldn't figure out the full approach of the patch. What is the patch trying to do, with respect to deltas? When you have a recursive node (which is only Container, right?), we need to build the deltas, one level at a time. But are we assuming two types of levels only: container-group and container-process? Or is the container meant to be generic, where it could be a group, a process, a core, or anything as defined by the services? Do we need to modify getDeltaFlags() also? Maybe you can use the multi-core work group conference call to explain the details of the approach? Some comments on the patch: I don't understand why we need changes to the ThreadVM nodes part of the code. That node is not recursive right? So, don't we only need to change the Container code? DMContexts - How about using getAllAncestrosOfType() to define getTopMostAncestorOfType()? Like this: public static <V extends IDMContext> V getTopMostAncestorOfType(IDMContext ctx, Class<V> ancestorType) { V[] v = getAllAncestorsOfType(ctx, ancestorType); if (v == null) return null; return v[v.length-1]; } This is not totally identical because it allows for one layer not to have the ancestor, but I think that is ok. DefaultVMModelProxyStrategy - can you add javadoc for the new methods? This is particularly important in DSF as it is a framework - in callChildNodesToBuildDelta, in the new recursive case, why can we always use 0 as the nodeOffset in the call to childNode.buildDelta()? - I think the line if( isDeltaElementOfType(delta, node)) should use 'childNode' instead of 'node'. It is more understandable but also, although they are equals(), they may not be identical, as the equals() method may ignore some differences. - I don't understand why you must check if( isDeltaElementOfType(..) What is the point of this call? - I'm not sure about the nodeOffset parameter, but can we replace the entire block of if( isDeltaElementOfType(..)) with: if( !isDeltaElementOfType(..)) continue That way, we re-use the existing code more. AbstractExecutionContextVMNode - How about having addEventType( Class<? extends IDMEvent<?>> eventClass, boolean containerEvent) to avoid checking if IDMEvent.isAssignableFrom()?
Created attachment 189534 [details] reapply the same patch toward CDT head
(In reply to comment #44) > Dobrin, > the commit for thePin&Clone icon changed some of the same files you change in > your patch, so the patch does not apply cleanly. When you have time, can you > update it to HEAD? > Thanks Updated.
(In reply to comment #45) > I know very little about deltas so I couldn't figure out the full approach of > the patch. What is the patch trying to do, with respect to deltas? When you > have a recursive node (which is only Container, right?), we need to build the > deltas, one level at a time. But are we assuming two types of levels only: > container-group and container-process? Or is the container meant to be > generic, where it could be a group, a process, a core, or anything as defined > by the services? It is generic. I want by default the user to be able to add groups within groups. In our debugger we also subclass AbstractContainerVMNode and we expect the Debug View to show containers within containers. > Do we need to modify getDeltaFlags() also? In theory yes. But so far there is no need to. This method is "the worst case scenario" so far it return the flags conservatively enough. Do you have a specific user case which will require its change? > Maybe you can use the multi-core work group conference call to explain the > details of the approach? > Some comments on the patch: > I don't understand why we need changes to the ThreadVM nodes part of the code. > That node is not recursive right? So, don't we only need to change the > Container code? The only change inside ThreadVM is overriding getContextsForEvent(). The reason is that the original methods always return the top most container. We are adjusting the delta getContextsForRecursiveVMNode to return the immediate parent container instead of the top most container. You can see in AbstractExecutionContextVMNode.setImmediateParentAsContexts() we will return the context that corresponds exactly to the level of container for the delta we are interested in. > DMContexts > - How about using getAllAncestrosOfType() to define getTopMostAncestorOfType()? > Like this: > public static <V extends IDMContext> V getTopMostAncestorOfType(IDMContext > ctx, Class<V> ancestorType) { > V[] v = getAllAncestorsOfType(ctx, ancestorType); > if (v == null) return null; > return v[v.length-1]; > } > This is not totally identical because it allows for one layer not to have the > ancestor, but I think that is ok. I don't mind in general, but won't it be a performance overhead? What if the launch has 20 processes and each has 10 threads. We will do extensive loop to get 200 elements and then just return the last one. In my implementation we will just do two iterations and return the top container. May be I am overly concern? > DefaultVMModelProxyStrategy > - can you add javadoc for the new methods? This is particularly important in > DSF as it is a framework Done. Let me know if that is clear. > - in callChildNodesToBuildDelta, in the new recursive case, why can we always > use 0 as the nodeOffset in the call to childNode.buildDelta()? We user current assumption that recursive container can be added as first children if the list of VMNodes. If we decide to make the patch more generic ( allow recursive node to be at different index) we need to remove this simplification. > - I think the line > if( isDeltaElementOfType(delta, node)) > should use 'childNode' instead of 'node'. It is more understandable but > also, although they are equals(), they may not be identical, as the equals() > method may ignore some differences. Good point. Changed. > - I don't understand why you must check if( isDeltaElementOfType(..) What is > the point of this call? Keep in mind that the container node can add both containers and threads to the delta. We don't want ask the Container.buildDelta() if the top of the delta already is a thread. We want to make sure Container.buildDelta() is called only if the container is at the top if the delta. > - I'm not sure about the nodeOffset parameter, but can we replace the entire > block of if( isDeltaElementOfType(..)) with: > if( !isDeltaElementOfType(..)) continue > That way, we re-use the existing code more. I personally prefer to keep the code split because as we expand the patch we might need to change parameter values. I guess if I see more duplication I'll change my mind :) I think at this time my preference is to emphasize the different paths instead of try to optimize the code in terms of lines code. It's you call. > AbstractExecutionContextVMNode > - How about having > addEventType( Class<? extends IDMEvent<?>> eventClass, boolean > containerEvent) > to avoid checking if IDMEvent.isAssignableFrom()? Done. Good point! I added a new patch with the modifications.
Created attachment 189650 [details] updated based on Mark's comments
Next patch for this bug will be part of the Grouping features: https://bugs.eclipse.org/bugs/show_bug.cgi?id=336876
Just a note that I'm working on the Grouping feature (bug 336876) and reviving's Dobrin's work. The goal is to get the grouping feature to work with GDB. As a first step, I will not try to to use a single VMNode for groups/containers/processes, as the work in this bug tries to do. Right now a ContainerVMNode is used to represent a process and is not generic. We can make it generic to represent any group, which includes processes, however, generating the label is not the same for groups as for processes, so we'd need to make that logic more generic. Since that is not a priority to get groups working, I have chosen not to do that at this time. Instead, I'm probably going to use the ContainerVMNode for processes as we do now, and create a new GroupVMNode to handle groups. The progress of my work will be in bug 336876 and in gerrit review: https://git.eclipse.org/r/#/c/46107/
