Bugzilla – Full Text Bug Listing
| Summary: | [expressions][run control][cdi] DSF is missing certain debug actions from editor's context menu | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | [Tools] CDT | Reporter: | Navid Mehregani <nmehrega> | ||||||||||
| Component: | cdt-debug-dsf | Assignee: | Marc Khouzam <marc.khouzam> | ||||||||||
| Status: | RESOLVED FIXED | QA Contact: | Pawel Piech <pawel.1.piech> | ||||||||||
| Severity: | enhancement | ||||||||||||
| Priority: | P3 | CC: | john.cortell, ken.ryall, malaperle, marc.khouzam, pawel.1.piech | ||||||||||
| Version: | 6.0 | Flags: | john.cortell:
review+
|
||||||||||
| Target Milestone: | 7.0 | ||||||||||||
| Hardware: | PC | ||||||||||||
| OS: | Windows XP | ||||||||||||
| Whiteboard: | |||||||||||||
| Attachments: |
|
||||||||||||
|
Description
Navid Mehregani
Created attachment 156549 [details]
Snapshot
The DSF Disassembly view is also missing: o Run to line o Move to line o Resume at line It should be relatively easy to fix both the editor and DSF Disassembly view at the same time. Note that the DSF Disassembly view targetID is "#DisassemblyPartContext" The missing "Add Watch Expression..." is being tracked by Bug 274951 so I will not do it as part of this bug. Created attachment 159033 [details]
Preliminary fix
This patch adds the missing menus to the editor.
I still have to do the Disassembly view.
Created attachment 159054 [details]
Final fix
This fix is actually pretty simple. The patch seems large because I've updated the MoveToLineActionDelegate and ResumeAtLineActionDelegate to match the latest RunToLineActionDelegate from the platform.
Also, I've copied EvaluationContextManager from cdt.debug.ui to a cdt.dsf.gdb.ui specific version that checks for a DSF debugger. This class may be interesting in DSF itself.
Committed to HEAD.
John, can you review this change? I'm trying to spread the reviews, and I think this one is something you have seen in CDI (In reply to comment #6) > John, can you review this change? I'm trying to spread the reviews, and I > think this one is something you have seen in CDI Marc, I tried Move to Line and it has a quirk. The relocated PC arrow does not appear at the new location unless I select the frame in the Debug view. The arrow disappears from the previous location, just doesn't appear in the new location. Resume at Line has the same issue, but not Run to Line. It's a strange coincidence that the buggy behavior is in the delegates you updated and not in the one you didn't, so I'll let you investigate this before I review the changes. (In reply to comment #7) > (In reply to comment #6) > > John, can you review this change? I'm trying to spread the reviews, and I > > think this one is something you have seen in CDI > > Marc, I tried Move to Line and it has a quirk. The relocated PC arrow does not > appear at the new location unless I select the frame in the Debug view. The > arrow disappears from the previous location, just doesn't appear in the new > location. Thanks for trying it. I wasn't able to reproduce this so I guess I don't understand what you mean. What steps do you exactly do? What I do is: 1- launch DSF-GDB 2- select a new line in the editor 3- right-click and select move-to-line But I'm not sure when you don't have the frame selected. On Windows... 1. Launch DSF session 2. Program stops at main. Select the main frame in the Debug view if not already selected. 3. In the editor, right click on a line further down in main() 4. Select Move to Line The PC arrow in the editor disappears from where it was but doesn't show up where you requested it move to. 5. Click on the main() frame in the Debug view. PC arrow now shows up in the editor, at the new location. (In reply to comment #9) > On Windows... > 1. Launch DSF session > 2. Program stops at main. Select the main frame in the Debug view if not > already selected. > 3. In the editor, right click on a line further down in main() > 4. Select Move to Line > The PC arrow in the editor disappears from where it was but doesn't show up > where you requested it move to. > 5. Click on the main() frame in the Debug view. > PC arrow now shows up in the editor, at the new location. Thanks John. I see this now. It only happens with GDB's older than 7.0. I had see this problem and I thought I had fixed it. Maybe something fell out of my patch :-O I'll let you know when it fixed. John, do you have the platform debug plugins checked-out? Surprisingly, when I close those two plugins, things work again. Can you try? (In reply to comment #11) > John, do you have the platform debug plugins checked-out? > Surprisingly, when I close those two plugins, things work again. Can you try? Indeed, I do. And yes, when I close those plugins, the problem goes away. This may call for doing a diff between M5 and HEAD of those platform sources to see what change may be causing this. (In reply to comment #12) > (In reply to comment #11) > > John, do you have the platform debug plugins checked-out? > > Surprisingly, when I close those two plugins, things work again. Can you try? > > Indeed, I do. And yes, when I close those plugins, the problem goes away. This > may call for doing a diff between M5 and HEAD of those platform sources to see > what change may be causing this. Sigh... That's what I did for CDT, when I ran out of things that were different, and closed my platform plugins :-) I'll try diffing the platform. (In reply to comment #13) The problem was introduced in bug 299958 but I have not idea why. I'm hoping Pawel will know? (In reply to comment #14) > The problem was introduced in bug 299958 but I have not idea why. > I'm hoping Pawel will know? After investigating a bit, I think the problem is on the GDB side. In short: The reason the IP is never painted, is because the "move to line" operation does not generate a suspended event for a thread. It does generate a suspended event for the process, which causes the view to refresh the contents, but its not enough to cause the selection in the view. Bug 299958 fixed an issue (among other things) where a selection event was being generated way too often. This bug was introduced in 3.6 M4. The long explanation: The debug view has historically painted the IP in editor in response to a stack frame being selected in Debug view, either as a result of a suspended event on a thread or by user action. This behavior works well in most scenarios, but it's particularly buggy when it comes to handling the "Set PC to Here" action. I've spent many hours fixing this behavior in our product (when it was not DSF based), but there were always some edge cases where it didn't work, because we were generating an artificial running/suspend event pair at the high level... which didn't always clear the caches as needed. The simplest way to fix this would be for MIRunControl to track on which thread the set-pc-to-here action was performed, and add this information to the container suspended event. A more sophisticated way to fix this would be to override platform's handling of when the IP is painted in the editor. Then you could customize it so that it re-paints the IP upon a container-suspended-event without a trigger thread. Doing this would also allow us to fix bug 225919. Created attachment 160115 [details] Further fix (In reply to comment #15) > (In reply to comment #14) > > The problem was introduced in bug 299958 but I have not idea why. > > I'm hoping Pawel will know? > After investigating a bit, I think the problem is on the GDB side. Thanks a lot Pawel for your explanation, it helped a lot. > The simplest way to fix this would be for MIRunControl to track on which thread > the set-pc-to-here action was performed, and add this information to the > container suspended event. The attached patch does this. I was able to extract the thread context from the context that was used to perform the CLI command ('jump'). However, the problem remains when doing a RunControl command from the gdb console, such as 'next'. In this case, we don't have the current context. Is there a way to obtain the current debug context when I'm not in a UI plugin? > A more sophisticated way to fix this would be to override platform's handling > of when the IP is painted in the editor. Then you could customize it so that > it re-paints the IP upon a container-suspended-event without a trigger thread. > Doing this would also allow us to fix bug 225919. I posted a patch for bug 225919 to get this started. We already override Platform's handling of the IP stuff in DsfSourceDisplayAdapter, but I think we have to agree on how we want to handle the IP before making big changes. I'll leave this for bug 225919. John, you can now review this bug :-) MoveToLine still does not work perfectly in the case of multi-threaded programs. I'm tracking the problem with bug 302597. The current bug is actually properly solved since it is just about making the actions available in the menu (In reply to comment #18) I'll be reviewing it this morning (In reply to comment #19) > (In reply to comment #18) > I'll be reviewing it this morning I just realized this bug is marked as DSF. My solution is only for DSF-GDB. I can move it to DSF if Pawel agrees. (In reply to comment #20) > I can move it to DSF if Pawel agrees. +1 (In reply to comment #17) > John, you can now review this bug :-) Looks good. I just have the following minor comments: 1. Is there a practical advantage to calling IAdapterManager.hasAdapter() before calling IAdapterManager.loadAdapter()? 2. Class fields explicitly initialized to null (default value) is a personal pet peeve of mine. They are unnecessary, but more importantly, they are a nuisance when stepping into the construction of the object. 3. I believe the use a WorkbenchJob is preferable to calling Disaply.asyncExec(). (In reply to comment #21) > (In reply to comment #20) > > I can move it to DSF if Pawel agrees. > +1 But we are planning on updating the solution to no be GDB specific, right? (The discussion Warren started) (In reply to comment #23) > (In reply to comment #21) > > (In reply to comment #20) > > > I can move it to DSF if Pawel agrees. > > +1 > > But we are planning on updating the solution to no be GDB specific, right? (The > discussion Warren started) I thought we just wanted to move classes from DSF-GDB to DSF. Is there more to do for Warren's request? (In reply to comment #24) > (In reply to comment #23) > > (In reply to comment #21) > > > (In reply to comment #20) > > > > I can move it to DSF if Pawel agrees. > > > +1 > > > > But we are planning on updating the solution to no be GDB specific, right? (The > > discussion Warren started) > > I thought we just wanted to move classes from DSF-GDB to DSF. Is there more to > do for Warren's request? Never mind. I think I might have misinterpreted your question to Pawel. Regardless, we are doing more than just moving classes, right? We talked about creating an IRunControl2 and using that instead of an MI specific DSF interface. (In reply to comment #25) > Regardless, we are doing more than just moving classes, right? We talked about > creating an IRunControl2 and using that instead of an MI specific DSF > interface. True. But we are crossing bugs :-) The patch of this bug just deals with menus. The actual performing of the moveToLine et al operations are in another bug, that should also be moved to DSF. > True.
> But we are crossing bugs :-) The patch of this bug just deals with menus. The
> actual performing of the moveToLine et al operations are in another bug, that
> should also be moved to DSF.
Ah...
(In reply to comment #27) > > True. > > But we are crossing bugs :-) The patch of this bug just deals with menus. The > > actual performing of the moveToLine et al operations are in another bug, that > > should also be moved to DSF. > > Ah... That's bug 303968, working on it now. (In reply to comment #28) > (In reply to comment #27) > > > True. > > > But we are crossing bugs :-) The patch of this bug just deals with menus. The > > > actual performing of the moveToLine et al operations are in another bug, that > > > should also be moved to DSF. > > > > Ah... > > That's bug 303968, working on it now. I'm not completely clear about this. Will bug 303968 also cover moving the current bug changes to DSF? (In reply to comment #18) > MoveToLine still does not work perfectly in the case of multi-threaded > programs. I'm tracking the problem with bug 302597. Just a note that bug 302597 has been resolved. (In reply to comment #22) > (In reply to comment #17) > > John, you can now review this bug :-) > > Looks good. I just have the following minor comments: Thanks > 1. Is there a practical advantage to calling IAdapterManager.hasAdapter() > before calling IAdapterManager.loadAdapter()? I copied it from that platform but I'm not familiar with this stuff. > 2. Class fields explicitly initialized to null (default value) is a personal > pet peeve of mine. They are unnecessary, but more importantly, they are a > nuisance when stepping into the construction of the object. I never thought about it this way. I'm from a C/C++ background where it was important to initialize fields. Is it a general practice in Java to not initialize fields to their default value? You have a point about the nuisance so I'll adopt that approach from now on. Thanks > 3. I believe the use a WorkbenchJob is preferable to calling > Disaply.asyncExec(). I copied it from the platform. Is there a reason why you recommend WorkbenchJob instead? (In reply to comment #31) > > 2. Class fields explicitly initialized to null (default value) is a personal > > pet peeve of mine. They are unnecessary, but more importantly, they are a > > nuisance when stepping into the construction of the object. > > I never thought about it this way. I'm from a C/C++ background where it was > important to initialize fields. Is it a general practice in Java to not > initialize fields to their default value? You have a point about the nuisance > so I'll adopt that approach from now on. Thanks Most Java code I see follows that practice (not explicitly initializing fields to defaults). It helps keep clutter to a minimum, and makes debugging less cumbersome. > > 3. I believe the use a WorkbenchJob is preferable to calling > > Disaply.asyncExec(). > > I copied it from the platform. Is there a reason why you recommend > WorkbenchJob instead? WorkbenchJob adds shutdown protection. Years ago I asked Darin why the platform debug code has Display.asycExec calls instead of WorkbenchJobs, and he said it's probably because the code I was looking at was written before WorkbenchJob was available (3.0) :-) Thanks for addressing my points. |