Re: [PATCH v2 08/10] Fix up a bunch of "gcc -Werror=sign-compare" complaints
On Wed, Jan 5, 2022 at 9:46 AM Bruno Haible wrote: ... > 2022-01-05 Bruno Haible > > gnulib-tool: Avoid known warnings that reflect Gnulib's coding style. > * m4/gnulib-common.m4 (gl_CC_GNULIB_WARNINGS): New macro. > * gnulib-tool (func_emit_lib_Makefile_am): Add the > GL_CFLAG_GNULIB_WARNINGS to the CFLAGS of all the compilation units of > the library. > (func_emit_tests_Makefile_am): Add the GL_CFLAG_GNULIB_WARNINGS to the > CFLAGS. > (func_import): Emit an invocation of gl_CC_GNULIB_WARNINGS. Thanks for the carefully-considered response/patch and for sharing your decision process.
Re: [PATCH v2 08/10] Fix up a bunch of "gcc -Werror=sign-compare" complaints
Jim Meyering wrote:
> I don't have terribly strong feelings here, but I would be inclined to
> retain some of those.
>
> This exposes low-maint-cost ways to improve APIs, both to help the
> compiler help us (e.g., via better optimization or better static
> analysis) and to help readers know more at a glance about a labeled
> function:
> > 72 -Wattribute-warning,
OK, I'm not going to exclude this warning then.
> The following are issues that are usually easy/safe to address and
> have so few violations that they may be worth addressing or explicitly
> suppressing (I haven't looked):
> > 7 -Wimplicit-fallthrough=
> > 5 -Wmissing-field-initializers
OK, I'm going to keep -Wimplicit-fallthrough. And I didn't suggest to
exclude -Wmissing-field-initializers warnings.
> This one is special: historically low-value, but has been known to
> catch real bugs. Mark each FP instance to suppress the warning there?
> > 9 -Wmaybe-uninitialized
In order to catch these, I'm not excluding the warning from testdirs,
only from Gnulib code imported into other packages. We as gnulib maintainers
can easily create a testdir, compile it, and analyze all warnings.
Paul Eggert wrote:
> Like Jim I don't have strong feelings about this, but here are some
> comments anyway
>
>
> > I therefore propose to disable these warnings:
>
> >-Wattribute-warning
> I haven't dealt with this much, since gcc-warning.spec means it's not
> used by the packages I help maintain. Could you give examples of why it
> misfires on Gnulib?
It often misfires because e.g. strchr (s, ' ') is OK for all multibyte
locales but strchr (s, '0') is not. But the attribute-warning can only
be attached to a function (here: strchr) as a whole, regardless of which
arguments are being passed.
> > -Wcast-qual
> > -Wconversion
> > -Wfloat-conversion
> > -Wfloat-equal
> > -Wpedantic
> > -Wsign-conversion
> > -Wundef
> > -Wunsuffixed-float-constants
> These aren't enabled by -Wall -Wextra. So I assume that gnulib-tool
> would be appending -Wno-cast-qual etc. to disable these even if the main
> CFLAGS enables them?
I want to disable them if the package's AM_CFLAGS enables them.
If the user who compiles the package has them enabled, they shall take
effect - because "the user is always right".
> >-Wimplicit-fallthrough
> I typically use -Wimplicit-fallthrough=5; would it be OK to standardize
> on that in Gnulib?
We can standardize on '-Wimplicit-fallthrough=3'.
If we wanted to standardize on '-Wimplicit-fallthrough=5', gperf-generated
code would need to use the more modern syntax instead of /*FALLTHROUGH*/.
This, in turn, requires a gperf 3.2 release, which is not yet out-of-the-door.
> >-Wmaybe-uninitialized
> I find this one useful as it catches real bugs. Admittedly it is a pain
> because it also has quite a few false alarms. At RMS's suggestion Emacs
> does something like this in config.h:
>
>#ifdef GCC_LINT
># define UNINIT = {0,}
>#else
># define UNINIT /* empty */
>#endif
>
> and something like this in configure.ac:
>
>AS_IF([test $gl_gcc_warnings = no],
> AC_DEFINE([GCC_LINT], [1], [Define to 1 if --enable-gcc-warnings.]))
>
> and this in source files for variables that would otherwise be warned
> about with the latest GCC:
>
> char *p UNINIT;
>
> Perhaps we should move this idea into Gnulib?
This idea has not met unanimous agreement, IIRC:
- Some distro builders insist on building production binaries in the
same way as "debugging" binaries.
- While some other people think that debugging/linting code should not
be included in production binaries.
> >-Wrestrict
> This one seems like it could find real bugs; could you give an example
> or two of misfires? Perhaps we could disable it in individual files that
> play nonstandard games with pointers.
It does not misfire. It highlights real bugs in crypto code that Simon
contributed. OK, I'll not exclude -Wrestrict warnings.
Bruno
2022-01-05 Bruno Haible
gnulib-tool: Avoid known warnings that reflect Gnulib's coding style.
* m4/gnulib-common.m4 (gl_CC_GNULIB_WARNINGS): New macro.
* gnulib-tool (func_emit_lib_Makefile_am): Add the
GL_CFLAG_GNULIB_WARNINGS to the CFLAGS of all the compilation units of
the library.
(func_emit_tests_Makefile_am): Add the GL_CFLAG_GNULIB_WARNINGS to the
CFLAGS.
(func_import): Emit an invocation of gl_CC_GNULIB_WARNINGS.
diff --git a/gnulib-tool b/gnulib-tool
index 02d4f8661d..a91847f55a 100755
--- a/gnulib-tool
+++ b/gnulib-tool
@@ -3974,6 +3974,9 @@ func_emit_lib_Makefile_am ()
fi
echo
echo "${libname}_${libext}_SOURCES ="
+ if ! $for_test; then
+echo "${libname}_${libext}_CFLAGS = \$(AM_CFLAGS)
\$(GL_CFLAG_GNULIB_WARNINGS)"
+ fi
# Here we use $(LIBOBJS), not @LIBOBJS@. The value is the same. However,
# automake during its analysis looks for $(LIBOBJS), not for @LIBOBJS@.
echo
Re: [PATCH v2 08/10] Fix up a bunch of "gcc -Werror=sign-compare" complaints
Bruno Haible writes: > Paul Eggert replied: > >> Every other Gnulib-using project I know takes the third approach. > > So, how about adding the third approach to gnulib-tool? > > Rationale: > Gnulib does not want to dictate their preferred GCC flags to coreutils, > grub, etc. > And similarly, we don't want coreutils, grub, etc. to dictate the coding > style in which gnulib is written. > > Implementation idea: > Since 2021-06-10, gnulib-tool already ensures that the Gnulib modules are > compiled with '-Wno-error'. This code could be extended to add > '-Wno-sign-compare' and other warning-disabling options. Given the spirit of the proposal, I'd feel weird commenting on the chosen warning set, but I do like the idea itself, thanks! Be well, --Robbie signature.asc Description: PGP signature
Re: [PATCH v2 08/10] Fix up a bunch of "gcc -Werror=sign-compare" complaints
On 12/8/21 07:48, Bruno Haible wrote:
2) When I collect all warnings in the gllib/ directory of a full testdir,
I get (with gcc-9.3):
Typically, I don't worry much about warnings by older GCCs, as it's
enough of a pain to worry about the latest version.
Like Jim I don't have strong feelings about this, but here are some
comments anyway
I therefore propose to disable these warnings:
-Wattribute-warning
I haven't dealt with this much, since gcc-warning.spec means it's not
used by the packages I help maintain. Could you give examples of why it
misfires on Gnulib?
-Wcast-qual
-Wconversion
-Wfloat-conversion
-Wfloat-equal
-Wpedantic
-Wsign-conversion
-Wundef
-Wunsuffixed-float-constants
These aren't enabled by -Wall -Wextra. So I assume that gnulib-tool
would be appending -Wno-cast-qual etc. to disable these even if the main
CFLAGS enables them? (And I guess similarly for all the flags.)
-Wimplicit-fallthrough
I typically use -Wimplicit-fallthrough=5; would it be OK to standardize
on that in Gnulib?
-Wmaybe-uninitialized
I find this one useful as it catches real bugs. Admittedly it is a pain
because it also has quite a few false alarms. At RMS's suggestion Emacs
does something like this in config.h:
#ifdef GCC_LINT
# define UNINIT = {0,}
#else
# define UNINIT /* empty */
#endif
and something like this in configure.ac:
AS_IF([test $gl_gcc_warnings = no],
AC_DEFINE([GCC_LINT], [1], [Define to 1 if --enable-gcc-warnings.]))
and this in source files for variables that would otherwise be warned
about with the latest GCC:
char *p UNINIT;
Perhaps we should move this idea into Gnulib?
-Wrestrict
This one seems like it could find real bugs; could you give an example
or two of misfires? Perhaps we could disable it in individual files that
play nonstandard games with pointers.
Re: [PATCH v2 08/10] Fix up a bunch of "gcc -Werror=sign-compare" complaints
On Wed, Dec 8, 2021 at 7:48 AM Bruno Haible wrote: > > > especially given it's in the gcc -Wextra set, not some random flag > > Warnings in the '-Wall' category are typically worth eliminating: they > regularly point to real bugs. > > Eliminating '-Wsign-compare' warnings, OTOH, is usually a waste of time, in > my experience: I have hardly ever found a bug while chasing them. > > Robbie Harwood wrote: > > > As you point out with coreutils, gnulib taking a position like this on > > > flags has the knock-on effect that, for our grub, we'll need to do one > > > of the following: > > > > > > - Carry a gnulib patch to make the flag work (what we've been doing). > > > - Change flags in the outer work (i.e., change build options for grub) > > > - Maintain logic to keep flags gnulib-disliked flags out when building > > >the gnulib modules > > Paul Eggert replied: > > Every other Gnulib-using project I know takes the third approach. > > So, how about adding the third approach to gnulib-tool? > > Rationale: > Gnulib does not want to dictate their preferred GCC flags to coreutils, > grub, etc. > And similarly, we don't want coreutils, grub, etc. to dictate the coding > style in which gnulib is written. > > Implementation idea: > Since 2021-06-10, gnulib-tool already ensures that the Gnulib modules are > compiled with '-Wno-error'. This code could be extended to add > '-Wno-sign-compare' and other warning-disabling options. > > Question: > Which warning options should we disable this way? > 1) We have some comments in build-aux/gcc-warning.spec. > 2) When I collect all warnings in the gllib/ directory of a full testdir, > I get (with gcc-9.3): > > $ grep warning: gllib.log | sed -e 's/^.*\[\(-W.*\)\]$/\1/' | LC_ALL=C sort | > LC_ALL=C uniq -c > 72 -Wattribute-warning > 169 -Wcast-qual > 457 -Wconversion > 1 -Wempty-body > 10 -Wfloat-conversion > 24 -Wfloat-equal > 7 -Wimplicit-fallthrough= > 9 -Wmaybe-uninitialized > 5 -Wmissing-field-initializers > 1 -Wpedantic > 7 -Wredundant-decls > 8 -Wrestrict > 156 -Wsign-compare >4324 -Wsign-conversion > 2 -Wtype-limits > 888 -Wundef > 139 -Wunsuffixed-float-constants > 2 -Wunused-function > 129 -Wunused-parameter > > And with gcc-11.2.0: > $ grep warning: gllib.log | sed -e 's/^.*\[\(-W.*\)\]$/\1/' | LC_ALL=C sort | > LC_ALL=C uniq -c > 72 -Wattribute-warning > 172 -Wcast-qual > 509 -Wconversion > 4 -Wcpp > 1 -Wempty-body > 9 -Wfloat-conversion > 26 -Wfloat-equal > 7 -Wimplicit-fallthrough= > 9 -Wmaybe-uninitialized > 5 -Wmissing-field-initializers > 432 -Wpedantic > 7 -Wredundant-decls > 8 -Wrestrict > 2 -Wreturn-local-addr > 168 -Wsign-compare >4362 -Wsign-conversion > 2 -Wtype-limits > 878 -Wundef > 155 -Wunsuffixed-float-constants > 2 -Wunused-function > 131 -Wunused-parameter > > I therefore propose to disable these warnings: > -Wattribute-warning > -Wcast-qual > -Wconversion > -Wfloat-conversion > -Wfloat-equal > -Wimplicit-fallthrough > -Wmaybe-uninitialized > -Wpedantic > -Wrestrict > -Wsign-compare > -Wsign-conversion > -Wtype-limits > -Wundef > -Wunsuffixed-float-constants > -Wunused-function > -Wunused-parameter Hi Bruno, I don't have terribly strong feelings here, but I would be inclined to retain some of those. This exposes low-maint-cost ways to improve APIs, both to help the compiler help us (e.g., via better optimization or better static analysis) and to help readers know more at a glance about a labeled function: > 72 -Wattribute-warning, The following are issues that are usually easy/safe to address and have so few violations that they may be worth addressing or explicitly suppressing (I haven't looked): > 7 -Wimplicit-fallthrough= > 5 -Wmissing-field-initializers This one is special: historically low-value, but has been known to catch real bugs. Mark each FP instance to suppress the warning there? > 9 -Wmaybe-uninitialized
Re: [PATCH v2 08/10] Fix up a bunch of "gcc -Werror=sign-compare" complaints
> especially given it's in the gcc -Wextra set, not some random flag Warnings in the '-Wall' category are typically worth eliminating: they regularly point to real bugs. Eliminating '-Wsign-compare' warnings, OTOH, is usually a waste of time, in my experience: I have hardly ever found a bug while chasing them. Robbie Harwood wrote: > > As you point out with coreutils, gnulib taking a position like this on > > flags has the knock-on effect that, for our grub, we'll need to do one > > of the following: > > > > - Carry a gnulib patch to make the flag work (what we've been doing). > > - Change flags in the outer work (i.e., change build options for grub) > > - Maintain logic to keep flags gnulib-disliked flags out when building > >the gnulib modules Paul Eggert replied: > Every other Gnulib-using project I know takes the third approach. So, how about adding the third approach to gnulib-tool? Rationale: Gnulib does not want to dictate their preferred GCC flags to coreutils, grub, etc. And similarly, we don't want coreutils, grub, etc. to dictate the coding style in which gnulib is written. Implementation idea: Since 2021-06-10, gnulib-tool already ensures that the Gnulib modules are compiled with '-Wno-error'. This code could be extended to add '-Wno-sign-compare' and other warning-disabling options. Question: Which warning options should we disable this way? 1) We have some comments in build-aux/gcc-warning.spec. 2) When I collect all warnings in the gllib/ directory of a full testdir, I get (with gcc-9.3): $ grep warning: gllib.log | sed -e 's/^.*\[\(-W.*\)\]$/\1/' | LC_ALL=C sort | LC_ALL=C uniq -c 72 -Wattribute-warning 169 -Wcast-qual 457 -Wconversion 1 -Wempty-body 10 -Wfloat-conversion 24 -Wfloat-equal 7 -Wimplicit-fallthrough= 9 -Wmaybe-uninitialized 5 -Wmissing-field-initializers 1 -Wpedantic 7 -Wredundant-decls 8 -Wrestrict 156 -Wsign-compare 4324 -Wsign-conversion 2 -Wtype-limits 888 -Wundef 139 -Wunsuffixed-float-constants 2 -Wunused-function 129 -Wunused-parameter And with gcc-11.2.0: $ grep warning: gllib.log | sed -e 's/^.*\[\(-W.*\)\]$/\1/' | LC_ALL=C sort | LC_ALL=C uniq -c 72 -Wattribute-warning 172 -Wcast-qual 509 -Wconversion 4 -Wcpp 1 -Wempty-body 9 -Wfloat-conversion 26 -Wfloat-equal 7 -Wimplicit-fallthrough= 9 -Wmaybe-uninitialized 5 -Wmissing-field-initializers 432 -Wpedantic 7 -Wredundant-decls 8 -Wrestrict 2 -Wreturn-local-addr 168 -Wsign-compare 4362 -Wsign-conversion 2 -Wtype-limits 878 -Wundef 155 -Wunsuffixed-float-constants 2 -Wunused-function 131 -Wunused-parameter I therefore propose to disable these warnings: -Wattribute-warning -Wcast-qual -Wconversion -Wfloat-conversion -Wfloat-equal -Wimplicit-fallthrough -Wmaybe-uninitialized -Wpedantic -Wrestrict -Wsign-compare -Wsign-conversion -Wtype-limits -Wundef -Wunsuffixed-float-constants -Wunused-function -Wunused-parameter Bruno
Re: [PATCH v2 08/10] Fix up a bunch of "gcc -Werror=sign-compare" complaints
On 12/7/21 11:44, Robbie Harwood wrote: it's odd to me to hear disabling it being the suggested course of action (especially given it's in the gcc -Wextra set, not some random flag). Not odd at all. High-quality static-checking tools have tons of options for a reason, and one shouldn't assume that the tools' defaults are right for your project because (assuming the project is sufficiently complex, which GDB certainly is) they probably aren't. Part of the problem here is that there are two conflicting attitudes about types like size_t. The traditional attitude is that one should compute sizes using size_t arithmetic, and be very careful about integer overflow because it's well-defined behavior in C so your runtime won't report it and your program will likely misbehave in mysterious ways later. Gnulib was originally written that way; however, some of its modules have been redone to follow a newer approach, in which one should compute size using ptrdiff_t arithmetic, and (while still being very careful about overflow) hope that your runtime will report it right away so that bugs are more likely to be caught and can be fixed more easily. This latter hope is realized with gcc -fsanitize=undefined, so this is a win over the traditional approach. gcc's -Wsign-compare flag was designed more with the traditional style in mind, and it hasn't really caught up with the newer style. I hope GCC catches up someday; in the meantime, I've found it better to disable -Wsign-compare than to pacify it, as pacification makes code harder to maintain (and so, likely buggier) and can mask some other errors. As you point out with coreutils, gnulib taking a position like this on flags has the knock-on effect that, for our grub, we'll need to do one of the following: - Carry a gnulib patch to make the flag work (what we've been doing). - Change flags in the outer work (i.e., change build options for grub) - Maintain logic to keep flags gnulib-disliked flags out when building the gnulib modules Every other Gnulib-using project I know takes the third approach.
Re: [PATCH v2 08/10] Fix up a bunch of "gcc -Werror=sign-compare" complaints
Paul Eggert writes:
> On 12/1/21 13:02, Robbie Harwood wrote:
>
>> diff --git a/lib/argp-fmtstream.c b/lib/argp-fmtstream.c
>> index f679751b9..4aa401e2d 100644
>> --- a/lib/argp-fmtstream.c
>> +++ b/lib/argp-fmtstream.c
>> @@ -234,7 +234,7 @@ __argp_fmtstream_update (argp_fmtstream_t fs)
>> else
>> {
>>size_t display_width = mbsnwidth (buf, nl - buf, MBSW_STOP_AT_NUL);
>> - if (display_width < (ssize_t) fs->rmargin)
>> + if (display_width < fs->rmargin)
>> {
>>/* The buffer contains a full line that fits within the maximum
>> line width. Reset point and scan the next line. */
>
> This fixes a problem introduced in PATCH v2 04/10 "Fix width
> computation". Please fix that patch instead.
Willdo, thanks.
> Let's not make this sort of change. We are moving to a coding style that
> prefers signed values, since they allow us to use better runtime checks.
> If -Wsign-compare complains, let's disable -Wsign-compare instead of
> changing carefully-written signed integers to be unsigned.
>
> The remaining changes in this patch also seem to be unnecessary; they're
> put in only to pacify -Wsign-compare, so the solution is to not use
> -Wsign-compare. That's what Coreutils does, for example.
I'm not going to pretend it's the most important flag or anything like
that, but it's odd to me to hear disabling it being the suggested course
of action (especially given it's in the gcc -Wextra set, not some random
flag).
As you point out with coreutils, gnulib taking a position like this on
flags has the knock-on effect that, for our grub, we'll need to do one
of the following:
- Carry a gnulib patch to make the flag work (what we've been doing).
- Change flags in the outer work (i.e., change build options for grub)
- Maintain logic to keep flags gnulib-disliked flags out when building
the gnulib modules
Be well,
--Robbie
signature.asc
Description: PGP signature
Re: [PATCH v2 08/10] Fix up a bunch of "gcc -Werror=sign-compare" complaints
On 12/1/21 13:02, Robbie Harwood wrote:
diff --git a/lib/argp-fmtstream.c b/lib/argp-fmtstream.c
index f679751b9..4aa401e2d 100644
--- a/lib/argp-fmtstream.c
+++ b/lib/argp-fmtstream.c
@@ -234,7 +234,7 @@ __argp_fmtstream_update (argp_fmtstream_t fs)
else
{
size_t display_width = mbsnwidth (buf, nl - buf, MBSW_STOP_AT_NUL);
- if (display_width < (ssize_t) fs->rmargin)
+ if (display_width < fs->rmargin)
{
/* The buffer contains a full line that fits within the maximum
line width. Reset point and scan the next line. */
This fixes a problem introduced in PATCH v2 04/10 "Fix width
computation". Please fix that patch instead.
- Idx node = init_state->nodes.elems[node_cnt];
+ size_t node = init_state->nodes.elems[node_cnt];
Let's not make this sort of change. We are moving to a coding style that
prefers signed values, since they allow us to use better runtime checks.
If -Wsign-compare complains, let's disable -Wsign-compare instead of
changing carefully-written signed integers to be unsigned.
- if (__mbrtowc (, (const char *) buf, p - buf,
+ if ((ssize_t)__mbrtowc (, (const char *) buf, p - buf,
) == p - buf
Let's leave this alone as well. The code is OK as-is, and I'd rather not
insert casts (they're too powerful and error-prone in C).
@@ -860,7 +861,8 @@ init_dfa (re_dfa_t *dfa, size_t pat_len)
dfa->sb_char = (re_bitset_ptr_t) utf8_sb_map;
else
{
- int i, j, ch;
+ int i, j;
+ wint_t ch;
ch can be at most 65536 so 'int' should be OK here and it's better to
use a signed integer as mentioned earlier.
The remaining changes in this patch also seem to be unnecessary; they're
put in only to pacify -Wsign-compare, so the solution is to not use
-Wsign-compare. That's what Coreutils does, for example.
