perl 5.36, C99, -Wdeclaration-after-statement -Wshadow=compatible-local

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

On 01.11.22 19:01, Andres Freund wrote:

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.

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.

Python has the same issues. There are a few other Python-embedding
projects that use -Wdeclaration-after-statement and complain if the
Python headers violate it. But it's getting tedious. -isystem would be
a better solution.

Hi,

On 2022-11-01 17:00:27 -0400, Peter Eisentraut wrote:

On 01.11.22 19:01, Andres Freund wrote:

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.

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.

Python has the same issues. There are a few other Python-embedding projects
that use -Wdeclaration-after-statement and complain if the Python headers
violate it. But it's getting tedious. -isystem would be a better solution.

Which dependencies should we convert to -isystem? And I assume we should do so
with meson and autoconf? It's easy with meson, it provides a function to
change a dependency to use -isystem without knowing how the compiler spells
that. I guess with autoconf we'd have to check if the compiler understands
-isystem?

The other alternative would be to drop -Wdeclaration-after-statement :)

Greetings,

Andres Freund

Andres Freund <andres@anarazel.de> writes:

On 2022-11-01 17:00:27 -0400, Peter Eisentraut wrote:

Python has the same issues. There are a few other Python-embedding projects
that use -Wdeclaration-after-statement and complain if the Python headers
violate it. But it's getting tedious. -isystem would be a better solution.

Which dependencies should we convert to -isystem?

Color me confused about what's being discussed here. I see nothing
in the gcc manual suggesting that -isystem has any effect on warning
levels?

regards, tom lane

Hi,

On 2022-11-02 19:57:45 -0400, Tom Lane wrote:

Andres Freund <andres@anarazel.de> writes:

On 2022-11-01 17:00:27 -0400, Peter Eisentraut wrote:

Python has the same issues. There are a few other Python-embedding projects
that use -Wdeclaration-after-statement and complain if the Python headers
violate it. But it's getting tedious. -isystem would be a better solution.

Which dependencies should we convert to -isystem?

Color me confused about what's being discussed here. I see nothing
in the gcc manual suggesting that -isystem has any effect on warning
levels?

It's only indirectly explained :(

The -isystem and -idirafter options also mark the directory as a system directory, so that it gets the same special treatment that is applied to
the standard system directories.

and then https://gcc.gnu.org/onlinedocs/cpp/System-Headers.html

Greetings,

Andres Freund

Hi,

On 2022-11-02 17:03:34 -0700, Andres Freund wrote:

On 2022-11-02 19:57:45 -0400, Tom Lane wrote:

Andres Freund <andres@anarazel.de> writes:

On 2022-11-01 17:00:27 -0400, Peter Eisentraut wrote:

Python has the same issues. There are a few other Python-embedding projects
that use -Wdeclaration-after-statement and complain if the Python headers
violate it. But it's getting tedious. -isystem would be a better solution.

Which dependencies should we convert to -isystem?

Color me confused about what's being discussed here. I see nothing
in the gcc manual suggesting that -isystem has any effect on warning
levels?

It's only indirectly explained :(

The -isystem and -idirafter options also mark the directory as a system directory, so that it gets the same special treatment that is applied to
the standard system directories.

and then https://gcc.gnu.org/onlinedocs/cpp/System-Headers.html

The attached *prototype* patch is a slightly different spin on the idea of
using -isystem: It adds a
#pragma GCC system_header
to plperl.h if supported by the compiler. That also avoids warnings from
within plperl and subsidiary headers.

I don't really have an opinion about whether using the pragma or -isystem is
preferrable. I chose the pragma because it makes it easier to grep for headers
where we chose to do this.

I added the pragma detection only to the meson build, but if others think this
is a good way to go, I'll do the necessary autoconf wrangling as well.

In the compiler test, I chose to not check whether -Werror=unknown-pragmas is
supported - it appears to be an old gcc flag, and the worst outcome is that
HAVE_PRAGMA_SYSTEM_HEADER isn't defined.

We could alternatively define HAVE_PRAGMA_SYSTEM_HEADER or such based on
__GNUC__ being defined.

