* Re: Adding -Wshadow=local to gcc build rules
2019-09-18 13:08 Adding -Wshadow=local to gcc build rules Bernd Edlinger
@ 2019-09-18 13:39 ` Martin Liška
2019-09-18 15:16 ` Tom Tromey
` (3 subsequent siblings)
4 siblings, 0 replies; 8+ messages in thread
From: Martin Liška @ 2019-09-18 13:39 UTC (permalink / raw)
To: Bernd Edlinger, gcc
On 9/18/19 3:08 PM, Bernd Edlinger wrote:
> Hi,
>
> I'm currently trying to add -Wshadow=local to the gcc build rules.
> I started with -Wshadow, but gave up that idea immediately.
>
> As you could expect the current code base has plenty of shadowed
> local variables. Most are trivial to resolve, some are less trivial.
> I am not finished yet, but it is clear that it will be a rather big
> patch.
>
> I would like to ask you if you agree that would be a desirable step,
> in improving code quality in the gcc tree.
>
>
> Thanks
> Bernd.
>
Hello.
I did the very same for godot project some time ago:
https://github.com/godotengine/godot/pull/25853
I fully support such a patch.
Martin
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: Adding -Wshadow=local to gcc build rules
2019-09-18 13:08 Adding -Wshadow=local to gcc build rules Bernd Edlinger
2019-09-18 13:39 ` Martin Liška
@ 2019-09-18 15:16 ` Tom Tromey
2019-09-19 10:20 ` Richard Biener
` (2 subsequent siblings)
4 siblings, 0 replies; 8+ messages in thread
From: Tom Tromey @ 2019-09-18 15:16 UTC (permalink / raw)
To: Bernd Edlinger; +Cc: gcc
>>>>> "Bernd" == Bernd Edlinger <bernd.edlinger@hotmail.de> writes:
Bernd> I'm currently trying to add -Wshadow=local to the gcc build rules.
Bernd> I started with -Wshadow, but gave up that idea immediately.
Bernd> As you could expect the current code base has plenty of shadowed
Bernd> local variables. Most are trivial to resolve, some are less trivial.
Bernd> I am not finished yet, but it is clear that it will be a rather big
Bernd> patch.
Bernd> I would like to ask you if you agree that would be a desirable step,
Bernd> in improving code quality in the gcc tree.
We did this in gdb and it was worthwhile. According to my notes, it
found 3 real bugs (though one was by chance). You can see what else we
tried here: https://tromey.com/blog/?p=1035
Tom
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: Adding -Wshadow=local to gcc build rules
2019-09-18 13:08 Adding -Wshadow=local to gcc build rules Bernd Edlinger
2019-09-18 13:39 ` Martin Liška
2019-09-18 15:16 ` Tom Tromey
@ 2019-09-19 10:20 ` Richard Biener
2019-09-20 18:20 ` Bernd Edlinger
2019-09-21 9:44 ` Segher Boessenkool
2019-10-02 19:10 ` Bernd Edlinger
4 siblings, 1 reply; 8+ messages in thread
From: Richard Biener @ 2019-09-19 10:20 UTC (permalink / raw)
To: Bernd Edlinger; +Cc: gcc
On Wed, Sep 18, 2019 at 3:09 PM Bernd Edlinger
<bernd.edlinger@hotmail.de> wrote:
>
> Hi,
>
> I'm currently trying to add -Wshadow=local to the gcc build rules.
> I started with -Wshadow, but gave up that idea immediately.
>
> As you could expect the current code base has plenty of shadowed
> local variables. Most are trivial to resolve, some are less trivial.
> I am not finished yet, but it is clear that it will be a rather big
> patch.
>
> I would like to ask you if you agree that would be a desirable step,
> in improving code quality in the gcc tree.
I wonder if -Wshadow=compatible-local is easier to achieve?
Richard.
>
>
> Thanks
> Bernd.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: Adding -Wshadow=local to gcc build rules
2019-09-19 10:20 ` Richard Biener
@ 2019-09-20 18:20 ` Bernd Edlinger
2019-09-20 19:13 ` Jason Merrill
0 siblings, 1 reply; 8+ messages in thread
From: Bernd Edlinger @ 2019-09-20 18:20 UTC (permalink / raw)
To: Richard Biener; +Cc: gcc
On 9/19/19 12:19 PM, Richard Biener wrote:
> On Wed, Sep 18, 2019 at 3:09 PM Bernd Edlinger
> <bernd.edlinger@hotmail.de> wrote:
>>
>> Hi,
>>
>> I'm currently trying to add -Wshadow=local to the gcc build rules.
>> I started with -Wshadow, but gave up that idea immediately.
>>
>> As you could expect the current code base has plenty of shadowed
>> local variables. Most are trivial to resolve, some are less trivial.
>> I am not finished yet, but it is clear that it will be a rather big
>> patch.
>>
>> I would like to ask you if you agree that would be a desirable step,
>> in improving code quality in the gcc tree.
>
> I wonder if -Wshadow=compatible-local is easier to achieve?
>
I tried that and it does not make a big difference, while
it misses for instance:
* gcc/c-family/c-ada-spec.c (dump_ada_macros)
unsigned char *s, shadowed by const unsigned char *s. :-/
On the other hand, -Wshadow=global is a lot more difficult,
because we have lots of globals, for instance:
context.h:
/* The global singleton context aka "g".
(the name is chosen to be easy to type in a debugger). */
extern gcc::context *g;
But unfortunately Wshadow=local does not find class members
shadowed by local variable, which happens for instance in
wide-int
With -Wshadow, I had to change this in wide-int.h:
Index: gcc/wide-int.h
===================================================================
--- gcc/wide-int.h (revision 275545)
+++ gcc/wide-int.h (working copy)
@@ -648,9 +648,9 @@ namespace wi
storage_ref () {}
storage_ref (const HOST_WIDE_INT *, unsigned int, unsigned int);
- const HOST_WIDE_INT *val;
- unsigned int len;
- unsigned int precision;
+ const HOST_WIDE_INT *m_val;
+ unsigned int m_len;
+ unsigned int m_precision;
So this change was not necessary for -Wshadow=local, although
I would think that, shadowing class members is not much different from
shadowing a local scope.
Not sure if shadowing class members should be part of -Wshadow=local
instead of -Wshadow=global.
Bernd.
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: Adding -Wshadow=local to gcc build rules
2019-09-20 18:20 ` Bernd Edlinger
@ 2019-09-20 19:13 ` Jason Merrill
0 siblings, 0 replies; 8+ messages in thread
From: Jason Merrill @ 2019-09-20 19:13 UTC (permalink / raw)
To: Bernd Edlinger; +Cc: Richard Biener, gcc Mailing List
On Fri, Sep 20, 2019, 2:21 PM Bernd Edlinger <bernd.edlinger@hotmail.de>
wrote:
> On 9/19/19 12:19 PM, Richard Biener wrote:
> > On Wed, Sep 18, 2019 at 3:09 PM Bernd Edlinger
> > <bernd.edlinger@hotmail.de> wrote:
> >>
> >> Hi,
> >>
> >> I'm currently trying to add -Wshadow=local to the gcc build rules.
> >> I started with -Wshadow, but gave up that idea immediately.
> >>
> >> As you could expect the current code base has plenty of shadowed
> >> local variables. Most are trivial to resolve, some are less trivial.
> >> I am not finished yet, but it is clear that it will be a rather big
> >> patch.
> >>
> >> I would like to ask you if you agree that would be a desirable step,
> >> in improving code quality in the gcc tree.
> >
> > I wonder if -Wshadow=compatible-local is easier to achieve?
> >
>
> I tried that and it does not make a big difference, while
> it misses for instance:
>
> * gcc/c-family/c-ada-spec.c (dump_ada_macros)
> unsigned char *s, shadowed by const unsigned char *s. :-/
>
> On the other hand, -Wshadow=global is a lot more difficult,
> because we have lots of globals, for instance:
>
> context.h:
> /* The global singleton context aka "g".
> (the name is chosen to be easy to type in a debugger). */
> extern gcc::context *g;
>
> But unfortunately Wshadow=local does not find class members
> shadowed by local variable, which happens for instance in
> wide-int
>
> With -Wshadow, I had to change this in wide-int.h:
>
> Index: gcc/wide-int.h
> ===================================================================
> --- gcc/wide-int.h (revision 275545)
> +++ gcc/wide-int.h (working copy)
> @@ -648,9 +648,9 @@ namespace wi
> storage_ref () {}
> storage_ref (const HOST_WIDE_INT *, unsigned int, unsigned int);
>
> - const HOST_WIDE_INT *val;
> - unsigned int len;
> - unsigned int precision;
> + const HOST_WIDE_INT *m_val;
> + unsigned int m_len;
> + unsigned int m_precision;
>
>
> So this change was not necessary for -Wshadow=local, although
> I would think that, shadowing class members is not much different from
> shadowing a local scope.
>
> Not sure if shadowing class members should be part of -Wshadow=local
> instead of -Wshadow=global.
>
That would make sense to me.
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: Adding -Wshadow=local to gcc build rules
2019-09-18 13:08 Adding -Wshadow=local to gcc build rules Bernd Edlinger
` (2 preceding siblings ...)
2019-09-19 10:20 ` Richard Biener
@ 2019-09-21 9:44 ` Segher Boessenkool
2019-10-02 19:10 ` Bernd Edlinger
4 siblings, 0 replies; 8+ messages in thread
From: Segher Boessenkool @ 2019-09-21 9:44 UTC (permalink / raw)
To: Bernd Edlinger; +Cc: gcc
Hi!
On Wed, Sep 18, 2019 at 01:08:50PM +0000, Bernd Edlinger wrote:
> I'm currently trying to add -Wshadow=local to the gcc build rules.
> I started with -Wshadow, but gave up that idea immediately.
>
> As you could expect the current code base has plenty of shadowed
> local variables. Most are trivial to resolve, some are less trivial.
> I am not finished yet, but it is clear that it will be a rather big
> patch.
It shouldn't be a big patch. It should be a biggish collection of small
patches, most of which are trivial and obvious and you can just commit
without further approval!
Segher
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: Adding -Wshadow=local to gcc build rules
2019-09-18 13:08 Adding -Wshadow=local to gcc build rules Bernd Edlinger
` (3 preceding siblings ...)
2019-09-21 9:44 ` Segher Boessenkool
@ 2019-10-02 19:10 ` Bernd Edlinger
4 siblings, 0 replies; 8+ messages in thread
From: Bernd Edlinger @ 2019-10-02 19:10 UTC (permalink / raw)
To: gcc
On 9/18/19 3:08 PM, Bernd Edlinger wrote:
> Hi,
>
> I'm currently trying to add -Wshadow=local to the gcc build rules.
> I started with -Wshadow, but gave up that idea immediately.
>
> As you could expect the current code base has plenty of shadowed
> local variables. Most are trivial to resolve, some are less trivial.
> I am not finished yet, but it is clear that it will be a rather big
> patch.
>
> I would like to ask you if you agree that would be a desirable step,
> in improving code quality in the gcc tree.
>
>
> Thanks
> Bernd.
>
Hurray!!!
The build is now working with -Wshadow=local and I need to sort out the
changes. I hope you are gonna like it :-)
Currently it is a 600K patch file all together.
Some parts are not trivial at all, but I hope it will pay off in more
easy to follow code in various places and less errors in new code.
Also one or two real bugs were hiding near the shadowed variable.
Most of which are of the obvious kind where, one variable is shadowed by
another one, and either the inner or the outer variable need to be
renamed from name to name1, or i to j/k, or p to q, or x to y.
Sometimes also to name2 or name3, etc.
If it is a location_t I follow common practice that loc is renamed to loc0.
I avoid renaming a parameter, unless it causes much more changes. Also
when a renamed variable appears to be named in a comment I also update
the comment.
I tried to make the change smaller if possible, rather often the
shadowed variable is of the exact same type, and no longer used at the
place where the variable is declared again. I consider "unsigned"
and "unsigned int" as too different to re-use the variable (or parameter).
This means I did consider the control flow and data flow around each
shadowed variable if avoiding the duplicate variable is safe.
Unless there is disagreement I will go ahead and commit trivial parts
after careful regression-testing under the obvious rule.
I did also try to build a lot of cross compilers to make sure that the
targets do not break when the warning is enabled. But it is nevertheless
rather likely that you will see some fall-out once the warning will be
enabled. So far each target (except tilepro) needed at least a few changes.
This is the list of changed files:
M ada/gcc-interface/decl.c
M ada/gcc-interface/trans.c
M asan.c
M attribs.c
M auto-inc-dec.c
M bb-reorder.c
M brig/brigfrontend/brig-basic-inst-handler.cc
M brig/brigfrontend/brig-code-entry-handler.cc
M brig/brigfrontend/brig-function-handler.cc
M brig/brigfrontend/brig-util.cc
M builtins.c
M c/c-decl.c
M c/c-parser.c
M c/c-typeck.c
M c/gimple-parser.c
M c-family/c-ada-spec.c
M c-family/c-common.c
M c-family/c-cppbuiltin.c
M c-family/c-lex.c
M c-family/c-omp.c
M caller-save.c
M calls.c
M cfg.c
M cfgbuild.c
M cfgcleanup.c
M cfgexpand.c
M cfgloopmanip.c
M cfgrtl.c
M cgraph.c
M cgraph.h
M cgraphbuild.c
M cgraphclones.c
M cgraphunit.c
M combine.c
M config/aarch64/aarch64.c
M config/aarch64/cortex-a57-fma-steering.c
M config/aarch64/falkor-tag-collision-avoidance.c
M config/alpha/alpha.c
M config/arm/arm.c
M config/csky/csky.c
M config/elfos.h
M config/i386/i386-expand.c
M config/i386/i386-features.c
M config/i386/i386-options.c
M config/i386/i386.c
M config/i386/x86-tune-sched.c
M config/ia64/ia64.c
M config/m68k/m68k.c
M config/m68k/m68k.md
M config/microblaze/microblaze.c
M config/mips/mips.c
M config/nios2/nios2.c
M config/pa/pa.c
M config/pa/pa.md
M config/rs6000/rs6000-call.c
M config/rs6000/rs6000-logue.c
M config/rs6000/rs6000-p8swap.c
M config/rs6000/rs6000.c
M config/rs6000/rs6000.md
M config/s390/s390.c
M config/sh/sh.c
M config/sparc/sparc.c
M config/xtensa/xtensa.c
M configure
M configure.ac
M cp/call.c
M cp/class.c
M cp/constexpr.c
M cp/constraint.cc
M cp/cp-gimplify.c
M cp/decl.c
M cp/decl2.c
M cp/error.c
M cp/friend.c
M cp/init.c
M cp/method.c
M cp/name-lookup.c
M cp/parser.c
M cp/pt.c
M cp/rtti.c
M cp/semantics.c
M cp/typeck.c
M cp/typeck2.c
M cprop.c
M cse.c
M cselib.c
M d/d-codegen.cc
M d/decl.cc
M d/dmd/doc.c
M d/dmd/dscope.c
M d/dmd/initsem.c
M d/expr.cc
M defaults.h
M df-problems.c
M df-scan.c
M diagnostic-show-locus.c
M doc/invoke.texi
M dse.c
M dumpfile.h
M dwarf2cfi.c
M dwarf2out.c
M emit-rtl.c
M expmed.c
M expr.c
M final.c
M fold-const.c
M fortran/arith.c
M fortran/class.c
M fortran/decl.c
M fortran/dependency.c
M fortran/expr.c
M fortran/frontend-passes.c
M fortran/gfortranspec.c
M fortran/io.c
M fortran/match.c
M fortran/module.c
M fortran/openmp.c
M fortran/parse.c
M fortran/primary.c
M fortran/resolve.c
M fortran/simplify.c
M fortran/trans-array.c
M fortran/trans-common.c
M fortran/trans-decl.c
M fortran/trans-expr.c
M fortran/trans-intrinsic.c
M fortran/trans-openmp.c
M fortran/trans-stmt.c
M fortran/trans-types.c
M fortran/trans.c
M function.c
M fwprop.c
M gcc.c
M gcov.c
M gcse.c
M genattrtab.c
M genautomata.c
M genextract.c
M gengtype.c
M genmatch.c
M genmodes.c
M genrecog.c
M gensupport.c
M ggc-page.c
M gimple-fold.c
M gimple-pretty-print.c
M gimple-ssa-isolate-paths.c
M gimple-ssa-split-paths.c
M gimple-ssa-sprintf.c
M gimple-ssa-store-merging.c
M gimple-ssa-warn-restrict.c
M gimple-streamer-in.c
M gimplify-me.c
M gimplify.c
M go/gofrontend/ast-dump.cc
M go/gofrontend/escape.cc
M go/gofrontend/expressions.cc
M go/gofrontend/gogo.cc
M go/gofrontend/parse.cc
M go/gofrontend/statements.cc
M go/gofrontend/types.cc
M graphite-sese-to-poly.c
M haifa-sched.c
M hash-table.h
M hsa-brig.c
M hsa-common.c
M hsa-gen.c
M hsa-regalloc.c
M ifcvt.c
M inchash.h
M input.c
M ipa-comdats.c
M ipa-cp.c
M ipa-devirt.c
M ipa-fnsummary.c
M ipa-icf.c
M ipa-predicate.c
M ipa-prop.c
M ipa-pure-const.c
M ipa-reference.c
M ipa-split.c
M ipa-sra.c
M ipa.c
M ira-build.c
M ira-color.c
M ira-conflicts.c
M ira-costs.c
M ira-emit.c
M ira-lives.c
M ira.c
M jit/jit-recording.c
M lcm.c
M loop-iv.c
M lower-subreg.c
M lra-constraints.c
M lra-eliminations.c
M lra-lives.c
M lra-remat.c
M lto/lto-common.c
M lto/lto-lang.c
M lto/lto-partition.c
M lto/lto.c
M lto-cgraph.c
M lto-streamer-in.c
M lto-streamer-out.c
M lto-wrapper.c
M mode-switching.c
M modulo-sched.c
M objc/objc-act.c
M omp-expand.c
M omp-general.c
M omp-low.c
M omp-simd-clone.c
M optabs.c
M opts.c
M passes.c
M postreload.c
M predict.c
M pretty-print.c
M print-tree.c
M profile.c
M read-md.c
M read-rtl-function.c
M real.c
M recog.c
M ree.c
M reg-stack.c
M regcprop.c
M regstat.c
M reload.c
M reload1.c
M reorg.c
M resource.c
M rtl.h
M rtlanal.c
M sanopt.c
M sched-deps.c
M sched-rgn.c
M sel-sched.c
M shrink-wrap.c
M simplify-rtx.c
M stmt.c
M stor-layout.c
M store-motion.c
M symtab.c
M tracer.c
M trans-mem.c
M tree-affine.c
M tree-call-cdce.c
M tree-cfg.c
M tree-complex.c
M tree-data-ref.c
M tree-dump.c
M tree-eh.c
M tree-if-conv.c
M tree-inline.c
M tree-into-ssa.c
M tree-loop-distribution.c
M tree-nested.c
M tree-outof-ssa.c
M tree-parloops.c
M tree-scalar-evolution.c
M tree-sra.c
M tree-ssa-alias.c
M tree-ssa-ccp.c
M tree-ssa-coalesce.c
M tree-ssa-dce.c
M tree-ssa-dom.c
M tree-ssa-dse.c
M tree-ssa-forwprop.c
M tree-ssa-ifcombine.c
M tree-ssa-live.c
M tree-ssa-loop-ch.c
M tree-ssa-loop-im.c
M tree-ssa-loop-ivopts.c
M tree-ssa-loop-niter.c
M tree-ssa-loop-unswitch.c
M tree-ssa-math-opts.c
M tree-ssa-operands.c
M tree-ssa-pre.c
M tree-ssa-reassoc.c
M tree-ssa-sccvn.c
M tree-ssa-scopedtables.c
M tree-ssa-sink.c
M tree-ssa-strlen.c
M tree-ssa-structalias.c
M tree-ssa-threadedge.c
M tree-ssa-threadupdate.c
M tree-ssa-uncprop.c
M tree-ssa-uninit.c
M tree-ssa.c
M tree-switch-conversion.c
M tree-tailcall.c
M tree-vect-data-refs.c
M tree-vect-generic.c
M tree-vect-loop-manip.c
M tree-vect-loop.c
M tree-vect-patterns.c
M tree-vect-slp.c
M tree-vect-stmts.c
M tree-vectorizer.c
M tree.c
M var-tracking.c
M varasm.c
M varpool.c
M vr-values.c
M vtable-verify.c
M wide-int.cc
... I guess I will sort it out now and start
with the non-trivial patches.
Thanks
Bernd.
^ permalink raw reply [flat|nested] 8+ messages in thread