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

[Andrew Burgess <aburgess@redhat.com>]

Abdul Basit Ijaz <abdul.b.ijaz@intel.com> writes:

> 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>

Not really a significant issue, but I see you changed the docs, so maybe
this should be updated too.  You initialise X to 99, then break on a
line that also sets X to 99.  So clearly X will have the value 99, but
in the docs the example is I think clearer, you initialise X to 4, then
break on the line that sets X to 99.  As expected, X has the value 4
indicating that the breakpoint line has not yet run.  Like I say, it's a
trivial inconsistency, but as I have some real changes below, you might
one to sync the commit message and docs?

>
> 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.  RUST language test "var_reuse.exp"
> fails with rustc compiler version >= 1.73 so XFAIL is added accordingly.
>
> Fix regex expression in the gdb.opt/inline-locals.exp test according to
> this change.  The test update is only required due to the existing gdb
> known ticket gdb/25695 where this issue is seen with 7.5.0 version on
> sles15sp6 but it is not seen anymore on the newer gcc versions e.g.
> gcc-11.4.0.
>
> The symtab()/filename() nullptr check was added specifically to avoid
> the crash seen in gdb.dwarf2/missing-type-name-for-templates.exp where
> template symbols may have no associated source file.
>
> Reviewed-By: Guinevere Larsen <guinevere@redhat.com>
> Reviewed-By: Eli Zaretskii <eliz@gnu.org>
> Co-Authored-By: Andrew Burgess <aburgess@redhat.com>
> ---
>  gdb/NEWS                                      |   4 +
>  gdb/c-typeprint.c                             |   3 +-
>  gdb/doc/gdb.texinfo                           |  26 ++++
>  gdb/language.c                                |  20 +++
>  gdb/language.h                                |  19 +++
>  gdb/printcmd.c                                |  38 +++++-
>  gdb/stack.c                                   | 115 ++++++++++++++++--
>  gdb/stack.h                                   |  14 +++
>  gdb/testsuite/gdb.ada/var_shadowing.exp       |  39 ++++++
>  .../gdb.ada/var_shadowing/var_shadowing.adb   |  30 +++++
>  gdb/testsuite/gdb.base/var-shadowing.c        |  51 ++++++++
>  gdb/testsuite/gdb.base/var-shadowing.exp      |  92 ++++++++++++++
>  gdb/testsuite/gdb.base/var-shadowing2.c       |  16 +++
>  gdb/testsuite/gdb.opt/inline-locals.exp       |  21 ++--
>  gdb/testsuite/gdb.rust/var_reuse.exp          |  36 ++++++
>  gdb/testsuite/gdb.rust/var_reuse.rs           |  20 +++
>  gdb/value.h                                   |  26 +++-
>  17 files changed, 547 insertions(+), 23 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
>
> diff --git a/gdb/NEWS b/gdb/NEWS
> index ec9b5a33787..6807a6c966b 100644
> --- a/gdb/NEWS
> +++ b/gdb/NEWS
> @@ -201,6 +201,10 @@ New command class for help
>    commands that we, as developers, believe would be close to a minimal
>    set of commands for a new user of GDB.
>  
> +info locals
> +  GDB now shows the "shadowed" annotation and the location information in
> +  the output of this command for variables shadowing case.

The wording here seems a little off, how about this instead:

  GDB now shows the "shadowed" annotation and the location information
  for variables that are shadowed, or which are shadowing.