Greetings,

Andres Freund

Andres Freund <andres@anarazel.de> writes:

The attached *prototype* patch is a slightly different spin on the idea of
using -isystem: It adds a
#pragma GCC system_header
to plperl.h if supported by the compiler. That also avoids warnings from
within plperl and subsidiary headers.

I don't really have an opinion about whether using the pragma or -isystem is
preferrable. I chose the pragma because it makes it easier to grep for headers
where we chose to do this.

This seems like a reasonable answer. It feels quite a bit less magic
in the way that it suppresses warnings than -isystem, and also less
likely to have unexpected side-effects (I have a nasty feeling that
-isystem is more magic on macOS than elsewhere). So far it seems
like only the Perl headers have much of an issue, though I can
foresee Python coming along soon.

In the compiler test, I chose to not check whether -Werror=unknown-pragmas is
supported - it appears to be an old gcc flag, and the worst outcome is that
HAVE_PRAGMA_SYSTEM_HEADER isn't defined.
We could alternatively define HAVE_PRAGMA_SYSTEM_HEADER or such based on
__GNUC__ being defined.

Hmm ... I guess the buildfarm would tell us whether that detection works
correctly on platforms where it matters. Let's keep it simple if we
can.

regards, tom lane

On 2022-12-28 13:43:27 -0500, Tom Lane wrote:

In the compiler test, I chose to not check whether -Werror=unknown-pragmas is
supported - it appears to be an old gcc flag, and the worst outcome is that
HAVE_PRAGMA_SYSTEM_HEADER isn't defined.
We could alternatively define HAVE_PRAGMA_SYSTEM_HEADER or such based on
__GNUC__ being defined.

Hmm ... I guess the buildfarm would tell us whether that detection works
correctly on platforms where it matters. Let's keep it simple if we
can.

Quick clarification question: Are you suggesting to use #ifdef __GNUC__, or
that it suffices to use -Werror=unknown-pragmas without a separate configure
check?

Andres Freund <andres@anarazel.de> writes:

On 2022-12-28 13:43:27 -0500, Tom Lane wrote:

Hmm ... I guess the buildfarm would tell us whether that detection works
correctly on platforms where it matters. Let's keep it simple if we
can.

Quick clarification question: Are you suggesting to use #ifdef __GNUC__, or
that it suffices to use -Werror=unknown-pragmas without a separate configure
check?

I'd try -Werror=unknown-pragmas, and then go to the #ifdef if that
doesn't seem to work well.

regards, tom lane

Hi,

On 2022-12-28 19:05:35 -0500, Tom Lane wrote:

Andres Freund <andres@anarazel.de> writes:

On 2022-12-28 13:43:27 -0500, Tom Lane wrote:

Hmm ... I guess the buildfarm would tell us whether that detection works
correctly on platforms where it matters. Let's keep it simple if we
can.

Quick clarification question: Are you suggesting to use #ifdef __GNUC__, or
that it suffices to use -Werror=unknown-pragmas without a separate configure
check?

I'd try -Werror=unknown-pragmas, and then go to the #ifdef if that
doesn't seem to work well.

It turns out to not work terribly well. gcc, quite reasonably, warns about the
pragma used in .c files, and there's no easy way that I found to have autoconf
name its test .h. We could include a test header in the compile test, but that
also adds some complication. As gcc has supported the pragma since 2000, I
think a simple
#ifdef __GNUC__
#define HAVE_PRAGMA_SYSTEM_HEADER 1
#endif
should suffice.

I started to wonder if what we should do instead is to do something like

#ifdef HAVE_PRAGMA_GCC_DIAGNOSTIC
#pragma GCC diagnostic push
#pragma GCC diagnostic ignored "-Wdeclaration-after-statement"
#pragma GCC diagnostic ignored "-Wshadow=compatible-local"
#endif

#include "EXTERN.h"
#include "perl.h"

#ifdef HAVE_PRAGMA_GCC_DIAGNOSTIC
#pragma GCC diagnostic pop
#endif

but that ends up quite complicated because gcc will warn about unknown
warnings being ignored:

../../../../home/andres/src/postgresql/src/pl/plperl/plperl.h:87:32: warning: unknown option after ‘#pragma GCC diagnostic’ kind [-Wpragmas]
87 | #pragma GCC diagnostic ignored "-Wfrakbar"

which would mean we'd need to define a pg_config.h symbol for each potentially
ignored warning, and to guard each '#pragma GCC diagnostic ignored "..."' with
an #ifdef.

Thus I propose the attached.

Should we backpatch this? Given the volume of warnings it's probably a good
idea. But I'd let it step in HEAD for a few days of buildfarm coverage first.

Greetings,

Andres Freund

Andres Freund <andres@anarazel.de> writes:

It turns out to not work terribly well. gcc, quite reasonably, warns about the
pragma used in .c files, and there's no easy way that I found to have autoconf
name its test .h. We could include a test header in the compile test, but that
also adds some complication. As gcc has supported the pragma since 2000, I
think a simple
#ifdef __GNUC__
#define HAVE_PRAGMA_SYSTEM_HEADER 1
#endif
should suffice.

We might find that some GCC-impostor compilers have trouble with it,
but if so we can adjust the #ifdef here.

Getting nitpicky, I suggest calling it "HAVE_PRAGMA_GCC_SYSTEM_HEADER"
to align better with what you actually have to write. Also:

+ * Newer versions the perl headers trigger a lot of warnings with our compiler

"Newer versions of ..." please. Otherwise LGTM.

Should we backpatch this? Given the volume of warnings it's probably a good
idea. But I'd let it step in HEAD for a few days of buildfarm coverage first.

+1 to both points.

regards, tom lane

Hi,

On 2022-12-29 13:51:37 -0500, Tom Lane wrote:

We might find that some GCC-impostor compilers have trouble with it,
but if so we can adjust the #ifdef here.

Yea. I suspect it's widely enough used that any compiler claiming to be gcc
compatible has it, but ...

Getting nitpicky, I suggest calling it "HAVE_PRAGMA_GCC_SYSTEM_HEADER"
to align better with what you actually have to write.

Makes sense.

+ * Newer versions the perl headers trigger a lot of warnings with our compiler

"Newer versions of ..." please. Otherwise LGTM.

Oops.

Should we backpatch this? Given the volume of warnings it's probably a good
idea. But I'd let it step in HEAD for a few days of buildfarm coverage first.

+1 to both points.

Pushed to HEAD.

Greetings,

Andres Freund

Hi,

On 2022-12-29 13:40:13 -0800, Andres Freund wrote:

Should we backpatch this? Given the volume of warnings it's probably a good
idea. But I'd let it step in HEAD for a few days of buildfarm coverage first.

+1 to both points.

Pushed to HEAD.

I haven't seen any problems in HEAD, so I'm working on backpatching.

Greetings,

Andres Freund

On 2023-01-02 15:46:36 -0800, Andres Freund wrote:

On 2022-12-29 13:40:13 -0800, Andres Freund wrote:

Should we backpatch this? Given the volume of warnings it's probably a good
idea. But I'd let it step in HEAD for a few days of buildfarm coverage first.

+1 to both points.

Pushed to HEAD.

I haven't seen any problems in HEAD, so I'm working on backpatching.

And done.

Andres Freund <andres@anarazel.de> writes:

On 2023-01-02 15:46:36 -0800, Andres Freund wrote:

I haven't seen any problems in HEAD, so I'm working on backpatching.

And done.

It occurs to me that we should now be able to drop configure's
probe for -Wno-compound-token-split-by-macro, since that was only
needed to suppress warnings in the Perl headers. Won't save much
of course, but every test we can get rid of is worth doing IMO.

regards, tom lane

I wrote:

It occurs to me that we should now be able to drop configure's
probe for -Wno-compound-token-split-by-macro, since that was only
needed to suppress warnings in the Perl headers.

... or not. A bit of experimentation says that they still come out,
apparently because the warnings are triggered by *use* of relevant
Perl macros not by their *definitions*. Oh well.

regards, tom lane