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

[Andrew Burgess <aburgess@redhat.com>]

Hi!

Thanks for the update.  I have some feedback below.  There's one,
relating the use of lbasename which might need further discussion, I've
laid out my position inline below.

There's also a few other minor points, but nothing serious.

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

> 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 = 3;
> 2:  {
> 3:    int x = 4;
> 4:    int y = 5;
> 5:    x = 99; /* break here */
> 6:  }
>
> 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="4"},{name="y",value="5"},{name="x",value="3"}]
> (gdb)
> -stack-list-locals 2
> ^done,locals=[{name="x",type="int",value="4"},{name="y",type="int",value="5"},{name="x",type="int",value="3"}]
> (gdb)
> -stack-list-variables 0
> ^done,variables=[{name="x"},{name="y"},{name="x"}]
> (gdb)
> -stack-list-variables 1
> ^done,variables=[{name="x",value="4"},{name="y",value="5"},{name="x",value="3"}]
> (gdb)
> -stack-list-variables 2
> ^done,variables=[{name="x",type="int",value="4"},{name="y",type="int",value="5"},{name="x",type="int",value="3"}]
> ~~~
>
> 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="4",filename="test.c",fullname="/home/src/test.c",line="5"},{name="y",value="5"},{name="x",value="3",filename="test.c",fullname="/home/src/test.c",line="2",shadowed="true"}]

Line numbers are wrong, they should be "3" and "1".  I think many of the
line numbers in the below might be off too.

> (gdb)
> -stack-list-locals 2
> ^done,locals=[{name="x",type="int",value="4",filename="test.c",fullname="/home/src/test.c",line="5"},{name="y",type="int",value="5"},{name="x",type="int",value="3",filename="test.c",fullname="/home/src/test.c",line="2",shadowed="true"}]
> (gdb)
> -stack-list-variables 0
> ^done,variables=[{name="x",filename="test.c",fullname="/home/src/test.c",line="5"},{name="y"},{name="x",filename="test.c",fullname="/home/src/test.c",line="2",shadowed="true"}]
> (gdb)
> -stack-list-variables 1
> ^done,variables=[{name="x",value="4",filename="test.c",fullname="/home/src/test.c",line="4"},{name="y",value="5"},{name="x",value="3",filename="test.c",fullname="/home/src/test.c",line="2",shadowed="true"}]
> (gdb)
> -stack-list-variables 2
> ^done,variables=[{name="x",type="int",value="4",filename="test.c",fullname="/home/src/test.c",line="4"},{name="y",type="int",value="5"},{name="x",type="int",value="3",filename="test.c",fullname="/home/src/test.c",line="2",shadowed="true"}]
> ~~~
> Reviewed-By: Guinevere Larsen <guinevere@redhat.com>
> ---
>  gdb/NEWS                                  |   4 +
>  gdb/doc/gdb.texinfo                       |  22 ++++
>  gdb/mi/mi-cmd-stack.c                     | 144 ++++++++++++++++------
>  gdb/testsuite/gdb.mi/mi-var-shadowing.c   |  50 ++++++++
>  gdb/testsuite/gdb.mi/mi-var-shadowing.exp | 141 +++++++++++++++++++++
>  5 files changed, 324 insertions(+), 37 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/NEWS b/gdb/NEWS
> index 2c8adf05b43..a3d0ad4ca11 100644
> --- a/gdb/NEWS
> +++ b/gdb/NEWS
> @@ -594,6 +594,10 @@ multi-wp-addr in qSupported
>     multiple times, and the same mapping was being reused.  In all
>     other cases, this field will have the value 'false'.
>  
> +** GDB now shows the "shadowed", "filename", "fullname" and "line"
> +   fields in the output of '-stack-list-locals/variables' mi
> +   commands for variables that are shadowed, or which are shadowing.
> +
>  * Support for stabs debugging format and the a.out/dbx object format is
>    deprecated, and will be removed in GDB 18.
>  
> diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
> index ee33bfdba13..883859e69c4 100644
> --- a/gdb/doc/gdb.texinfo
> +++ b/gdb/doc/gdb.texinfo
> @@ -35585,6 +35585,28 @@ 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
> +@group
> +1: int x = 3;
> +2: @{
> +3:       int x = 4;
> +4:       int y = 5;
> +5:       x = 99; // breakpoint-line
> +6: @}
> +@end group
> +@group
> +(gdb) -stack-list-variables 2
> +^done,variables=[@{name="x",type="int",value="4",
> +filename="name.c",fullname="/home/src/name.c",line="3"@},
> +@{name="y",type="int",value="5"@},@{name="x",type="int",
> +value="3",filename="name.c",fullname="/home/src/name.c",
> +line="1",shadowed="true"@}]
> +@end group
> +@end smallexample
> +
> +The location information and @samp{shadowed} attribute is only added
> +for shadowed variables (@pxref{shadowed variables}).

