[PATCH v6 2/2] gdb: add shadowed field in '-stack-list-locals/variables' mi commands
Ijaz, Abdul B
abdul.b.ijaz@intel.com
Mon Sep 23 16:47:15 GMT 2024
Thanks for the feedback Baris.
Will fix the review comments in Patch V7 update. Here is added feedback to the review.
>> + std::unordered_set<std::string> &printed_vars)
Baris> I would repeat my comment that gave in the previous patch.
Baris> Why not pass shadowed_vars as a (const) ref?
Abdul> Similar to other patch, you are right, it would be better to use "shadowed_vars" as a const ref. So will try to update and use it as const reference.
>> + !printed_vars.insert (name).second; printed_vars.insert (name);
Baris> This line seems redundant.
Abdul> ok, I see there is no call back function for printing. So could be redundant. Will try and if there is no need for it then it will be removed in V7.
Thanks & Best Regards
Abdul Basit
-----Original Message-----
From: Aktemur, Tankut Baris <tankut.baris.aktemur@intel.com>
Sent: Monday, September 23, 2024 3:45 PM
To: Ijaz, Abdul B <abdul.b.ijaz@intel.com>; gdb-patches@sourceware.org
Cc: blarsen@redhat.com; pedro@palves.net; philippe.waroquiers@skynet.be; aburgess@redhat.com; Schimpe, Christina <christina.schimpe@intel.com>; lsix@lancelotsix.com; eliz@gnu.org
Subject: RE: [PATCH v6 2/2] gdb: add shadowed field in '-stack-list-locals/variables' mi commands
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",va
> lue="42"}]
> (gdb)
> -stack-list-locals 2
> ^done,locals=[{name="x",type="int",value="99"},{name="y",type="int",va
> lue="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",v
> alue="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",shad
> owed="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",s
> hadowed="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
More information about the Gdb-patches
mailing list