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

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.0 -0700
+++ lib/getopt.c2020-05-27 14:57:42.0 -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