Horton

Thread

Commits

  1. meson: Restore implicit warning/debug/optimize flags for extensions

  1. meson: Specify -Wformat as a common warning flag for extensions

    Sutou Kouhei <kou@clear-code.com> — 2024-01-22T05:11:39Z

    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

  2. Re: meson: Specify -Wformat as a common warning flag for extensions

    Sutou Kouhei <kou@clear-code.com> — 2024-02-22T07:41:48Z

    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

  3. Re: meson: Specify -Wformat as a common warning flag for extensions

    Tristan Partin <tristan@neon.tech> — 2024-03-08T05:39:39Z

    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)

  4. Re: meson: Specify -Wformat as a common warning flag for extensions

    Michael Paquier <michael@paquier.xyz> — 2024-03-08T06:32:22Z

    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

  5. Re: meson: Specify -Wformat as a common warning flag for extensions

    Sutou Kouhei <kou@clear-code.com> — 2024-03-08T09:17:47Z

    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

  6. Re: meson: Specify -Wformat as a common warning flag for extensions

    Tristan Partin <tristan@neon.tech> — 2024-03-08T16:05:27Z

    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)

  7. Re: meson: Specify -Wformat as a common warning flag for extensions

    Sutou Kouhei <kou@clear-code.com> — 2024-03-12T23:56:38Z

    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

  8. Re: meson: Specify -Wformat as a common warning flag for extensions

    Tristan Partin <tristan@neon.tech> — 2024-03-13T05:43:11Z

    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)

  9. Re: meson: Specify -Wformat as a common warning flag for extensions

    Sutou Kouhei <kou@clear-code.com> — 2024-03-13T07:12:00Z

    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

  10. Re: meson: Specify -Wformat as a common warning flag for extensions

    Peter Eisentraut <peter@eisentraut.org> — 2024-03-13T07:38:28Z

    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.

  11. Re: meson: Specify -Wformat as a common warning flag for extensions

    Sutou Kouhei <kou@clear-code.com> — 2024-03-15T09:36:55Z

    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

  12. Re: meson: Specify -Wformat as a common warning flag for extensions

    Andres Freund <andres@anarazel.de> — 2024-04-07T23:26:35Z

    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

  13. Re: meson: Specify -Wformat as a common warning flag for extensions

    Sutou Kouhei <kou@clear-code.com> — 2024-04-08T01:01:17Z

    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

  14. Re: meson: Specify -Wformat as a common warning flag for extensions

    Peter Eisentraut <peter@eisentraut.org> — 2024-05-29T06:31:05Z

    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.

  15. Re: meson: Specify -Wformat as a common warning flag for extensions

    Sutou Kouhei <kou@clear-code.com> — 2024-05-29T06:47:08Z

    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

  16. Re: meson: Specify -Wformat as a common warning flag for extensions

    Peter Eisentraut <peter@eisentraut.org> — 2024-06-04T07:00:40Z

    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.

  17. Re: meson: Specify -Wformat as a common warning flag for extensions

    Peter Eisentraut <peter@eisentraut.org> — 2024-06-07T07:44:26Z

    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.