> +
>  * Removed commands
>  
>  target ctf
> diff --git a/gdb/c-typeprint.c b/gdb/c-typeprint.c
> index e418aca56a4..9397160d29b 100644
> --- a/gdb/c-typeprint.c
> +++ b/gdb/c-typeprint.c
> @@ -818,7 +818,8 @@ c_type_print_template_args (const struct type_print_options *flags,
>        if (sym->loc_class () == LOC_TYPEDEF)
>  	c_print_type (sym->type (), "", stream, -1, 0, language, flags);
>        else
> -	print_variable_value (sym, {}, stream, 0, language_def (language));
> +	print_variable_value (sym, {}, stream, 0, language_def (language),
> +			      var_shadowing::NONE);
>      }
>  
>    gdb_puts (_("] "), stream);
> diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
> index a698b2b8451..ee33bfdba13 100644
> --- a/gdb/doc/gdb.texinfo
> +++ b/gdb/doc/gdb.texinfo
> @@ -9188,6 +9188,32 @@ The optional flag @samp{-q}, which stands for @samp{quiet}, disables
>  printing header information and messages explaining why no local variables
>  have been printed.
>  
> +@smallexample
> +@group
> +1: int x = 3;
> +2: @{
> +3:       int x = 4;
> +4:       int y = 52;
> +5:       x = 99; // breakpoint-line
> +6: @}
> +@end group
> +@group
> +(gdb) info locals
> +x = 4	<file.c:3>
> +y = 52
> +x = 3	<file.c:1, shadowed>
> +@end group
> +@end smallexample
> +
> +@anchor{shadowed variables}
> +@cindex shadowed variables
> +A variable is @dfn{shadowed} when there's another variable with the
> +same name which is declared within an inner scope (decision block,
> +method, or inner class).  For such cases, its location for the
> +outermost scope is followed by @samp{shadowed}.  The location can
> +help to locate the instances of shadowed variables.  So,
> +location information is only added for shadowed variables.
> +
>  @item info locals [-q] [-t @var{type_regexp}] [@var{regexp}]
>  Like @kbd{info locals}, but only print the local variables selected
>  with the provided regexp(s).
> diff --git a/gdb/language.c b/gdb/language.c
> index 3e3be66a676..972018e8f7b 100644
> --- a/gdb/language.c
> +++ b/gdb/language.c
> @@ -1090,6 +1090,26 @@ language_lookup_primitive_type_as_symbol (const struct language_defn *la,
>    return sym;
>  }
>  
> +/* See language.h.  */
> +
> +lang_vars_shadowing get_lang_vars_shadowing_option (enum language lang)
> +{
> +  switch (lang)
> +    {
> +    case language_c:
> +    case language_cplus:
> +    case language_fortran:
> +    case language_ada:
> +      return lang_vars_shadowing::PRINT;
> +
> +    case language_rust:
> +      return lang_vars_shadowing::HIDE;
> +
> +    default:
> +      return lang_vars_shadowing::NONE;
> +    }
> +}
> +
>  /* Initialize the language routines.  */
>  
>  INIT_GDB_FILE (language)
> diff --git a/gdb/language.h b/gdb/language.h
> index 75154d9c591..d838cc825a1 100644
> --- a/gdb/language.h
> +++ b/gdb/language.h
> @@ -83,6 +83,21 @@ enum macro_expansion
>      macro_expansion_no, macro_expansion_c
>    };
>  
> +/* In the case of variable shadowing if extra information should be
> +   printed for the current language is compared against it.  */

This comment doesn't read well.  How about:

  How should shadowed, or shadowing, variables be printed.

