RFC: Skip declarations in "info variables"

RFC: Skip declarations in "info variables"

[Daniel Jacobowitz]

This has bugged me for a while, but today it caused me a test failure:
I added a reference to an otherwise unused variable in
gdb.base/included.c.  The info var integer output became:

All variables matching regular expression "integer":

File .../included.c:
static int integer;
int integer;

There's two problems here.  First, one of the copies is static.  The
dwarf2read.c portion of the patch below fixes this.  We now add
external symbols without a location to list_in_scope, in order to
correctly support:

  int x;
  {
    extern int x;
    // x here references the global
  }

But that caused us to display this as a static variable:

extern int x;

So we should special-case file scope variables.

The second problem is that integer is listed twice.  Jan pointed me to
GCC PR 37982; the file has a declaration and definition for the
variable if it is referenced (only a definition, otherwise).
I don't think we should display the declaration.  So I've changed
search_symbols not to display variables of class LOC_UNRESOLVED, and
updated the documentation to match.  While this is a change of
behavior, I don't think it's problematic.

Any comments?  OK to commit?

Tested on arm-none-eabi and x86_64-linux.

2009-11-13  Daniel Jacobowitz  <dan@codesourcery.com>

	* dwarf2read.c (new_symbol): Add file-scope external unresolved
	symbols to global_symbols.
	* symtab.c (search_symbols): Skip LOC_UNRESOLVED symbols.

	* gdb.texinfo (Symbols): "info variables" prints definitions, not
	declarations.

---
 gdb/doc/gdb.texinfo |    2 +-
 gdb/dwarf2read.c    |    9 ++++++++-
 gdb/symtab.c        |    5 ++++-
 3 files changed, 13 insertions(+), 3 deletions(-)

