Community
Participate
Working Groups
Build Identifier: I am attaching a patch that implements new view that displays information about various OS object as reported by GDB's -info-os command. This command is not yet in GDB CVS, but is in progress and is not likely to change much, so it's best to submit Eclipse side of the story now and have it ready. Reproducible: Always
Created attachment 204798 [details] Patch that implements the OS awareness view.
There are two known issues with this patch: 1. There's no icon for the view. 2. The view attempts to populate the view menu with a list of columns, so that each column can be hidden if necessary -- which means that initially, the menu is empty, and it is then populated when the view is updated for specific resource kind. However, while this worked in Eclipse 3.6, in 3.7 the menu is never shown. I can handle that with an extra kluge below, but I feel like I am missing something here. --- a/dsf-gdb/org.eclipse.cdt.dsf.gdb.ui/src/org/eclipse/cdt/dsf/gdb/internal/ui/osview/OSView.java +++ b/dsf-gdb/org.eclipse.cdt.dsf.gdb.ui/src/org/eclipse/cdt/dsf/gdb/internal/ui/osview/OSView.java @@ -130,6 +130,11 @@ public class OSView extends ViewPart { */ IActionBars bars = getViewSite().getActionBars(); bars.getMenuManager().add(new Separator("dummy group")); //$NON-NLS-1$ + Action a = new Action("Dymmy action", IAction.AS_CHECK_BOX) { + + }; + bars.getMenuManager().add(a); + fResourceClassEditor = new ResourceClassContributionItem(); fResourceClassEditor.setListener(new ResourceClassContributionItem.Listener() {
The new -info-os command has now been posted to the GDB mailing list and is being reviewed for inclusion: http://sourceware.org/ml/gdb-patches/2011-10/msg00368.html and http://sourceware.org/ml/gdb-patches/2011-10/msg00400.html
At the Multicore Debug conference call today, it was suggested that the use of -info-os should be part of a DSF service. This would allow the information to be re-used for other features. This is something I also felt should be done, as it is the way DSF is used everywhere else. The Visualizer View effort proposes to introduce a new service IGDBHardware. It would probably be a good idea to use that new service to include the -info-os features. What I can do is commit the new IGDBHardware service, and then the new OS awareness feature can make use of it. I may not be able to do this until next month though.
BTW, the Visualizer feature is tracked by Bug 335027
To be honest, I am not entirely thrilled about the idea to rewrite anything based on not-yet-checked-in interfaces. I'd much rather we focus on having this done using what we have at present.
(In reply to comment #6) > To be honest, I am not entirely thrilled about the idea to rewrite anything > based on not-yet-checked-in interfaces. I'd much rather we focus on having this > done using what we have at present. I can understand you would prefer to wait until the new service is committed. Once that is available, the modification for your patch should not be much: simply replace using the controlService with using the new service when fetching the information from GDB. The interesting effort will simply be to define an interface that makes sense to make use of the -info-os commands.
(In reply to comment #7) > The interesting effort will simply be to define an interface that makes sense > to make use of the -info-os commands. Right, so that's why I am suggesting to have the current patch reviewed, and only when we get some experience using this functionality in practice, proceed to defining any services or interfaces.
(In reply to comment #8) > > The interesting effort will simply be to define an interface that makes sense > > to make use of the -info-os commands. > > Right, so that's why I am suggesting to have the current patch reviewed, and > only when we get some experience using this functionality in practice, proceed > to defining any services or interfaces. I don't mind going that route, it will allow the contribution to come in more easily. My hope will then be that we can migrate to the new service before the Juno release. I would prefer not to have one version of this feature for Juno, and a new one for the following release.
Let's address the details of the contribution IP right away. Now that Mentor bought CodeSourcery and that Mentor is an eclipse member, it should be ok for Mikhail to commit patches on behalf of Mentor employees. We just need to confirm that Mentor has signed a Member Committer Agreement (see bug 322658 comment 49). Mikhail, are you ok with that? It would save us from having to go through the IP Review. Can Volodya or Mikhail confirm that Mentor has signed a Member Committer Agreement, or should we ask the foundation?
(In reply to comment #10) > Mikhail, are you ok with that? It would save us from having to go through the > IP Review. > > Can Volodya or Mikhail confirm that Mentor has signed a Member Committer > Agreement, or should we ask the foundation? Marc, I am pretty sure that Mentor has signed the agreement. I didn't need to do any paperwork to confirm my committer rights.
(In reply to comment #11) > Marc, I am pretty sure that Mentor has signed the agreement. I didn't need to > do any paperwork to confirm my committer rights. Nice! So you will be ok to commit the patch once the review is finished?
Thanks Volodya for the patch. Below are the most important comments. There are other minor ones, but I will make those changes myself once your patch is committed, it will save us both some time. 1- Please put (Mentor Graphics) after your name for each copyright header. 2- Please put a copyright header at the top of files: messages.properties and Messages.java (see /org.eclipse.cdt.dsf.gdb/src/org/eclipse/cdt/dsf/mi/service/Messages.properties for the format which uses # instead of *) 3- Please rename messages.properties to Messages.properties, and change Messages.java to remove BUNDLE_NAME and use Messages.class.getName() in the call to NLS.initializeMessages (see /org.eclipse.cdt.dsf.gdb/src/org/eclipse/cdt/dsf/mi/service/Messages.java) SessionOSData.java 4- Move all access to fTracker.getService to inside the DSF executor. When you test your eclipse patches, I recomment you add -ea to your vmargs in your Arguments launch tab. 5- there needs to be some kind of dispose() method to dipose of the fTracker and to call session.removeServiceEventListener(). This would happen when the debug context is set to something that is not an IDMVMContext in OSView.java. You must also dipose of the DsfServicesTracker OSView.java 6- Refresh action icon is not shown. To use the refresh icon from the platform, I found this code that seems to work. Could you change: ImageDescriptor imageDescriptor =i GdbUIPlugin.imageDescriptorFromPlugin(GdbUIPlugin.PLUGIN_ID, "icons/full/elcl16/refresh_nav.gif"); //$NON-NLS-1$ fRefreshAction.setImageDescriptor(imageDescriptor); with try { Bundle bundle= Platform.getBundle("org.eclipse.ui"); //$NON-NLS-1$ URL url = bundle.getEntry("/"); //$NON-NLS-1$ url = new URL(url, "icons/full/elcl16/refresh_nav.gif"); //$NON-NLS-1$ ImageDescriptor candidate = ImageDescriptor.createFromURL(url); if (candidate != null && candidate.getImageData() != null) { fRefreshAction.setImageDescriptor(candidate); } } catch (Exception e) { } 7- In setDebugContext, you can move the if(newSession != null) block into the above block, and avoid having to suppress the warning for "null". You will need to have a if(newSession == null) outside the block to set fSessionData = null. 8- You need to cleanup fSessionDataCache when a session terminates, or else it will cause a memory leak. You can use a DsfSession.SessionEndedListener to find out when sessions end. 9- When the session ends, you should should call a SessionOSData.dispose() method to remove your event handler and dispose of the DsfServicesTracker. OSData.java 10- in the constructor, I don't believe it is safe to use columnHasOther[j] before it is initialized. I don't know that local arrays are all set to false in that case. I suggest you initialize that array to false before the loops. You can use Arrays.fill(columnHasOther, false) 11- I think the parsing of the result of the MIInfoOs command should be done in MIInfoOsInfo (the code in the constructor of OSData). OSData can then be initialized with the pre-parsed result. This will make re-use easier and will follow the patter of the other MI commands. General testing --------------- For my testing, I took GDB HEAD and applied the patches from: http://sourceware.org/ml/gdb-patches/2011-11/msg00631.html http://sourceware.org/ml/gdb-patches/2011-11/msg00836.html 12- I think having a column waaaaaay on the right hand-side will cause the user to not notice that column. This happens for Processes and Process Groups and Kernel modules. Can we re-order the colums to have the big column the last one on the right? Or can we make that big column less wide? 13- I'm getting an NPE when trying to show Semaphores and Message queues (same NPE for both cases) java.lang.NullPointerException at org.eclipse.cdt.dsf.gdb.internal.ui.osview.OSData.columnIsInteger(OSData.java:114) at org.eclipse.cdt.dsf.gdb.internal.ui.osview.OSView.populateTable(OSView.java:409) at org.eclipse.cdt.dsf.gdb.internal.ui.osview.OSView.access$5(OSView.java:330) at org.eclipse.cdt.dsf.gdb.internal.ui.osview.OSView$6.runInUIThread(OSView.java:289) at org.eclipse.ui.progress.UIJob$1.run(UIJob.java:95) - We will need to disable the view when using a session where GDB is too old. This will be possible once we have a service that will wrap the -info-os command. We can leave this point for later.
Created attachment 208643 [details] Patch to apply after this contribution gets accepted I find that writing out comments in bugzilla and waiting for the contributor to address them take more time from both parties then necessary. I'm trying a new way. Here is a patch of minor things I would like to change on top of the contribution. Once the contribution is in, I will apply those changes myself. It was much faster to code the changes and create a patch than to write them out in text.
(In reply to comment #12) > (In reply to comment #11) > > Marc, I am pretty sure that Mentor has signed the agreement. I didn't need to > > do any paperwork to confirm my committer rights. > > Nice! So you will be ok to commit the patch once the review is finished? Sure.
Created attachment 208682 [details] Proposed icon for the view How about this icon for the OSAwareness view? We can always change it later on if someone has a better one.
(In reply to comment #16) > How about this icon for the OSAwareness view? > > We can always change it later on if someone has a better one. It seems OK conceptually. I have no idea what are the sizes/formats that Eclipse might want.
Created attachment 208694 [details] Screenshot of view with icon (In reply to comment #17) > It seems OK conceptually. I have no idea what are the sizes/formats that > Eclipse might want. I took an existing CDT icon, so it fits properly. Here is a screenshot of the view with the icon
(In reply to comment #18) > Created attachment 208694 [details] > Screenshot of view with icon > > (In reply to comment #17) > > > It seems OK conceptually. I have no idea what are the sizes/formats that > > Eclipse might want. > > I took an existing CDT icon, so it fits properly. Here is a screenshot of the > view with the icon This seems fine to me, thanks!
(In reply to comment #2) > 2. The view attempts to populate the view menu with a list of columns, so that > each column can be hidden if necessary -- which means that initially, the menu > is empty, and it is then populated when the view is updated for specific > resource kind. However, while this worked in Eclipse 3.6, in 3.7 the menu is > never shown. In OSView.populateTable(), after the loop that adds all the actions to the menu, you need to call: bars.updateActionBars(); The annoying thing is that the menu will only appear once the view has something to display. To keep the menu always showing, we can add in createPartControl() bars.getMenuManager().add(new Action(){}); although it does add an blank entry, which is weird. Or, what we could do is add the fRefreshAction action into the menu in createPartControl(). I think this is more user-friendly. So, instead of having an empty or missing menu at startup, the user will have a disabled Refresh action. Note also that the menu entries should probably be disabled when we select a wrong context, just like the rest of the view gets disabled.
There is a bit of overlap of this with RSE and TCF's Target Explorer. We'll need to find a solution when all three are active at the same time.
(In reply to comment #21) > There is a bit of overlap of this with RSE and TCF's Target Explorer. We'll > need to find a solution when all three are active at the same time. Ok, let's talk about this again in the new year, to see what parts overlap.
A possible enhancement to the proposed solution would be to use '-info-os' without parameters to figure out what types of resources are supported. This would mean that if a new version of GDB added a new type of resource, we would automatically be able to handle it. i.e., interpreter-exec mi -info-os ^done,OSDataTable={nr_rows="9",nr_cols="2",hdr= [{width="10",alignment="-1",col_name="col0",colhdr="Type"},{width="10",alignment="-1",col_name="col1",colhdr="Description"}], body=[item={col0="processes",col1="Listing of all processes"}, item={col0="procgroups",col1="Listing of all process groups"}, item={col0="threads",col1="Listing of all threads"}, item={col0="files",col1="Listing of all file descriptors"}, item={col0="sockets",col1="Listing of all internet-domain sockets"}, item={col0="shm",col1="Listing of all shared-memory regions"}, item={col0="semaphores",col1="Listing of all semaphores"}, item={col0="msg",col1="Listing of all message queues"}, item={col0="modules",col1="Listing of all loaded kernel modules"}]} The supported resource types are all listed above.
(In reply to comment #22) > (In reply to comment #21) > > There is a bit of overlap of this with RSE and TCF's Target Explorer. We'll > > need to find a solution when all three are active at the same time. > > Ok, let's talk about this again in the new year, to see what parts overlap. We talked about this contribution at the CDT monthly meeting. It was agreed to accept this contribution in CDT as long as the view is not shown automatically. Outstanding points: 1- Vladimir to address comment 13 2- Vladimir to address comment 20 3- wait for -info-os to be accepted in GDB. It has missed the 7.4 release but the discussions continue and the patches should be in HEAD soon. Once those three points are done, we can commit the patch. After that, I'll the extra few changes I recommend in "Patch to apply after this contribution gets accepted". After that, we'll need to create a proper DSF service to use teh -info-os command instead of doing directly from the GUI.
Marc, thanks for paving way for this patch! I am not sure this was mentioned, but I'm on vacation until 15th, and then have a week of meetings, so probably won't be able to address those points until 23rd.
(In reply to comment #25) > Marc, > > thanks for paving way for this patch! > > I am not sure this was mentioned, but I'm on vacation until 15th, and then have > a week of meetings, so probably won't be able to address those points until > 23rd. That is ok since we can have Mikhail commit the patch without a CQ (CQ's deadline seem to be the 3rd of Feb!). Thanks for letting me know.
Hi Volodya, will you be able to allocate some time to this? I realized we missed GDB 7.4 to have the new -info-os command availabe, but GDB 7.5 is planned for June 2012. If this patch is ready, we may be able to squeeze it in for the Eclipse Juno release. If we don't we will need to wait an entire year to get this available. Thanks Marc
I have update of this patch scheduled for our current sprint ;-) By the end of this week is possible, but not 100% guaranteed. Sorry for delay.
(In reply to comment #28) > I have update of this patch scheduled for our current sprint ;-) By the end of > this week is possible, but not 100% guaranteed. Sorry for delay. Thanks for the quick answer. I will add time to my own sprint to review ;)
(In reply to comment #28) > I have update of this patch scheduled for our current sprint ;-) By the end of > this week is possible, but not 100% guaranteed. Sorry for delay. Ping.
I don't see this making it into Juno. However, I still think we will go with the idea of using the IGDBHardware service to handle -info-os (see comment 4) eventually. With that in mind, I would like to rename IGDBHardware to something more appropriate, before it is released in Juno. The service will handle hardware info (CPUs/cores) as well as OS info. Does anyone have a good name to use? I can only think of IGDBHardwareAndOS or IGDBOperatingSystem...
Created attachment 212903 [details] Rename IGDBHardware to IGDBHardwareAndOS I've committed this fix and went with the name IGDBHardwareAndOS. It has to be done now as API freeze is now.
*** cdt git genie on behalf of Marc Khouzam *** Bug 360314: Rename IGDBHardware to IGDBHardwareAndOS to prepare to use it for OS information as well. [*] http://git.eclipse.org/c/cdt/org.eclipse.cdt.git/commit/?id=00ef45a136f823e4a66afa5245c5fa8129589020
Marc, I have pushed a revised version of this patch to https://git.eclipse.org/r/#/c/5835/ I believe I have addressed all your comments, except for: - Comment about array initialization, since Java standard clearly say that arrays are default initialized. - Comment about old GDB versions, since you've mentioned it can be done later. Also, the behaviour of the view menu now is that it is only visible when we have any data shown, and at no point we have empty menu. Let me know what you think. [Incidentally, are we supposed to be testing with 4.2 platform? It seems to be a bit buggy yet. If we're going to use 3.9, where can I get right packages?]
(In reply to comment #34) > Marc, > > I have pushed a revised version of this patch to > > https://git.eclipse.org/r/#/c/5835/ Thanks, I will have a look this sprint. > [Incidentally, are we supposed to be testing with 4.2 platform? It seems to be > a bit buggy yet. If we're going to use 3.9, where can I get right packages?] Juno will be using 4.2 platform. There has been a lot of effort to fix things so that it would work as expected. The M7 build released last week should be of much better quality. Note that CDT should also work with 3.8, but Juno will be on 4.2. I've been using 4.2 daily for a good four to five months.
Forgot to mention that this will need to wait until after Juno, as feature freeze has passed. Also, it would be good for the GDB parts to be in open-source when we commit this to CDT.
The new "info os" bits have been committed to GDB: http://sourceware.org/ml/gdb-patches/2012-05/msg00463.html the post mentions: "A related patch for MI will follow soon" Once the MI bits are in, it should make this contribution easier.
Ok, the MI parts are part of GDB and I have downloaded HEAD and built it. I'm now working on the review.
BTW, I posted comments to Gerrit about the latest proposal. I think we are quite close. The comments may seem larger than they really are, and I believe it won't take too much effort to get this ready to commit.
Marc, I noticed the comments and started looking into them. I'd expect to have something on Wednesday or Thursday.
(In reply to comment #40) > Marc, > > I noticed the comments and started looking into them. I'd expect to have > something on Wednesday or Thursday. No problem. I wasn't trying to pressure you :) I just wasn't sure if you had your Gerrit email setup or not.
I appear unable to make comments in gerrit (or rather, I can comment on specific hunk, but neither publish them, nor make global comments), so let me ask here. It seems that making MIInfoOSInfo handle both the case of fetching names of resources and data for specific resource would make that class overly bloated. How about having separate classes for that?
(In reply to comment #42) > I appear unable to make comments in gerrit (or rather, I can comment on > specific hunk, but neither publish them, nor make global comments), so let me > ask here. After you wrote inline comments, you press Review and the input the global comments and the press Publish Comments. This is not available to you? > It seems that making MIInfoOSInfo handle both the case of fetching > names of resources and data for specific resource would make that class overly > bloated. How about having separate classes for that? Although the comment makes sense, it would go against all other *Info classes. How about having something like: private void parse() { if (fetching names) { parseNames(); } else { parseResourceData(); } } public String[] getResourceNames(); public ??? getResourceData();
Hmm, I did not realize that "Review" is a button I should push, when it was mine patch being reviewed. I am still not sure I like a class having two disjoint set of data members used in two different scenarious. Not that this class is very OOP in the first place, but with such change it will be nothing but bag of data? Maybe, I can have two different classes for MI commands -- with resource and and without, and two matching Info classes?
(In reply to comment #44) > Hmm, I did not realize that "Review" is a button I should push, when it was > mine patch being reviewed. That was not obvious to me either, until I realized it. > I am still not sure I like a class having two disjoint set of data members used > in two different scenarious. Not that this class is very OOP in the first > place, but with such change it will be nothing but bag of data? Maybe, I can > have two different classes for MI commands -- with resource and and without, > and two matching Info classes? We should try to keep one class for an existing MI command, and one Info for the result of that command. However, you can split things up using other classes called by the *Info class. For example, you can have an MIResourcesData class which would parse the data once you find out that you are in that case of the MI command. An example of this is MIThreadInfoInfo.java, which does some high-level parsing and then delegates to MIThread.java to parse the actual thread data.
Created attachment 219281 [details] Screenshot of current version It looks like in Eclipse 4.*, the "Fetched at" element is rendered in rather ugly and space-inefficient way. I suppose I no longer sure that exact time where OS resources was read is important, so I'm planning to remove this element completely. Let me know if this is really bad idea.
(In reply to comment #46) > It looks like in Eclipse 4.*, the "Fetched at" element is rendered in rather > ugly and space-inefficient way. I suppose I no longer sure that exact time > where OS resources was read is important, so I'm planning to remove this > element completely. Let me know if this is really bad idea. Sorry I haven't been able to review the changes yet. I am leaving for vacation in 3 hours, for 3 weeks, but I'll review when I return. You can leave the "fetched at" as is until the review, but let's remember to pay attention to it and to the change you've noticed. Thanks
I actually removed it already -- moving this information in the tooltip of refresh item. Have a nice vacation, and there will be a much upgraded patch waiting for you when you're back!
(In reply to comment #48) > I actually removed it already -- moving this information in the tooltip of > refresh item. Have a nice vacation, and there will be a much upgraded patch > waiting for you when you're back! Thanks!
New version is pushed to: https://git.eclipse.org/r/#/c/5835/ Marc, I believe I have addressed all your comments, and I don't know about any open issues from my testing, but I am sure there is something yet to improve.
(In reply to comment #50) > New version is pushed to: > > https://git.eclipse.org/r/#/c/5835/ This patch is much closer to a final solution. Good work! I posted comments in Gerrit.
And yet another version pushed to git.
Ok the last version on Gerrit is good to go. Mikhail, for IP handling I will need you to commit it since you also work at Mentor like Vladimir. You can just press the "Submit Patchset 9" button which will not need you to say that you reviewed the change yourself, it will just put me as the reviewer but you as the committer. If that is ok with you, you can go ahead any time. Vladimir may push a patch 10, but it is for two small changes that I can do myself after. Thanks! And thanks Valadimir for the nice contribution!
(In reply to comment #53) > Ok the last version on Gerrit is good to go. > > Mikhail, for IP handling I will need you to commit it since you also work at > Mentor like Vladimir. > > You can just press the "Submit Patchset 9" button which will not need you to > say that you reviewed the change yourself, it will just put me as the > reviewer but you as the committer. > > If that is ok with you, you can go ahead any time. > Done.
Thanks Marc and Mikhail! I am planning to address those two last items by the end of the week. Shall I create new gerrit review for that?
Thanks Mikhail. I've very happy this is in. (In reply to comment #55) > Thanks Marc and Mikhail! > > I am planning to address those two last items by the end of the week. Shall > I create new gerrit review for that? I'll take care of the Message one right now. But I'll leave the minor bug for you to potentially suggest something. I think a Gerrit review is best for this. I'll even do one myself for the little change of the message. Next and final step is to update the N&N page to advertise this nice feature. Vladimir, can you take care of that? http://wiki.eclipse.org/CDT/User/NewIn82
I've pushed the change of message from "No debug session is selected." to "Invalid debug session selected." here https://git.eclipse.org/r/7855 I will commit now.
To be honest, "Invalid debug session selected" message when you have no debug sessions at all does not sound good either. I'll see whether this can be improved further with "commercially reasonable effort" ;-)
(In reply to comment #58) > To be honest, "Invalid debug session selected" message when you have no > debug sessions at all does not sound good either. I'll see whether this can > be improved further with "commercially reasonable effort" ;-) Sure, a better wording or two different messages is fine with me
I've done the N&N entry because I needed for a status report today. Feel free to update if you want to change something. http://wiki.eclipse.org/CDT/User/NewIn82#OS_Resources_View
I think I've fixed two remaining items, and I've pushed it to https://git.eclipse.org/r/8685 I am thinking that maybe selection of the gdb node should work too, just as selection of launch node works now, but I suppose we can then reasonable apply this logic for every single object inside DSF-GDB launch, and I'm not sure it's worth that really. Things like gdb and gdbserver inside the launch are not particularly interesting so spending effort on them might not be best.
(In reply to comment #61) > I am thinking that maybe selection of the gdb node should work too, just as > selection of launch node works now, but I suppose we can then reasonable > apply this logic for every single object inside DSF-GDB launch, and I'm not > sure it's worth that really. Things like gdb and gdbserver inside the launch > are not particularly interesting so spending effort on them might not be > best. I agree that it is not necessary. Currently, there are other things that don't work if the selection is the 'gdb' node (like the Trace Control view), so I don't think it is worth fixing it for OSResources.
(In reply to comment #61) > I think I've fixed two remaining items, and I've pushed it to > https://git.eclipse.org/r/8685 Reviewed and committed to master. And we're done. Thanks!
Thanks a lot!
