Re: What is lurking in the shadows?

On Fri, 14 May 2021 at 12:00, Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
> That bug led me to wonder if similar problems might be going
> undetected elsewhere in the code. There is a gcc compiler option [3]
> -Wshadow which informs about the similar scenario where one variable
> is "shadowing" another (e.g. redeclaring a variable with the same name
> as one at an outer scope).

> For now, I am not sure how to proceed with this information. Hence this post...

I'm inclined to think that since a bug has already been found due to a
local variable shadowing a global one that it would be good to review
these and then consider if it's worth doing any renaming. I think the
process of looking at each warning individually will allow us to
determine if; a) there are any bugs, or; b) if it's worth doing any
renaming.

I see GCC also has -Wshadow=compatible-local to warn when there
shadowing is going on in local vars where both vars have compatible
types. -Wshadow=local is any local var shadowing, then the option you
used which is the same as -Wshadow=global.

I'd say it might be worth aspiring to reduce the warnings from
building with these flags. If we reduced these down then it might
allow us to more easily identify cases where there are actual bugs.
Maybe we can get to a point where we could enable either
-Wshadow=compatible-local or -Wshadow=local. I doubt we could ever
get to a stage where -Wshadow=global would work for us. There's also
some quite old discussion in [1] that you might want to review.

I don't pretend to have found the best example of ones that we might
want to leave alone, but:

pg_controldata.c: In function ‘wal_level_str’:
pg_controldata.c:73:24: warning: declaration of ‘wal_level’ shadows a
global declaration [-Wshadow]
wal_level_str(WalLevel wal_level)
^
In file included from pg_controldata.c:24:0:
../../../src/include/access/xlog.h:187:24: warning: shadowed
declaration is here [-Wshadow]
extern PGDLLIMPORT int wal_level;

I wonder if it would really clear up much if the parameter name there
was renamed not to shadow the GUC variable's name.

Also, doing any renaming here is not without risk that we break
something, so certainly PG15 at the earliest, unless there is an
actual bug.

I imagine starting with a patch that fixes the ones where the name
does not have much meaning. e.g, i, buf, tmp, lc

We also need to take into account that renaming variables here can
increase the overhead of backpatching fixes. The process of fixing
those up to make the patch apply to the back branch does increase the
chances that bugs could make their way into the back branches.
However, it's probably more likely to end up as a bug if the patch was
written for the back branch then there's a bigger opportunity for the
patch author to pick the wrong variable name when converting the patch
to work with master. In the reverse case, that does not seem as likely
due to both variables having the same name.

David

[1] https://www.postgresql.org/message-id/flat/877k1psmpf.fsf%40mailbox.samurai.com