Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.

Bug 326670

Summary: [Disassembly View] Enhancement request for unlink and re-evaluate expression
Product: [Tools] CDT Reporter: Patrick Chuong <pchuong>
Component: cdt-debug-dsfAssignee: Anton Leherbauer <aleherb+eclipse>
Status: RESOLVED FIXED QA Contact: Pawel Piech <pawel.1.piech>
Severity: enhancement    
Priority: P3 CC: aleherb+eclipse, pawel.1.piech
Version: 7.0   
Target Milestone: 8.0   
Hardware: PC   
OS: Windows XP   
Whiteboard:
Attachments:
Description Flags
First attempt
none
Use toggle button to sync with debug view
none
Clean up actions aleherb+eclipse: iplog+

Description Patrick Chuong CLA 2010-09-30 10:02:32 EDT 
Build Identifier: 201006141710

1. Would like to be able to unlink the disassembly view from using the debug context's program counter to evaluate the start address. 

2. Would like to be able to disable expression evaluation for selection change and/or suspend event.

One way to solve #1 is to use a special marco (i.e Program Counter) for the default value of the address combo box in the view. When the address combo box has this special marco, than the view will retrieve the start address from the stackframe. For any other expression, it will go throught the expression service to retrieve the start address.

To address #2, a toggle toolbar button can be use to enable expression evaluation, this includes the special marcro in #1 as well. If expression evaluation is toggled off, changing the debug context will not cause the view to revaluate the start address, however the disassembly content will be refetch with the existing start address(s).

Reproducible: Always
Comment 1 Patrick Chuong CLA 2010-10-01 16:36:05 EDT 
Created attachment 180088 [details]
First attempt

This is my first attempt to implement the enhancement request, I am not sure if I have covered all different scenerios. I have tested on gdb-dsf and TI debugger that uses dsf as well and I haven't seen any issue yet.

The track expression option is added to the preference page of the disassembly view. When this option is enabled, the expression in the address combo box will be re-evaluate when debug context changed.
Comment 2 Anton Leherbauer CLA 2010-10-04 05:24:45 EDT 
Thanks for the patch. Some observations:

1) With the patch, the Disassembly view always sticks to the expression in the 
   address bar.  Selecting the "Program Counter" entry to link the view again to
   the active stack frame is not quite obvious.
   It is no longer possible to simply navigate to an address/expression without
   changing the behavior of the view.
   All in all, I think whether the view is linked to the active stack frame or 
   not, should be made more explicit, e.g. using a toggle button.

2) The Home button now goes to the expression in the address bar, no longer to 
   the "Program Counter".  A potential solution would be to turn it into a "Go"
   button next to the address bar, similar to the Memory Browser.

3) The instruction pointer annotation is set for any location in the address 
   bar.  This could be confusing, because it is usually only used for the PC.

4) The initial string of the Combo box is truncated and displays only 
   "rogram Counter" (on WinXP).
Comment 3 Patrick Chuong CLA 2010-10-04 17:14:18 EDT 
Created attachment 180211 [details]
Use toggle button to sync with debug view

