[v3,1/2] gdb: add annotation in 'info locals' command for variables shadowing case

  From: "Ijaz, Abdul B" <abdul.b.ijaz@intel.com>

For C/C++/Fortran/Ada languages GDB prints same name variable multiple
times in case of variable shadowing and it is confusing for user to identify
which variable belongs to the current scope.  So for such cases add location
info to the innermost listed variables and for super block variables add
"shadowed" annotation in the form of "<file.c:line, shadowed>".

Suppose we have

1:int x = 42;
2:  {
3:    int x = 99;
4:    int y = 52;
5:    x = 99; /* break here */
6:  }

Currently:

(gdb) info locals
x = 99
x = 42
y = 52

After applying this patch, we obtain:

(gdb) info locals
x = 99  <file.c:3>
y = 52
x = 42  <file.c:1, shadowed>

The patch adds the location annotations by keeping track of inner block
and already printed variables to identify shadowing.  So, GDB now prints
"<file.c:line, shadowed>" for shadowed super-block variables and
"<file.c:line>" for innermost declarations of such variables only.

The location annotations are printed for shadowed variables in case of
C/C++/Fortran/Ada languages.  In Rust, it is possible to declare a
variable with the same name many times.  So in this case, just the first
instance of the variable is printed.
---
 gdb/doc/gdb.texinfo                           | 13 +++
 gdb/printcmd.c                                | 14 ++-
 gdb/stack.c                                   | 67 ++++++++++++--
 gdb/stack.h                                   |  3 +-
 gdb/testsuite/gdb.ada/var_shadowing.exp       | 38 ++++++++
 .../gdb.ada/var_shadowing/var_shadowing.adb   | 30 +++++++
 gdb/testsuite/gdb.base/var-shadowing.c        | 49 ++++++++++
 gdb/testsuite/gdb.base/var-shadowing.exp      | 90 +++++++++++++++++++
 gdb/testsuite/gdb.base/var-shadowing2.c       | 16 ++++
 gdb/testsuite/gdb.rust/var_reuse.exp          | 32 +++++++
 gdb/testsuite/gdb.rust/var_reuse.rs           | 20 +++++
 gdb/tracepoint.c                              |  3 +-
 gdb/value.h                                   |  4 +-
 13 files changed, 368 insertions(+), 11 deletions(-)
 create mode 100644 gdb/testsuite/gdb.ada/var_shadowing.exp
 create mode 100644 gdb/testsuite/gdb.ada/var_shadowing/var_shadowing.adb
 create mode 100755 gdb/testsuite/gdb.base/var-shadowing.c
 create mode 100755 gdb/testsuite/gdb.base/var-shadowing.exp
 create mode 100644 gdb/testsuite/gdb.base/var-shadowing2.c
 create mode 100755 gdb/testsuite/gdb.rust/var_reuse.exp
 create mode 100755 gdb/testsuite/gdb.rust/var_reuse.rs
  

Hi Abdul,

Thanks for working on this.  I have included some comments on the patch.