How about updating this paragraph to:

  The location information and @samp{shadowed} attribute are only added
  when variable shadowing is detected (@pxref{shadowed variables}).

the original text says that information is only added for shadowed
variables, but this isn't the full story, the shadowing copy also gets
information.  My updated text avoids this (I think).

> +
>  @subsubheading Example
>  
>  @smallexample
> diff --git a/gdb/mi/mi-cmd-stack.c b/gdb/mi/mi-cmd-stack.c
> index 280a08d121e..d2e814750d5 100644
> --- a/gdb/mi/mi-cmd-stack.c
> +++ b/gdb/mi/mi-cmd-stack.c
> @@ -560,6 +577,69 @@ list_arg_or_local (const struct frame_arg *arg, enum what_to_list what,
>  	}
>        uiout->field_stream ("value", stb);
>      }
> +
> +  /* Only for C/C++/Fortran/Ada languages, in case of variables shadowing
> +     print shadowed field after the superblock variable and only location
> +     of the variables in the innerblock.  */
> +  if (get_lang_vars_shadowing_option (current_language->la_language)
> +      == lang_vars_shadowing::PRINT && shadowed
> +      && !(values == PRINT_NO_VALUES && what == locals))
> +    {
> +      if (arg->sym->symtab () != nullptr)
> +	{
> +	  symtab *symtab = arg->sym->symtab ();
> +
> +	  /* Use lbasename instead of symtab_to_filename_for_display as the
> +	     latter defaults to relative path while basename is preferred
> +	     here.  */
> +	  uiout->field_string ("filename", lbasename (symtab->filename ()));

I strongly feel this is the wrong choice in this case.

I saw your comment about lbasename on the previous patch iteration, so I
didn't push about the issue on patch #1, the CLI output (though I do
think symtab_to_filename_for_display would be better).  While for the
CLI we do need to balance between full information and screen space use,
and avoid making the output too busy, in the MI we don't have that
concern.  The MI output is only intended for things like IDEs to parse,
not humans, so I think consistency with other similar fields is far more
important.  If an IDE want to only show the basename then it can always
do that, but the IDE cannot (reliably) recreate the relative path if
that's what it wants to display.  So for that reason, for me, I'd really
like you to consider using symtab_to_filename_for_display for the MI case.

> +	  uiout->field_string ("fullname", symtab_to_fullname (symtab));
> +	  uiout->field_unsigned ("line", arg->sym->line ());
> +	}
> +
> +      if (already_printed)
> +	uiout->field_string ("shadowed", "true");
> +    }
> +}
> +
> +/* Returns true if address_class can be printed, otherwise returns false.  */
> +
> +static bool
> +can_print_aclass (struct symbol *sym, enum what_to_list what)
> +{
> +  bool print_me = false;
> +
> +  switch (sym->loc_class ())
> +    {
> +    default:
> +    case LOC_UNDEF:     /* catches errors        */
> +    case LOC_CONST:     /* constant              */
> +    case LOC_TYPEDEF:   /* local typedef         */
> +    case LOC_LABEL:     /* local label           */
> +    case LOC_BLOCK:     /* local function        */
> +    case LOC_CONST_BYTES:       /* loc. byte seq.        */
> +    case LOC_UNRESOLVED:        /* unresolved static     */
> +    case LOC_OPTIMIZED_OUT:     /* optimized out         */
> +      print_me = false;
> +      break;
> +
> +    case LOC_ARG:       /* argument              */
> +    case LOC_REF_ARG:   /* reference arg         */
> +    case LOC_REGPARM_ADDR:      /* indirect register arg */
> +    case LOC_LOCAL:     /* stack local           */
> +    case LOC_STATIC:    /* static                */
> +    case LOC_REGISTER:  /* register              */
> +    case LOC_COMPUTED:  /* computed location     */
> +      if (what == all)
> +	print_me = true;
> +      else if (what == locals)
> +	print_me = !sym->is_argument ();
> +      else
> +	print_me = sym->is_argument ();
> +      break;
> +    }
> +
> +    return print_me;

I think the return line needs less indentation.

>  }
>  
>  /* Print a list of the objects for the frame FI in a certain form,

> diff --git a/gdb/testsuite/gdb.mi/mi-var-shadowing.exp b/gdb/testsuite/gdb.mi/mi-var-shadowing.exp
> new file mode 100644
> index 00000000000..27326864083
> --- /dev/null
> +++ b/gdb/testsuite/gdb.mi/mi-var-shadowing.exp
> @@ -0,0 +1,141 @@
> +# 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 mi-support.exp
> +set MIFLAGS "-i=mi"
> +
> +gdb_exit
> +if [mi_gdb_start] {
> +    continue
> +}
> +
> +standard_testfile
> +
> +set opts {debug}
> +if [build_executable ${testfile}.exp ${testfile} ${srcfile} $opts] {
> +    return
> +}
> +
> +mi_delete_breakpoints
> +mi_gdb_reinitialize_dir $srcdir/$subdir
> +mi_gdb_load ${binfile}
> +mi_runto main
> +
> +set bp_line1 [gdb_get_line_number "bp for locals 1"]
> +set bp_line2 [gdb_get_line_number "bp for locals 2"]
> +set bp_line3 [gdb_get_line_number "bp for locals 3"]
> +set bp_line4 [gdb_get_line_number "bp for locals 4"]
> +set bp_line5 [gdb_get_line_number "bp for locals 5"]
> +
> +set val1_d1 [gdb_get_line_number "val1-d1"]
> +set val1_d2 [gdb_get_line_number "val1-d2"]
> +set val1_d3 [gdb_get_line_number "val1-d3"]
> +set val2_d1 [gdb_get_line_number "val2-d1"]
> +set val2_d2 [gdb_get_line_number "val2-d2"]
> +set val2_d3 [gdb_get_line_number "val2-d3"]
> +set val3_d1 [gdb_get_line_number "val3-d1"]
> +set val3_d2 [gdb_get_line_number "val3-d2"]
> +set a_line [gdb_get_line_number "entry bp"]
> +
> +set stack_test1_regx "\\^done,(locals|variables)=\\\[\{name=\"a\",type=\"int\",value=\"$decimal\"\},\{name=\"val1\",type=\"unsigned int\",value=\"1\"\},{name=\"val2\",type=\"unsigned int\",value=\"2\"\}\\\]"
> +set stack_test2_regx "\\^done,(locals|variables)=\\\[\{name=\"val2\",type=\"unsigned int\",value=\"3\",filename=\"$srcfile\",fullname=\"$srcdir/$subdir/$srcfile\",line=\"$val2_d2\"\},\{name=\"val3\",type=\"unsigned int\",value=\"4\"\},\{name=\"a\",type=\"int\",value=\"101\"\},\{name=\"val1\",type=\"unsigned int\",value=\"1\"\},\{name=\"val2\",type=\"unsigned int\",value=\"2\",filename=\"$srcfile\",fullname=\"$srcdir/$subdir/$srcfile\",line=\"$val2_d1\",shadowed=\"true\"\}\\\]"
> +set stack_test3_regx "\\^done,(locals|variables)=\\\[\{name=\"val1\",type=\"unsigned int\",value=\"5\",filename=\"$srcfile\",fullname=\"$srcdir/$subdir/$srcfile\",line=\"$val1_d2\"\},\{name=\"val2\",type=\"unsigned int\",value=\"3\",filename=\"$srcfile\",fullname=\"$srcdir/$subdir/$srcfile\",line=\"$val2_d2\"\},\{name=\"val3\",type=\"unsigned int\",value=\"4\"\},\{name=\"a\",type=\"int\",value=\"102\"\},\{name=\"val1\",type=\"unsigned int\",value=\"1\",filename=\"$srcfile\",fullname=\"$srcdir/$subdir/$srcfile\",line=\"$val1_d1\",shadowed=\"true\"\},\{name=\"val2\",type=\"unsigned int\",value=\"2\",filename=\"$srcfile\",fullname=\"$srcdir/$subdir/$srcfile\",line=\"$val2_d1\",shadowed=\"true\"\}\\\]"
> +set stack_test4_regx "\\^done,(locals|variables)=\\\[\{name=\"val1\",type=\"unsigned int\",value=\"6\",filename=\"$srcfile\",fullname=\"$srcdir/$subdir/$srcfile\",line=\"$val1_d3\"\},\{name=\"val2\",type=\"unsigned int\",value=\"7\",filename=\"$srcfile\",fullname=\"$srcdir/$subdir/$srcfile\",line=\"$val2_d3\"\},\{name=\"val3\",type=\"unsigned int\",value=\"8\",filename=\"$srcfile\",fullname=\"$srcdir/$subdir/$srcfile\",line=\"$val3_d2\"\},\{name=\"val1\",type=\"unsigned int\",value=\"5\",filename=\"$srcfile\",fullname=\"$srcdir/$subdir/$srcfile\",line=\"$val1_d2\",shadowed=\"true\"\},\{name=\"val2\",type=\"unsigned int\",value=\"3\",filename=\"$srcfile\",fullname=\"$srcdir/$subdir/$srcfile\",line=\"$val2_d2\",shadowed=\"true\"\},\{name=\"val3\",type=\"unsigned int\",value=\"4\",filename=\"$srcfile\",fullname=\"$srcdir/$subdir/$srcfile\",line=\"$val3_d1\",shadowed=\"true\"\},\{name=\"a\",type=\"int\",value=\"103\"\},\{name=\"val1\",type=\"unsigned int\",value=\"1\",filename=\"$srcfile\",fullname=\"$srcdir/$subdir/$srcfile\",line=\"$val1_d1\",shadowed=\"true\"\},\{name=\"val2\",type=\"unsigned int\",value=\"2\",filename=\"$srcfile\",fullname=\"$srcdir/$subdir/$srcfile\",line=\"$val2_d1\",shadowed=\"true\"\}\\\]"
> +set stack_test5_regx "\\^done,(locals|variables)=\\\[\{name=\"a\",type=\"int\",value=\"105\"\},\{name=\"val1\",type=\"unsigned int\",value=\"1\"\},\{name=\"val2\",type=\"unsigned int\",value=\"2\"\}\\\]"

There's a bunch of tests that fail for me here due to the use of:

  fullname=\"$srcdir/$subdir/$srcfile\"

I guess you must configure GDB with an absolute path to the configure
script, while I use a relative path.  As a result the expected pattern
for me becomes:

  fullname="/tmp/binutils-gdb/build/gdb/testsuite/../../../src/gdb/testsuite/gdb.mi/mi-var-shadowing.c"

which doesn't matter the absolute path that is actually emitted.
Looking around this seems to be a solved problem though, other tests
use:

  fullname=\"${fullname_syntax}$srcfile\"

and if I change all of the original fullname patterns to the updated
version, then the tests all pass for me.

If you update to use symtab_to_filename_for_display as I request then
you will also need to update the 'filename' patterns, but these can just
become:

  filename=\"[^\r\n\]+$srcfile\"

which will be fine.


If you're happy to make these changes, including the lbasename change,
then I think this would be fine:

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

Otherwise, happy to discuss the lbasename stuff more as needed.

Thanks,
Andrew