Re: [PATCH v2 08/10] Fix up a bunch of "gcc -Werror=sign-compare" complaints

[Bruno Haible]

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  <bruno@clisp.org>

        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 "${libname}_${libext}_LIBADD = \$(${macro_prefix}_${perhapsLT}LIBOBJS)"
@@ -4343,7 +4346,12 @@ func_emit_tests_Makefile_am ()
   #   CFLAGS, they have asked for errors, they will get errors. But they have
   #   no right to complain about these errors, because Gnulib does not support
   #   '-Werror'.
-  echo "CFLAGS = @GL_CFLAG_ALLOW_WARNINGS@ @CFLAGS@"
+  cflags_for_gnulib_code=
+  if ! $for_test; then
+    # Enable or disable warnings as suitable for the Gnulib coding style.
+    cflags_for_gnulib_code=" \$(GL_CFLAG_GNULIB_WARNINGS)"
+  fi
+  echo "CFLAGS = @GL_CFLAG_ALLOW_WARNINGS@${cflags_for_gnulib_code} @CFLAGS@"
   echo "CXXFLAGS = @GL_CXXFLAG_ALLOW_WARNINGS@ @CXXFLAGS@"
   echo
   echo "AM_CPPFLAGS = \\"
@@ -6056,6 +6064,7 @@ s,//*$,/,'
     func_emit_autoconf_snippets "$testsrelated_modules" "$main_modules 
$testsrelated_modules" func_verify_module true true true
     echo "  m4_popdef([gl_MODULE_INDICATOR_CONDITION])"
     func_emit_initmacro_end ${macro_prefix}tests $gentests
+    echo "  AC_REQUIRE([gl_CC_GNULIB_WARNINGS])"
     # _LIBDEPS and _LTLIBDEPS variables are not needed if this library is
     # created using libtool, because libtool already handles the dependencies.
     if test "$libtool" != true; then
diff --git a/m4/gnulib-common.m4 b/m4/gnulib-common.m4
index 87a9a751b6..179aac7aa1 100644
--- a/m4/gnulib-common.m4
+++ b/m4/gnulib-common.m4
@@ -1,4 +1,4 @@
-# gnulib-common.m4 serial 69
+# gnulib-common.m4 serial 70
 dnl Copyright (C) 2007-2022 Free Software Foundation, Inc.
 dnl This file is free software; the Free Software Foundation
 dnl gives unlimited permission to copy and/or distribute it,
@@ -879,6 +879,70 @@ AC_DEFUN([gl_CXX_ALLOW_WARNINGS],
   AC_SUBST([GL_CXXFLAG_ALLOW_WARNINGS])
 ])
 
+# gl_CC_GNULIB_WARNINGS
+# sets and substitutes a variable GL_CFLAG_GNULIB_WARNINGS, to a $(CC) option
+# set that enables or disables warnings as suitable for the Gnulib coding 
style.
+AC_DEFUN([gl_CC_GNULIB_WARNINGS],
+[
+  AC_REQUIRE([gl_CC_ALLOW_WARNINGS])
+  dnl Assume that the compiler supports -Wno-* options only if it also supports
+  dnl -Wno-error.
+  GL_CFLAG_GNULIB_WARNINGS=''
+  if test -n "$GL_CFLAG_ALLOW_WARNINGS"; then
+    dnl Enable these warning options:
+    dnl
+    dnl                                       GCC             clang
+    dnl -Wno-cast-qual                        >= 3            >= 3.9
+    dnl -Wno-conversion                       >= 3            >= 3.9
+    dnl -Wno-float-conversion                 >= 4.9          >= 3.9
+    dnl -Wno-float-equal                      >= 3            >= 3.9
+    dnl -Wimplicit-fallthrough                >= 3            >= 3.9
+    dnl -Wno-pedantic                         >= 4.8          >= 3.9
+    dnl -Wno-sign-compare                     >= 3            >= 3.9
+    dnl -Wno-sign-conversion                  >= 4.3          >= 3.9
+    dnl -Wno-type-limits                      >= 4.3          >= 3.9
+    dnl -Wno-undef                            >= 3            >= 3.9
+    dnl -Wno-unsuffixed-float-constants       >= 4.5
+    dnl -Wno-unused-function                  >= 3            >= 3.9
+    dnl -Wno-unused-parameter                 >= 3            >= 3.9
+    dnl
+    cat > conftest.c <<\EOF
+      #if __GNUC__ >= 3 || (__clang_major__ + (__clang_minor__ >= 9) > 3)
+      -Wno-cast-qual
+      -Wno-conversion
+      -Wno-float-equal
+      -Wimplicit-fallthrough
+      -Wno-sign-compare
+      -Wno-undef
+      -Wno-unused-function
+      -Wno-unused-parameter
+      #endif
+      #if __GNUC__ + (__GNUC_MINOR__ >= 9) > 4 || (__clang_major__ + 
(__clang_minor__ >= 9) > 3)
+      -Wno-float-conversion
+      #endif
+      #if __GNUC__ + (__GNUC_MINOR__ >= 8) > 4 || (__clang_major__ + 
(__clang_minor__ >= 9) > 3)
+      -Wno-pedantic
+      #endif
+      #if __GNUC__ + (__GNUC_MINOR__ >= 3) > 4 || (__clang_major__ + 
(__clang_minor__ >= 9) > 3)
+      -Wno-sign-conversion
+      -Wno-type-limits
+      #endif
+      #if __GNUC__ + (__GNUC_MINOR__ >= 5) > 4 || (__clang_major__ + 
(__clang_minor__ >= 9) > 3)
+      -Wno-unsuffixed-float-constants
+      #endif
+EOF
+    gl_command="$CC $CFLAGS $CPPFLAGS -E conftest.c > conftest.out"
+    if AC_TRY_EVAL([gl_command]); then
+      gl_options=`grep -v '#' conftest.out`
+      for word in $gl_options; do
+        GL_CFLAG_GNULIB_WARNINGS="$GL_CFLAG_GNULIB_WARNINGS $word"
+      done
+    fi
+    rm -f conftest.c conftest.out
+  fi
+  AC_SUBST([GL_CFLAG_GNULIB_WARNINGS])
+])
+
 dnl gl_CONDITIONAL_HEADER([foo.h])
 dnl takes a shell variable GL_GENERATE_FOO_H (with value true or false) as 
input
 dnl and produces