config.mak.dev: add -Wformat
PATCH 1/1 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. [...]
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.
Fix that, and make -Wformat-security actually useful by adding the -Wformat flag as well. git compiles cleanly with both these flags applied.
---
Sorry for not catching this before the patch made it to next.
config.mak.dev | 1 + 1 file changed, 1 insertion(+)
config.mak.dev +1 -0
@@ -7,6 +7,7 @@ CFLAGS += -pedantic CFLAGS += -DUSE_PARENS_AROUND_GETTEXT_N=0 endif CFLAGS += -Wdeclaration-after-statement +CFLAGS += -Wformat CFLAGS += -Wformat-security CFLAGS += -Wno-format-zero-length CFLAGS += -Wold-style-definition
Signature · 1 line
2.19.1.937.g12227c8702.dirty
Quoted 7 lines · Thomas Gummerer
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. [...]
Quoted 8 lines
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.
Fix that, and make -Wformat-security actually useful by adding the -Wformat flag as well. git compiles cleanly with both these flags applied.
-Wformat is part of -Wall, which we already turn on by default (even for non-developer builds).
So I don't think we need to do anything more, though I'm puzzled that you saw a failure. Do you set CFLAGS explicitly in your config.mak to something that doesn't include -Wall?
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.
-Peff
Quoted 13 lines · Jeff King
Quoted 11 lines · Thomas Gummerer
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.
[...]
Quoted 6 lines
-Wformat is part of -Wall, which we already turn on by default (even for non-developer builds).
So I don't think we need to do anything more, though I'm puzzled that you saw a failure. Do you set CFLAGS explicitly in your config.mak to something that doesn't include -Wall?
Thomas, do you use autoconf to generate config.mak.autogen? I'm wondering if that produces a CFLAGS that doesn't include -Wall.
Quoted 3 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.
Thanks,
Jonathan
Quoted 5 lines · Jonathan Nieder
Quoted 3 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?
Yes.
Quoted 1 line
Should we add -Wextra too? From a quick test, it seems to build okay.
We already do (though we have to then manually disable a few warnings that we're not ready for -- see config.mak.dev).
-Peff
Quoted 14 lines · Jonathan Nieder
Quoted 6 lines · Jeff King
Quoted 4 lines · Thomas Gummerer
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:Quoted 7 lines
Quoted 7 lines
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.
Quoted 7 lines
[...]
Quoted 6 lines
-Wformat is part of -Wall, which we already turn on by default (even for non-developer builds).
So I don't think we need to do anything more, though I'm puzzled that you saw a failure. Do you set CFLAGS explicitly in your config.mak to something that doesn't include -Wall?
Whoops embarrassing. I had this set in my config.mak:
CFLAGS = -O$(O) -g $(EXTRA_CFLAGS)
What happened is that I had included -Wall in an old config.mak that I copied from Thomas Rast when I started with my GSoC project. Then when "DEVELOPER=1" came around I switched to that at some point and just removed everything from CFLAGS, except the possibility to override the optimization level, the ability to add extra flags and including debug symbols, but failed to notice that I had lost -Wall.
Maybe it would still be a good to add -Wall to avoid the surprise for others. But then again if someone overrides CFLAGS they should at least check better what they're overriding ;)
Quoted 2 lines
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 :)
Quoted 7 lines
Quoted 3 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.
Quoted 2 lines
Thanks,
Jonathan
+cc: Masaya Suzuki
Quoted 18 lines · Thomas Gummerer
Quoted 17 lines · Jonathan Nieder
Quoted 13 lines · Jeff King
Quoted 11 lines · Thomas Gummerer
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.
[...]
Quoted 2 lines
-Wformat is part of -Wall, which we already turn on by default (even for non-developer builds).
[...]
Quoted 4 lines
Quoted 2 lines
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=-pthreadSo 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?
Quoted 9 lines
Quoted 7 lines
Quoted 3 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/
Quoted 19 lines · Jonathan Nieder
Quoted 5 lines · Thomas Gummerer
Quoted 4 lines · Jonathan Nieder
Jeff King wrote:
...Quoted 2 lines
-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
Quoted 4 lines
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 <[email protected]>
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.
--- config.mak.dev | 1 + 1 file changed, 1 insertion(+)
config.mak.dev +1 -0
@@ -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
Signature · 1 line
2.20.1-2-gb21ebb671b
Jonathan Nieder
Reviewed-byHi,
Quoted 31 lines · Junio C Hamano
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 <[email protected]>
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
Makefileby 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.
Quoted 8 lines
Signed-off-by Thomas Gummerer <[email protected]>Helped-by Jeff King <[email protected]>Helped-by Jonathan Nieder <[email protected]>[jc: s/-Wformat/-Wall/]Signed-off-by Junio C Hamano <[email protected]>--- config.mak.dev | 1 + 1 file changed, 1 insertion(+)
Thanks for tying up this loose end.
Quoted 21 lines · Junio C Hamano
Quoted 19 lines · Jonathan Nieder
Quoted 5 lines · Thomas Gummerer
Quoted 4 lines · Jonathan Nieder
Jeff King wrote:
...Quoted 2 lines
-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
Quoted 8 lines
Quoted 4 lines
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.
Just catching up after some time off over Christmas, thanks for tying this up!
I agree with the choice of adding -Wall to the CFLAGS here, so even if it is not added to the CFLAGS generated by autoconf (or in mnually set up CFLAGS such as in my original case), we still get a complete set of warnings when DEVELOPER=YesPlease is set.
Quoted 51 lines
-- >8 --
From: Thomas Gummerer <[email protected]>
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
Makefileby 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 <[email protected]>Helped-by Jeff King <[email protected]>Helped-by Jonathan Nieder <[email protected]>[jc: s/-Wformat/-Wall/]Signed-off-by Junio C Hamano <[email protected]>--- config.mak.dev | 1 + 1 file changed, 1 insertion(+)config.mak.dev +1
-0@@ -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-lengthSignature · 1 line
2.20.1-2-gb21ebb671b
Quoted 9 lines · Thomas Gummerer
I agree with the choice of adding -Wall to the CFLAGS here, so even if it is not added to the CFLAGS generated by autoconf (or in mnually set up CFLAGS such as in my original case), we still get a complete set of warnings when DEVELOPER=YesPlease is set.
Quoted 4 lines
-- >8 --
From: Thomas Gummerer <[email protected]>
Date: Fri, 12 Oct 2018 19:40:37 +0100
Subject: [PATCH] config.mak.dev: add -Wformat
Thanks. I noticed, before merging the topic to 'next', that I needed to retitle this further. I'd use something like this.
Subject: config.mak.dev: add -Wall to help autoconf users
Jonathan Nieder
Reviewed-byQuoted 10 lines · Junio C Hamano
Quoted 3 lines · Thomas Gummerer
Quoted 3 lines
From: Thomas Gummerer <[email protected]>
Date: Fri, 12 Oct 2018 19:40:37 +0100
Subject: [PATCH] config.mak.dev: add -WformatThanks. I noticed, before merging the topic to 'next', that I needed to retitle this further. I'd use something like this.
Subject: config.mak.dev: add -Wall to help autoconf users
With that change, it would still be
:)
Thanks.