manywarnings: Overhaul documentation

[Bruno Haible]

When using the 'manywarnings' module for the first time, I looked at an
example use, such as this one from grep/configure.ac:

- - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
  # This, $nw, is the list of warnings we disable.
  nw="$nw -Wvla"# suppress a warning in regexec.h
  nw="$nw -Winline" # suppress warnings from streq.h's streq5
  nw="$nw -Wsystem-headers" # Don't let system headers trigger warnings
  nw="$nw -Wstack-protector"# generates false alarms for useful code

  gl_MANYWARN_ALL_GCC([ws])
  gl_MANYWARN_COMPLEMENT([ws], [$ws], [$nw])
  for w in $ws; do
gl_WARN_ADD([$w])
  done
  gl_WARN_ADD([-Wno-missing-field-initializers]) # We need this one
  gl_WARN_ADD([-Wno-sign-compare]) # Too many warnings for now
  gl_WARN_ADD([-Wno-unused-parameter]) # Too many warnings for now
  gl_WARN_ADD([-Wno-cast-function-type]) # sig-handler.h's sa_handler_t cast
  gl_WARN_ADD([-Wno-deprecated-declarations]) # clang complains about sprintf
- - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -

and had a hard time understanding why some unwanted warning options are added
positively to 'nw', while some others are
  - negated (-Wno-...),
  - added via gl_WARN_ADD rather than a shell variable,
  - handled at the end, unlike the others, which are listed at the beginning.

The distinction comes from the list of warning options that are embedded in
m4/manywarnings.m4. But
  1) This distinction is not documented.
  2) It is cumbersome for a package maintainer to look up, for each warning type
 he wants to silence, whether this warning option is listed in
 m4/manywarnings.m4 or not.

The distinction can be automated by a single additional gl_MANYWARN_COMPLEMENT
invocation.

I'm therefore updating the sample of 'manywarnings' in the documentation to
be easier to understand and maintain. Also, let me add some other
recommendations, based on recent discussions and my recent experience.

Note: I'm for the first time using a TeXinfo style where newlines are present
after every sentence's full stop. It's being said that this is a reasonable
compromise between having small doc diffs in the future and readability at
the .texi level. See



2023-06-04  Bruno Haible  

manywarnings: Overhaul documentation.
* doc/manywarnings.texi: In the example, put all unwanted warning
options into 'nw', and use a second gl_MANYWARN_COMPLEMENT invocation to
sort out how these options need to get added to WARN_FLAGS.
Describe the first-time use in more detail: Recommend a new GCC.
Recommend to test builds with -O2 and with -O0. Suggest to sort the
warning by warning option. Add reference to the GCC pragma's
documentation.

