Bug 219040 - [variables][cdi] Variables view should show globals (or should it?)
Summary: [variables][cdi] Variables view should show globals (or should it?)
Status: ASSIGNED
Alias: None
Product: CDT
Classification: Tools
Component: cdt-debug-dsf (show other bugs)
Version: 0 DD 1.0   Edit
Hardware: PC Linux
: P3 enhancement with 5 votes (vote)
Target Milestone: ---   Edit
Assignee: Project Inbox CLA
QA Contact: Jonah Graham CLA
URL:
Whiteboard:
Keywords:
: 254680 285575 449392 (view as bug list)
Depends on:
Blocks: 237560
  Show dependency tree
 
Reported: 2008-02-14 20:17 EST by Pawel Piech CLA
Modified: 2021-08-25 13:32 EDT (History)
21 users (show)

See Also:


Attachments
Global variable view screenshot (189.36 KB, image/png)
2021-06-04 13:00 EDT, Vinod Appu CLA
no flags Details
Show full path support (49.92 KB, image/png)
2021-07-30 04:41 EDT, Vinod Appu CLA
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Pawel Piech CLA 2008-02-14 20:17:03 EST
CDI allows for global variables to be selectively displayed in the variables view.  To achieve feature parity with CDI, DSF variables view needs to do the same.  

However, adding globals to the variables view duplicates exactly the functionality of the expressions view.  I am rather conflicted by adding competing functionality in two views.  Still the user is always right, so I will send a question to cdt-dev to figure out why this feature was added to the CDT variables view in the first place.
Comment 1 Marc Khouzam CLA 2008-03-17 10:49:01 EDT
An idea to keep in mind.  We may want to use the scope operator :: before global variables to deal with cases such as:

int f=9;
main() {
  int f=7:
}

Note that this operator does not work in C but only C++
Comment 2 Pawel Piech CLA 2009-08-06 14:43:47 EDT
*** Bug 285575 has been marked as a duplicate of this bug. ***
Comment 3 John Cortell CLA 2010-02-15 13:01:04 EST
Pawel, if the user is in a function that is accessing locals and globals, he should be able to see all the relevant variables in one view, not have to deal with two, particularly since the Variables and Expressions view share the same pane (i.e., are mutually exclusive unless you move one out to a different pane).
Comment 4 Steve Sobek CLA 2010-03-05 16:42:07 EST
The CDI Add Global Variables dialog has scaling problems that probably should be addressed while looking at it as part of DSF feature parity.

The org.eclipse.cdt.debug.internal.ui.actions.AddGlobalsActionDelegate dialog shows a flat list of all global variables in alphabetical order. For a large executable, that can be thousands of entries.

* The dialog should have a filter so that you can search for variables of interest by name.

* Besides by name, the other way to organize a list of global variables is by file of origin. The dialog should support a tree view, organized by the files in which variables are defined. If the user cares about variables defined in foo.cpp, they should have a view that supports selecting those variables.
Comment 5 John Cortell CLA 2010-03-05 16:52:13 EST
(In reply to comment #4)
> * Besides by name, the other way to organize a list of global variables is by
> file of origin. The dialog should support a tree view, organized by the files
> in which variables are defined. If the user cares about variables defined in
> foo.cpp, they should have a view that supports selecting those variables.

This wouldn't scale well either. An executable could be associated with tens of thousands of source files. What if, via a preference/action in the dialog, the file name was appended to the global variable name, e.g., 

   g_someCount::foo.c

If the user wants to choose globals in foo.c, he just include "foo.c" in the filter you describe in your first suggestion.
Comment 6 Ed Swartz CLA 2010-03-08 12:14:33 EST
(In reply to comment #5)
> 
> This wouldn't scale well either. An executable could be associated with tens of
> thousands of source files. 

Yeah, that may be annoying, since in properly written programs (IMO) there won't be globals defined in most of those files.  So a user would have to first know what file to look in (scanning or filtering for this) and then expand the file so the debugger will fetch the globals, and then select such globals.

I think some UI smarts can be done with the wizard to ameliorate these issues.  

Mainly, the backend should be able to provide an ILazyContentProvider or something similar so there is no need to fetch and display all sources or all variables unless the user explicitly opens a tree node.  

But that provider can certainly provide already detected information.

For example:

1) If debug info for a source file has already been scanned, and we know it has no globals, don't put it in the tree to begin with.

2) If debug info has been scanned and a file has globals, expand its node so its variables are visible (like today).  Be sure the filter can match filenames or variable names.

3) The provider can choose whether to fetch global variables for other sources based on the size of the debug information.


