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

["Ijaz, Abdul B" <abdul.b.ijaz@intel.com>]

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