(In reply to comment #2)

With the wew patch

re #1: I have change it to use a toggle button to sync with the debug view.

re #2: Fixed, it goes to the program counter in both link mode and unlink mode.

re #3: The PC annotation and the PC History are removed in unlink mode.

re #4: I don't see this on my PC with XP, however I have revert the change not to use "Program Counter".

I also exposed the track expression action and link action in the context menu and local toolbar menu.

Is it possible to have the view set to support multiple instance? It shouldn't affect the current behavior, where there is no UI to open multiple view at the moment. With allowMultiple set to true in the plugin.xml, than vendor that uses this view can contribute the open new view action for their product.
Comment 4 Anton Leherbauer CLA 2010-10-12 05:09:02 EDT 
(In reply to comment #3)
> Created an attachment (id=180211) [details]
> Use toggle button to sync with debug view

Thanks, I like that better.
I think "Link with Debug view" and "Track expression" should be view local settings and not preferences.  Esp. when you intend to have multiple instances of the view.  What do you think?
Actually with multiple view instances, one could argue that almost all preference settings would need to be per instance, but that's a different topic.

Should "Track expression" be disabled when "Link with Debug view" is enabled?  It does not make much sense in this case, I think.

"Link with Debug view" is not quite correct, because it is linked to the active debug context, which might come from somewhere else.

> Is it possible to have the view set to support multiple instance? It shouldn't
> affect the current behavior, where there is no UI to open multiple view at the
> moment. With allowMultiple set to true in the plugin.xml, than vendor that uses
> this view can contribute the open new view action for their product.

That should be no problem.
Comment 5 Patrick Chuong CLA 2010-10-12 09:37:16 EDT 
(In reply to comment #4)

> I think "Link with Debug view" and "Track expression" should be view local
> settings and not preferences.  Esp. when you intend to have multiple 
> instances of the view.  What do you think?

Having it as a local view setting works for me as well. I guess the setting has to be presisted per view in the workspace.

> Should "Track expression" be disabled when "Link with Debug view" is 
> enabled? 

I think so too.

> It does not make much sense in this case, I think.
> "Link with Debug view" is not quite correct, because it is linked to the 
> active debug context, which might come from somewhere else.

Makes sense, I'll change it to "Link with Active Debug Context". Do you have another suggestion?

I'll rework the patch sometime this week. Thanks for taking the time to review patch.
Comment 6 Patrick Chuong CLA 2010-10-12 16:55:35 EDT 
Created attachment 180710 [details]
Clean up actions

I have clean up the patch with the suggested changes.

One additional thing that I am not sure about UI guildline were the two actions location. I have "Link..." in the toolbar but not view local menu, and "Track..." in the view local menu but not the toolbar. Also, both actions are in the context menu. Is this acceptable or I need arrange the action diffently?
Comment 7 Anton Leherbauer CLA 2010-10-13 06:48:54 EDT 
(In reply to comment #6)
> One additional thing that I am not sure about UI guildline were the two actions
> location. I have "Link..." in the toolbar but not view local menu, and
> "Track..." in the view local menu but not the toolbar. Also, both actions are
> in the context menu. Is this acceptable or I need arrange the action diffently?

I think "Link..." and "Track..." should be in the local view menu, but not in the context menu.  The "Show..." actions should also be only in the view local menu.
Things are different when the disassembly is used as editor, though, because there is no view local menu.

I'll commit the last patch with a few minor changes:
- removed trailing whitespace
- simplified for loop to redraw PC trail
- streamlined actions as indicated above
Comment 8 Anton Leherbauer CLA 2010-10-13 06:56:41 EDT 
(In reply to comment #7)
> I'll commit the last patch with a few minor changes:
> - removed trailing whitespace
> - simplified for loop to redraw PC trail
> - streamlined actions as indicated above

I forgot:
- removed bogus setDisabledImageDescriptor(...) from SyncAction
Comment 9 Anton Leherbauer CLA 2010-10-13 07:51:53 EDT 
Committed to HEAD.
Comment 10 CDT Genie CLA 2010-10-13 08:23:03 EDT 
*** cdt cvs genie on behalf of aleherbau ***
Bug 326670 - [Disassembly View] Enhancement request for unlink and re-evaluate expression
Patch from Patrick Chuong (TI)

[*] DisassemblyPreferenceConstants.java 1.3 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/dsf/org.eclipse.cdt.dsf.ui/src/org/eclipse/cdt/dsf/debug/internal/ui/disassembly/preferences/DisassemblyPreferenceConstants.java?root=Tools_Project&r1=1.2&r2=1.3

[*] JumpToAddressAction.java 1.4 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/dsf/org.eclipse.cdt.dsf.ui/src/org/eclipse/cdt/dsf/debug/internal/ui/disassembly/actions/JumpToAddressAction.java?root=Tools_Project&r1=1.3&r2=1.4

[*] plugin.xml 1.22 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/dsf/org.eclipse.cdt.dsf.ui/plugin.xml?root=Tools_Project&r1=1.21&r2=1.22

[*] DisassemblyImageRegistry.java 1.3 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/dsf/org.eclipse.cdt.dsf.ui/src/org/eclipse/cdt/dsf/debug/internal/ui/disassembly/DisassemblyImageRegistry.java?root=Tools_Project&r1=1.2&r2=1.3
[*] DisassemblyMessages.properties 1.8 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/dsf/org.eclipse.cdt.dsf.ui/src/org/eclipse/cdt/dsf/debug/internal/ui/disassembly/DisassemblyMessages.properties?root=Tools_Project&r1=1.7&r2=1.8
[*] DisassemblyView.java 1.6 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/dsf/org.eclipse.cdt.dsf.ui/src/org/eclipse/cdt/dsf/debug/internal/ui/disassembly/DisassemblyView.java?root=Tools_Project&r1=1.5&r2=1.6
[*] DisassemblyMessages.java 1.9 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/dsf/org.eclipse.cdt.dsf.ui/src/org/eclipse/cdt/dsf/debug/internal/ui/disassembly/DisassemblyMessages.java?root=Tools_Project&r1=1.8&r2=1.9
[*] DisassemblyPart.java 1.33 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/dsf/org.eclipse.cdt.dsf.ui/src/org/eclipse/cdt/dsf/debug/internal/ui/disassembly/DisassemblyPart.java?root=Tools_Project&r1=1.32&r2=1.33