On Fri, May 16, 2025 at 5:13 PM Matthew Sotoudeh
<[email protected]> wrote:
>
> This is a small patch to address
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=92386
>
> When a variable is used before being shadowed in the same scope, GCC
> outputs incorrect/misleading debug information. The Bugzilla report has
> a minimal example and explains why a direct fix was not desirable (debug
> info size would blow up).
>
> In the Bugzilla discussion, Richard Biener suggests "this probably
> deserves a warning and should be considered bad style?"
>
> This patch implements such a "-Wuse-before-shadow" warning for these
> scenarios. When gimplifying a named var, we throw a warning if it isn't
> the most-recently bound declaration for that name. I wasn't able to find
> a way to get the current stack of bindings for a given name, so I added
> a new hash map (binding_stacks) to keep track of this (only when
> -Wuse-before-shadow).
>
> I tested on x86-64-linux-gnu (Debian 12) with make -k check and saw no
> testsuite regressions using ./contrib/compare_tests. I also built Git
> and Linux with the flag: each triggered one 'true positive' warning,
> neither of which was an actual bug.
>
> I have not tested on major projects in other languages (C++, etc.) so
> there might be more false positives when -Wuse-before-shadow is enabled
> for those languages (could restrict the warning to just C if this is a
> concern?).
Maybe then the gimplifier is not the right place to do this then.
-Wshadow is handled in warn_if_shadowing inside c-decl.cc which is
called from pushdecl.
Maybe you could do something inside there where you search the current
statement list (cur_stmt_list) for previous usages of the name?
Thanks,
Andrew
>
> Any comments would be much appreciated; I'm new to the codebase.
>
> (Also, sorry for any duplicate sends --- I had some DMARC issues
> originally.)
>
> PR debug/92386 - gdb issue with variable-shadowing
>
> PR debug/92386
>
> gcc/ChangeLog:
>
> * common.opt: Added Wuse-before-shadow option; defaults to off.
>
> * gimplify.cc (gimplify_bind_expr): when gimplifying a bind,
> keep track of the stack of variables bound to each identifier.
> (gimplify_var_or_parm_decl): when gimplifying a named var,
> check if it is the most-recently bound variable for that
> identifier.
>
> gcc/testsuite/ChangeLog:
>
> * gcc.dg/warn-use-before-shadow.c: New test.
> ---
> gcc/common.opt | 5 ++
> gcc/gimplify.cc | 70 +++++++++++++++++++
> gcc/testsuite/gcc.dg/warn-use-before-shadow.c | 28 ++++++++
> 3 files changed, 103 insertions(+)
> create mode 100644 gcc/testsuite/gcc.dg/warn-use-before-shadow.c
>
> diff --git a/gcc/common.opt b/gcc/common.opt
> index e3fa0dacec4..cb4a2754912 100644
> --- a/gcc/common.opt
> +++ b/gcc/common.opt
> @@ -880,6 +880,11 @@ Wunused-variable
> Common Var(warn_unused_variable) Warning EnabledBy(Wunused)
> Warn when a variable is unused.
>
> +Wuse-before-shadow
> +Common Var(warn_use_before_shadow) Warning Init(0)
> +Warn if a variable from an outer scope is used before it is shadowed in the
> +current scope.
> +
> Wcoverage-mismatch
> Common Var(warn_coverage_mismatch) Init(1) Warning
> Warn in case profiles in -fprofile-use do not match.
> diff --git a/gcc/gimplify.cc b/gcc/gimplify.cc
> index 4f385b1b779..6854ced7b06 100644
> --- a/gcc/gimplify.cc
> +++ b/gcc/gimplify.cc
> @@ -120,6 +120,17 @@ tree_associate_condition_with_expr (tree stmt, unsigned
> uid)
> /* Hash set of poisoned variables in a bind expr. */
> static hash_set<tree> *asan_poisoned_variables = NULL;
>
> +/* Maps a var name to a stack of variables currently bound to that name. This
> + global is only used if -Wuse-before-shadow is passed, in which case every
> + time a variable is gimplified we check this stack and throw a warning if
> + that variable is not the innermost binding to its name. */
> +static hash_map <tree, auto_vec <tree>> binding_stacks;
> +
> +/* A list of all of the currently bound named variables. Basically a
> flattened
> + version of @binding_stacks. Used to pop bindings off @binding_stacks when
> + gimplify_bind_expr returns. */
> +static auto_vec <tree> all_bindings;
> +
> /* Hash set of already-resolved calls to OpenMP "declare variant"
> functions. A call can resolve to the original function and
> we don't want to repeat the resolution multiple times. */
> @@ -1417,6 +1428,25 @@ gimplify_bind_expr (tree *expr_p, gimple_seq *pre_p)
>
> tree temp = voidify_wrapper_expr (bind_expr, NULL);
>
> + /* Update the stack of variables bound to a given name. Used for
> + -Wuse-before-shadow to detect when a variable is used in a scope where it
> is
> + not the innermost bound instance of that name. Note that gimplification
> will
> + sometimes change the DECL_NAME associated with a tree (e.g., promoting an
> + initialized const array to a static) so we store the names in a separate
> + vector @all_bindings to pop from after exiting the bind expr. */
> + size_t all_bindings_reset_idx = all_bindings.length ();
> + if (warn_use_before_shadow)
> + {
> + for (tree t = BIND_EXPR_VARS (bind_expr); t ; t = DECL_CHAIN (t))
> + {
> + if (VAR_P (t) && DECL_NAME (t) != NULL_TREE)
> + {
> + binding_stacks.get_or_insert (DECL_NAME (t)).safe_push (t);
> + all_bindings.safe_push (DECL_NAME (t));
> + }
> + }
> + }
> +
> /* Mark variables seen in this bind expr. */
> for (t = BIND_EXPR_VARS (bind_expr); t ; t = DECL_CHAIN (t))
> {
> @@ -1828,6 +1858,17 @@ gimplify_bind_expr (tree *expr_p, gimple_seq *pre_p)
>
> gimplify_seq_add_stmt (pre_p, bind_stmt);
>
> + /* Pop off bindings used for -Wuse-before-shadow now that we're
> + exiting the bind. */
> + while (all_bindings.length () > all_bindings_reset_idx)
> + {
> + tree name = all_bindings.pop ();
> + binding_stacks.get (name)->pop ();
> + if (!binding_stacks.get (name)->length ())
> + binding_stacks.remove (name);
> + }
> +
> +
> if (temp)
> {
> *expr_p = temp;
> @@ -3330,6 +3371,35 @@ gimplify_var_or_parm_decl (tree *expr_p)
> {
> tree decl = *expr_p;
>
> + /* Check that any named variable we use is the one actually bound to that
> + name in this scope. */
> + if (warn_use_before_shadow && VAR_P (decl) && DECL_NAME (decl) != NULL_TREE
> + && !warning_suppressed_p (decl, OPT_Wuse_before_shadow))
> + {
> + vec <tree> *binding_stack = binding_stacks.get (DECL_NAME (decl));
> + if (binding_stack && binding_stack->length ())
> + {
> + tree scope_decl = binding_stack->last ();
> + if ((!DECL_EXTERNAL (scope_decl) && scope_decl != decl)
> + || (DECL_EXTERNAL (scope_decl) && !is_global_var (decl)))
> + {
> + if (warning_at (input_location, OPT_Wuse_before_shadow,
> + "variable %qD is used before being
> shadowed",
> + decl))
> + {
> + inform (DECL_SOURCE_LOCATION (scope_decl),
> + "the shadowing declaration of %qD in this
> scope",
> + scope_decl);
> + inform (DECL_SOURCE_LOCATION (decl),
> + "outer scope declaration of %qD being "
> + "used instead",
> + decl);
> + }
> + suppress_warning (decl, OPT_Wuse_before_shadow);
> + }
> + }
> + }
> +
> /* ??? If this is a local variable, and it has not been seen in any
> outer BIND_EXPR, then it's probably the result of a duplicate
> declaration, for which we've already issued an error. It would
> diff --git a/gcc/testsuite/gcc.dg/warn-use-before-shadow.c
> b/gcc/testsuite/gcc.dg/warn-use-before-shadow.c
> new file mode 100644
> index 00000000000..5d381e4c30f
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/warn-use-before-shadow.c
> @@ -0,0 +1,28 @@
> +/* { dg-do compile } */
> +/* { dg-options "-Wuse-before-shadow" } */
> +int A, B, C, D;
> +
> +int main(void) {
> + A++; /* { dg-warning "used before being shadowed" } */
> + B++;
> + int A = 7, C = 8;
> + {
> + C++; /* { dg-warning "used before being shadowed" } */
> + extern int C, D;
> + C++;
> + D++;
> + }
> +
> + D++;
> + extern int D;
> + D++;
> +
> + int x = 5, x2 = 6;
> + {
> + x++; /* { dg-warning "used before being shadowed" } */
> + int x = 7, x2 = 7;
> + x--;
> + x2--;
> + }
> + x++;
> +}
> --
> 2.39.5
>
