From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from lndn.lancelotsix.com (lndn.lancelotsix.com [51.195.220.111]) by sourceware.org (Postfix) with ESMTPS id B381B3858D32 for ; Mon, 18 Sep 2023 22:46:21 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org B381B3858D32 Authentication-Results: sourceware.org; dmarc=pass (p=reject dis=none) header.from=lancelotsix.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=lancelotsix.com Received: from octopus (cust120-dsl54.idnet.net [212.69.54.120]) by lndn.lancelotsix.com (Postfix) with ESMTPSA id 4CF6D813EC; Mon, 18 Sep 2023 22:46:20 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=lancelotsix.com; s=2021; t=1695077180; bh=ckrbyAlO19146EF7Dx06MDyp9PVjGKnErhfH44hMBSQ=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=HT+gciHMGxuAStAg6tMiQui++5+HNodvtF/o4LEzEmTp7m8iVeIJKUpR7WXpDVI8K IF78HKeK9wabNoN6/EHbuKo2uDJHQZVupNeK5oOoZaDVV66bIssCdUVKKbXbUVqEuw 4EjlTbxWJxEeDB+759ZC1QqG7LO2ssrqtoyoflcZ1B9pgNfhIlvC6xnD9Gb7TK0uG7 wp1UvbvsE6jPZO00fW0xZI1Ct0WFSpxZKXptbyLRXoOoBiHeHI6fORc4FVFszqrWwS 8Yf7yYbYk3pLEHEefrbgqfnCsHlLzD2TxrBMCTJVHcyExcWY3CHFC6T1WFx7ouhNvc wMiw4j5e92ruw== Date: Mon, 18 Sep 2023 23:46:15 +0100 From: Lancelot SIX To: Abdul Basit Ijaz Cc: gdb-patches@sourceware.org, pedro@palves.net Subject: Re: [PATCH v3 1/2] gdb: add annotation in 'info locals' command for variables shadowing case Message-ID: <20230918224557.czfvzbzzmq3ceaj5@octopus> References: <20230918164738.17082-1-abdul.b.ijaz@intel.com> <20230918164738.17082-2-abdul.b.ijaz@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20230918164738.17082-2-abdul.b.ijaz@intel.com> X-Greylist: Sender succeeded SMTP AUTH, not delayed by milter-greylist-4.6.2 (lndn.lancelotsix.com [0.0.0.0]); Mon, 18 Sep 2023 22:46:20 +0000 (UTC) X-Spam-Status: No, score=-11.9 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,KAM_SHORT,SPF_HELO_NONE,SPF_PASS,TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: Hi Abdul, Thanks for working on this. I have included some comments on the patch. > diff --git a/gdb/printcmd.c b/gdb/printcmd.c > index 8d7d04231fe..17e385138d2 100644 > --- a/gdb/printcmd.c > +++ b/gdb/printcmd.c > @@ -2401,7 +2401,8 @@ clear_dangling_display_expressions (struct objfile *objfile) > void > print_variable_and_value (const char *name, struct symbol *var, > frame_info_ptr frame, > - struct ui_file *stream, int indent) > + struct ui_file *stream, int indent, > + bool shadowed, bool printed) > { > > if (!name) > @@ -2414,6 +2415,11 @@ print_variable_and_value (const char *name, struct symbol *var, > { > struct value *val; > struct value_print_options opts; > + std::string file_path = var->owner.symtab->filename; > + size_t slash_index = file_path.find_last_of ("\\/"); I think you might prefer to use lbasename (include/libiberty.h) as I guess this search can fail. For example, "\" can in a filename on Linux (not saying it is a wise thing to do). const char *file_name = lbasename (var->owner.symtab->filename); > + std::string file_name = (!file_path.empty () > + ? file_path.substr (slash_index + 1) > + : "NA"); As for the "NA" case, I am not sure when it can happen. If the symtab's filename can be nullptr (or start with \0), a test might be necessary. > > /* READ_VAR_VALUE needs a block in order to deal with non-local > references (i.e. to handle nested functions). In this context, we > @@ -2424,6 +2430,12 @@ print_variable_and_value (const char *name, struct symbol *var, > opts.deref_ref = true; > common_val_print_checked (val, stream, indent, &opts, current_language); > > + if (shadowed) > + /* Print location and shadowed variable information. */ > + fprintf_styled (stream, metadata_style.style (), > + _("\t<%s:%d%s>"), file_name.c_str (), > + var->line (), printed ? ", shadowed" : ""); > + > /* common_val_print invalidates FRAME when a pretty printer calls inferior > function. */ > frame = NULL; > diff --git a/gdb/stack.c b/gdb/stack.c > index 0b35d62f82f..f0f384c6270 100644 > --- a/gdb/stack.c > +++ b/gdb/stack.c > @@ -56,6 +56,7 @@ > #include "cli/cli-option.h" > #include "cli/cli-style.h" > #include "gdbsupport/buildargv.h" > +#include > > /* The possible choices of "set print frame-arguments", and the value > of this setting. */ > @@ -2211,10 +2212,18 @@ backtrace_command_completer (struct cmd_list_element *ignore, > > static void > iterate_over_block_locals (const struct block *b, > - iterate_over_block_arg_local_vars_cb cb) > + iterate_over_block_arg_local_vars_cb cb, > + std::unordered_set shadowed_vars, This shadowed_vars parameter is only searched, not modified. Did you mean to pass it by const ref? Otherwise, you end-up copying the entire set. > + std::unordered_set &printed_vars) > { > for (struct symbol *sym : block_iterator_range (b)) > { > + const char *name = sym->print_name (); > + bool already_printed > + = printed_vars.find (name) != printed_vars.end (); > + printed_vars.insert (name); I think you can simplify this a bit by using std::unordered_set::insert's return value. It is a pair whose second element is a bool indicating if the insertion took place. If false, it means that the element was already in the set. bool already_printed = !printed_vars.insert (name).second; > + bool shadowed = shadowed_vars.find (name) != shadowed_vars.end (); > + > switch (sym->aclass ()) > { > case LOC_CONST: > @@ -2227,7 +2236,27 @@ iterate_over_block_locals (const struct block *b, > break; > if (sym->domain () == COMMON_BLOCK_DOMAIN) > break; > - cb (sym->print_name (), sym); > + /* Only for C/C++/Fortran/Ada languages, in case of variables > + shadowing print 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 latest declaration of variable > + is accessible. So print only the first 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); > + } As there is just one line in the else, no need for the { }. > break; > > default: > @@ -2244,9 +2273,30 @@ void > iterate_over_block_local_vars (const struct block *block, > iterate_over_block_arg_local_vars_cb cb) > { > + std::unordered_set 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) According to the GDB coding style, this should be while (block != nullptr) > + { > + for (struct symbol *sym : block_iterator_range (block)) > + { > + const char *name = sym->print_name (); > + if (collected_vars.find (name) != collected_vars.end ()) > + shadowed_vars.insert (name); > + else > + collected_vars.insert (name); Similarly, this could be if (!collected_vars.insert (name).second) shadowed_vars.insert (name); > + } > + if (block->function ()) > + break; > + block = block->superblock (); > + } > + > + block = orig_block; > while (block) > { > - iterate_over_block_locals (block, cb); > + 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. */ > diff --git a/gdb/testsuite/gdb.base/var-shadowing.c b/gdb/testsuite/gdb.base/var-shadowing.c > new file mode 100755 > index 00000000000..00597b402ba > --- /dev/null > +++ b/gdb/testsuite/gdb.base/var-shadowing.c > @@ -0,0 +1,49 @@ > +/* Copyright (C) 2023 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 . */ > + > +#include > + > +void > +shadowing () ^ This should be void shadowing (void) > +{ > + int a; /* bp for entry */ > + unsigned int val1 = 1; /* val1-d1 */ > + unsigned int val2 = 2; /* val2-d1 */ > + a = 101; /* bp for locals 1 */ > + { > + unsigned int val2 = 3; /* val2-d2 */ > + unsigned int val3 = 4; /* val3-d1 */ > + a = 102; /* bp for locals 2 */ > + { > + unsigned int val1 = 5; /* val1-d2 */ > + a = 103; /* bp for locals 3 */ > + { > + #include "var-shadowing2.c" > + unsigned int val1 = 6; /* val1-d3 */ > + unsigned int val2 = 7; /* val2-d3 */ > + unsigned int val3 = 8; /* val3-d2 */ > + a = 104; /* bp for locals 4 */ > + } > + } > + } > + a = 0; /* bp for locals 5 */ > +} > + > +int > +main (void) > +{ > + shadowing (); > + return 0; > +} Best, Lancelot.