Index: src/gdb/dwarf2read.c
===================================================================
--- src.orig/gdb/dwarf2read.c	2009-11-13 16:22:50.000000000 -0500
+++ src/gdb/dwarf2read.c	2009-11-13 16:27:57.000000000 -0500
@@ -8441,8 +8441,15 @@ new_symbol (struct die_info *die, struct
 	      if (attr2 && (DW_UNSND (attr2) != 0)
 		  && dwarf2_attr (die, DW_AT_type, cu) != NULL)
 		{
+		  struct pending **list_to_add;
+
+		  /* A variable with DW_AT_external is never static, but it
+		     may be block-scoped.  */
+		  list_to_add = (cu->list_in_scope == &file_symbols
+				 ? &global_symbols : cu->list_in_scope);
+
 		  SYMBOL_CLASS (sym) = LOC_UNRESOLVED;
-		  add_symbol_to_list (sym, cu->list_in_scope);
+		  add_symbol_to_list (sym, list_to_add);
 		}
 	      else if (!die_is_declaration (die, cu))
 		{
Index: src/gdb/symtab.c
===================================================================
--- src.orig/gdb/symtab.c	2009-11-13 16:22:50.000000000 -0500
+++ src/gdb/symtab.c	2009-11-13 16:27:57.000000000 -0500
@@ -3234,7 +3234,9 @@ search_symbols (char *regexp, domain_enu
 		&& ((regexp == NULL
 		     || re_exec (SYMBOL_NATURAL_NAME (*psym)) != 0)
 		    && ((kind == VARIABLES_DOMAIN && SYMBOL_CLASS (*psym) != LOC_TYPEDEF
-			 && SYMBOL_CLASS (*psym) != LOC_BLOCK)
+			 && SYMBOL_CLASS (*psym) != LOC_UNRESOLVED
+			 && SYMBOL_CLASS (*psym) != LOC_BLOCK
+			 && SYMBOL_CLASS (*psym) != LOC_CONST)
 			|| (kind == FUNCTIONS_DOMAIN && SYMBOL_CLASS (*psym) == LOC_BLOCK)
 			|| (kind == TYPES_DOMAIN && SYMBOL_CLASS (*psym) == LOC_TYPEDEF))))
 	      {
@@ -3310,6 +3312,7 @@ search_symbols (char *regexp, domain_enu
 		  && ((regexp == NULL
 		       || re_exec (SYMBOL_NATURAL_NAME (sym)) != 0)
 		      && ((kind == VARIABLES_DOMAIN && SYMBOL_CLASS (sym) != LOC_TYPEDEF
+			   && SYMBOL_CLASS (sym) != LOC_UNRESOLVED
 			   && SYMBOL_CLASS (sym) != LOC_BLOCK
 			   && SYMBOL_CLASS (sym) != LOC_CONST)
 			  || (kind == FUNCTIONS_DOMAIN && SYMBOL_CLASS (sym) == LOC_BLOCK)
Index: src/gdb/doc/gdb.texinfo
===================================================================
--- src.orig/gdb/doc/gdb.texinfo	2009-11-13 16:28:20.000000000 -0500
+++ src/gdb/doc/gdb.texinfo	2009-11-13 16:28:43.000000000 -0500
@@ -12920,7 +12920,7 @@ that conflict with the regular expressio
 
 @kindex info variables
 @item info variables
-Print the names and data types of all variables that are declared
+Print the names and data types of all variables that are defined
 outside of functions (i.e.@: excluding local variables).
 
 @item info variables @var{regexp}

-- 
Daniel Jacobowitz
CodeSourcery

Re: RFC: Skip declarations in "info variables"

[Joel Brobecker]

> I don't think we should display the declaration.  So I've changed
> search_symbols not to display variables of class LOC_UNRESOLVED, and
> updated the documentation to match.  While this is a change of
> behavior, I don't think it's problematic.

I actually see this as an improvement!

-- 
Joel

Re: RFC: Skip declarations in "info variables"

[Eli Zaretskii]

> Date: Fri, 13 Nov 2009 16:44:48 -0500
> From: Daniel Jacobowitz <drow@false.org>
> 
> The second problem is that integer is listed twice.  Jan pointed me to
> GCC PR 37982; the file has a declaration and definition for the
> variable if it is referenced (only a definition, otherwise).
> I don't think we should display the declaration.  So I've changed
> search_symbols not to display variables of class LOC_UNRESOLVED, and
> updated the documentation to match.  While this is a change of
> behavior, I don't think it's problematic.

To decide whether or not this is a Good Thing, we need to answer a
question: What is the purpose of "info variables"?  More specifically,
what are the main use-cases for using it?  Can people please share
their experience with this command?

Without that, I'm not sure we are not removing a potentially useful
behavior.

Alternatively, we could add a new `info' command, or a variant of
"info variables", that would show the declared variables as well.
Then we don't need to worry about the possibility of removing useful
functionality.

> Index: src/gdb/doc/gdb.texinfo
> ===================================================================
> --- src.orig/gdb/doc/gdb.texinfo	2009-11-13 16:28:20.000000000 -0500
> +++ src/gdb/doc/gdb.texinfo	2009-11-13 16:28:43.000000000 -0500
> @@ -12920,7 +12920,7 @@ that conflict with the regular expressio
>  
>  @kindex info variables
>  @item info variables
> -Print the names and data types of all variables that are declared
> +Print the names and data types of all variables that are defined
>  outside of functions (i.e.@: excluding local variables).
>  
>  @item info variables @var{regexp}

This is an almost mechanical change, so okay.  But I think we need a
NEWS entry about the behavior change.

Re: RFC: Skip declarations in "info variables"

[Daniel Jacobowitz]

On Sat, Nov 14, 2009 at 11:35:00AM +0200, Eli Zaretskii wrote:
> To decide whether or not this is a Good Thing, we need to answer a
> question: What is the purpose of "info variables"?  More specifically,
> what are the main use-cases for using it?  Can people please share
> their experience with this command?
> 
> Without that, I'm not sure we are not removing a potentially useful
> behavior.

A very good question.

I use "info variables" to find definitions - and some information
about where they come from.  With all the declarations showing up I
can't figure out what file defined the variable, which I find
inconvenient.  Actually, I'd like it even better if I had the
memory location...

There's usually at least one definition, so the only thing the
declarations add is a list of files that (may or may not depending on
the whim of the compiler) have referenced the global variable.

Thoughts?  Anyone see a use for the declarations that I don't?

-- 
Daniel Jacobowitz
CodeSourcery

Re: RFC: Skip declarations in "info variables"

[Paul Pluzhnikov]

On Sat, Nov 14, 2009 at 7:53 AM, Daniel Jacobowitz <drow@false.org> wrote:

> I use "info variables" to find definitions ...
> I'd like it even better if I had the memory location...

Exactly my usage as well.
-- 
Paul Pluzhnikov

Re: RFC: Skip declarations in "info variables"

[Daniel Jacobowitz]

I forgot to come back to this patch.

On Sat, Nov 14, 2009 at 11:35:00AM +0200, Eli Zaretskii wrote:
> To decide whether or not this is a Good Thing, we need to answer a
> question: What is the purpose of "info variables"?  More specifically,
> what are the main use-cases for using it?  Can people please share
> their experience with this command?

Based on the feedback we received, I think definitions are the useful
part of the "info variables" output.

> This is an almost mechanical change, so okay.  But I think we need a
> NEWS entry about the behavior change.

Agreed.  How about this?

-- 
Daniel Jacobowitz
CodeSourcery

2009-12-28  Daniel Jacobowitz  <dan@codesourcery.com>

	* NEWS: Document "info variables" change.
	* dwarf2read.c (new_symbol): Add file-scope external unresolved
	symbols to global_symbols.
	* symtab.c (search_symbols): Skip LOC_UNRESOLVED symbols.

	* gdb.texinfo (Symbols): "info variables" prints definitions, not
	declarations.

---
 gdb/doc/gdb.texinfo |    2 +-
 gdb/dwarf2read.c    |    9 ++++++++-
 gdb/symtab.c        |    5 ++++-
 3 files changed, 13 insertions(+), 3 deletions(-)

Index: src/gdb/dwarf2read.c
===================================================================
--- src.orig/gdb/dwarf2read.c	2009-11-13 16:22:50.000000000 -0500
+++ src/gdb/dwarf2read.c	2009-11-13 16:27:57.000000000 -0500
@@ -8441,8 +8441,15 @@ new_symbol (struct die_info *die, struct
 	      if (attr2 && (DW_UNSND (attr2) != 0)
 		  && dwarf2_attr (die, DW_AT_type, cu) != NULL)
 		{
+		  struct pending **list_to_add;
+
+		  /* A variable with DW_AT_external is never static, but it
+		     may be block-scoped.  */
+		  list_to_add = (cu->list_in_scope == &file_symbols
+				 ? &global_symbols : cu->list_in_scope);
+
 		  SYMBOL_CLASS (sym) = LOC_UNRESOLVED;
-		  add_symbol_to_list (sym, cu->list_in_scope);
+		  add_symbol_to_list (sym, list_to_add);
 		}
 	      else if (!die_is_declaration (die, cu))
 		{
Index: src/gdb/symtab.c
===================================================================
--- src.orig/gdb/symtab.c	2009-11-13 16:22:50.000000000 -0500
+++ src/gdb/symtab.c	2009-11-13 16:27:57.000000000 -0500
@@ -3234,7 +3234,9 @@ search_symbols (char *regexp, domain_enu
 		&& ((regexp == NULL
 		     || re_exec (SYMBOL_NATURAL_NAME (*psym)) != 0)
 		    && ((kind == VARIABLES_DOMAIN && SYMBOL_CLASS (*psym) != LOC_TYPEDEF
-			 && SYMBOL_CLASS (*psym) != LOC_BLOCK)
+			 && SYMBOL_CLASS (*psym) != LOC_UNRESOLVED
+			 && SYMBOL_CLASS (*psym) != LOC_BLOCK
+			 && SYMBOL_CLASS (*psym) != LOC_CONST)
 			|| (kind == FUNCTIONS_DOMAIN && SYMBOL_CLASS (*psym) == LOC_BLOCK)
 			|| (kind == TYPES_DOMAIN && SYMBOL_CLASS (*psym) == LOC_TYPEDEF))))
 	      {
@@ -3310,6 +3312,7 @@ search_symbols (char *regexp, domain_enu
 		  && ((regexp == NULL
 		       || re_exec (SYMBOL_NATURAL_NAME (sym)) != 0)
 		      && ((kind == VARIABLES_DOMAIN && SYMBOL_CLASS (sym) != LOC_TYPEDEF
+			   && SYMBOL_CLASS (sym) != LOC_UNRESOLVED
 			   && SYMBOL_CLASS (sym) != LOC_BLOCK
 			   && SYMBOL_CLASS (sym) != LOC_CONST)
 			  || (kind == FUNCTIONS_DOMAIN && SYMBOL_CLASS (sym) == LOC_BLOCK)
Index: src/gdb/doc/gdb.texinfo
===================================================================
--- src.orig/gdb/doc/gdb.texinfo	2009-11-13 16:28:20.000000000 -0500
+++ src/gdb/doc/gdb.texinfo	2009-11-13 16:28:43.000000000 -0500
@@ -12920,7 +12920,7 @@ that conflict with the regular expressio
 
 @kindex info variables
 @item info variables
-Print the names and data types of all variables that are declared
+Print the names and data types of all variables that are defined
 outside of functions (i.e.@: excluding local variables).
 
 @item info variables @var{regexp}
Index: NEWS
===================================================================
RCS file: /cvs/src/src/gdb/NEWS,v
retrieving revision 1.340
diff -u -p -r1.340 NEWS
--- NEWS	8 Dec 2009 00:17:45 -0000	1.340
+++ NEWS	28 Dec 2009 21:04:33 -0000
@@ -30,6 +30,10 @@ disassemble
   The disassemble command, when invoked with two arguments, now requires
   the arguments to be comma-separated.
 
+info variables
+  The info variables command now displays variable definitions.  Files
+  which only declare a variable are not shown.
+
 * New commands (for set/show, see "New options" below)
 
 record save [<FILENAME>]

Re: RFC: Skip declarations in "info variables"

[Eli Zaretskii]

> Date: Mon, 28 Dec 2009 16:05:26 -0500
> From: Daniel Jacobowitz <drow@false.org>
> Cc: gdb-patches@sourceware.org
> 
> > This is an almost mechanical change, so okay.  But I think we need a
> > NEWS entry about the behavior change.
> 
> Agreed.  How about this?

OK.  Thanks.

Re: RFC: Skip declarations in "info variables"

[Daniel Jacobowitz]

On Mon, Dec 28, 2009 at 11:23:16PM +0200, Eli Zaretskii wrote:
> OK.  Thanks.

Thanks!  Checked in.

-- 
Daniel Jacobowitz
CodeSourcery