[PATCH v7 1/2] gdb: add annotation in 'info locals' command for variables shadowing case
Ijaz, Abdul B
abdul.b.ijaz@intel.com
Tue Dec 17 18:48:23 GMT 2024
Hi Andrew,
Thanks again for the detailed feedback.
Andrew> (gdb) info locals
Andrew> x = 99 <file.c:3>
Andrew> x = 42 <file.c:1, shadowed>
Andrew>So a user reading the docs might think that every local will get location information, and become confused when they see something like the local 'y', with no information.
Andrew>I think your docs example should include a 'y' case (no location), and should explain why some locals get a location, and some do not.
It was my bad as I interpreted it wrong, sure will update the documentation accordingly.
Andrew>The case where we know a local is shadowed, but can't compute its location is interesting, and if reproducible, should be tested for, but that wasn't the case I was really asking about.
Will try it and will update it accordingly if I am able to reproduce it easily.
Abdul>> "shadowing mode" enum, what is meant by "replace". Also I have
Andrew>I was thinking about the rust case. The shadowing behaviour is not on or off, instead you have three behaviours:
Andrew> show shadowed locals: C, C++, Fortran, Ada
Andrew> only show inner most local: Rust
Andrew> show everything, no shadowing: Every other language.
Andrew> So a new language method could return an enum value to indicate which of these shadowing methods are appropriate.
Sure, will update the per language shadowing handling using a function accordingly.
Andrew>I imagine that would be something that is a layer on top of the language
Andrew>behaviour: if the language supports shadowing then the user can control what happens via settings.
Yes you are right, that will be a global setting.
Andrew>I haven't tried to create a block containing a symbol of a non-matching aclass, or with the COMMON_BLOCK_DOMAIN, and maybe doing such a thing is not possible... but when I look at the code as it is presented, my immediate concern Andrew>is that there might be cases which could confuse things. I'd expect to see at least a comment that explains why this is not possible
Now I understand your concern, ok here as far as I remember I could not find any use case as well and also did not find any issue with existing gdb testsuite for such cases specially COMMON_BLOCK_DOMAIN so that is why it was not handled in updating shadowed variables list.
Andrew>But, better yet I think, would be to share the iterate_over_block_locals logic. I've attached a patch which applies on top of your series which update stack.c so that recording of locals, and printing of locals, are both driven from Andrew>iterate_over_block_locals, I'd love to hear what you think of this change.
First of all, thanks a lot for putting the efforts in creating this patch. I tried the patch locally and at least for variable shadowing case it is doing what is required. Will test it with complete testsuite before finalizing it. Regarding the patch, after sharing now the iterate_over_block_locals this patch now handles all the corner scenarios in case there is any for the above discussed issue. So now the shadowed_vars or printing vars are handing the symbols exactly same way. In short this patch is handling shadowed_vars in a better way then mine and only need slight changes regarding "print_name/name handling". So I will replace my changes in patch 1 with yours and mention you in the tag using " Co-Authored-By:" Tag if its fine with you.
Thanks & Best Regards
Abdul Basit
-----Original Message-----
From: Andrew Burgess <aburgess@redhat.com>
Sent: Tuesday, December 17, 2024 3:11 PM
To: Ijaz, Abdul B <abdul.b.ijaz@intel.com>; gdb-patches@sourceware.org
Cc: pedro@palves.net; philippe.waroquiers@skynet.be; Aktemur, Tankut Baris <tankut.baris.aktemur@intel.com>; Schimpe, Christina <christina.schimpe@intel.com>; lsix@lancelotsix.com; eliz@gnu.org
Subject: RE: [PATCH v7 1/2] gdb: add annotation in 'info locals' command for variables shadowing case
"Ijaz, Abdul B" <abdul.b.ijaz@intel.com> writes:
> Hi Andrew,
>
> Thanks a lot for the feedback. Please see my response below:
>
>> (gdb) info locals
>> x = 99 <file.c:3>
>> y = 52
>> x = 42 <file.c:1, shadowed>
>
> Andrew> I suspect that we'll end up wanting to add location information for every local. I know when I look at that output I immediately wonder why 'y' doesn't have a location when 'x' does.
> Abdul > Location info for such shadowed variables was added based on these discussions. As the main issue was to easily distinguish between two or multiple nested variables with same name in the output. For other variable like 'y' in this case this info may not be that useful. As location only make it easier here to identify the variable with same name otherwise it is unnecessary. This is regarding only for explaining the "why" part but let me know what you think would be better here, as adding location info to all variable may just be extra info IMHO.
> https://sourceware.org/pipermail/gdb-patches/2022-January/184822.html
> https://sourceware.org/pipermail/gdb-patches/2022-January/184819.html
>
> Also just FYI and may be not related there was an additional print option discussed here also regarding these "shadowed" variables , so that we may also have another option to just completely disable printing of these shadowed variables if someone does not want to see this.
> https://sourceware.org/pipermail/gdb-patches/2023-November/203961.html
>
>> +(gdb) info locals
>> +x = 4 <file.c:3>
>> +x = 3 <file.c:1, shadowed>
>> +@end smallexample
> Andrew>Given my comment about location information above. If we are sticking with the current approach, then I think this example needs to be expanded to include a local without location information, and the text below should explain why some locals get a location, and others don't.
>
> Abdul>I see. Will handle it then accordingly update it. E.g. "
> Abdul><shadowed: Line info is not available>" will be added in such a
> Abdul>case, will this be fine with you instead of line number for such
> Abdul>cases.
Sorry, I don't think I was being very clear. What I mean is that, in your commit message you give an example like this:
(gdb) info locals
x = 99 <file.c:3>
y = 52
x = 42 <file.c:1, shadowed>
Here 'y' doesn't get location information. But in your docs example you only do:
(gdb) info locals
x = 99 <file.c:3>
x = 42 <file.c:1, shadowed>
So a user reading the docs might think that every local will get location information, and become confused when they see something like the local 'y', with no information.
I think your docs example should include a 'y' case (no location), and should explain why some locals get a location, and some do not.
The case where we know a local is shadowed, but can't compute its location is interesting, and if reproducible, should be tested for, but that wasn't the case I was really asking about.
>
>
>> + /* Print location and shadowed variable information. */
>> + fprintf_styled (stream, metadata_style.style (),
>> + _("\t<%s:%d%s>"), file_name,
>> + var->line (), printed ? ", shadowed" : "");
>
> Andrew>I don't think this styling is correct. I think the 'shadowed' makes sense to have the metadata style, but 'file_name' should have the file_name_style. The other character '<', ',', and '>' should be unstyled.
> Abdul> Thanks for pointing this out, will verify and then it will be fixed in V8.
>
>> + if ((current_language->la_language == language_c
>> + || current_language->la_language == language_cplus
>> + || current_language->la_language == language_fortran
>> + || current_language->la_language == language_ada)
>
> Andrew>It would be better to have a new method on the language,
> Andrew>something like 'bool allows_variable_shadowing () const', and
> Andrew>then override this in each language that does to return true.
> Andrew>Or maybe instead of allows_variable_shadowing(), have a
> Andrew>function that returns a shadowing mode enum? With choices
> Andrew>something like none, shadow, replace ? Then you'd be able to
> Andrew>switch between the two types of language.
>
> Abdul> Sure it can be updated. For the second case mentioned above
> Abdul> "shadowing mode" enum, what is meant by "replace". Also I have
I was thinking about the rust case. The shadowing behaviour is not on or off, instead you have three behaviours:
show shadowed locals: C, C++, Fortran, Ada
only show inner most local: Rust
show everything, no shadowing: Every other language.
So a new language method could return an enum value to indicate which of these shadowing methods are appropriate.
> Abdul> mentioned about adding of a print option above which will allow
> Abdul> using print option to hide shadowed variables completely. So in
I imagine that would be something that is a layer on top of the language
behaviour: if the language supports shadowing then the user can control what happens via settings.
> Abdul> this case as per my understanding first case of adding a method
> Abdul> on language to handle it per language sounds better to me. So
> Abdul> far only override function is needed for rust. But let me know
> Abdul> I will then update it accordingly.
>
> Andrew>It seems weird to me that you use different logic to build the
> Andrew>list of shadowed locals than you do to print the list of
> Andrew>locals. It feels like there might be some nasty edge case bugs
> Andrew>lurking where something is counted by the shadowing loop, but
> Andrew>not counted by the printing loop.
>
> Abdul> For building list of shadowed variable it is simple formula
> Abdul> that if variable come twice it is shadowed but printing list
> Abdul> was required in order to decide for printing the extra location
> Abdul> + shadowed information for shadowed variables or nothing for
> Abdul> other variables. As printing of variables in GDB starts from
> Abdul> innermost block and then goes to outermost. So always first
> Abdul> the original/current scope variable is printed and then the
> Abdul> shadowed variable and here shadowing list help to distinguish
> Abdul> such variables from rest. Then later printing list decide if
> Abdul> only location should be added or "shadowed" text should also be
> Abdul> added for all the variable in shadowed_vars list. All this
> Abdul> implementation is during the gdb printing variables from inner
> Abdul> to outer most blocks so it should be handling all cases. As
> Abdul> these lists just add info to normal printing of locals in gdb
> Abdul> and if a local variable with same name is been printed second
> Abdul> time in gdb then it will have extra "shadowed" related info but
> Abdul> if you already have any such source code e.g. for this scenario
> Abdul> in mind then let me know I will try and then we can discuss
> Abdul> accordingly.
My concern is that in iterate_over_block_locals there are two additional chances for a symbol to be ignored, the `switch` on `sym->aclass ()`, and then the `sym->domain ()` check. Neither of these are present in your loop which records the symbol names. I haven't tried to create a block containing a symbol of a non-matching aclass, or with the COMMON_BLOCK_DOMAIN, and maybe doing such a thing is not possible... but when I look at the code as it is presented, my immediate concern is that there might be cases which could confuse things. I'd expect to see at least a comment that explains why this is not possible.
But, better yet I think, would be to share the iterate_over_block_locals logic. I've attached a patch which applies on top of your series which update stack.c so that recording of locals, and printing of locals, are both driven from iterate_over_block_locals, I'd love to hear what you think of this change.
Thanks,
Andrew
---
diff --git a/gdb/stack.c b/gdb/stack.c
index fcb0b7331de..86df34bf2ca 100644
--- a/gdb/stack.c
+++ b/gdb/stack.c
@@ -2214,17 +2214,12 @@ backtrace_command_completer (struct cmd_list_element *ignore,
/* Iterate over the local variables of a block B, calling CB. */
static void
-iterate_over_block_locals (const struct block *b,
- iterate_over_block_arg_local_vars_cb cb,
- const std::unordered_set<std::string> *shadowed_vars,
- std::unordered_set<std::string> &printed_vars)
+iterate_over_block_locals
+ (const struct block *b,
+ gdb::function_view<void (const char *, struct symbol *)> cb)
{
for (struct symbol *sym : block_iterator_range (b))
{
- const char *name = sym->print_name ();
- bool already_printed = !printed_vars.insert (name).second;
- bool shadowed = shadowed_vars->find (name) != shadowed_vars->end ();
-
switch (sym->aclass ())
{
case LOC_CONST:
@@ -2237,25 +2232,7 @@ iterate_over_block_locals (const struct block *b,
break;
if (sym->domain () == COMMON_BLOCK_DOMAIN)
break;
- /* Only for C/C++/Fortran/Ada languages, in case of variables
- shadowing print <file:line, shadowed> annotation after
- the superblock variable. Iteration of block starts from inner
- block which is printed only with location information. */
- if ((current_language->la_language == language_c
- || current_language->la_language == language_cplus
- || current_language->la_language == language_fortran
- || current_language->la_language == language_ada)
- && shadowed)
- cb (name, sym, true, already_printed);
- /* In case of Rust language it is possible to declare variable with
- same name multiple times and only innermost instance of variable
- is accessible. So print only the innermost instance and there is
- no need of printing duplicates. */
- else if (current_language->la_language == language_rust
- && shadowed && already_printed)
- break;
- else
- cb (name, sym, false, false);
+ cb (sym->print_name (), sym);
break;
default:
@@ -2265,6 +2242,23 @@ iterate_over_block_locals (const struct block *b,
}
}
+/* Call iterate_over_block_locals, passing through CB for BLOCK, and every
+ superblock up to, and including, the enclosing function block. */
+
+static void
+iterate_over_local_vars_to_function_block
+ (const struct block *block,
+ gdb::function_view <void (const char *, struct symbol *)> cb) {
+ while (block)
+ {
+ iterate_over_block_locals (block, cb);
+ if (block->function ())
+ break;
+ block = block->superblock ();
+ }
+}
+
/* Iterate over all the local variables in block B, including all its
superblocks, stopping when the top-level block is reached. */
@@ -2273,37 +2267,56 @@ iterate_over_block_local_vars (const struct block *block,
iterate_over_block_arg_local_vars_cb cb) {
std::unordered_set<std::string> collected_vars, shadowed_vars, printed_vars;
- const struct block *orig_block = block;
- /* Iterate over all the local variables in a block and store the list of
- shadowed variables to later distinguish them from other variables. */
- while (block != nullptr)
+ /* Phase one: iterate over all locals within the block, and every parent
+ block up to the enclosing function block. Record all of the locals
+ seen, this allows us to know which locals are shadowing locals from a
+ more outer scope. */
+ iterate_over_local_vars_to_function_block
+ (block, [&] (const char *print_name, struct symbol *sym)
{
- for (struct symbol *sym : block_iterator_range (block))
+ if (!sym->is_argument ())
{
- if (!sym->is_argument ())
- {
- const char *name = sym->print_name ();
- if (!collected_vars.insert (name).second)
- shadowed_vars.insert (name);
- }
+ const char *name = sym->print_name ();
+ if (!collected_vars.insert (name).second)
+ shadowed_vars.insert (name);
}
- if (block->function ())
- break;
- block = block->superblock ();
- }
+ });
- block = orig_block;
- while (block)
+ /* Phase two: iterate over all locals within the block, and every parent
+ block up to the enclosing function block. Print all the locals seen
+ by calling CB. Depending on the current language we vary the
+ arguments to CB to indicate shadowing. Or in some cases, we don't
+ print the local at all. */
+ iterate_over_local_vars_to_function_block
+ (block, [&] (const char *print_name, struct symbol *sym)
{
- iterate_over_block_locals (block, cb, &shadowed_vars, printed_vars);
- /* After handling the function's top-level block, stop. Don't
- continue to its superblock, the block of per-file
- symbols. */
- if (block->function ())
- break;
- block = block->superblock ();
- }
+ const char *name = sym->print_name ();
+ bool already_printed = !printed_vars.insert (name).second;
+ bool shadowed = shadowed_vars.find (name) != shadowed_vars.end
+ ();
+
+ /* Only for C/C++/Fortran/Ada languages, in case of variables
+ shadowing print <file:line, shadowed> annotation after
+ the superblock variable. Iteration of block starts from inner
+ block which is printed only with location information. */
+ if ((current_language->la_language == language_c
+ || current_language->la_language == language_cplus
+ || current_language->la_language == language_fortran
+ || current_language->la_language == language_ada)
+ && shadowed)
+ cb (name, sym, true, already_printed);
+ /* In case of Rust language it is possible to declare variable with
+ same name multiple times and only innermost instance of variable
+ is accessible. So print only the innermost instance and there is
+ no need of printing duplicates. */
+ else if (current_language->la_language == language_rust
+ && shadowed && already_printed)
+ {
+ /* Nothing. */
+ }
+ else
+ cb (name, sym, false, false);
+ });
}
/* Data to be passed around in the calls to the locals and args
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