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

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

Hi Andrew,

Thanks for the feedback and approval. I will update the changes you suggested including replacing lbasename with symtab_to_filename_for_display before merging.

>> -stack-list-locals 1
>> ^done,locals=[{name="x",value="4",filename="test.c",fullname="/home/sr
>> c/test.c",line="5"},{name="y",value="5"},{name="x",value="3",filename=
>> "test.c",fullname="/home/src/test.c",line="2",shadowed="true"}]

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

Abdul> Right, will fix this and the below before mering.


>> +The location information and @samp{shadowed} attribute is only added 
>> +for shadowed variables (@pxref{shadowed variables}).

Andrew>How about updating this paragraph to:
Andrew>The location information and @samp{shadowed} attribute are only added
Andew> when variable shadowing is detected (@pxref{shadowed variables}).

Abdul> Will fix it before merging.

>> +	  uiout->field_string ("filename", lbasename (symtab->filename ()));

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

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

Abdul> Fine with me. I will replace lbasename to symtab_to_filename_for_display. As there was already complete path info available with the "fullname" so decided to use lbasename instead of it in the "filename".  But you are right its up to IDEs to parse and keep the MI output consistent.


>> +    return print_me;

Andrew> I think the return line needs less indentation

Abdul> Will fix before merging.


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

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

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

Abdul> Yes my testsuite folder is configured with absolute Path. Will test it with relative path also before merging.

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

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

Abdul> Thanks, will update it accordingly before merging.

Thanks & Best Regards
Abdul Basit

-----Original Message-----
From: Andrew Burgess <aburgess@redhat.com> 
Sent: Friday, July 17, 2026 4:05 PM
To: Ijaz, Abdul B <abdul.b.ijaz@intel.com>; gdb-patches@sourceware.org
Cc: pedro@palves.net; philippe.waroquiers@skynet.be; Schimpe, Christina <christina.schimpe@intel.com>; lsix@lancelotsix.com; eliz@gnu.org; Ijaz, Abdul B <abdul.b.ijaz@intel.com>; guinevere@redhat.com
Subject: Re: [PATCH v14 2/2] gdb: add shadowed field in '-stack-list-locals/variables' mi commands


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",valu
> e="3"}]
> (gdb)
> -stack-list-locals 2
> ^done,locals=[{name="x",type="int",value="4"},{name="y",type="int",val
> ue="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",v
> alue="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/sr
> c/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",fullnam
> e="/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/sr
> c/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",filena
> me="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",full
> name="/home/src/test.c",line="4"},{name="y",type="int",value="5"},{nam
> e="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

Intel Deutschland GmbH

Registered Address: Dornacher Strasse 1, 85622 Feldkirchen, Germany
Tel: +49 89 991 430, www.intel.de
Managing Directors: Harry Demas, Jeffrey Schneiderman, Yin Chong Sorrell
Chairperson of the Supervisory Board: Nicole Lau
Registered Seat: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928