[PATCH v6 2/2] gdb: add shadowed field in '-stack-list-locals/variables' mi commands

[Aktemur, Tankut Baris]

On Wednesday, November 22, 2023 9:13 AM, Ijaz, Abdul B wrote:
> From: "Ijaz, Abdul B" <abdul.b.ijaz@intel.com>
> 
> For C/C++/Fortran 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 GDB now prints location information
> for shadowed variables and add 'shadowed' field also in '-stack-list-locals'
> and '-stack-list-variables' mi commands for super-block shadowed variable.
> 
> Suppose we have test.c file
> 
> 1:int x = 42;
> 2:  {
> 3:    int x = 99;
> 4:    int y = 52;
> 4:    x = 99; /* break here */
> 5:  }
> 
> The "-stack-list-locals" and "-stack-list-variables" mi commands at the
> "break here" line gives the following output:
> 
> Before the change:
> 
> ~~~
> (gdb)
> -stack-list-locals 0
> ^done,locals=[name="x",name="y",name="x"]
> (gdb)
> -stack-list-locals 1
> ^done,locals=[{name="x",value="99"},{name="y",value="52"},{name="x",value="42"}]
> (gdb)
> -stack-list-locals 2
> ^done,locals=[{name="x",type="int",value="99"},{name="y",type="int",value="52"},{name="x",ty
> pe="int",value="42"}]
> (gdb)
> -stack-list-variables 0
> ^done,variables=[{name="x"},{name="y"},{name="x"}]
> (gdb)
> -stack-list-variables 1
> ^done,variables=[{name="x",value="99"},{name="y",value="52"},{name="x",value="42"}]
> (gdb)
> -stack-list-variables 2
> ^done,variables=[{name="x",type="int",value="99"},{name="y",type="int",value="52"},{name="x"
> ,type="int",value="42"}]
> ~~~
> 
> With this patch we obtain:
> 
> ~~~
> (gdb)
> -stack-list-locals 0
> ^done,locals=[name="x",name="y",name="x"]
> (gdb)
> -stack-list-locals 1
> ^done,locals=[{name="x",value="99",file="test.c",line="4"},{name="y",value="52"},{name="x",v
> alue="42",file="test.c",line="2",shadowed="true"}]
> (gdb)
> -stack-list-locals 2
> ^done,locals=[{name="x",type="int",value="99",file="test.c",line="4"},{name="y",type="int",v
> alue="52"},{name="x",type="int",value="42",file="test.c",line="2",shadowed="true"}]
> (gdb)
> -stack-list-variables 0
> ^done,variables=[{name="x",file="test.c",line="4"},{name="y"},{name="x",file="test.c",line="
> 2",shadowed="true"}]
> (gdb)
> -stack-list-variables 1
> ^done,variables=[{name="x",value="99",file="test.c",line="4"},{name="y",value="52"},{name="x
> ",value="42",file="test.c",line="2",shadowed="true"}]
> (gdb)
> -stack-list-variables 2
> ^done,variables=[{name="x",type="int",value="99",file="test.c",line="4"},{name="y",type="int
> ",value="52"},{name="x",type="int",value="42",file="test.c",line="2",shadowed="true"}]
> ~~~
> ---
>  gdb/doc/gdb.texinfo                       |  14 +++
>  gdb/mi/mi-cmd-stack.c                     |  66 +++++++++-
>  gdb/testsuite/gdb.mi/mi-var-shadowing.c   |  48 ++++++++
>  gdb/testsuite/gdb.mi/mi-var-shadowing.exp | 141 ++++++++++++++++++++++
>  4 files changed, 265 insertions(+), 4 deletions(-)
>  create mode 100644 gdb/testsuite/gdb.mi/mi-var-shadowing.c
>  create mode 100644 gdb/testsuite/gdb.mi/mi-var-shadowing.exp
> 
> diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
> index 6efcc5a8dc9..348844bbb8d 100644
> --- a/gdb/doc/gdb.texinfo
> +++ b/gdb/doc/gdb.texinfo
> @@ -34772,6 +34772,20 @@ If the @code{--skip-unavailable} option is specified, local
> variables
>  and arguments that are not available are not listed.  Partially
>  available arguments and local variables are still displayed, however.
> 
> +@smallexample
> +1: int x = 3;
> +2: @{
> +3:       int x = 4; // breakpt
> +4: @}
> +(gdb) -stack-list-variables 2
> +^done,variables=[@{name="x",type="int",value="4",file="name.c",line="3"@},@{name="x",type="
> int",value="3",file="name.c",line="1",shadowed="true"@}]
> +@end smallexample

If we had stopped at line "breakpt", the " = 4" assignment would not have
taken place, yet.  So, instead of seeing "4", seeing garbage is more likely.
It would help if "// breakpt" was removed after the second definition line.
Something like:

+@smallexample
+1: int x = 3;
+2: @{
+3:       int x = 4;
+4:       x = 99; // breakpt
+5: @}

> +
> +A variable is shadowed when there's another variable by 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} attribute.
> +
>  @subsubheading Example
> 
>  @smallexample
> diff --git a/gdb/mi/mi-cmd-stack.c b/gdb/mi/mi-cmd-stack.c
> index e473be7d465..21638796b63 100644
> --- a/gdb/mi/mi-cmd-stack.c
> +++ b/gdb/mi/mi-cmd-stack.c
> @@ -38,6 +38,8 @@
>  #include "gdbsupport/gdb-safe-ctype.h"
>  #include "inferior.h"
>  #include "observable.h"
> +#include "include/libiberty.h"
> +#include <unordered_set>
> 
>  enum what_to_list { locals, arguments, all };
> 
> @@ -491,7 +493,9 @@ mi_cmd_stack_list_variables (const char *command, const char *const
> *argv,
> 
>  static void
>  list_arg_or_local (const struct frame_arg *arg, enum what_to_list what,
> -		   enum print_values values, int skip_unavailable)
> +		   enum print_values values, int skip_unavailable,
> +		   const std::unordered_set<std::string> *shadowed_vars,
> +		   std::unordered_set<std::string> &printed_vars)

I would repeat my comment that gave in the previous patch.
Why not pass shadowed_vars as a (const) ref?

>  {
>    struct ui_out *uiout = current_uiout;
> 
> @@ -520,6 +524,18 @@ list_arg_or_local (const struct frame_arg *arg, enum what_to_list what,
>      tuple_emitter.emplace (uiout, nullptr);
> 
>    string_file stb;
> +  const char *name = arg->sym->print_name ();
> +  bool already_printed = !printed_vars.insert (name).second;
> +  printed_vars.insert (name);

This line seems redundant.

Thanks,
-Baris


Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de
Managing Directors: Sean Fennelly, Jeffrey Schneiderman, Tiffany Doon Silva
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928