But this doesn't really handle the case where the user is searching blindly for a variable by name, e.g. "errno", and doesn't know what file provides it.  If the lazy provider hasn't expanded the files, the filter will not work.  Should the UI somehow forcibly look up globals in other files to find a match, or does the user have to "expand all", or should there be a "find" button?
Comment 7 David Dubrow CLA 2010-03-08 13:01:10 EST
(In reply to comment #5)
> This wouldn't scale well either. An executable could be associated with tens of
> thousands of source files. What if, via a preference/action in the dialog, the
> file name was appended to the global variable name, e.g., 
> 
>    g_someCount::foo.c
> 
> If the user wants to choose globals in foo.c, he just include "foo.c" in the
> filter you describe in your first suggestion.

I am assuming that the filter would work similarly to the one in the Open Resource dialog where it would not necessarily include anything by default unless the user specifically types a filter matching everything. In such a case, the user would have specifically initiated a potentially lengthy operation and with progress monitor support, could cancel the search at any time.

(In reply to comment #6)
> 1) If debug info for a source file has already been scanned, and we know it has
> no globals, don't put it in the tree to begin with.

I think I'd disagree with this right away because to the user there would be no apparent reason for why the tree of source files was incomplete given that some (as yet unscanned) files would be in it while others might be missing.
Comment 8 Steve Sobek CLA 2010-03-25 11:36:29 EDT
(In reply to comment #5)

I agree with John's suggestion of being able to specify the source file name as part of the search for a variable. However, we can't overload "::" to delimit the file name, because if I have global variable "foo" and source file "foo", does "bar::foo" mean search file "foo" for global "bar" or search class "bar" for global "foo"?

With that ability to do file name specific matches, I don't see any real need for presenting a source file based view.

I would require that the file name be at the end of the string. Looking at my keyboard, I would prefix the file name with "@" or put the file name in "<>". It would be nice if we could find a way to use the same syntax in the Expressions view. However, EDC for one passes the expression string to GNUCPPSourceParser() for C++ knowledgeable parsing, and that routine would just return a syntax error.

I'd also recommend that an empty string in front of the file be treated that same as an asterisk. E.g., "@foo.c" or "<foo.c>" is the same as "*@foo.c" or "*<foo.c>".

Since multiple files can have the same base name (e.g.,"/src1/foo.c" and "/src2/foo.c"), we'd need to allow specification of paths such as "/src/foo.c", "\src\foo.c", "x:\src\foo.c", and "x:/src/foo.c".

Also, I'd probably allow a toggle of showing base names versus full names with the matches. No file,too? E.g., the same result might be "bar", "bar <foo.c>", or "bar <c:/src2/foo.c>".

(In reply to comment #7)

I agree with David's suggestion of supporting a filter like Open Resource, where nothing is shown by default and what matches changes as you continue typing. Being careful to use file names and don't use wildcard characters speeds your search.

If it's all one search string we shouldn't allow separate case sensitivity/insensitivity for variable names and file names.
Comment 9 Marc Khouzam CLA 2011-05-17 11:21:37 EDT
*** Bug 254680 has been marked as a duplicate of this bug. ***
Comment 10 Jon Beniston CLA 2013-09-20 04:29:22 EDT
I agree with the above that it would be useful to be able to include globals along side locals in the Variables view so that you can easily see them both at once. It is also confusing to some users that you have the Add Globals menu, but it is always greyed out. One other difference is that the variables context menu has the View Memory option, which the expressions context menu doesn't, which can be useful.
Comment 11 Marc Khouzam CLA 2013-09-20 05:46:39 EDT
(In reply to Jon Beniston from comment #10)
> One other difference is that the
> variables context menu has the View Memory option, which the expressions
> context menu doesn't, which can be useful.

That is a good observation.  I've opened Bug 417681 to add that option to the Expressions view.
Comment 12 Steve Waring CLA 2014-08-25 11:41:21 EDT
(In reply to Pawel Piech from comment #0)
> CDI allows for global variables to be selectively displayed in the variables
> view.  To achieve feature parity with CDI, DSF variables view needs to do
> the same.  
> 
> However, adding globals to the variables view duplicates exactly the
> functionality of the expressions view.  I am rather conflicted by adding
> competing functionality in two views.  Still the user is always right, so I
> will send a question to cdt-dev to figure out why this feature was added to
> the CDT variables view in the first place.
In Juno at least, the variables view does not duplicate the functionality of the expressions view. This is because in the variables view there is a lower section. Highlighting a variable shows the value of the variable, using toString() for objects. In the expressions view you have to add .toString() to the expression for  an object to see it's value.

Of course adding a lower section to the expressions view that gave a toString() representation of an object would mean that the variables view does duplicate the functionality of the expressions view (as erroneously stated). This would also improve the expressions view.
Comment 13 Nobody - feel free to take it CLA 2014-08-25 11:58:12 EDT
(In reply to Steve Waring from comment #12)
> (In reply to Pawel Piech from comment #0)
> > CDI allows for global variables to be selectively displayed in the variables
> > view.  To achieve feature parity with CDI, DSF variables view needs to do
> > the same.  
> > 
> > However, adding globals to the variables view duplicates exactly the
> > functionality of the expressions view.  I am rather conflicted by adding
> > competing functionality in two views.  Still the user is always right, so I
> > will send a question to cdt-dev to figure out why this feature was added to
> > the CDT variables view in the first place.
> In Juno at least, the variables view does not duplicate the functionality of
> the expressions view. This is because in the variables view there is a lower
> section. Highlighting a variable shows the value of the variable, using
> toString() for objects. In the expressions view you have to add .toString()
> to the expression for  an object to see it's value.
> 
> Of course adding a lower section to the expressions view that gave a
> toString() representation of an object would mean that the variables view
> does duplicate the functionality of the expressions view (as erroneously
> stated). This would also improve the expressions view.

What do you mean by "lower section"? The expressions view has exactly the same structure as the variables view. The layout of the either view can be changed from the system menu (the little triangle on the right of the view).
Comment 14 Steve Waring CLA 2014-08-25 12:20:00 EDT
(In reply to Mikhail Khodjaiants from comment #13)
> (In reply to Steve Waring from comment #12)
> > (In reply to Pawel Piech from comment #0)
> > > CDI allows for global variables to be selectively displayed in the variables
> > > view.  To achieve feature parity with CDI, DSF variables view needs to do
> > > the same.  
> > > 
> > > However, adding globals to the variables view duplicates exactly the
> > > functionality of the expressions view.  I am rather conflicted by adding
> > > competing functionality in two views.  Still the user is always right, so I
> > > will send a question to cdt-dev to figure out why this feature was added to
> > > the CDT variables view in the first place.
> > In Juno at least, the variables view does not duplicate the functionality of
> > the expressions view. This is because in the variables view there is a lower
> > section. Highlighting a variable shows the value of the variable, using
> > toString() for objects. In the expressions view you have to add .toString()
> > to the expression for  an object to see it's value.
> > 
> > Of course adding a lower section to the expressions view that gave a
> > toString() representation of an object would mean that the variables view
> > does duplicate the functionality of the expressions view (as erroneously
> > stated). This would also improve the expressions view.
> 
> What do you mean by "lower section"? The expressions view has exactly the
> same structure as the variables view. The layout of the either view can be
> changed from the system menu (the little triangle on the right of the view).
My mistake. For some reason the variables view was showing in vertical layout and the expressions view was showing in horizontal layout with no space to show the value of an object. I've learnt something useful here. Please ignore my comment.
Comment 15 Nobody - feel free to take it CLA 2014-10-31 10:29:30 EDT
*** Bug 449392 has been marked as a duplicate of this bug. ***
Comment 16 Simeon Andreev CLA 2020-03-05 06:46:10 EST
Hi Jonah,

following up on my question on bug 484900, we have some code that crawls over binary files provided by org.eclipse.cdt.debug.core.model.ICModule.getSymbolsFileName() and then adds more IGlobalVariableDescriptor[] via the removed https://www.cct.lsu.edu/~rguidry/eclipse-doc36/org/eclipse/cdt/debug/core/model/ICDebugTarget.html method getGlobals().

I read on this bug that global variables are not supported with DSF, or am I misunderstanding something?

Best regards and thanks,
Simeon
Comment 17 Simeon Andreev CLA 2020-03-27 05:22:30 EDT
So far I've re-added the "add global variables" action + selection dialog to our product, for selected variables I call "CommandFactory.createMIVarCreate(IExpressionDMContext, String, String, String)" (I tried also with createMIVarEvaluateExpression()). Unfortunately this doesn't seem to do anything for the Variables view. I was able to hook an extension of GdbVariableVMProvider, yet even with that class I don't know how to tell the Variables view to show an extra variable. Any hints?

I think the contents in the Variables view are coming from MIStack.getVariableData()? Do I have to somehow extend this method?

Note that I'm only asking for our product, my goal is to restore functionality lost with the removal of CDI in our product only.
Comment 18 Jonah Graham CLA 2020-07-20 11:20:02 EDT
(In reply to Simeon Andreev from comment #17)

Is there still an open question here - I missed these comments, but I believe we discussed some of this on other forums.
Comment 19 Simeon Andreev CLA 2020-07-20 11:29:47 EDT
(In reply to Jonah Graham from comment #18)
> (In reply to Simeon Andreev from comment #17)
> 
> Is there still an open question here - I missed these comments, but I
> believe we discussed some of this on other forums.

Hi Jonah,

I was able to hook into MIStack (among others) to add more entries to the Variables view. And I used CLIInfoSharedLibraryInfo to find global variables. I.e. I was able to restore the previous CDI global variables support to our product.

So from my perspective nothing else here.

Best regards,
Simeon
Comment 20 Vinod Appu CLA 2021-01-14 02:39:45 EST
I would like to engage this issue, since our product need an easy way to see all the global variables. The discussions are not conclusive so far, I infer. I appreciate if we can conclude.
Comment 21 Jonah Graham CLA 2021-01-14 08:39:56 EST
(In reply to Vinod Appu from comment #20)
> I would like to engage this issue, since our product need an easy way to see
> all the global variables. The discussions are not conclusive so far, I
> infer. I appreciate if we can conclude.

Hi Vinod, ignoring Simeon's special use case, there has been no discussion on this in many years and none of the people involved in those old comments are still active in CDT. 

Therefore, this is an opportunity for you to come to a conclusion as to the correct thing to do. A lot of the earlier conversations were about design and optimization for the feature. However I would assume you are in the best position to know what the best design is now. 

I look forward to more discussion, or a new Gerrit.
Comment 22 Vinod Appu CLA 2021-03-04 03:18:01 EST
(In reply to Jonah Graham from comment #21)
> (In reply to Vinod Appu from comment #20)
> > I would like to engage this issue, since our product need an easy way to see
> > all the global variables. The discussions are not conclusive so far, I
> > infer. I appreciate if we can conclude.
> 
> Hi Vinod, ignoring Simeon's special use case, there has been no discussion
> on this in many years and none of the people involved in those old comments
> are still active in CDT. 
> 
> Therefore, this is an opportunity for you to come to a conclusion as to the
> correct thing to do. A lot of the earlier conversations were about design
> and optimization for the feature. However I would assume you are in the best
> position to know what the best design is now. 
> 
> I look forward to more discussion, or a new Gerrit.

I'm closing this inhouse as a new view extending VariablesView, the global variables are fetched from gdb itself, using "info variables" command. Any comments on the approach are welcome.
Comment 23 Jonah Graham CLA 2021-03-04 12:59:18 EST
(In reply to Vinod Appu from comment #22)
> > I look forward to more discussion, or a new Gerrit.
> 
> I'm closing this inhouse as a new view extending VariablesView, the global
> variables are fetched from gdb itself, using "info variables" command. Any
> comments on the approach are welcome.

Contribution to CDT potential?
Comment 24 Vinod Appu CLA 2021-03-05 05:32:01 EST
Yes, I guess. It'll take some time though since I've some tight coupling with our own plugins there, I've to remove those. Once I'm done with those, I can go for a gerrit, you can review and decide. I might need some help since it's my first attempt.
Comment 25 Jonah Graham CLA 2021-03-05 11:10:42 EST
(In reply to Vinod Appu from comment #24)
> Yes, I guess. It'll take some time though since I've some tight coupling
> with our own plugins there, I've to remove those. Once I'm done with those,
> I can go for a gerrit, you can review and decide. 

Excellent!

> I might need some help since it's my first attempt.

No problem - I am happy to help.
Comment 26 Vinod Appu CLA 2021-05-06 05:28:57 EDT
On further testing I found the following cases are not covered.

1. Same symbol in multiple files
2. C++ testing is yet to do
3. Find hangs if there are many symbols
Comment 27 Vinod Appu CLA 2021-06-04 13:00:27 EDT
Created attachment 286528 [details]
Global variable view screenshot
Comment 28 Vinod Appu CLA 2021-07-30 04:41:42 EDT
Created attachment 286858 [details]
Show full path support
Comment 29 Vinod Appu CLA 2021-07-30 13:10:08 EDT
All issues mentioned at https://bugs.eclipse.org/bugs/show_bug.cgi?id=219040#c26 except C++ testing are done. Please review UI, mentioned at https://bugs.eclipse.org/bugs/show_bug.cgi?id=219040#c28, so that I'll upload new patch set.
Comment 30 Jonah Graham CLA 2021-08-25 13:32:08 EDT
(In reply to Vinod Appu from comment #29)
> All issues mentioned at
> https://bugs.eclipse.org/bugs/show_bug.cgi?id=219040#c26 except C++ testing
> are done. Please review UI, mentioned at
> https://bugs.eclipse.org/bugs/show_bug.cgi?id=219040#c28, so that I'll
> upload new patch set.

That looks like a nice screenshot.