> +
> +enum class lang_vars_shadowing
> +  {
> +    /* Adds shadowed information for such variables.  */
> +    PRINT,
> +
> +    /* Does not print shadowed variables.  */
> +    HIDE,
> +
> +    /* Print variables without shadow information.  */
> +    NONE,
> +  };
> +
>  \f
>  /* Per architecture (OS/ABI) language information.  */
>  
> @@ -801,6 +816,10 @@ void c_get_string (struct value *value,
>  symbol_name_matcher_ftype *get_symbol_name_matcher
>    (const language_defn *lang, const lookup_name_info &lookup_name);
>  
> +/* Returns the shadowing option supported for the input language.  */
> +
> +extern lang_vars_shadowing get_lang_vars_shadowing_option (enum language lang);
> +
>  /* Save the current language and restore it upon destruction.  */
>  
>  class scoped_restore_current_language
> diff --git a/gdb/printcmd.c b/gdb/printcmd.c
> index a337a6b7db9..b75a0d06f0f 100644
> --- a/gdb/printcmd.c
> +++ b/gdb/printcmd.c
> @@ -2377,12 +2377,17 @@ clear_dangling_display_expressions (struct objfile *objfile)
>  void
>  print_variable_value (symbol *var, const frame_info_ptr &frame,
>  		      ui_file *stream, int indent,
> -		      const language_defn *language)
> +		      const language_defn *language,
> +		      var_shadowing shadow_status)
>  {
>    try
>      {
>        struct value *val;
>        struct value_print_options opts;
> +      const char *file_name = nullptr;
> +
> +      if (var->symtab () && var->symtab ()->filename ())

GDB style is not to use pointers as booleans, so this should be:

  if (var->symtab () != nullptr && ....)

Couldn't this whole FILE_NAME calculation logic be moved down inside of
the `if (shadow_status != var_shadowing::NONE)` block?  It's only used
in that block, but see the comment below too...

> +	file_name = lbasename (var->symtab ()->filename ());
>  
>        /* READ_VAR_VALUE needs a block in order to deal with non-local
>  	 references (i.e. to handle nested functions).  In this context, we
> @@ -2392,6 +2397,31 @@ print_variable_value (symbol *var, const frame_info_ptr &frame,
>        get_user_print_options (&opts);
>        opts.deref_ref = true;
>        common_val_print_checked (val, stream, indent, &opts, language);
> +
> +      /* Print <%line, shadowed> after the variable value only when it is variable
> +	 shadowing case.  */
> +      if (shadow_status != var_shadowing::NONE)
> +	{
> +	  bool printed = (shadow_status == var_shadowing::SHADOWED);
> +	  string_file out (current_uiout->can_emit_style_escape ());
> +
> +	  gdb_printf (&out, "\t<%ps:",
> +		      styled_string (file_name_style.style (), file_name));

... what if FILE_NAME is NULL at this point?  Is there a reason why we
cannot have both a missing symtab AND shadowing going on?

> +
> +	  if (var->line () > 0)
> +	    gdb_printf (&out, "%ps",
> +			styled_string (line_number_style.style (),
> +				       pulongest (var->line ())));
> +	  else
> +	    gdb_puts ("No line number information available", &out);
> +
> +	  gdb_printf (&out, "%ps",
> +		      styled_string (metadata_style.style (),
> +				     printed ? ", shadowed" : ""));

This will also style the ', ' text, I think what you should write is:

	  if (printed)
	    {
	      gdb_puts (", ", &out);
	      fputs_styled ("shadowed", metadata_style.style (), &out);
	    }

> +	  gdb_puts (">", &out);
> +
> +	  gdb_puts (out.c_str ());

Other output from this function is sent to STREAM.  I think this line
should be:

  gdb_puts (out, stream);

> +	}
>      }
>    catch (const gdb_exception_error &except)
>      {
> @@ -2406,7 +2436,8 @@ print_variable_value (symbol *var, const frame_info_ptr &frame,
>  void
>  print_variable_and_value (const char *name, symbol *var,
>  			  const frame_info_ptr &frame,
> -			  ui_file *stream, int indent)
> +			  ui_file *stream, int indent,
> +			  var_shadowing shadow_status)
>  {
>    if (name == nullptr)
>      name = var->print_name ();
> @@ -2414,7 +2445,8 @@ print_variable_and_value (const char *name, symbol *var,
>    gdb_printf (stream, "%*s%ps = ", 2 * indent, "",
>  	      styled_string (variable_name_style.style (), name));
>  
> -  print_variable_value (var, frame, stream, indent, current_language);
> +  print_variable_value (var, frame, stream, indent, current_language,
> +			shadow_status);
>  
>    gdb_printf (stream, "\n");
>  }
> diff --git a/gdb/stack.c b/gdb/stack.c
> index e084976eabf..79aa36a47e4 100644
> --- a/gdb/stack.c
> +++ b/gdb/stack.c
> @@ -59,6 +59,7 @@
>  #include "cli/cli-option.h"
>  #include "cli/cli-style.h"
>  #include "gdbsupport/buildargv.h"
> +#include "gdbsupport/unordered_set.h"
>  
>  /* The possible choices of "set print frame-arguments", and the value
>     of this setting.  */
> @@ -2133,8 +2134,9 @@ backtrace_command_completer (struct cmd_list_element *ignore,
>  /* Iterate over the local variables of a block B, calling CB.  */
>  
>  static void
> -iterate_over_block_locals (const struct block *b,
> -			   iterate_over_block_arg_local_vars_cb cb)
> +iterate_over_block_locals
> +  (const struct block *b,
> +   gdb::function_view <void (const char *, struct symbol *)> cb)
>  {
>    for (struct symbol *sym : block_iterator_range (b))
>      {
> @@ -2180,6 +2182,71 @@ iterate_over_block_local_vars (const struct block *block,
>      }
>  }
>  
> +/* Iterate over all the local variables in block B, including all its
> +   superblocks, stopping when the top-level block is reached.  */

This comment should be in the header file, should mention CB.  Place the
usual '/* See .... */' comment here instead.

> +
> +void
> +iterate_over_block_local_vars_printing
> +  (const struct block *block,
> +   iterate_over_block_arg_local_vars_cb_printing cb)
> +{
> +  gdb::unordered_set<std::string> collected_vars, shadowed_vars, printed_vars;
> +
> +  /* Phase one: iterate over all locals within the block, and every parent
> +     block up to the enclosing function block.  Record all of the locals
> +     seen, this allows us to know which locals are shadowing locals from a
> +     more outer scope.  */
> +  iterate_over_block_local_vars
> +    (block, [&] (const char *print_name, struct symbol *sym)
> +    {
> +      if (!sym->is_argument ())
> +	{
> +	  if (!collected_vars.insert (print_name).second)
> +	    shadowed_vars.insert (print_name);
> +	}
> +    });
> +
> +  /* Phase two: iterate over all locals within the block, and every parent
> +     block up to the enclosing function block.  Print all the locals seen
> +     by calling CB.  Depending on the current language we vary the
> +     arguments to CB to indicate shadowing.  Or in some cases, we don't
> +     print the local at all.  */
> +  iterate_over_block_local_vars
> +    (block, [&] (const char *print_name, struct symbol *sym)
> +    {
> +      bool already_printed = !printed_vars.insert (print_name).second;
> +      bool shadowed = shadowed_vars.find (print_name) != shadowed_vars.end ();
> +
> +      enum var_shadowing shadowing_status;
> +      if (already_printed && shadowed)
> +	shadowing_status = var_shadowing::SHADOWED;
> +      else if (!already_printed && shadowed)
> +	shadowing_status = var_shadowing::SHADOWING;
> +      else
> +	shadowing_status = var_shadowing::NONE;
> +
> +      /* 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 (get_lang_vars_shadowing_option (current_language->la_language)
> +	  == lang_vars_shadowing::PRINT && shadowing_status != var_shadowing::NONE)

Long line, wrap at '&&' please.

> +	cb (print_name, sym, shadowing_status);
> +      /* In case of Rust language it is possible to declare variable with
> +	 same name multiple times and only innermost instance of variable
> +	 is accessible.  So print only the innermost instance and there is
> +	 no need of printing duplicates.  */
> +      else if (get_lang_vars_shadowing_option (current_language->la_language)
> +	       == lang_vars_shadowing::HIDE
> +	       && shadowing_status == var_shadowing::SHADOWED)
> +	{
> +	  /* Nothing.  */
> +	}
> +      else
> +	cb (print_name, sym, var_shadowing::NONE);
> +    });
> +}
> +
>  /* Data to be passed around in the calls to the locals and args
>     iterators.  */
>  
> @@ -2192,14 +2259,16 @@ struct print_variable_and_value_data
>    struct ui_file *stream;
>    int values_printed;
>  
> -  void operator() (const char *print_name, struct symbol *sym);
> +  void operator() (const char *print_name, struct symbol *sym,
> +		   var_shadowing shadow_status);
>  };
>  
>  /* The callback for the locals and args iterators.  */
>  
>  void
>  print_variable_and_value_data::operator() (const char *print_name,
> -					   struct symbol *sym)
> +					   struct symbol *sym,
> +					   var_shadowing shadow_status)
>  {
>    frame_info_ptr frame;
>  
> @@ -2219,7 +2288,8 @@ print_variable_and_value_data::operator() (const char *print_name,
>        return;
>      }
>  
> -  print_variable_and_value (print_name, sym, frame, stream, num_tabs);
> +  print_variable_and_value (print_name, sym, frame, stream, num_tabs,
> +			    shadow_status);
>  
>    values_printed = 1;
>  }
> @@ -2288,7 +2358,7 @@ print_frame_local_vars (const frame_info_ptr &frame,
>    scoped_restore_selected_frame restore_selected_frame;
>    select_frame (frame);
>  
> -  iterate_over_block_local_vars (block, cb_data);
> +  iterate_over_block_local_vars_printing (block, cb_data);
>  
>    if (!cb_data.values_printed && !quiet)
>      {
> @@ -2386,6 +2456,37 @@ iterate_over_block_arg_vars (const struct block *b,
>      }
>  }
>  
> +/* Iterate over all the argument variables in block B.  */

This comment should be in the header file, should mention CB.  Place the
usual '/* See .... */' comment here instead.

> +
> +void
> +iterate_over_block_arg_vars_printing
> +  (const struct block *b,
> +   iterate_over_block_arg_local_vars_cb_printing cb)
> +{
> +  for (struct symbol *sym : block_iterator_range (b))
> +    {
> +      /* Don't worry about things which aren't arguments.  */
> +      if (sym->is_argument ())
> +	{
> +	  /* We have to look up the symbol because arguments can have
> +	     two entries (one a parameter, one a local) and the one we
> +	     want is the local, which lookup_symbol will find for us.
> +	     This includes gcc1 (not gcc2) on the sparc when passing a
> +	     small structure and gcc2 when the argument type is float
> +	     and it is passed as a double and converted to float by
> +	     the prologue (in the latter case the type of the LOC_ARG
> +	     symbol is double and the type of the LOC_LOCAL symbol is
> +	     float).  There are also LOC_ARG/LOC_REGISTER pairs which
> +	     are not combined in symbol-reading.  */
> +
> +	  struct symbol *sym2
> +	    = lookup_symbol_search_name (sym->search_name (),
> +					 b, SEARCH_VAR_DOMAIN).symbol;
> +	  cb (sym->print_name (), sym2, var_shadowing::NONE);
> +	}
> +    }
> +}
> +
>  /* Print all argument variables of the function of FRAME.
>     Print them with values to STREAM.
>     If REGEXP is not NULL, only print argument variables whose name
> @@ -2428,7 +2529,7 @@ print_frame_arg_vars (const frame_info_ptr &frame,
>    cb_data.stream = stream;
>    cb_data.values_printed = 0;
>  
> -  iterate_over_block_arg_vars (func->value_block (), cb_data);
> +  iterate_over_block_arg_vars_printing (func->value_block (), cb_data);
>  
>    if (!cb_data.values_printed && !quiet)
>      {
> diff --git a/gdb/stack.h b/gdb/stack.h
> index ad2700b59a7..6e2bba15b32 100644
> --- a/gdb/stack.h
> +++ b/gdb/stack.h
> @@ -20,6 +20,8 @@
>  #ifndef GDB_STACK_H
>  #define GDB_STACK_H
>  
> +enum class var_shadowing;
> +
>  gdb::unique_xmalloc_ptr<char> find_frame_funname (const frame_info_ptr &frame,
>  						  enum language *funlang,
>  						  struct symbol **funcp);
> @@ -27,12 +29,24 @@ gdb::unique_xmalloc_ptr<char> find_frame_funname (const frame_info_ptr &frame,
>  typedef gdb::function_view<void (const char *print_name, struct symbol *sym)>
>       iterate_over_block_arg_local_vars_cb;
>  
> +typedef gdb::function_view<void (const char *print_name, struct symbol *sym,
> +				 var_shadowing shadow_status)>
> +    iterate_over_block_arg_local_vars_cb_printing;
> +
>  void iterate_over_block_arg_vars (const struct block *block,
>  				  iterate_over_block_arg_local_vars_cb cb);
>  
>  void iterate_over_block_local_vars (const struct block *block,
>  				    iterate_over_block_arg_local_vars_cb cb);
>  
> +void iterate_over_block_arg_vars_printing
> +  (const struct block *block,
> +   iterate_over_block_arg_local_vars_cb_printing cb);
> +
> +void iterate_over_block_local_vars_printing
> +  (const struct block *block,
> +   iterate_over_block_arg_local_vars_cb_printing cb);
> +
>  /* Initialize *WHAT to be a copy of the user desired print what frame info.
>     If !WHAT.has_value (), the printing function chooses a default set of
>     information to print, otherwise the printing function should print
> diff --git a/gdb/testsuite/gdb.ada/var_shadowing.exp b/gdb/testsuite/gdb.ada/var_shadowing.exp
> new file mode 100644
> index 00000000000..47e2b3831fd
> --- /dev/null
> +++ b/gdb/testsuite/gdb.ada/var_shadowing.exp
> @@ -0,0 +1,39 @@
> +# Copyright 2023-2026 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/>.
> +
> +load_lib "ada.exp"
> +
> +require allow_ada_tests
> +
> +standard_ada_testfile var_shadowing
> +
> +if {[gdb_compile_ada "${srcfile}" "${binfile}" \
> +    executable [list debug]] != "" } {
> +    return -1

This 'return -1' should be just 'return'.  There are an additional 4
places below which need this fix, but I'll not point out every one of
them.

> +}
> +
> +clean_restart ${testfile}
> +
> +set i_level1 [gdb_get_line_number "I-Level1"]
> +set i_level2 [gdb_get_line_number "I-Level2"]
> +set i_level3 [gdb_get_line_number "I-Level3"]
> +set bp_location [gdb_get_line_number "BREAK"]
> +runto "var_shadowing.adb:$bp_location"
> +
> +gdb_test "info locals" [multi_line \
> +    "i = 111\t<$testfile.adb:$i_level3>"  \
> +    "i = 11\t<$testfile.adb:$i_level2, shadowed>"  \
> +    "i = 1\t<$testfile.adb:$i_level1, shadowed>"  \
> +] "info locals at innermost level"

Thanks,
Andrew