> diff --git a/gdb/printcmd.c b/gdb/printcmd.c
> index 8d7d04231fe..17e385138d2 100644
> --- a/gdb/printcmd.c
> +++ b/gdb/printcmd.c
> @@ -2401,7 +2401,8 @@ clear_dangling_display_expressions (struct objfile *objfile)
>  void
>  print_variable_and_value (const char *name, struct symbol *var,
>  			  frame_info_ptr frame,
> -			  struct ui_file *stream, int indent)
> +			  struct ui_file *stream, int indent,
> +			  bool shadowed, bool printed)
>  {
>  
>    if (!name)
> @@ -2414,6 +2415,11 @@ print_variable_and_value (const char *name, struct symbol *var,
>      {
>        struct value *val;
>        struct value_print_options opts;
> +      std::string file_path = var->owner.symtab->filename;
> +      size_t slash_index = file_path.find_last_of ("\\/");

I think you might prefer to use lbasename (include/libiberty.h) as I
guess this search can fail.  For example, "\" can in a filename on Linux
(not saying it is a wise thing to do).

  const char *file_name = lbasename (var->owner.symtab->filename);

> +      std::string file_name = (!file_path.empty ()
> +			       ? file_path.substr (slash_index + 1)
> +			       : "NA");

As for the "NA" case, I am not sure when it can happen.  If the symtab's
filename can be nullptr (or start with \0), a test might be necessary.

>  
>        /* READ_VAR_VALUE needs a block in order to deal with non-local
>  	 references (i.e. to handle nested functions).  In this context, we
> @@ -2424,6 +2430,12 @@ print_variable_and_value (const char *name, struct symbol *var,
>        opts.deref_ref = true;
>        common_val_print_checked (val, stream, indent, &opts, current_language);
>  
> +      if (shadowed)
> +	/* Print location and shadowed variable information.  */
> +	fprintf_styled (stream, metadata_style.style (),
> +			_("\t<%s:%d%s>"), file_name.c_str (),
> +			var->line (), printed ? ", shadowed" : "");
> +
>        /* common_val_print invalidates FRAME when a pretty printer calls inferior
>  	 function.  */
>        frame = NULL;
> diff --git a/gdb/stack.c b/gdb/stack.c
> index 0b35d62f82f..f0f384c6270 100644
> --- a/gdb/stack.c
> +++ b/gdb/stack.c
> @@ -56,6 +56,7 @@
>  #include "cli/cli-option.h"
>  #include "cli/cli-style.h"
>  #include "gdbsupport/buildargv.h"
> +#include <unordered_set>
>  
>  /* The possible choices of "set print frame-arguments", and the value
>     of this setting.  */
> @@ -2211,10 +2212,18 @@ backtrace_command_completer (struct cmd_list_element *ignore,
>  
>  static void
>  iterate_over_block_locals (const struct block *b,
> -			   iterate_over_block_arg_local_vars_cb cb)
> +			   iterate_over_block_arg_local_vars_cb cb,
> +			   std::unordered_set<std::string> shadowed_vars,

This shadowed_vars parameter is only searched, not modified.  Did you
mean to pass it by const ref?  Otherwise, you end-up copying the entire
set.

> +			   std::unordered_set<std::string> &printed_vars)
>  {
>    for (struct symbol *sym : block_iterator_range (b))
>      {
> +      const char *name = sym->print_name ();
> +      bool already_printed
> +	= printed_vars.find (name) != printed_vars.end ();
> +      printed_vars.insert (name);

I think you can simplify this a bit by using
std::unordered_set<T>::insert's return value.  It is a pair whose second
element is a bool indicating if the insertion took place.  If false, it
means that the element was already in the set.

    bool already_printed = !printed_vars.insert (name).second;

> +      bool shadowed = shadowed_vars.find (name) != shadowed_vars.end ();
> +
>        switch (sym->aclass ())
>  	{
>  	case LOC_CONST:
> @@ -2227,7 +2236,27 @@ iterate_over_block_locals (const struct block *b,
>  	    break;
>  	  if (sym->domain () == COMMON_BLOCK_DOMAIN)
>  	    break;
> -	  cb (sym->print_name (), sym);
> +	  /* Only for C/C++/Fortran/Ada languages, in case of variables
> +	     shadowing print <file:line, shadowed> annotation after
> +	     the superblock variable.  Iteration of block starts from inner
> +	     block which is printed only with location information.  */
> +	  if ((current_language->la_language == language_c
> +	       || current_language->la_language == language_cplus
> +	       || current_language->la_language == language_fortran
> +	       || current_language->la_language == language_ada)
> +	      && shadowed)
> +	    cb (name, sym, true, already_printed);
> +	  /* In case of Rust language it is possible to declare variable with
> +	     same name multiple times and only latest declaration of variable
> +	     is accessible.  So print only the first instance and there is no
> +	     need of printing duplicates.  */
> +	  else if (current_language->la_language == language_rust
> +		   && shadowed && already_printed)
> +	    break;
> +	  else
> +	    {
> +	      cb (name, sym, false, false);
> +	    }

As there is just one line in the else, no need for the { }.

>  	  break;
>  
>  	default:
> @@ -2244,9 +2273,30 @@ void
>  iterate_over_block_local_vars (const struct block *block,
>  			       iterate_over_block_arg_local_vars_cb cb)
>  {
> +  std::unordered_set<std::string> collected_vars, shadowed_vars, printed_vars;
> +  const struct block *orig_block = block;
> +
> +  /* Iterate over all the local variables in a block and store the list of
> +     shadowed variables to later distinguish them from other variables.  */
> +  while (block)

According to the GDB coding style, this should be

  while (block != nullptr)

> +    {
> +      for (struct symbol *sym : block_iterator_range (block))
> +	{
> +	  const char *name = sym->print_name ();
> +	  if (collected_vars.find (name) != collected_vars.end ())
> +	    shadowed_vars.insert (name);
> +	  else
> +	    collected_vars.insert (name);

Similarly, this could be

  if (!collected_vars.insert (name).second)
    shadowed_vars.insert (name);

> +	}
> +	if (block->function ())
> +	  break;
> +	block = block->superblock ();
> +    }
> +
> +  block = orig_block;
>    while (block)
>      {
> -      iterate_over_block_locals (block, cb);
> +      iterate_over_block_locals (block, cb, shadowed_vars, printed_vars);
>        /* After handling the function's top-level block, stop.  Don't
>  	 continue to its superblock, the block of per-file
>  	 symbols.  */
> diff --git a/gdb/testsuite/gdb.base/var-shadowing.c b/gdb/testsuite/gdb.base/var-shadowing.c
> new file mode 100755
> index 00000000000..00597b402ba
> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/var-shadowing.c
> @@ -0,0 +1,49 @@
> +/* Copyright (C) 2023 Free Software Foundation, Inc.
> +
> +   This program is free software; you can redistribute it and/or modify
> +   it under the terms of the GNU General Public License as published by
> +   the Free Software Foundation; either version 3 of the License, or
> +   (at your option) any later version.
> +
> +   This program is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +   GNU General Public License for more details.
> +
> +   You should have received a copy of the GNU General Public License
> +   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
> +
> +#include <stdlib.h>
> +
> +void
> +shadowing ()
              ^
This should be

void
shadowing (void)

> +{
> +  int a;  /* bp for entry */
> +  unsigned int val1 = 1;		/* val1-d1 */
> +  unsigned int val2 = 2;		/* val2-d1 */
> +  a = 101;  /* bp for locals 1 */
> +  {
> +    unsigned int val2 = 3;		/* val2-d2 */
> +    unsigned int val3 = 4;		/* val3-d1 */
> +    a = 102;  /* bp for locals 2 */
> +    {
> +      unsigned int val1 = 5;		/* val1-d2 */
> +      a = 103;  /* bp for locals 3 */
> +      {
> +	#include "var-shadowing2.c"
> +	unsigned int val1 = 6;	/* val1-d3 */
> +	unsigned int val2 = 7;	/* val2-d3 */
> +	unsigned int val3 = 8;	/* val3-d2 */
> +	a = 104;  /* bp for locals 4 */
> +      }
> +    }
> +  }
> +  a = 0; /* bp for locals 5 */
> +}
> +
> +int
> +main (void)
> +{
> +  shadowing ();
> +  return 0;
> +}

Best,
Lancelot.
  

Hi Lancelot,

Thanks a lot for your feedback and pointing out the improvements. All these will be fixed in V4 patch series which I will try to test and push for review today.

> > +      size_t slash_index = file_path.find_last_of ("\\/");

> I think you might prefer to use lbasename (include/libiberty.h) as I guess this search can fail.  
This is helpful thanks for pointing to this header file.  Will be updated in Patch v4.

>> +      std::string file_name = (!file_path.empty ()
>> +			       ? file_path.substr (slash_index + 1)
>> +			       : "NA");
> As for the "NA" case, I am not sure when it can happen.  If the symtab's filename can be nullptr (or start with \0), a test might be necessary.
Will not be using NA anymore in Patch v4 as I am also not aware of NA case.

>> +			   std::unordered_set<std::string> shadowed_vars,

> This shadowed_vars parameter is only searched, not modified.  Did you mean to pass it by const ref?  Otherwise, you end-up copying the entire set.
Using const and by reference would be right here so will be added in V4 patch.

>> +      printed_vars.insert (name);

>I think you can simplify this a bit by using std::unordered_set<T>::insert's return value.  It is a pair whose second element is a bool indicating if the insertion took place.  If false, it means that the element was already in the set.

  >  bool already_printed = !printed_vars.insert (name).second;
Will be updated in v4 Patch.

>> +	  else
>> +	    {
>> +	      cb (name, sym, false, false);
>> +	    }

>As there is just one line in the else, no need for the { }.
Removing {} in Patch v4.

>>+  while (block)

>According to the GDB coding style, this should be

>  while (block != nullptr)
Updating in V4 patch.

>> +	    collected_vars.insert (name);

>Similarly, this could be

>  if (!collected_vars.insert (name).second)
 >   shadowed_vars.insert (name);
Fixed in V4 patch.

>> +void
>> +shadowing ()
 >             ^
>This should be

>void
>shadowing (void)
Fixing in V4 patch.

Thanks & Best Regards
Abdul Basit

-----Original Message-----
From: Lancelot SIX <lsix@lancelotsix.com> 
Sent: Tuesday, September 19, 2023 12:46 AM
To: Ijaz, Abdul B <abdul.b.ijaz@intel.com>
Cc: gdb-patches@sourceware.org; pedro@palves.net
Subject: Re: [PATCH v3 1/2] gdb: add annotation in 'info locals' command for variables shadowing case

Hi Abdul,

Thanks for working on this.  I have included some comments on the patch.

> diff --git a/gdb/printcmd.c b/gdb/printcmd.c index 
> 8d7d04231fe..17e385138d2 100644
> --- a/gdb/printcmd.c
> +++ b/gdb/printcmd.c
> @@ -2401,7 +2401,8 @@ clear_dangling_display_expressions (struct 
> objfile *objfile)  void  print_variable_and_value (const char *name, 
> struct symbol *var,
>  			  frame_info_ptr frame,
> -			  struct ui_file *stream, int indent)
> +			  struct ui_file *stream, int indent,
> +			  bool shadowed, bool printed)
>  {
>  
>    if (!name)
> @@ -2414,6 +2415,11 @@ print_variable_and_value (const char *name, struct symbol *var,
>      {
>        struct value *val;
>        struct value_print_options opts;
> +      std::string file_path = var->owner.symtab->filename;
> +      size_t slash_index = file_path.find_last_of ("\\/");

I think you might prefer to use lbasename (include/libiberty.h) as I guess this search can fail.  For example, "\" can in a filename on Linux (not saying it is a wise thing to do).

  const char *file_name = lbasename (var->owner.symtab->filename);

> +      std::string file_name = (!file_path.empty ()
> +			       ? file_path.substr (slash_index + 1)
> +			       : "NA");

As for the "NA" case, I am not sure when it can happen.  If the symtab's filename can be nullptr (or start with \0), a test might be necessary.

>  
>        /* READ_VAR_VALUE needs a block in order to deal with non-local
>  	 references (i.e. to handle nested functions).  In this context, we 
> @@ -2424,6 +2430,12 @@ print_variable_and_value (const char *name, struct symbol *var,
>        opts.deref_ref = true;
>        common_val_print_checked (val, stream, indent, &opts, 
> current_language);
>  
> +      if (shadowed)
> +	/* Print location and shadowed variable information.  */
> +	fprintf_styled (stream, metadata_style.style (),
> +			_("\t<%s:%d%s>"), file_name.c_str (),
> +			var->line (), printed ? ", shadowed" : "");
> +
>        /* common_val_print invalidates FRAME when a pretty printer calls inferior
>  	 function.  */
>        frame = NULL;
> diff --git a/gdb/stack.c b/gdb/stack.c index 0b35d62f82f..f0f384c6270 
> 100644
> --- a/gdb/stack.c
> +++ b/gdb/stack.c
> @@ -56,6 +56,7 @@
>  #include "cli/cli-option.h"
>  #include "cli/cli-style.h"
>  #include "gdbsupport/buildargv.h"
> +#include <unordered_set>
>  
>  /* The possible choices of "set print frame-arguments", and the value
>     of this setting.  */
> @@ -2211,10 +2212,18 @@ backtrace_command_completer (struct 
> cmd_list_element *ignore,
>  
>  static void
>  iterate_over_block_locals (const struct block *b,
> -			   iterate_over_block_arg_local_vars_cb cb)
> +			   iterate_over_block_arg_local_vars_cb cb,
> +			   std::unordered_set<std::string> shadowed_vars,

This shadowed_vars parameter is only searched, not modified.  Did you mean to pass it by const ref?  Otherwise, you end-up copying the entire set.

> +			   std::unordered_set<std::string> &printed_vars)
>  {
>    for (struct symbol *sym : block_iterator_range (b))
>      {
> +      const char *name = sym->print_name ();
> +      bool already_printed
> +	= printed_vars.find (name) != printed_vars.end ();
> +      printed_vars.insert (name);

I think you can simplify this a bit by using std::unordered_set<T>::insert's return value.  It is a pair whose second element is a bool indicating if the insertion took place.  If false, it means that the element was already in the set.

    bool already_printed = !printed_vars.insert (name).second;

> +      bool shadowed = shadowed_vars.find (name) != shadowed_vars.end 
> + ();
> +
>        switch (sym->aclass ())
>  	{
>  	case LOC_CONST:
> @@ -2227,7 +2236,27 @@ iterate_over_block_locals (const struct block *b,
>  	    break;
>  	  if (sym->domain () == COMMON_BLOCK_DOMAIN)
>  	    break;
> -	  cb (sym->print_name (), sym);
> +	  /* Only for C/C++/Fortran/Ada languages, in case of variables
> +	     shadowing print <file:line, shadowed> annotation after
> +	     the superblock variable.  Iteration of block starts from inner
> +	     block which is printed only with location information.  */
> +	  if ((current_language->la_language == language_c
> +	       || current_language->la_language == language_cplus
> +	       || current_language->la_language == language_fortran
> +	       || current_language->la_language == language_ada)
> +	      && shadowed)
> +	    cb (name, sym, true, already_printed);
> +	  /* In case of Rust language it is possible to declare variable with
> +	     same name multiple times and only latest declaration of variable
> +	     is accessible.  So print only the first instance and there is no
> +	     need of printing duplicates.  */
> +	  else if (current_language->la_language == language_rust
> +		   && shadowed && already_printed)
> +	    break;
> +	  else
> +	    {
> +	      cb (name, sym, false, false);
> +	    }

As there is just one line in the else, no need for the { }.

>  	  break;
>  
>  	default:
> @@ -2244,9 +2273,30 @@ void
>  iterate_over_block_local_vars (const struct block *block,
>  			       iterate_over_block_arg_local_vars_cb cb)  {
> +  std::unordered_set<std::string> collected_vars, shadowed_vars, 
> + printed_vars;  const struct block *orig_block = block;
> +
> +  /* Iterate over all the local variables in a block and store the list of
> +     shadowed variables to later distinguish them from other 
> + variables.  */  while (block)

According to the GDB coding style, this should be

  while (block != nullptr)

> +    {
> +      for (struct symbol *sym : block_iterator_range (block))
> +	{
> +	  const char *name = sym->print_name ();
> +	  if (collected_vars.find (name) != collected_vars.end ())
> +	    shadowed_vars.insert (name);
> +	  else
> +	    collected_vars.insert (name);

Similarly, this could be

  if (!collected_vars.insert (name).second)
    shadowed_vars.insert (name);

> +	}
> +	if (block->function ())
> +	  break;
> +	block = block->superblock ();
> +    }
> +
> +  block = orig_block;
>    while (block)
>      {
> -      iterate_over_block_locals (block, cb);
> +      iterate_over_block_locals (block, cb, shadowed_vars, 
> + printed_vars);
>        /* After handling the function's top-level block, stop.  Don't
>  	 continue to its superblock, the block of per-file
>  	 symbols.  */
> diff --git a/gdb/testsuite/gdb.base/var-shadowing.c 
> b/gdb/testsuite/gdb.base/var-shadowing.c
> new file mode 100755
> index 00000000000..00597b402ba
> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/var-shadowing.c
> @@ -0,0 +1,49 @@
> +/* Copyright (C) 2023 Free Software Foundation, Inc.
> +
> +   This program is free software; you can redistribute it and/or modify
> +   it under the terms of the GNU General Public License as published by
> +   the Free Software Foundation; either version 3 of the License, or
> +   (at your option) any later version.
> +
> +   This program is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +   GNU General Public License for more details.
> +
> +   You should have received a copy of the GNU General Public License
> +   along with this program.  If not, see 
> + <http://www.gnu.org/licenses/>.  */
> +
> +#include <stdlib.h>
> +
> +void
> +shadowing ()
              ^
This should be

void
shadowing (void)

> +{
> +  int a;  /* bp for entry */
> +  unsigned int val1 = 1;		/* val1-d1 */
> +  unsigned int val2 = 2;		/* val2-d1 */
> +  a = 101;  /* bp for locals 1 */
> +  {
> +    unsigned int val2 = 3;		/* val2-d2 */
> +    unsigned int val3 = 4;		/* val3-d1 */
> +    a = 102;  /* bp for locals 2 */
> +    {
> +      unsigned int val1 = 5;		/* val1-d2 */
> +      a = 103;  /* bp for locals 3 */
> +      {
> +	#include "var-shadowing2.c"
> +	unsigned int val1 = 6;	/* val1-d3 */
> +	unsigned int val2 = 7;	/* val2-d3 */
> +	unsigned int val3 = 8;	/* val3-d2 */
> +	a = 104;  /* bp for locals 4 */
> +      }
> +    }
> +  }
> +  a = 0; /* bp for locals 5 */
> +}
> +
> +int
> +main (void)
> +{
> +  shadowing ();
> +  return 0;
> +}

Best,
Lancelot.
Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de <http://www.intel.de>
Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva  
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928