[PATCH] gimplify: Add -Wuse-before-shadow [PR92386]
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?).
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 *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 > 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 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_e
[PATCH v2] c-decl: Add -Wshadow=used [PR92386]
This is a small patch to address
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=92386 updated thanks to Andrew's
feedback.
This patch implements "-Wshadow=used," which throws a warning for shadowed
variables only if the shadowed variable was previously used in the same scope
where it is being shadowed.
This type of shadowing is particularly bad because it causes GCC to output
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).
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: Git had 2 benign true positives while Linux had 5 benign true
positives.
Any comments would be much appreciated; I'm new to the codebase.
Changes from v1:
* Implement as part of the normal variable shadowing warnings rather
than during gimplification.
PR debug/92386 - gdb issue with variable-shadowing
PR debug/92386
gcc/c/ChangeLog:
* c-decl.cc (find_var_usage): helper to walk a tree looking for uses of
a specific variable.
(warn_if_shadowing): when Wshadow=used is passed, throw a warning if
the variable being shadowed is used earlier in the same scope.
gcc/ChangeLog:
* common.opt: Added Wshadow=used option.
gcc/testsuite/ChangeLog:
* gcc.dg/warn-use-before-shadow.c: New test.
---
gcc/c/c-decl.cc | 43 ++-
gcc/common.opt| 7 +++
gcc/testsuite/gcc.dg/warn-use-before-shadow.c | 28
3 files changed, 77 insertions(+), 1 deletion(-)
create mode 100644 gcc/testsuite/gcc.dg/warn-use-before-shadow.c
diff --git a/gcc/c/c-decl.cc b/gcc/c/c-decl.cc
index 8c420f22976..8a98300423b 100644
--- a/gcc/c/c-decl.cc
+++ b/gcc/c/c-decl.cc
@@ -3205,6 +3205,20 @@ duplicate_decls (tree newdecl, tree olddecl)
}
+/* Helper used by warn_if_shadowing to search for uses of a specific variable
+ in a tree. */
+tree find_var_usage (tree *px, int *walk_subtrees, void *data)
+{
+ if (*px == (tree) data)
+return *px;
+ if (TREE_CODE (*px) == DECL_EXPR)
+{
+ tree di = DECL_INITIAL ( DECL_EXPR_DECL (*px));
+ return walk_tree (&di, find_var_usage, data, NULL);
+}
+ return 0;
+}
+
/* Check whether decl-node NEW_DECL shadows an existing declaration. */
static void
warn_if_shadowing (tree new_decl)
@@ -3214,7 +3228,8 @@ warn_if_shadowing (tree new_decl)
/* Shadow warnings wanted? */
if (!(warn_shadow
|| warn_shadow_local
-|| warn_shadow_compatible_local)
+|| warn_shadow_compatible_local
+|| warn_shadow_used)
/* No shadow warnings for internally generated vars. */
|| DECL_IS_UNDECLARED_BUILTIN (new_decl))
return;
@@ -3298,6 +3313,32 @@ warn_if_shadowing (tree new_decl)
if (warned)
inform (DECL_SOURCE_LOCATION (old_decl),
"shadowed declaration is here");
+/* If we haven't issued any other shadowing warning for this
+ declaration, but -Wshadow=used was passed, issue a warning if the
+ now-shadowed variable was used earlier in this scope. */
+else if (warn_shadow_used && building_stmt_list_p ())
+ {
+for (tree_stmt_iterator tsi = tsi_start (cur_stmt_list);
+ !tsi_end_p (tsi); tsi_next (&tsi))
+ {
+tree stmt = tsi_stmt (tsi),
+ found = walk_tree (&stmt, find_var_usage, old_decl, NULL);
+if (found)
+ {
+if (warning_at (EXPR_LOCATION (stmt), OPT_Wshadow_used,
+"variable %qD is used before being "
+"shadowed", found))
+ {
+inform (DECL_SOURCE_LOCATION (new_decl),
+"shadowing declaration is here");
+inform (DECL_SOURCE_LOCATION (old_decl),
+"shadowed declaration being used "
+"instead is here");
+ }
+break;
+ }
+ }
+ }
break;
}
diff --git a/gcc/common.opt b/gcc/common.opt
index e3fa0dacec4..aac3e652ef1 100644
--- a/gcc/common.opt
+++ b/gcc/common.opt
@@ -751,6 +751,13 @@ Warn when one local variable shadows another local
variable or parameter of comp
Wshadow-compatible-local
Common Warning Undocumented Alias(Wshadow=compatible-local)
+Wshadow=used
+Common Var(warn_shadow_used) Warning EnabledBy(Wshadow=used)
+Warn if a variable from an outer scope is used before it is shadowed in the
current scope.
+
+Wshadow-used
+Common Warning Undocumented Alias(Wshadow=used)
+
Wstack-protector
Common Var(warn_stack_protect) Warning
Warn when not iss
