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

From: Andres Freund <[email-masked]>
Date: Tue, 1 Nov 2022 11:01:58 -0700

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.

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

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);
      |     ^~~~~

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 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 suspect the shadowing issue might get fixed if we report it, there've been a
bunch of fixes around that not too long ago.

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.

Greetings,

Andres Freund

---

From: Dagfinn Ilmari Mannsåker <[email-masked]>
Date: Tue, 01 Nov 2022 18:55:43 +0000

> 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.

> 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.

> 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.

> /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]

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.

> 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.

> Andres Freund

- ilmari

---

From: Peter Eisentraut <[email-masked]>
Date: Tue, 1 Nov 2022 17:00:27 -0400

> 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.

---

From: Andres Freund <[email-masked]>
Date: Wed, 2 Nov 2022 16:43:46 -0700

Hi,

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? 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

---

From: Tom Lane <[email-masked]>
Date: Wed, 02 Nov 2022 19:57:45 -0400

> 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

---

From: Andres Freund <[email-masked]>
Date: Wed, 2 Nov 2022 17:03:34 -0700

Hi,

> in the gcc manual suggesting that -isystem has any effect on warning

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

---

From: Andres Freund <[email-masked]>
Date: Wed, 28 Dec 2022 10:24:55 -0800

Hi,

> 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

---

From: Tom Lane <[email-masked]>
Date: Wed, 28 Dec 2022 13:43:27 -0500

> 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.

> We could alternatively define HAVE_PRAGMA_SYSTEM_HEADER or such based on

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

---

From: Andres Freund <[email-masked]>
Date: Wed, 28 Dec 2022 16:02:23 -0800

> correctly on platforms where it matters.  Let's keep it simple if we

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

---

From: Tom Lane <[email-masked]>
Date: Wed, 28 Dec 2022 19:05:35 -0500

> that it suffices to use -Werror=unknown-pragmas without a separate configure

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

			regards, tom lane

---

From: Andres Freund <[email-masked]>
Date: Thu, 29 Dec 2022 10:42:36 -0800

Hi,

> 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

---

From: Tom Lane <[email-masked]>
Date: Thu, 29 Dec 2022 13:51:37 -0500

>   #define HAVE_PRAGMA_SYSTEM_HEADER	1

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.

> idea. But I'd let it step in HEAD for a few days of buildfarm coverage first.

+1 to both points.

			regards, tom lane

---

From: Andres Freund <[email-masked]>
Date: Thu, 29 Dec 2022 13:40:13 -0800

Hi,

> 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 ...

> to align better with what you actually have to write.

Makes sense.

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

Oops.

> > idea. But I'd let it step in HEAD for a few days of buildfarm coverage first.

Pushed to HEAD.

Greetings,

Andres Freund