Feedback-ID: i1ff147bf:Fastmail
From: =?utf-8?Q?Dagfinn_Ilmari_Manns=C3=A5ker?= <ilmari@ilmari.org>
To: Andres Freund <andres@anarazel.de>
Cc: pgsql-hackers@postgresql.org,  Tom Lane <tgl@sss.pgh.pa.us>,  David
 Rowley <dgrowleyml@gmail.com>
Subject: Re: perl 5.36, C99, -Wdeclaration-after-statement
 -Wshadow=compatible-local
References: <20221101180158.l4zg6ljah5ow2vvo@awork3.anarazel.de>
Date: Tue, 01 Nov 2022 18:55:43 +0000
In-Reply-To: <20221101180158.l4zg6ljah5ow2vvo@awork3.anarazel.de> (Andres
	Freund's message of "Tue, 1 Nov 2022 11:01:58 -0700")
Message-ID: <87y1suijww.fsf@wibble.ilmari.org>
User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/27.1 (gnu/linux)
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 8bit
Archived-At: 
 <https://www.postgresql.org/message-id/87y1suijww.fsf%40wibble.ilmari.org>
Precedence: bulk

Andres Freund <andres@anarazel.de> writes:

> Hi,
>
> Tom pinged me privately because mylodon, an animal enforcing C89/C99
> compatibility, was failed. This is due to perl on the machine being upgraded
> to perl 5.36.
>
> Mylodon was failing because of:
>
> configure:18839: ccache clang-13 -c -Wall -Wmissing-prototypes -Wpointer-arith -Wdeclaration-after-statement -Werror=vla -Werror=unguarded-availability-new -Wendif-labels -Wmissing-format-attribute -Wcast-function-type -Wformat-security -fno-strict-aliasing -fwrapv -Wno-unused-command-line-argument -Wno-compound-token-split-by-macro -g -O1 -ggdb -g3 -fno-omit-frame-pointer -Wall -Wextra -Wno-unused-parameter -Wno-sign-compare -Wno-missing-field-initializers -Wno-array-bounds -std=c99 -Wc11-extensions -Werror=c11-extensions  -D_GNU_SOURCE -I/usr/include/libxml2  -I/usr/lib/x86_64-linux-gnu/perl/5.36/CORE conftest.c >&5
> In file included from conftest.c:170:
> In file included from /usr/lib/x86_64-linux-gnu/perl/5.36/CORE/perl.h:5777:
> /usr/lib/x86_64-linux-gnu/perl/5.36/CORE/thread.h:386:8: error: '_Thread_local' is a C11 extension [-Werror,-Wc11-extensions]
> extern PERL_THREAD_LOCAL void *PL_current_context;
>        ^
> /usr/lib/x86_64-linux-gnu/perl/5.36/CORE/config.h:5154:27: note: expanded from macro 'PERL_THREAD_LOCAL'
> #define PERL_THREAD_LOCAL _Thread_local /**/
>                           ^
> 1 error generated.
>
>
> I.e. perl's headers use C11 features, which unsurprisingly doesn't work when
> using -Wc11-extensions -Werror=c11-extensions.

Like Postgres, Perl only requires C99, so any newer features such as
_Thread_local are conditional on compiler support (probed by Configure).

We're not actively testing the fallbacks at the moment, but I'll look at
adding a CI job with the appropriate -Werror flags to make sure it
doesn't break in future.

> For now I worked around this by disabling perl for mylodon, but that's
> obviously not a great fix.

An option would be to build a custom perl with the same -Werror flags
mylodon uses for Postgres (via the -Accflags option to Configure), and
then building Postgres against that.

> perl 5.36 also causes a bunch of warnings locally, where I obviously don't
> use -Wc11-extensions -Werror=c11-extensions:
>
> -Wdeclaration-after-statement produces a few copies of:
> [1767/2259 42  78%] Compiling C object src/pl/plperl/plperl.so.p/meson-generated_.._SPI.c.o
> In file included from /usr/lib/x86_64-linux-gnu/perl/5.36/CORE/perl.h:7242,
>                  from ../../../../home/andres/src/postgresql/src/pl/plperl/plperl.h:82,
>                  from ../../../../home/andres/src/postgresql/src/pl/plperl/SPI.xs:15:
> /usr/lib/x86_64-linux-gnu/perl/5.36/CORE/inline.h: In function ‘Perl_cop_file_avn’:
> /usr/lib/x86_64-linux-gnu/perl/5.36/CORE/inline.h:3489:5: warning: ISO C90 forbids mixed declarations and code [-Wdeclaration-after-statement]
>  3489 |     const char *file = CopFILE(cop);
>       |     ^~~~~
>
> I don't know how much longer we can rely on headers being
> -Wdeclaration-after-statement clean, my impression is that people don't have a
> lot of patience for C89isms anymore.

Perl's C99 policy (https://perldoc.perl.org/perlhacktips#C99) explicitly
permits mixed declarations and code, so I don't think that's likely to
change.

> And -Wshadow=compatible-local triggers the following, very verbose, warning:
>
> [1767/2259 42  78%] Compiling C object src/pl/plperl/plperl.so.p/meson-generated_.._SPI.c.o
> ...
> In file included from /usr/lib/x86_64-linux-gnu/perl/5.36/CORE/perl.h:4155:
> /usr/lib/x86_64-linux-gnu/perl/5.36/CORE/sv_inline.h: In function ‘Perl_newSV_type’:
> /usr/lib/x86_64-linux-gnu/perl/5.36/CORE/handy.h:97:35: warning:
> declaration of ‘p_’ shadows a previous local [-Wshadow=compatible-local]
>    97 | #  define MUTABLE_PTR(p) ({ void *p_ = (p); p_; })
>       |                                   ^~
> /usr/lib/x86_64-linux-gnu/perl/5.36/CORE/sv.h:1394:54: note: in definition of macro ‘SvSTASH_set’
>  1394 |                 (((XPVMG*)  SvANY(sv))->xmg_stash = (val)); } STMT_END
>       |                                                      ^~~
> /usr/lib/x86_64-linux-gnu/perl/5.36/CORE/handy.h:105:32: note: in expansion of macro ‘MUTABLE_PTR’
>   105 | #define MUTABLE_HV(p)   ((HV *)MUTABLE_PTR(p))
>       |                                ^~~~~~~~~~~
> /usr/lib/x86_64-linux-gnu/perl/5.36/CORE/sv_inline.h:487:29: note: in expansion of macro ‘MUTABLE_HV’
>   487 |             SvSTASH_set(io, MUTABLE_HV(SvREFCNT_inc(GvHV(iogv))));
>       |                             ^~~~~~~~~~
> /usr/lib/x86_64-linux-gnu/perl/5.36/CORE/handy.h:107:32: note: in expansion of macro ‘MUTABLE_PTR’
>   107 | #define MUTABLE_SV(p)   ((SV *)MUTABLE_PTR(p))
>       |                                ^~~~~~~~~~~
> /usr/lib/x86_64-linux-gnu/perl/5.36/CORE/sv.h:346:59: note: in expansion of macro ‘MUTABLE_SV’
>   346 | #define SvREFCNT_inc(sv)                Perl_SvREFCNT_inc(MUTABLE_SV(sv))
>       |                                                           ^~~~~~~~~~
> /usr/lib/x86_64-linux-gnu/perl/5.36/CORE/sv_inline.h:487:40: note: in expansion of macro ‘SvREFCNT_inc’
>   487 |             SvSTASH_set(io, MUTABLE_HV(SvREFCNT_inc(GvHV(iogv))));
>       |                                        ^~~~~~~~~~~~
> /usr/lib/x86_64-linux-gnu/perl/5.36/CORE/handy.h:97:35: note: shadowed declaration is here
>    97 | #  define MUTABLE_PTR(p) ({ void *p_ = (p); p_; })
>       |                                   ^~
> /usr/lib/x86_64-linux-gnu/perl/5.36/CORE/sv.h:1394:54: note: in definition of macro ‘SvSTASH_set’
>  1394 |                 (((XPVMG*)  SvANY(sv))->xmg_stash = (val)); } STMT_END
>       |                                                      ^~~
> /usr/lib/x86_64-linux-gnu/perl/5.36/CORE/handy.h:105:32: note: in expansion of macro ‘MUTABLE_PTR’
>   105 | #define MUTABLE_HV(p)   ((HV *)MUTABLE_PTR(p))
>       |                                ^~~~~~~~~~~
> /usr/lib/x86_64-linux-gnu/perl/5.36/CORE/sv_inline.h:487:29: note: in expansion of macro ‘MUTABLE_HV’
>   487 |             SvSTASH_set(io, MUTABLE_HV(SvREFCNT_inc(GvHV(iogv))));
>       |                             ^~~~~~~~~~
>
> I suspect the shadowing issue might get fixed if we report it, there've been a
> bunch of fixes around that not too long ago.

This one might be a bit tricky to fix. The root cause is the MUTABLE_PTR
macro, which is meant to allow casting between different pointer types
without accientally losing constness, which (when GCC brace groups are
supported) is defined as:

#  define MUTABLE_PTR(p) ({ void *p_ = (p); p_; })

And then we have:

#define MUTABLE_xV(p)    ((xV *)MUTABLE_PTR(p))

etc. for all the different value types (AV, GV, HV, SV, etc.)

In the above case, the SvREFCNT_inc() inside the MUTABLE_HV() expands to
something that contains a MUTABLE_SV() call, causing the inner `p_`
variable to shadow the outer one.

> I wonder if we should try to use -isystem for a bunch of external
> dependencies. That way we can keep the more aggressive warnings with a lower
> likelihood of conflicting with stuff outside of our control.

That is worth considering, at least if the above can't easily be fixed,
or if we run across more dependencies with similar problems.

> Greetings,
>
> Andres Freund

- ilmari