getopt.c warnings patch

getopt.c warnings patch

[Paul J. Lucas]

The getopt.c file generates the following warnings from Apple’s gcc (Apple clang version 11.0.3 (clang-1103.0.32.62)):

--------------------------------------------------------------------------------
getopt.c:208:21: warning: implicit conversion changes signedness: 'long' to
      'size_t' (aka 'unsigned long') [-Wsign-conversion]
  namelen = nameend - d->__nextchar;
          ~ ~~~~~~~~^~~~~~~~~~~~~~~
getopt.c:255:34: warning: implicit conversion changes signedness: 'int' to
      'unsigned long' [-Wsign-conversion]
                        else if ((ambig_set = malloc (n_options)) == NULL)
                                              ~~~~~~  ^~~~~~~~~
getopt.c:369:16: warning: variable 'option_index' may be uninitialized when used
      here [-Wconditional-uninitialized]
    *longind = option_index;
               ^~~~~~~~~~~~
getopt.c:204:19: note: initialize the variable 'option_index' to silence this
      warning
  int option_index;
                  ^
                   = 0
3 warnings generated.
--------------------------------------------------------------------------------

when compiled with these warnings enabled:

-Wall -Wcast-align -Wcomma -Wconditional-type-mismatch -Wconditional-uninitialized -Wconversion -Wextra -Wfloat-equal -Wfor-loop-analysis -Widiomatic-parentheses -Wimplicit-fallthrough -Wlogical-op-parentheses -Wnewline-eof -Wno-unknown-warning-option -Wredundant-decls -Wshadow -Wshift-sign-overflow -Wsign-compare -Wsign-conversion -Wsometimes-uninitialized -Wstring-conversion -Wuninitialized -Wunreachable-code-break -Wunreachable-code -Wunused -Wwrite-strings

Below is a patch that fixes all these warnings.

- Paul


--- lib/getopt.c.ORIG	2020-05-27 14:45:22.000000000 -0700
+++ lib/getopt.c	2020-05-27 14:57:42.000000000 -0700
@@ -201,11 +201,11 @@
   const struct option *p;
   const struct option *pfound = NULL;
   int n_options;
-  int option_index;
+  int option_index = 0;
 
   for (nameend = d->__nextchar; *nameend && *nameend != '='; nameend++)
     /* Do nothing.  */ ;
-  namelen = nameend - d->__nextchar;
+  namelen = (size_t)(nameend - d->__nextchar);
 
   /* First look for an exact match, counting the options as a side
      effect.  */
@@ -252,7 +252,7 @@
 		      {
 			if (__libc_use_alloca (n_options))
 			  ambig_set = alloca (n_options);
-			else if ((ambig_set = malloc (n_options)) == NULL)
+			else if ((ambig_set = malloc ((size_t)n_options)) == NULL)
 			  /* Fall back to simpler error message.  */
 			  ambig_fallback = 1;
 			else

Re: getopt.c warnings patch

[Bruno Haible]

Hi,

Paul J. Lucas wrote:
> The getopt.c file generates the following warnings from Apple’s gcc (Apple clang version 11.0.3 (clang-1103.0.32.62)):
> 
> --------------------------------------------------------------------------------
> getopt.c:208:21: warning: implicit conversion changes signedness: 'long' to
>       'size_t' (aka 'unsigned long') [-Wsign-conversion]
>   namelen = nameend - d->__nextchar;
>           ~ ~~~~~~~~^~~~~~~~~~~~~~~
> getopt.c:255:34: warning: implicit conversion changes signedness: 'int' to
>       'unsigned long' [-Wsign-conversion]
>                         else if ((ambig_set = malloc (n_options)) == NULL)
>                                               ~~~~~~  ^~~~~~~~~

The obvious "fix" for these warnings is to introduce a cast. But such casts
would decrease the robustness of the code. As I wrote in [1], such explicit
casts introduce bugs when the standards change or some platform is not 100%
standards compliant.

Therefore it is best to ignore warnings of this type. That's what gnulib does,
through the file build-aux/gcc-warning.spec, when you use the
gl_MANYWARN_ALL_GCC macro.

> getopt.c:369:16: warning: variable 'option_index' may be uninitialized when used
>       here [-Wconditional-uninitialized]
>     *longind = option_index;
>                ^~~~~~~~~~~~
> getopt.c:204:19: note: initialize the variable 'option_index' to silence this
>       warning
>   int option_index;
>                   ^
>                    = 0

Here the code is copying an uninitialized value, if pfound == NULL. But this is
harmless, because
  1) The documentation of _getopt_internal_r says that
       "LONGIND returns the index in LONGOPT of the long-named option found.
        It is only valid when a long-named option has been found by the most
        recent call."
  2) valgrind does not complain about copying an uninitialized value, if it ends
     up being unused.

Bruno

[1] https://bugs.llvm.org/show_bug.cgi?id=46025