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

[Andrew Burgess <aburgess@redhat.com>]

Hi, I have a last couple of super minor nits, see inline below.  If
you're happy to fix these as I suggest then I don't think you need to
post another version of the patch:

Approved-By: Andrew Burgess <aburgess@redhat.com>

But please wait for approval on patch #2 before merging both together.

Thanks,
Andrew

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 = 3;
> 2:  {
> 3:    int x = 4;
> 4:    int y = 52;
> 5:    x = 99; /* break here */
> 6:  }
>
> Currently:
>
> (gdb) info locals
> x = 4
> y = 52
> x = 3
>
> After applying this patch, we obtain:
>
> (gdb) info locals
> x = 4  <file.c:3>
> y = 52
> x = 3  <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.  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                                      |   5 +
>  gdb/c-typeprint.c                             |   3 +-
>  gdb/doc/gdb.texinfo                           |  26 ++++
>  gdb/language.c                                |  20 +++
>  gdb/language.h                                |  18 +++
>  gdb/printcmd.c                                |  44 ++++++-
>  gdb/stack.c                                   | 115 ++++++++++++++++--
>  gdb/stack.h                                   |  22 ++++
>  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, 561 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..2c8adf05b43 100644
> --- a/gdb/NEWS
> +++ b/gdb/NEWS
> @@ -201,6 +201,11 @@ 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
> +  for variables that are shadowed, or which are shadowing.
> +
> +

Double blank line here, there should be only one.

>  * Removed commands
>  
>  target ctf

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

I found the second sentence of this paragraph a little hard to parse.
Also the justification for the location seems unnecessary (to me).  How
about this instead:

  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).  When shadowing is detected, location
  information is added to all instances of the shadowed variable name.
  The outermost instances are additionally followed by @samp{shadowed}
  to indicate that they are not the active variable.

> +
>  @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/stack.h b/gdb/stack.h
> index ad2700b59a7..a03103b9847 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,32 @@ 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;
> +

After the recent commit:

  commit ef5907287aec1dc7f5323c66042a4a26f275c5cf
  Date:   Mon Jul 13 15:06:35 2026 +0200

    [gdb] Convert template typedefs to using

the previous 'typedef' has been updated to 'using'.  Could you update
this to match please.

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