diff --git a/doc/manywarnings.texi b/doc/manywarnings.texi
index 01e23116cc..19b58f8e47 100644
--- a/doc/manywarnings.texi
+++ b/doc/manywarnings.texi
@@ -12,48 +12,112 @@
  'expensive' in addition generates expensive warnings.])])
 
 AS_IF([test "$enable_gcc_warnings" != no],
-  [gl_MANYWARN_ALL_GCC([warnings])
-
+  [
# Set up the list of unwanted warning options.
nw=
if test "$enable_gcc_warnings" != expensive; then
  nw="$nw -fanalyzer"
fi
-   nw="$nw -Winline"   # It's OK to not inline.
-   nw="$nw -Wstrict-overflow"  # It's OK to optimize strictly.
-   nw="$nw -Wsystem-headers"   # Don't warn in system headers.
-
-   # Enable all GCC warnings not in this list.
-   gl_MANYWARN_COMPLEMENT([warnings], [$warnings], [$nw])
-   for w in $warnings; do
+   nw="$nw -Wbad-function-cast" # Casting a function's result is not more
+# dangerous than casting any other value.
+   nw="$nw -Winline"# It's OK to not inline.
+   nw="$nw -Wsign-compare"  # Too many false alarms.
+   nw="$nw -Wstrict-overflow"   # It's OK to optimize strictly.
+   nw="$nw -Wsystem-headers"# Don't warn in system headers.
+
+   # Setup the list of meaningful warning options for the C compiler.
+   # The list comes from manywarnings.m4. Warning options that are not
+   # generally meaningful have already been filtered out (cf.
+   # build-aux/gcc-warning.spec).
+   gl_MANYWARN_ALL_GCC([possible_warning_options])
+
+   # Compute the list of warning options that are desired.
+   gl_MANYWARN_COMPLEMENT([desired_warning_options],
+  [$possible_warning_options], [$nw])
+   # Compute the list of remaining undesired warning options.
+   # Namely those, that were not in manywarnings.m4 because they were
+   # already listed in build-aux/gcc-warning.spec; this includes those
+   # that are implied by -Wall.
+   gl_MANYWARN_COMPLEMENT([remaining_undesired_warning_options],
+  [$nw], [$possible_warning_options])
+
+   # Add the desired warning options to WARN_CFLAGS.
+   for w in 

Re: gcc -Wall vs. manywarnings

[Bruno Haible]

Paul Eggert wrote on 2023-05-27:
> If there are other ways to generate warnings to find bugs that are worth 
> the trouble of pacifying GCC, then it'd be helpful to use those ways 
> too. But plain 'gcc -Wall' is not one of those ways.
> ...
> I ... update the manywarnings module accordingly. This 
> process works well, for programs that use that module. It works better 
> than gcc -Wall does, in my experience.

OK, I took your word at face value, and changed GNU gettext to use
the manywarnings module. I care only about gcc 13, on a Linux/x86_64
system. Here are my findings:

* Actual bugs were found through these warning options:
  -Wanalyzer-null-dereference   (1)
  -Wanalyzer-possible-null-dereference  (1)
  -Wanalyzer-possible-null-argument (3)
  -Wmissing-field-initializers  (1)
  -Wunused-macros   (1)

* I can agree that "it works better than 'gcc -Wall'", in the sense
  that it found bugs that I had not spotted with merely "gcc -Wall".

* But I cannot agree that "it's not worth pacifying the warnings
  generated by 'gcc -Wall'", except for -Wsign-compare.
  'gcc -Wall' has the advantage that it is a good compromise and
  easy to use across packages.

* With more time investment, one gets more benefit. With just 'gcc -Wall',
  I didn't see the 7 bugs listed above, but I didn't have to spend much
  time learning about the various warnings.
  With the manywarnings module, I spent several days evaluating ca. 30
  different warning options. The benefit is that many more gcc warning
  options will be automatically enabled and thus useful in the future.

* Evaluating warnings is one possible way to improve the quality of a
  package. Alternatively, I could have spent the several days on a more
  elaborate fuzzing approach, or on writing more unit tests, or on
  improving the valgrind integration etc. — I can't say which is better,
  since I cannot measure "quality".

> It can be a random compiler if you use GCC the wrong way. If you use an 
> old version of GCC it might generate bogus diagnostics. Or if you 
> combine -Wall with some other options (-mx32, -Og, etc.) it might 
> generate bogus diagnostics. We shouldn't waste time pacifying gcc on 
> every possible platform with every possible set of options

I agree. In order to not waste time, we should better use new gcc versions
(or possibly also new clang versions?) to look at the diagnostics.

(Why do coreutils, grep, gzip, sed enable the gcc warnings for gcc ≥ 4.6
and not, say, only for gcc ≥ 10 ?)

Therefore I consider '-Werror' a disservice, especially if it's used
with old versions of gcc and/or on not-so-relevant platforms.
It encourages users to report any remaining warning even on not-so-relevant
platforms.

> For programs like diffutils that use the manywarnings module, the best 
> way to generate warnings is to configure with --enable-gcc-warnings.

For GNU gettext, I chose to make things work a bit differently than in
diffutils, coreutils, grep, sed,
see 
https://git.savannah.gnu.org/gitweb/?p=gettext.git;a=blob;f=m4/more-warnings.m4 
:
  * -Werror is not used (see above).
  * There is an option --enable-more-warnings, but hardly anyone will ever
need to specify it explicitly, since specifying -Wall triggers
--enable-more-warnings by default.
  * Compilations from a git checkout and compilations from a tarball
behave the same way.

Bruno

Re: error: Support the compiler's control flow analysis better (was: copy-file: Silence gcc warnings)

[Bruno Haible]

I did:
>   (error): Define as a macro that explicitly invokes exit().
>   (error_at_line): Likewise.

Oops. That patch broke the library namespacing via config.h. Namely,
when a package uses a config.h definition such as
  #define error libgettextpo_error
the intent is that the Gnulib module defines a function named
'libgettextpo_error', not 'error'.

With the error.in.h change, I now get redefinition warnings and compilation
errors (on platforms other than glibc and cygwin) such as

./error.h:458:11: warning: 'error' macro redefined [-Wmacro-redefined]
#  define error(status, ...) \
  ^
./config.h:51:9: note: previous definition is here
#define error libgettextpo_error
^

...

../../../gettext-tools/libgettextpo/gettext-po.c:1326:5: error: use of 
undeclared identifier 'error'
error (EXIT_FAILURE, 0, _("memory exhausted"));
^
./error.h:459:23: note: expanded from macro 'error'
 __gl_error_call (error, status, __VA_ARGS__)
  ^

The fix is:
  1) Capture the name of the error function through an static function.
  2) Since this static function is a varargs function and needs to call a
 varargs function, it needs to invoke __builtin_va_arg_pack().
 (The alternative would be to add a dependency from 'error' to the
 'verror' module. Which is not really better...)
  3) Since __builtin_va_arg_pack is only allowed in always-inline function,
 we need to mark it as _GL_ATTRIBUTE_ALWAYS_INLINE.
  4) The compiler then warns that the inlining may fail. To silence this
 warning, we need a '#pragma GCC diagnostic ignored "-Wattributes"'.
