Обсуждение: meson: Specify -Wformat as a common warning flag for extensions

Hi,

I'm an extension developer. If I use PostgreSQL built with
Meson, I get the following warning:

    cc1: warning: '-Wformat-security' ignored without '-Wformat' [-Wformat-security]

Because "pg_config --cflags" includes -Wformat-security but
doesn't include -Wformat.

Can we specify -Wformat as a common warning flag too? If we
do it, "pg_config --cflags" includes both of
-Wformat-security and -Wformat. So I don't get the warning.


Thanks,
-- 
kou
From 0913033512c9b75ee3d2941c89ff8696f3c5f53b Mon Sep 17 00:00:00 2001
From: Sutou Kouhei <kou@clear-code.com>
Date: Mon, 22 Jan 2024 13:51:58 +0900
Subject: [PATCH v1] meson: Specify -Wformat explicitly for extensions

We specify -Wformat-security as a common warning flag explicitly. Our
common warning flags are used by extensions via
pgxs/src/Makefile.global or "pg_config --cflags".

If -Wformat-security is used without -Wall/-Wformat, GCC shows the
following warning:

    cc1: warning: '-Wformat-security' ignored without '-Wformat' [-Wformat-security]

We can't assume that all extensions use -Wall/-Wformat. So specifying
only -Wformat-security may cause the warning.

If we specify -Wformat explicitly, the warning isn't shown.
---
 meson.build | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/meson.build b/meson.build
index 55184db248..de6ce778fc 100644
--- a/meson.build
+++ b/meson.build
@@ -1822,6 +1822,14 @@ common_warning_flags = [
   '-Wimplicit-fallthrough=3',
   '-Wcast-function-type',
   '-Wshadow=compatible-local',
+  # This is for preventing the "cc1: warning: '-Wformat-security'
+  # ignored without '-Wformat' [-Wformat-security]" warning. We don't
+  # need this for PostgreSQL itself. This is just for
+  # extensions. Extensions use "pg_config --cflags" to build
+  # themselves. If extensions use only -Wformat-security, the warning
+  # is shown. If we have this here, extensions use both of -Wformat
+  # and -Wformat-security. So the warning isn't shown.
+  '-Wformat',
   # This was included in -Wall/-Wformat in older GCC versions
   '-Wformat-security',
 ]
-- 
2.43.0

Hi,

Could someone take a look at this?

Patch is attached in the original e-mail:
https://www.postgresql.org/message-id/20240122.141139.931086145628347157.kou%40clear-code.com


Thanks,
-- 
kou

In <20240122.141139.931086145628347157.kou@clear-code.com>
  "meson: Specify -Wformat as a common warning flag for extensions" on Mon, 22 Jan 2024 14:11:39 +0900 (JST),
  Sutou Kouhei <kou@clear-code.com> wrote:

> Hi,
> 
> I'm an extension developer. If I use PostgreSQL built with
> Meson, I get the following warning:
> 
>     cc1: warning: '-Wformat-security' ignored without '-Wformat' [-Wformat-security]
> 
> Because "pg_config --cflags" includes -Wformat-security but
> doesn't include -Wformat.
> 
> Can we specify -Wformat as a common warning flag too? If we
> do it, "pg_config --cflags" includes both of
> -Wformat-security and -Wformat. So I don't get the warning.
> 
> 
> Thanks,
> -- 
> kou

On Sun Jan 21, 2024 at 11:11 PM CST, Sutou Kouhei wrote:
> Hi,
>
> I'm an extension developer. If I use PostgreSQL built with
> Meson, I get the following warning:
>
>     cc1: warning: '-Wformat-security' ignored without '-Wformat' [-Wformat-security]
>
> Because "pg_config --cflags" includes -Wformat-security but
> doesn't include -Wformat.
>
> Can we specify -Wformat as a common warning flag too? If we
> do it, "pg_config --cflags" includes both of
> -Wformat-security and -Wformat. So I don't get the warning.

The GCC documentation[0] says the following:

> If -Wformat is specified, also warn about uses of format functions
> that represent possible security problems. At present, this warns
> about calls to printf and scanf functions where the format string is
> not a string literal and there are no format arguments, as in printf
> (foo);. This may be a security hole if the format string came from
> untrusted input and contains ‘%n’. (This is currently a subset of what
> -Wformat-nonliteral warns about, but in future warnings may be added
> to -Wformat-security that are not included in -Wformat-nonliteral.)

It sounds like a legitimate issue. I have confirmed the issue exists
with a pg_config compiled with Meson. I can also confirm that this issue
exists in the autotools build.

Here is a v2 of your patch which includes the fix for autotools. I will
mark this "Ready for Committer" in the commitfest. Thanks!

[0]: https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html

