Re: [PATCH] config.mak.dev: add -Wformat
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Jan 3, 2019, 16:55 UTC
- Message-ID
- <xmqqa7khisue.fsf@gitster-ct.c.googlers.com>
- In-Reply-To
- <20181227185900.GE146609@google.com>
Jonathan Nieder <jrnieder@gmail.com> writes:
Show 23 quoted lines
> In October, Thomas Gummerer wrote: >> On 10/12, Jonathan Nieder wrote: >>> Jeff King wrote: >>> ... >>>> -Wformat is part of -Wall, which we already turn on by default (even for >>>> non-developer builds). > ... > 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?
I think it is a good idea to give fallback/redundancy for this case. I do not have strong opinion between -Wall and -Wformat, but I'd probably vote for the former if pressed.
-- >8 -- From: Thomas Gummerer <t.gummerer@gmail.com> Date: Fri, 12 Oct 2018 19:40:37 +0100 Subject: [PATCH] config.mak.dev: add -Wformat
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. [...]The commit did however not add the "-Wformat" flag, but instead relied on the fact that "-Wall" is set in the Makefile by default and that "-Wformat" is part of "-Wall".
Unfortunately, those who use config.mak.autogen generated with the autoconf to configure toolchain do *not* get "-Wall" in their CFLAGS and the added -Wformat-security had no effect. Worse yet, newer versions of gcc (gcc 8.2.1 in this particular case) warn about the lack of "-Wformat" and thus compilation fails only with this option set.
We could fix it by adding "-Wformat", but in general we do want all checks included in "-Wall", so let's add it to config.mak.dev to cover more cases.
Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com> Helped-by: Jeff King <peff@peff.net> Helped-by: Jonathan Nieder <jrnieder@gmail.com> [jc: s/-Wformat/-Wall/] Signed-off-by: Junio C Hamano <gitster@pobox.com> --- config.mak.dev | 1 + 1 file changed, 1 insertion(+)
diff --git a/config.mak.dev b/config.mak.dev index bfbd3df4e8..74337f1f92 100644 --- a/config.mak.dev +++ b/config.mak.dev @@ -1,6 +1,7 @@ ifeq ($(filter no-error,$(DEVOPTS)),) CFLAGS += -Werror endif +CFLAGS += -Wall CFLAGS += -Wdeclaration-after-statement CFLAGS += -Wformat-security CFLAGS += -Wno-format-zero-length
-- 2.20.1-2-gb21ebb671b