Re: [PATCH] config.mak.dev: add -Wformat

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Dec 27, 2018, 18:59 UTC
Message-ID: <20181227185900.GE146609@google.com>
In-Reply-To: <20181012191531.GA22611@hank.intra.tgummerer.com>

+cc: Masaya Suzuki In October, Thomas Gummerer wrote:

> On 10/12, Jonathan Nieder wrote:
>> Jeff King wrote:
>>> On Fri, Oct 12, 2018 at 07:40:37PM +0100, Thomas Gummerer wrote:

Show 14 quoted lines

>>>> 801fa63a90 ("config.mak.dev: add -Wformat-security", 2018-09-08) added
>>>> the -Wformat-security to the flags set in config.mak.dev.  In the gcc
>>>> man page this is documented as:
>>>>
>>>>          If -Wformat is specified, also warn about uses of format
>>>>          functions that represent possible security problems.  [...]
>>>>
>>>> That commit did however not add the -Wformat flag, and -Wformat is not
>>>> specified anywhere else by default, so the added -Wformat-security had
>>>> no effect.  Newer versions of gcc (gcc 8.2.1 in this particular case)
>>>> warn about this and thus compilation fails with this option set.
>> [...]
>>> -Wformat is part of -Wall, which we already turn on by default (even for
>>> non-developer builds).

[...]

>> Thomas, do you use autoconf to generate config.mak.autogen?  I'm
>> wondering if that produces a CFLAGS that doesn't include -Wall.
>
> No, this was all my mistake :)

As discussed in [1], autoconf appears to not put -Wall in CFLAGS:

 $ make configure
     GEN configure
 $ ./configure
[...]
 config.status: creating config.mak.autogen
 config.status: executing config.mak.autogen commands
 $ grep CFLAGS config.mak.autogen
 CFLAGS = -g -O2
 PTHREAD_CFLAGS=-pthread

So this trap for the unwary is still around.

Can we revive this patch? Does it just need a clearer commit message, or were there other objections?

Show 9 quoted lines

>>> I'm not opposed to making config.mak.dev a bit more redundant to handle
>>> this case, but we'd probably want to include all of -Wall, since it
>>> contains many other warnings we'd want to make sure are enabled.
>>
>> Do you mean putting -Wall instead of -Wformat?
>>
>> Should we add -Wextra too?  From a quick test, it seems to build okay.
>
> We do have that with setting DEVELOPER=extra-all.

Even better. What do you think of making DEVELOPER=YesPlease imply that?

Thanks, Jonathan

[1] https://public-inbox.org/git/CAJB1erVmZQd_kLU1fqL7cURrEUz2EJ4Br0kgVQt7T-mk3s95dQ@mail.gmail.com/