--
Tristan Partin
Neon (https://neon.tech)

On Thu, Mar 07, 2024 at 11:39:39PM -0600, Tristan Partin wrote:
> It sounds like a legitimate issue. I have confirmed the issue exists with a
> pg_config compiled with Meson. I can also confirm that this issue exists in
> the autotools build.

First time I'm hearing about that, but I'll admit that I am cheating
because -Wformat is forced in my local builds for some time now.  I'm
failing to see the issue with meson and ./configure even if I remove
the switch, though, using a recent version of gcc at 13.2.0, but
perhaps Debian does something underground.  Are there version and/or
environment requirements to be aware of?

Forcing -Wformat implies more stuff that can be disabled with
-Wno-format-contains-nul, -Wno-format-extra-args, and
-Wno-format-zero-length, but the thing is that we're usually very
conservative with such additions in the scripts.  See also
8b6f5f25102f, done, I guess, as an answer to this thread:
https://www.postgresql.org/message-id/4D431505.9010002%40dunslane.net

A quick look at the past history of pgsql-hackers does not mention
that as a problem, either, but I may have missed something.
--
Michael

Hi,

In <Zeqw9vGrYlb250aO@paquier.xyz>
  "Re: meson: Specify -Wformat as a common warning flag for extensions" on Fri, 8 Mar 2024 15:32:22 +0900,
  Michael Paquier <michael@paquier.xyz> wrote:

>                                             Are there version and/or
> environment requirements to be aware of?

I'm using Debian GNU/Linux sid and I can reproduce with gcc
8-13:

$ for x in {8..13}; do; echo gcc-${x}; gcc-${x} -Wformat-security -E - < /dev/null > /dev/null; done
gcc-8
cc1: warning: -Wformat-security ignored without -Wformat [-Wformat-security]
gcc-9
cc1: warning: '-Wformat-security' ignored without '-Wformat' [-Wformat-security]
gcc-10
cc1: warning: '-Wformat-security' ignored without '-Wformat' [-Wformat-security]
gcc-11
cc1: warning: '-Wformat-security' ignored without '-Wformat' [-Wformat-security]
gcc-12
cc1: warning: '-Wformat-security' ignored without '-Wformat' [-Wformat-security]
gcc-13
cc1: warning: '-Wformat-security' ignored without '-Wformat' [-Wformat-security]
$

I tried this on Ubuntu 22.04 too but this isn't reproduced:

$ gcc-11 -Wformat-security -E - < /dev/null > /dev/null
$

It seems that Ubuntu enables -Wformat by default:

$ gcc-11 -Wno-format -Wformat-security -E - < /dev/null > /dev/null
cc1: warning: '-Wformat-security' ignored without '-Wformat' [-Wformat-security]

I tried this on AlmaLinux 9 too and this is reproduced:

$ gcc --version
gcc (GCC) 11.4.1 20230605 (Red Hat 11.4.1-2)
Copyright (C) 2021 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
$ gcc -Wformat-security -E - < /dev/null > /dev/null
cc1: warning: '-Wformat-security' ignored without '-Wformat' [-Wformat-security]

> Forcing -Wformat implies more stuff that can be disabled with
> -Wno-format-contains-nul, -Wno-format-extra-args, and
> -Wno-format-zero-length, but the thing is that we're usually very
> conservative with such additions in the scripts.  See also
> 8b6f5f25102f, done, I guess, as an answer to this thread:
> https://www.postgresql.org/message-id/4D431505.9010002%40dunslane.net

I think that this is not a problem. Because the comment
added by 8b6f5f25102f ("This was included in -Wall/-Wformat
in older GCC versions") implies that we want to always use
-Wformat-security. -Wformat-security isn't worked without
-Wformat:

https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html#index-Wformat-security

> If -Wformat is specified, also warn about uses of format
> functions that represent possible security problems.


Thanks,
-- 
kou

On Fri Mar 8, 2024 at 12:32 AM CST, Michael Paquier wrote:
> On Thu, Mar 07, 2024 at 11:39:39PM -0600, Tristan Partin wrote:
> > It sounds like a legitimate issue. I have confirmed the issue exists with a
> > pg_config compiled with Meson. I can also confirm that this issue exists in
> > the autotools build.
>
> First time I'm hearing about that, but I'll admit that I am cheating
> because -Wformat is forced in my local builds for some time now.  I'm
> failing to see the issue with meson and ./configure even if I remove
> the switch, though, using a recent version of gcc at 13.2.0, but
> perhaps Debian does something underground.  Are there version and/or
> environment requirements to be aware of?
>
> Forcing -Wformat implies more stuff that can be disabled with
> -Wno-format-contains-nul, -Wno-format-extra-args, and
> -Wno-format-zero-length, but the thing is that we're usually very
> conservative with such additions in the scripts.  See also
> 8b6f5f25102f, done, I guess, as an answer to this thread:
> https://www.postgresql.org/message-id/4D431505.9010002%40dunslane.net
>
> A quick look at the past history of pgsql-hackers does not mention
> that as a problem, either, but I may have missed something.

Ok, I figured this out. -Wall implies -Wformat=1. We set warning_level
to 1 in the Meson project() call, which implies -Wall, and set -Wall in
CFLAGS for autoconf. That's the reason we don't get issues building
Postgres. A user making use of the pg_config --cflags option, as Sutou
is, *will* run into the aforementioned issues, since we don't propogate
-Wall into pg_config.

    $ gcc $(pg_config --cflags) -E - < /dev/null > /dev/null
    cc1: warning: ‘-Wformat-security’ ignored without ‘-Wformat’ [-Wformat-security]
    $ gcc -Wall $(pg_config --cflags) -E - < /dev/null > /dev/null
    (nothing printed)

--
Tristan Partin
Neon (https://neon.tech)

Hi,

In <CZOHWDYQJQCQ.23A5RRV1E05N2@neon.tech>
  "Re: meson: Specify -Wformat as a common warning flag for extensions" on Fri, 08 Mar 2024 10:05:27 -0600,
  "Tristan Partin" <tristan@neon.tech> wrote:

> Ok, I figured this out. -Wall implies -Wformat=1. We set warning_level
> to 1 in the Meson project() call, which implies -Wall, and set -Wall
> in CFLAGS for autoconf. That's the reason we don't get issues building
> Postgres. A user making use of the pg_config --cflags option, as Sutou
> is, *will* run into the aforementioned issues, since we don't
> propogate -Wall into pg_config.
> 
>     $ gcc $(pg_config --cflags) -E - < /dev/null > /dev/null
>     cc1: warning: ‘-Wformat-security’ ignored without ‘-Wformat’
>     [-Wformat-security]
>     $ gcc -Wall $(pg_config --cflags) -E - < /dev/null > /dev/null
>     (nothing printed)

Thanks for explaining this. You're right. This is the reason
why we don't need this for PostgreSQL itself but we need
this for PostgreSQL extensions. Sorry. I should have
explained this in the first e-mail...


What should we do to proceed this patch?


Thanks,
-- 
kou

On Tue Mar 12, 2024 at 6:56 PM CDT, Sutou Kouhei wrote:
> Hi,
>
> In <CZOHWDYQJQCQ.23A5RRV1E05N2@neon.tech>
>   "Re: meson: Specify -Wformat as a common warning flag for extensions" on Fri, 08 Mar 2024 10:05:27 -0600,
>   "Tristan Partin" <tristan@neon.tech> wrote:
>
> > Ok, I figured this out. -Wall implies -Wformat=1. We set warning_level
> > to 1 in the Meson project() call, which implies -Wall, and set -Wall
> > in CFLAGS for autoconf. That's the reason we don't get issues building
> > Postgres. A user making use of the pg_config --cflags option, as Sutou
> > is, *will* run into the aforementioned issues, since we don't
> > propogate -Wall into pg_config.
> >
> >     $ gcc $(pg_config --cflags) -E - < /dev/null > /dev/null
> >     cc1: warning: ‘-Wformat-security’ ignored without ‘-Wformat’
> >     [-Wformat-security]
> >     $ gcc -Wall $(pg_config --cflags) -E - < /dev/null > /dev/null
> >     (nothing printed)
>
> Thanks for explaining this. You're right. This is the reason
> why we don't need this for PostgreSQL itself but we need
> this for PostgreSQL extensions. Sorry. I should have
> explained this in the first e-mail...
>
>
> What should we do to proceed this patch?

Perhaps adding some more clarification in the comments that I wrote.

-  # -Wformat-security requires -Wformat, so check for it
+  # -Wformat-secuirty requires -Wformat. We compile with -Wall in
+  # Postgres, which includes -Wformat=1. -Wformat is shorthand for
+  # -Wformat=1. The set of flags which includes -Wformat-security is
+  # persisted into pg_config --cflags, which is commonly used by
+  # PGXS-based extensions. The lack of -Wformat in the persisted flags
+  # will produce a warning on many GCC versions, so even though adding
+  # -Wformat here is a no-op for Postgres, it silences other use cases.

That might be too long-winded though :).

--
Tristan Partin
Neon (https://neon.tech)

Hi,

In <CZSDSNYEUHUL.399XLPGCJSJ5H@neon.tech>
  "Re: meson: Specify -Wformat as a common warning flag for extensions" on Wed, 13 Mar 2024 00:43:11 -0500,
  "Tristan Partin" <tristan@neon.tech> wrote:

> Perhaps adding some more clarification in the comments that I wrote.
> 
> -  # -Wformat-security requires -Wformat, so check for it
> + # -Wformat-secuirty requires -Wformat. We compile with -Wall in + #
> Postgres, which includes -Wformat=1. -Wformat is shorthand for + #
> -Wformat=1. The set of flags which includes -Wformat-security is + #
> persisted into pg_config --cflags, which is commonly used by + #
> PGXS-based extensions. The lack of -Wformat in the persisted flags
> + # will produce a warning on many GCC versions, so even though adding
> + # -Wformat here is a no-op for Postgres, it silences other use
> cases.
> 
> That might be too long-winded though :).

Thanks for the wording! I used it for the v3 patch.


Thanks,
-- 
kou

From 0ba2e6dd55b00ee8a57313a629a1e5fa8c9e40cc Mon Sep 17 00:00:00 2001
From: Sutou Kouhei <kou@clear-code.com>
Date: Wed, 13 Mar 2024 16:10:34 +0900
Subject: [PATCH v3] Add -Wformat to common warning flags

We specify -Wformat-security as a common warning flag explicitly. GCC
requires -Wformat to be added for -Wformat-security to take effect. If
-Wformat-security is used without -Wformat, GCC shows the following
warning:

    cc1: warning: '-Wformat-security' ignored without '-Wformat' [-Wformat-security]

Note that this is not needed for PostgreSQL itself because PostgreSQL
uses -Wall, which includes -Wformat=1. -Wformat is shorthand for
-Wformat=1. These flags without -Wall are persisted into "pg_config
--cflags", which is commonly used by PGXS-based extensions. So
PGXS-based extensions will get the warning without -Wformat.

Co-authored-by: Tristan Partin <tristan@neon.tech>
---
 configure    | 99 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 configure.ac | 10 ++++++
 meson.build  |  9 +++++
 3 files changed, 118 insertions(+)

diff --git a/configure b/configure
index 70a1968003..7b0fda3825 100755
--- a/configure
+++ b/configure
@@ -6013,6 +6013,105 @@ if test x"$pgac_cv_prog_CXX_cxxflags__Wshadow_compatible_local" = x"yes"; then
 fi
 
 
+  # -Wformat-security requires -Wformat. We compile with -Wall in
+  # PostgreSQL, which includes -Wformat=1. -Wformat is shorthand for
+  # -Wformat=1. The set of flags which includes -Wformat-security is
+  # persisted into pg_config --cflags, which is commonly used by
+  # PGXS-based extensions. The lack of -Wformat in the persisted flags
+  # will produce a warning on many GCC versions, so even though adding
+  # -Wformat here is a no-op for PostgreSQL, it silences other use
+  # cases., so check for it.
+
+{ $as_echo "$as_me:${as_lineno-$LINENO}: checking whether ${CC} supports -Wformat, for CFLAGS" >&5
+$as_echo_n "checking whether ${CC} supports -Wformat, for CFLAGS... " >&6; }
+if ${pgac_cv_prog_CC_cflags__Wformat+:} false; then :
+  $as_echo_n "(cached) " >&6
+else
+  pgac_save_CFLAGS=$CFLAGS
+pgac_save_CC=$CC
+CC=${CC}
+CFLAGS="${CFLAGS} -Wformat"
+ac_save_c_werror_flag=$ac_c_werror_flag
+ac_c_werror_flag=yes
+cat confdefs.h - <<_ACEOF >conftest.$ac_ext
+/* end confdefs.h.  */
+
+int
+main ()
+{
+
+  ;
+  return 0;
+}
+_ACEOF
+if ac_fn_c_try_compile "$LINENO"; then :
+  pgac_cv_prog_CC_cflags__Wformat=yes
+else
+  pgac_cv_prog_CC_cflags__Wformat=no
+fi
+rm -f core conftest.err conftest.$ac_objext conftest.$ac_ext
+ac_c_werror_flag=$ac_save_c_werror_flag
+CFLAGS="$pgac_save_CFLAGS"
+CC="$pgac_save_CC"
+fi
+{ $as_echo "$as_me:${as_lineno-$LINENO}: result: $pgac_cv_prog_CC_cflags__Wformat" >&5
+$as_echo "$pgac_cv_prog_CC_cflags__Wformat" >&6; }
+if test x"$pgac_cv_prog_CC_cflags__Wformat" = x"yes"; then
+  CFLAGS="${CFLAGS} -Wformat"
+fi
+
+
+  { $as_echo "$as_me:${as_lineno-$LINENO}: checking whether ${CXX} supports -Wformat, for CXXFLAGS" >&5
+$as_echo_n "checking whether ${CXX} supports -Wformat, for CXXFLAGS... " >&6; }
+if ${pgac_cv_prog_CXX_cxxflags__Wformat+:} false; then :
+  $as_echo_n "(cached) " >&6
+else
+  pgac_save_CXXFLAGS=$CXXFLAGS
+pgac_save_CXX=$CXX
+CXX=${CXX}
+CXXFLAGS="${CXXFLAGS} -Wformat"
+ac_save_cxx_werror_flag=$ac_cxx_werror_flag
+ac_cxx_werror_flag=yes
+ac_ext=cpp
+ac_cpp='$CXXCPP $CPPFLAGS'
+ac_compile='$CXX -c $CXXFLAGS $CPPFLAGS conftest.$ac_ext >&5'
+ac_link='$CXX -o conftest$ac_exeext $CXXFLAGS $CPPFLAGS $LDFLAGS conftest.$ac_ext $LIBS >&5'
+ac_compiler_gnu=$ac_cv_cxx_compiler_gnu
+
+cat confdefs.h - <<_ACEOF >conftest.$ac_ext
+/* end confdefs.h.  */
+
+int
+main ()
+{
+
+  ;
+  return 0;
+}
+_ACEOF
+if ac_fn_cxx_try_compile "$LINENO"; then :
+  pgac_cv_prog_CXX_cxxflags__Wformat=yes
+else
+  pgac_cv_prog_CXX_cxxflags__Wformat=no
+fi
+rm -f core conftest.err conftest.$ac_objext conftest.$ac_ext
+ac_ext=c
+ac_cpp='$CPP $CPPFLAGS'
+ac_compile='$CC -c $CFLAGS $CPPFLAGS conftest.$ac_ext >&5'
+ac_link='$CC -o conftest$ac_exeext $CFLAGS $CPPFLAGS $LDFLAGS conftest.$ac_ext $LIBS >&5'
+ac_compiler_gnu=$ac_cv_c_compiler_gnu
+
+ac_cxx_werror_flag=$ac_save_cxx_werror_flag
+CXXFLAGS="$pgac_save_CXXFLAGS"
+CXX="$pgac_save_CXX"
+fi
+{ $as_echo "$as_me:${as_lineno-$LINENO}: result: $pgac_cv_prog_CXX_cxxflags__Wformat" >&5
+$as_echo "$pgac_cv_prog_CXX_cxxflags__Wformat" >&6; }
+if test x"$pgac_cv_prog_CXX_cxxflags__Wformat" = x"yes"; then
+  CXXFLAGS="${CXXFLAGS} -Wformat"
+fi
+
+
   # This was included in -Wall/-Wformat in older GCC versions
 
 { $as_echo "$as_me:${as_lineno-$LINENO}: checking whether ${CC} supports -Wformat-security, for CFLAGS" >&5
diff --git a/configure.ac b/configure.ac
index 52fd7af446..e4776b9963 100644
--- a/configure.ac
+++ b/configure.ac
@@ -533,6 +533,16 @@ if test "$GCC" = yes -a "$ICC" = no; then
   PGAC_PROG_CXX_CFLAGS_OPT([-Wcast-function-type])
   PGAC_PROG_CC_CFLAGS_OPT([-Wshadow=compatible-local])
   PGAC_PROG_CXX_CFLAGS_OPT([-Wshadow=compatible-local])
+  # -Wformat-security requires -Wformat. We compile with -Wall in
+  # PostgreSQL, which includes -Wformat=1. -Wformat is shorthand for
+  # -Wformat=1. The set of flags which includes -Wformat-security is
+  # persisted into pg_config --cflags, which is commonly used by
+  # PGXS-based extensions. The lack of -Wformat in the persisted flags
+  # will produce a warning on many GCC versions, so even though adding
+  # -Wformat here is a no-op for PostgreSQL, it silences other use
+  # cases., so check for it.
+  PGAC_PROG_CC_CFLAGS_OPT([-Wformat])
+  PGAC_PROG_CXX_CFLAGS_OPT([-Wformat])
   # This was included in -Wall/-Wformat in older GCC versions
   PGAC_PROG_CC_CFLAGS_OPT([-Wformat-security])
   PGAC_PROG_CXX_CFLAGS_OPT([-Wformat-security])
diff --git a/meson.build b/meson.build
index 55184db248..732cb59b1f 100644
--- a/meson.build
+++ b/meson.build
@@ -1822,6 +1822,15 @@ common_warning_flags = [
   '-Wimplicit-fallthrough=3',
   '-Wcast-function-type',
   '-Wshadow=compatible-local',
+  # -Wformat-security requires -Wformat. We compile with -Wall in
+  # PostgreSQL, which includes -Wformat=1. -Wformat is shorthand for
+  # -Wformat=1. The set of flags which includes -Wformat-security is
+  # persisted into pg_config --cflags, which is commonly used by
+  # PGXS-based extensions. The lack of -Wformat in the persisted flags
+  # will produce a warning on many GCC versions, so even though adding
+  # -Wformat here is a no-op for PostgreSQL, it silences other use
+  # cases., so check for it.
+  '-Wformat',
   # This was included in -Wall/-Wformat in older GCC versions
   '-Wformat-security',
 ]
-- 
2.43.0

On 08.03.24 17:05, Tristan Partin wrote:
> Ok, I figured this out. -Wall implies -Wformat=1. We set warning_level 
> to 1 in the Meson project() call, which implies -Wall, and set -Wall in 
> CFLAGS for autoconf. That's the reason we don't get issues building 
> Postgres. A user making use of the pg_config --cflags option, as Sutou 
> is, *will* run into the aforementioned issues, since we don't propogate 
> -Wall into pg_config.

(The actual mechanism for extensions is that they get CFLAGS from 
Makefile.global, but pg_config has the same underlying issue.)

I think the fix then is to put -Wall into CFLAGS in Makefile.global. 
Looking at a diff of Makefile.global between an autoconf and a meson 
build, I also see that under meson, CFLAGS doesn't get -O2 -g (or 
similar, depending on settings).  This presumably has the same 
underlying issue that meson handles those flags internally.

For someone who wants to write a fix for this, the relevant variable is 
var_cflags in our meson scripts.  And var_cxxflags as well.

Hi,

In <49e97fd0-c17e-4cbc-aeee-80ac51400736@eisentraut.org>
  "Re: meson: Specify -Wformat as a common warning flag for extensions" on Wed, 13 Mar 2024 08:38:28 +0100,
  Peter Eisentraut <peter@eisentraut.org> wrote:

> I think the fix then is to put -Wall into CFLAGS in
> Makefile.global. Looking at a diff of Makefile.global between an
> autoconf and a meson build, I also see that under meson, CFLAGS
> doesn't get -O2 -g (or similar, depending on settings).  This
> presumably has the same underlying issue that meson handles those
> flags internally.
> 
> For someone who wants to write a fix for this, the relevant variable
> is var_cflags in our meson scripts.  And var_cxxflags as well.

How about the attached v4 patch?


Thanks,
-- 
kou
From a515720338dc49e764f598021200316c6d01ddf9 Mon Sep 17 00:00:00 2001
From: Sutou Kouhei <kou@clear-code.com>
Date: Fri, 15 Mar 2024 18:27:30 +0900
Subject: [PATCH v4] meson: Restore implicit warning/debug/optimize flags for
 extensions

Meson specifies warning/debug/optimize flags such as "-Wall", "-g" and
"-O2" automatically based on "--warnlevel" and "--buildtype"
options. And we use "--warning_level=1" and
"--buildtype=debugoptimized" by default.

We don't specify warning/debug/optimize flags explicitly to build
PostgreSQL with Meson. Because Meson does it automatically as we said.
But Meson doesn't care about flags in Makefile.global and
pg_config. So we need to care about them manually.

This changes do it. They detect warning/debug/optimize flags based on
warning_level/debug/optimization option values because Meson doesn't
tell us flags Meson guessed.
---
 meson.build             | 40 ++++++++++++++++++++++++++++++++++++++++
 src/include/meson.build |  4 ++--
 2 files changed, 42 insertions(+), 2 deletions(-)

diff --git a/meson.build b/meson.build
index c8fdfeb0ec..3c5c449a0a 100644
--- a/meson.build
+++ b/meson.build
@@ -1824,6 +1824,46 @@ endif
 vectorize_cflags = cc.get_supported_arguments(['-ftree-vectorize'])
 unroll_loops_cflags = cc.get_supported_arguments(['-funroll-loops'])
 
+# They aren't used for building PostgreSQL itself because Meson does
+# everything internally. They are used by extensions via pg_config or
+# Makefile.global.
+common_builtin_flags = []
+
+warning_level = get_option('warning_level')
+# See https://mesonbuild.com/Builtin-options.html#details-for-warning_level for
+# warning_level values.
+if warning_level == '1'
+  common_builtin_flags += ['-Wall', '/W2']
+elif warning_level == '2'
+  common_builtin_flags += ['-Wall', '-Wextra', '/W3']
+elif warning_level == '3'
+  common_builtin_flags += ['-Wall', '-Wextra', '-Wpedantic', '/W4']
+elif warning_level == 'everything'
+  common_builtin_flags += ['-Weverything', '/Wall']
+endif
+
+if get_option('debug')
+  common_builtin_flags += ['-g']
+endif
+
+optimization = get_option('optimization')
+if optimization == '0'
+  common_builtin_flags += ['-O0']
+elif optimization == '1'
+  common_builtin_flags += ['-O1']
+elif optimization == '2'
+  common_builtin_flags += ['-O2']
+elif optimization == '3'
+  common_builtin_flags += ['-O3']
+elif optimization == 's'
+  common_builtin_flags += ['-Os']
+endif
+
+cflags_builtin = cc.get_supported_arguments(common_builtin_flags)
+if llvm.found()
+  cxxflags_builtin = cpp.get_supported_arguments(common_builtin_flags)
+endif
+
 common_warning_flags = [
   '-Wmissing-prototypes',
   '-Wpointer-arith',
diff --git a/src/include/meson.build b/src/include/meson.build
index a28f115d86..58b7a9c1e7 100644
--- a/src/include/meson.build
+++ b/src/include/meson.build
@@ -44,9 +44,9 @@ config_paths_data.set_quoted('MANDIR', dir_prefix / dir_man)
 
 var_cc = ' '.join(cc.cmd_array())
 var_cpp = ' '.join(cc.cmd_array() + ['-E'])
-var_cflags = ' '.join(cflags + cflags_warn + get_option('c_args'))
+var_cflags = ' '.join(cflags + cflags_builtin + cflags_warn + get_option('c_args'))
 if llvm.found()
-  var_cxxflags = ' '.join(cxxflags + cxxflags_warn + get_option('cpp_args'))
+  var_cxxflags = ' '.join(cxxflags + cxxflags_builtin + cxxflags_warn + get_option('cpp_args'))
 else
   var_cxxflags = ''
 endif
-- 
2.43.0

Hi,

On 2024-03-15 18:36:55 +0900, Sutou Kouhei wrote:
> +warning_level = get_option('warning_level')
> +# See https://mesonbuild.com/Builtin-options.html#details-for-warning_level for
> +# warning_level values.
> +if warning_level == '1'
> +  common_builtin_flags += ['-Wall', '/W2']
> +elif warning_level == '2'
> +  common_builtin_flags += ['-Wall', '-Wextra', '/W3']
> +elif warning_level == '3'
> +  common_builtin_flags += ['-Wall', '-Wextra', '-Wpedantic', '/W4']
> +elif warning_level == 'everything'
> +  common_builtin_flags += ['-Weverything', '/Wall']
> +endif

> +cflags_builtin = cc.get_supported_arguments(common_builtin_flags)
> +if llvm.found()
> +  cxxflags_builtin = cpp.get_supported_arguments(common_builtin_flags)
> +endif

This seems like a fair amount of extra configure tests. Particularly because
/W* isn't ever interesting for Makefile.global - they're msvc flags - because
you can't use that with msvc.

I'm also doubtful that it's worth supporting warning_level=3/everything, you
end up with a completely flood of warnings that way.

Greetings,

Andres Freund

Hi Andres,

Thanks for reviewing this!

In <20240407232635.fq4kc5556lahaoej@awork3.anarazel.de>
  "Re: meson: Specify -Wformat as a common warning flag for extensions" on Sun, 7 Apr 2024 16:26:35 -0700,
  Andres Freund <andres@anarazel.de> wrote:

> This seems like a fair amount of extra configure tests. Particularly because
> /W* isn't ever interesting for Makefile.global - they're msvc flags - because
> you can't use that with msvc.
> 
> I'm also doubtful that it's worth supporting warning_level=3/everything, you
> end up with a completely flood of warnings that way.

OK. I've removed "/W*" flags and warning_level==3/everything
cases.

How about the attached v5 patch?


Thanks,
-- 
kou
From 205ef88c66cf1050eedfc1e72d951de93a02e53a Mon Sep 17 00:00:00 2001
From: Sutou Kouhei <kou@clear-code.com>
Date: Fri, 15 Mar 2024 18:27:30 +0900
Subject: [PATCH v5] meson: Restore implicit warning/debug/optimize flags for
 extensions

Meson specifies warning/debug/optimize flags such as "-Wall", "-g" and
"-O2" automatically based on "--warnlevel" and "--buildtype"
options. And we use "--warning_level=1" and
"--buildtype=debugoptimized" by default.

We don't specify warning/debug/optimize flags explicitly to build
PostgreSQL with Meson. Because Meson does it automatically as we said.
But Meson doesn't care about flags in Makefile.global and
pg_config. So we need to care about them manually.

This changes do it. They detect warning/debug/optimize flags based on
warning_level/debug/optimization option values because Meson doesn't
tell us flags Meson guessed.
---
 meson.build             | 41 +++++++++++++++++++++++++++++++++++++++++
 src/include/meson.build |  4 ++--
 2 files changed, 43 insertions(+), 2 deletions(-)

diff --git a/meson.build b/meson.build
index 5acf083ce3c..11bd56f79a7 100644
--- a/meson.build
+++ b/meson.build
@@ -1848,6 +1848,47 @@ endif
 vectorize_cflags = cc.get_supported_arguments(['-ftree-vectorize'])
 unroll_loops_cflags = cc.get_supported_arguments(['-funroll-loops'])
 
+# They aren't used for building PostgreSQL itself because Meson does
+# everything internally. They are used by extensions via pg_config or
+# Makefile.global.
+common_builtin_flags = []
+
+warning_level = get_option('warning_level')
+# See https://mesonbuild.com/Builtin-options.html#details-for-warning_level for
+# warning_level values.
+#
+# We don't use "/W*" flags here because we don't need to care about MSVC here.
+#
+# We don't have "warning_level == 3" and "warning_level ==
+# 'everything'" here because we don't use these warning levels.
+if warning_level == '1'
+  common_builtin_flags += ['-Wall']
+elif warning_level == '2'
+  common_builtin_flags += ['-Wall', '-Wextra']
+endif
+
+if get_option('debug')
+  common_builtin_flags += ['-g']
+endif
+
+optimization = get_option('optimization')
+if optimization == '0'
+  common_builtin_flags += ['-O0']
+elif optimization == '1'
+  common_builtin_flags += ['-O1']
+elif optimization == '2'
+  common_builtin_flags += ['-O2']
+elif optimization == '3'
+  common_builtin_flags += ['-O3']
+elif optimization == 's'
+  common_builtin_flags += ['-Os']
+endif
+
+cflags_builtin = cc.get_supported_arguments(common_builtin_flags)
+if llvm.found()
+  cxxflags_builtin = cpp.get_supported_arguments(common_builtin_flags)
+endif
+
 common_warning_flags = [
   '-Wmissing-prototypes',
   '-Wpointer-arith',
diff --git a/src/include/meson.build b/src/include/meson.build
index a28f115d867..58b7a9c1e7e 100644
--- a/src/include/meson.build
+++ b/src/include/meson.build
@@ -44,9 +44,9 @@ config_paths_data.set_quoted('MANDIR', dir_prefix / dir_man)
 
 var_cc = ' '.join(cc.cmd_array())
 var_cpp = ' '.join(cc.cmd_array() + ['-E'])
-var_cflags = ' '.join(cflags + cflags_warn + get_option('c_args'))
+var_cflags = ' '.join(cflags + cflags_builtin + cflags_warn + get_option('c_args'))
 if llvm.found()
-  var_cxxflags = ' '.join(cxxflags + cxxflags_warn + get_option('cpp_args'))
+  var_cxxflags = ' '.join(cxxflags + cxxflags_builtin + cxxflags_warn + get_option('cpp_args'))
 else
   var_cxxflags = ''
 endif
-- 
2.43.0

On 07.04.24 18:01, Sutou Kouhei wrote:
> +# We don't have "warning_level == 3" and "warning_level ==
> +# 'everything'" here because we don't use these warning levels.
> +if warning_level == '1'
> +  common_builtin_flags += ['-Wall']
> +elif warning_level == '2'
> +  common_builtin_flags += ['-Wall', '-Wextra']
> +endif

I would trim this even further and always export just '-Wall'.  The 
other options aren't really something we support.

The other stanzas, on '-g' and '-O*', look good to me.

Hi,

In <4707d4ed-f268-43c0-b4dd-cdbc7520f508@eisentraut.org>
  "Re: meson: Specify -Wformat as a common warning flag for extensions" on Tue, 28 May 2024 23:31:05 -0700,
  Peter Eisentraut <peter@eisentraut.org> wrote:

> On 07.04.24 18:01, Sutou Kouhei wrote:
>> +# We don't have "warning_level == 3" and "warning_level ==
>> +# 'everything'" here because we don't use these warning levels.
>> +if warning_level == '1'
>> +  common_builtin_flags += ['-Wall']
>> +elif warning_level == '2'
>> +  common_builtin_flags += ['-Wall', '-Wextra']
>> +endif
> 
> I would trim this even further and always export just '-Wall'.  The
> other options aren't really something we support.

OK. How about the v6 patch? It always uses '-Wall'.

Thanks,
-- 
kou
From 8238adba3f3fc96d4a9e50af611b1cb3566abc0e Mon Sep 17 00:00:00 2001
From: Sutou Kouhei <kou@clear-code.com>
Date: Fri, 15 Mar 2024 18:27:30 +0900
Subject: [PATCH v6] meson: Restore implicit warning/debug/optimize flags for
 extensions

Meson specifies warning/debug/optimize flags such as "-Wall", "-g" and
"-O2" automatically based on "--warnlevel" and "--buildtype"
options. And we use "--warning_level=1" and
"--buildtype=debugoptimized" by default.

We don't specify warning/debug/optimize flags explicitly to build
PostgreSQL with Meson. Because Meson does it automatically as we said.
But Meson doesn't care about flags in Makefile.global and
pg_config. So we need to care about them manually.

This changes do it. They detect debug/optimize flags based on
debug/optimization option values because Meson doesn't tell us flags
Meson guessed. We always use -Wall for warning flags.
---
 meson.build             | 27 +++++++++++++++++++++++++++
 src/include/meson.build |  4 ++--
 2 files changed, 29 insertions(+), 2 deletions(-)

diff --git a/meson.build b/meson.build
index d6401fb8e30..d7239dbb114 100644
--- a/meson.build
+++ b/meson.build
@@ -1851,6 +1851,33 @@ endif
 vectorize_cflags = cc.get_supported_arguments(['-ftree-vectorize'])
 unroll_loops_cflags = cc.get_supported_arguments(['-funroll-loops'])
 
+# They aren't used for building PostgreSQL itself because Meson does
+# everything internally. They are used by extensions via pg_config or
+# Makefile.global.
+common_builtin_flags = ['-Wall']
+
+if get_option('debug')
+  common_builtin_flags += ['-g']
+endif
+
+optimization = get_option('optimization')
+if optimization == '0'
+  common_builtin_flags += ['-O0']
+elif optimization == '1'
+  common_builtin_flags += ['-O1']
+elif optimization == '2'
+  common_builtin_flags += ['-O2']
+elif optimization == '3'
+  common_builtin_flags += ['-O3']
+elif optimization == 's'
+  common_builtin_flags += ['-Os']
+endif
+
+cflags_builtin = cc.get_supported_arguments(common_builtin_flags)
+if llvm.found()
+  cxxflags_builtin = cpp.get_supported_arguments(common_builtin_flags)
+endif
+
 common_warning_flags = [
   '-Wmissing-prototypes',
   '-Wpointer-arith',
diff --git a/src/include/meson.build b/src/include/meson.build
index a28f115d867..58b7a9c1e7e 100644
--- a/src/include/meson.build
+++ b/src/include/meson.build
@@ -44,9 +44,9 @@ config_paths_data.set_quoted('MANDIR', dir_prefix / dir_man)
 
 var_cc = ' '.join(cc.cmd_array())
 var_cpp = ' '.join(cc.cmd_array() + ['-E'])
-var_cflags = ' '.join(cflags + cflags_warn + get_option('c_args'))
+var_cflags = ' '.join(cflags + cflags_builtin + cflags_warn + get_option('c_args'))
 if llvm.found()
-  var_cxxflags = ' '.join(cxxflags + cxxflags_warn + get_option('cpp_args'))
+  var_cxxflags = ' '.join(cxxflags + cxxflags_builtin + cxxflags_warn + get_option('cpp_args'))
 else
   var_cxxflags = ''
 endif
-- 
2.43.0

On 29.05.24 08:47, Sutou Kouhei wrote:
> In <4707d4ed-f268-43c0-b4dd-cdbc7520f508@eisentraut.org>
>    "Re: meson: Specify -Wformat as a common warning flag for extensions" on Tue, 28 May 2024 23:31:05 -0700,
>    Peter Eisentraut <peter@eisentraut.org> wrote:
> 
>> On 07.04.24 18:01, Sutou Kouhei wrote:
>>> +# We don't have "warning_level == 3" and "warning_level ==
>>> +# 'everything'" here because we don't use these warning levels.
>>> +if warning_level == '1'
>>> +  common_builtin_flags += ['-Wall']
>>> +elif warning_level == '2'
>>> +  common_builtin_flags += ['-Wall', '-Wextra']
>>> +endif
>>
>> I would trim this even further and always export just '-Wall'.  The
>> other options aren't really something we support.
> 
> OK. How about the v6 patch? It always uses '-Wall'.

Yes, this looks good to me.

All: I think we should backpatch this.  Otherwise, meson-based installs 
will get suboptimal behavior for extension builds via pgxs.

On 29.05.24 08:47, Sutou Kouhei wrote:
> In <4707d4ed-f268-43c0-b4dd-cdbc7520f508@eisentraut.org>
>    "Re: meson: Specify -Wformat as a common warning flag for extensions" on Tue, 28 May 2024 23:31:05 -0700,
>    Peter Eisentraut <peter@eisentraut.org> wrote:
> 
>> On 07.04.24 18:01, Sutou Kouhei wrote:
>>> +# We don't have "warning_level == 3" and "warning_level ==
>>> +# 'everything'" here because we don't use these warning levels.
>>> +if warning_level == '1'
>>> +  common_builtin_flags += ['-Wall']
>>> +elif warning_level == '2'
>>> +  common_builtin_flags += ['-Wall', '-Wextra']
>>> +endif
>>
>> I would trim this even further and always export just '-Wall'.  The
>> other options aren't really something we support.
> 
> OK. How about the v6 patch? It always uses '-Wall'.

I have committed this.  Thanks.