That's more involved than what I had expected.


2023-06-04  Bruno Haible  

error: Fix support for library namespacing (regression 2023-05-27).
* lib/error.in.h (error): If error is defined as a macro, define a
static inline function _gl_inline_error that invokes it, and let the
new error macro invoke that function.
(error_at_line): If error_at_line is defined as a macro, define a static
inline function _gl_inline_error_at_line that invokes it, and let the
new error_at_line macro invoke that function.

diff --git a/lib/error.in.h b/lib/error.in.h
index ef4b3c3815..94477fde08 100644
--- a/lib/error.in.h
+++ b/lib/error.in.h
@@ -30,7 +30,8 @@
 #ifndef _@GUARD_PREFIX@_ERROR_H
 #define _@GUARD_PREFIX@_ERROR_H
 
-/* This file uses _GL_ATTRIBUTE_FORMAT.  */
+/* This file uses _GL_ATTRIBUTE_ALWAYS_INLINE, _GL_ATTRIBUTE_FORMAT,
+  _GL_ATTRIBUTE_MAYBE_UNUSED.  */
 #if !_GL_CONFIG_H_INCLUDED
  #error "Please include config.h first."
 #endif
@@ -108,8 +109,28 @@ _GL_FUNCDECL_SYS (error, void,
 _GL_CXXALIAS_SYS (error, void,
   (int __status, int __errnum, const char *__format, ...));
 # ifndef _GL_NO_INLINE_ERROR
-#  define error(status, ...) \
- __gl_error_call (error, status, __VA_ARGS__)
+#  ifdef error
+/* Only gcc ≥ 4.7 has __builtin_va_arg_pack.  */
+#   if _GL_GNUC_PREREQ (4, 7)
+#pragma GCC diagnostic push
+#pragma GCC diagnostic ignored "-Wattributes"
+_GL_ATTRIBUTE_MAYBE_UNUSED
+static void
+_GL_ATTRIBUTE_ALWAYS_INLINE
+_GL_ATTRIBUTE_FORMAT ((_GL_ATTRIBUTE_SPEC_PRINTF_ERROR, 3, 4))
+_gl_inline_error (int __status, int __errnum, const char *__format, ...)
+{
+  return error (__status, __errnum, __format, __builtin_va_arg_pack ());
+}
+#pragma GCC diagnostic pop
+#undef error
+#define error(status, ...) \
+   __gl_error_call (_gl_inline_error, status, __VA_ARGS__)
+#   endif
+#  else
+#   define error(status, ...) \
+  __gl_error_call (error, status, __VA_ARGS__)
+#  endif
 # endif
 #endif
 #if __GLIBC__ >= 2
@@ -146,8 +167,30 @@ _GL_CXXALIAS_SYS (error_at_line, void,
   (int __status, int __errnum, const char *__filename,
unsigned int __lineno, const char *__format, ...));
 # ifndef _GL_NO_INLINE_ERROR
-#  define error_at_line(status, ...) \
- __gl_error_call (error_at_line, status, __VA_ARGS__)
+#  ifdef error_at_line
+/* Only gcc ≥ 4.7 has __builtin_va_arg_pack.  */
+#   if _GL_GNUC_PREREQ (4, 7)
+#pragma GCC diagnostic push
+#pragma GCC diagnostic ignored "-Wattributes"
+_GL_ATTRIBUTE_MAYBE_UNUSED
+static void
+_GL_ATTRIBUTE_ALWAYS_INLINE
+_GL_ATTRIBUTE_FORMAT ((_GL_ATTRIBUTE_SPEC_PRINTF_ERROR, 5, 6))
+_gl_inline_error_at_line (int __status, int __errnum, const char *__filename,
+  unsigned int __lineno, const char *__format, ...)
+{
+  return error_at_line (__status, __errnum, __filename, __lineno, __format,
+__builtin_va_arg_pack ());
+}
+#pragma GCC diagnostic pop
+#undef error_at_line
+#define error_at_line(status, ...) \
+   __gl_error_call (_gl_inline_error_at_line, status, __VA_ARGS__)
+#   endif
+#  else
+#   define error_at_line(status, ...) \
+  __gl_error_call (error_at_line, status, __VA_ARGS__)
+#  endif
 # endif
 #endif
 _GL_CXXALIASWARN (error_at_line);

terminfo, termcap: Silence "discards 'const' qualifier" warnings

[Bruno Haible]

In GNU gettext, I see these warnings (with gcc 12):

libtextstyle/lib/tparm.c:350:19: warning: assignment discards 'const' qualifier 
from pointer target type [-Wdiscarded-qualifiers]
libtextstyle/lib/tparm.c:432:19: warning: assignment discards 'const' qualifier 
from pointer target type [-Wdiscarded-qualifiers]
libtextstyle/lib/tparm.c:456:21: warning: assignment discards 'const' qualifier 
from pointer target type [-Wdiscarded-qualifiers]
libtextstyle/lib/tparm.c:460:21: warning: assignment discards 'const' qualifier 
from pointer target type [-Wdiscarded-qualifiers]
libtextstyle/lib/tparm.c:464:21: warning: assignment discards 'const' qualifier 
from pointer target type [-Wdiscarded-qualifiers]
libtextstyle/lib/tparm.c:468:21: warning: assignment discards 'const' qualifier 
from pointer target type [-Wdiscarded-qualifiers]

This patch fixes them.


2023-06-04  Bruno Haible  

terminfo, termcap: Fix "discards 'const' qualifier" warnings.
* lib/tparm.c (tparm): Change type of 'fmt'. New local variable 'fmtp'.

diff --git a/lib/tparm.c b/lib/tparm.c
index e9214e292c..1bc7c74845 100644
--- a/lib/tparm.c
+++ b/lib/tparm.c
@@ -260,7 +260,7 @@ tparm (const char *str, ...)
   static char buf[MAX_LINE];
   const char *sp;
   char *dp;
-  char *fmt;
+  const char *fmt;
   char scan_for;
   int scan_depth;
   int if_depth;
@@ -473,21 +473,22 @@ tparm (const char *str, ...)
 case '6': case '7': case '8': case '9':
   if (fmt == NULL)
 {
+  char *fmtp;
   if (termcap)
 return OOPS;
   if (*sp == ':')
 sp++;
-  fmt = fmt_buf;
-  *fmt++ = '%';
+  fmtp = fmt_buf;
+  *fmtp++ = '%';
   while (*sp != 's' && *sp != 'x' && *sp != 'X' && *sp != 'd'
  && *sp != 'o' && *sp != 'c' && *sp != 'u')
 {
   if (*sp == '\0')
 return OOPS;
-  *fmt++ = *sp++;
+  *fmtp++ = *sp++;
 }
-  *fmt++ = *sp;
-  *fmt = '\0';
+  *fmtp++ = *sp;
+  *fmtp = '\0';
   fmt = fmt_buf;
 }
   {

uniname/uniname: Add comments

[Bruno Haible]

This patch adds comments, how to avoid excessive compilation times due to
"gcc -fanalyzer".


2023-06-04  Bruno Haible  

uniname/uniname: Add comments.
* modules/uniname/uniname (Makefile.am): Explain how to work around a
GCC bug.

diff --git a/modules/uniname/uniname b/modules/uniname/uniname
index d71fe4ed11..826a2290f3 100644
--- a/modules/uniname/uniname
+++ b/modules/uniname/uniname
@@ -18,6 +18,13 @@ gl_LIBUNISTRING_MODULE([1.1], [uniname/uniname])
 
 Makefile.am:
 if LIBUNISTRING_COMPILE_UNINAME_UNINAME
+# Note: Compilation of this file takes a long time with gcc ≥ 11 and option
+# -fanalyzer. See .
+# The best workaround is to install GNU libunistring first, and use module
+# 'libunistring-optional' in your package.
+# An alternative workaround would be to pass the option -fno-analyzer, using
+# the technique from
+# 
https://www.gnu.org/software/automake/manual/html_node/Per_002dObject-Flags.html
 lib_SOURCES += uniname/uniname.c
 endif
 

uniname/uniname: Fix -Wformat-signedness warning

[Bruno Haible]

In GNU gettext, with gcc 13, I see this warning:

gettext-tools/gnulib-lib/uniname/uniname.c:295:42: warning: format '%d' expects 
argument of type 'int', but argument 3 has type 'ucs4_t' {aka 'unsigned int'} 
[-Wformat=]

This patch fixes it. The other patch fixes some comments.


2023-06-04  Bruno Haible  

uniname/uniname: Fix -Wformat-signedness warning.
* lib/uniname/uniname.c (unicode_character_name): Use %u instead of %d
in format string.

2023-06-04  Bruno Haible  

uniname/uniname: Improve comments.
* lib/uniname/uniname.c (unicode_character_name): Fix comments.

>From 39167e40eca3e65351ace6db11c8f10adbdf1802 Mon Sep 17 00:00:00 2001
From: Bruno Haible 
Date: Sun, 4 Jun 2023 10:45:00 +0200
Subject: [PATCH 1/2] uniname/uniname: Improve comments.

* lib/uniname/uniname.c (unicode_character_name): Fix comments.
---
 ChangeLog | 5 +
 lib/uniname/uniname.c | 9 +
 2 files changed, 10 insertions(+), 4 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index 5c1d3a1edf..959b18d396 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,8 @@
+2023-06-04  Bruno Haible  
+
+	uniname/uniname: Improve comments.
+	* lib/uniname/uniname.c (unicode_character_name): Fix comments.
+
 2023-06-03  Bruno Haible  
 
 	classpath, csharpexec: Avoid "candidate for attribute 'malloc'" warning.
diff --git a/lib/uniname/uniname.c b/lib/uniname/uniname.c
index 99b303a3d0..f6ef6956e8 100644
--- a/lib/uniname/uniname.c
+++ b/lib/uniname/uniname.c
@@ -245,7 +245,7 @@ unicode_character_name (ucs4_t c, char *buf)
   unsigned int index3;
   const char *q;
 
-  /* buf needs to have at least 16 + 7 bytes here.  */
+  /* buf needs to have at least 16 + 7 + 1 bytes here.  */
   memcpy (buf, "HANGUL SYLLABLE ", 16);
   ptr = buf + 16;
 
@@ -274,7 +274,7 @@ unicode_character_name (ucs4_t c, char *buf)
   char *ptr;
   int i;
 
-  /* buf needs to have at least 28 + 5 bytes here.  */
+  /* buf needs to have at least 28 + 5 + 1 bytes here.  */
   memcpy (buf, "CJK COMPATIBILITY IDEOGRAPH-", 28);
   ptr = buf + 28;
 
@@ -291,7 +291,7 @@ unicode_character_name (ucs4_t c, char *buf)
   /* Special case for variation selectors. Keeps the tables
  small.  */
 
-  /* buf needs to have at least 19 + 3 bytes here.  */
+  /* buf needs to have at least 19 + 3 + 1 bytes here.  */
   sprintf (buf, "VARIATION SELECTOR-%d",
c <= 0xFE0F ? c - 0xFE00 + 1 : c - 0xE0100 + 17);
   return buf;
@@ -339,7 +339,8 @@ unicode_character_name (ucs4_t c, char *buf)
   if (words != NULL)
 {
   /* Found it in unicode_index_to_name. Now concatenate the words.  */
-  /* buf needs to have at least UNICODE_CHARNAME_MAX_LENGTH bytes.  */
+  /* buf needs to have at least UNICODE_CHARNAME_MAX_LENGTH + 1
+ bytes.  */
   char *ptr = buf;
   for (;;)
 {
-- 
2.34.1

>From 9e13c0d5a8fc925bfb7ad977f69d04f446179143 Mon Sep 17 00:00:00 2001
From: Bruno Haible 
Date: Sun, 4 Jun 2023 10:50:29 +0200
Subject: [PATCH 2/2] uniname/uniname: Fix -Wformat-signedness warning.

* lib/uniname/uniname.c (unicode_character_name): Use %u instead of %d
in format string.
---
 ChangeLog | 6 ++
 lib/uniname/uniname.c | 2 +-
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/ChangeLog b/ChangeLog
index 959b18d396..72afbccd72 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,9 @@
+2023-06-04  Bruno Haible  
+
+	uniname/uniname: Fix -Wformat-signedness warning.
+	* lib/uniname/uniname.c (unicode_character_name): Use %u instead of %d
+	in format string.
+
 2023-06-04  Bruno Haible  
 
 	uniname/uniname: Improve comments.
diff --git a/lib/uniname/uniname.c b/lib/uniname/uniname.c
index f6ef6956e8..48bae59bec 100644
--- a/lib/uniname/uniname.c
+++ b/lib/uniname/uniname.c
@@ -292,7 +292,7 @@ unicode_character_name (ucs4_t c, char *buf)
  small.  */
 
   /* buf needs to have at least 19 + 3 + 1 bytes here.  */
-  sprintf (buf, "VARIATION SELECTOR-%d",
+  sprintf (buf, "VARIATION SELECTOR-%u",
c <= 0xFE0F ? c - 0xFE00 + 1 : c - 0xE0100 + 17);
   return buf;
 }
-- 
2.34.1