diff mbox series

avoid -Waddress in vfprintf-internal.c

Message ID 14db3354-56a5-86cc-b840-010c9ba7fc83@gmail.com
State New
Headers show
Series avoid -Waddress in vfprintf-internal.c | expand

Checks

Context Check Description
dj/TryBot-apply_patch success Patch applied to master at the time it was sent
dj/TryBot-32bit success Build for i686

Commit Message

Martin Sebor Sept. 22, 2021, 12:31 a.m. UTC
Building Glibc with a GCC 12 enhanced to detect more instances
of comparing addresses to null that are guaranteed to evaluate
to a constanst triggers a large number of such instancesl.
The warnings, all isolated to the same file, are valid and
intended but the Glibc code is safe.  They show up because
the comparison is in a macro to which either null or a constant
address of an array element are alternately passed as an argument.

The attached patch avoids these warnings by introducing local
variables for the address being compared (an array element)
as well as for the null pointer.

Tested by building Glibc on x86_64, verifying the warnings
are gone, and by running the testsuite and checking for new
failures.

Martin

Comments

Florian Weimer Sept. 22, 2021, 6:13 a.m. UTC | #1
* Martin Sebor via Libc-alpha:

> Building Glibc with a GCC 12 enhanced to detect more instances
> of comparing addresses to null that are guaranteed to evaluate
> to a constanst triggers a large number of such instancesl.
> The warnings, all isolated to the same file, are valid and
> intended but the Glibc code is safe.  They show up because
> the comparison is in a macro to which either null or a constant
> address of an array element are alternately passed as an argument.
>
> The attached patch avoids these warnings by introducing local
> variables for the address being compared (an array element)
> as well as for the null pointer.
>
> Tested by building Glibc on x86_64, verifying the warnings
> are gone, and by running the testsuite and checking for new
> failures.

I'm testing patches to clean this up, with a net reduction in the number
of lines of code.

Thanks,
Florian
Florian Weimer Sept. 27, 2021, 1:48 p.m. UTC | #2
* Martin Sebor via Libc-alpha:

> Building Glibc with a GCC 12 enhanced to detect more instances
> of comparing addresses to null that are guaranteed to evaluate
> to a constanst triggers a large number of such instancesl.
> The warnings, all isolated to the same file, are valid and
> intended but the Glibc code is safe.  They show up because
> the comparison is in a macro to which either null or a constant
> address of an array element are alternately passed as an argument.
>
> The attached patch avoids these warnings by introducing local
> variables for the address being compared (an array element)
> as well as for the null pointer.
>
> Tested by building Glibc on x86_64, verifying the warnings
> are gone, and by running the testsuite and checking for new
> failures.

I believe the issue addressed in this patch no longer exists in current
Git.  Would you please verify?

Thanks,
Florian
Martin Sebor Sept. 27, 2021, 11:58 p.m. UTC | #3
On 9/27/21 7:48 AM, Florian Weimer wrote:
> * Martin Sebor via Libc-alpha:
> 
>> Building Glibc with a GCC 12 enhanced to detect more instances
>> of comparing addresses to null that are guaranteed to evaluate
>> to a constanst triggers a large number of such instancesl.
>> The warnings, all isolated to the same file, are valid and
>> intended but the Glibc code is safe.  They show up because
>> the comparison is in a macro to which either null or a constant
>> address of an array element are alternately passed as an argument.
>>
>> The attached patch avoids these warnings by introducing local
>> variables for the address being compared (an array element)
>> as well as for the null pointer.
>>
>> Tested by building Glibc on x86_64, verifying the warnings
>> are gone, and by running the testsuite and checking for new
>> failures.
> 
> I believe the issue addressed in this patch no longer exists in current
> Git.  Would you please verify?

I don't see the warnings in my latest build anymore.  The GCC patch
that enhances the warning is still waiting for formal approval but
I don't expect to make any substantive changes to it.  I've resolved
the bug I raised to keep track of the warnings (BZ #28368).

Thanks
Martin
diff mbox series

Patch

diff --git a/stdio-common/vfprintf-internal.c b/stdio-common/vfprintf-internal.c
index 3f3d1e148a..1a9f94e930 100644
--- a/stdio-common/vfprintf-internal.c
+++ b/stdio-common/vfprintf-internal.c
@@ -1447,6 +1447,9 @@  vfprintf (FILE *s, const CHAR_T *format, va_list ap, unsigned int mode_flags)
       UCHAR_T pad = L_(' ');/* Padding character.  */
       CHAR_T spec;
 
+      /* Passed to the process_arg macro instead of NULL to avoid -Waddress.  */
+      struct printf_spec * null_pspec = NULL;
+
       workend = work_buffer + WORK_BUFFER_SIZE;
 
       /* Get current character in format string.  */
@@ -1643,8 +1646,8 @@  vfprintf (FILE *s, const CHAR_T *format, va_list ap, unsigned int mode_flags)
       /* Process current format.  */
       while (1)
 	{
-	  process_arg (((struct printf_spec *) NULL));
-	  process_string_arg (((struct printf_spec *) NULL));
+	  process_arg (null_pspec);
+	  process_string_arg (null_pspec);
 
 	LABEL (form_unknown):
 	  if (spec == L_('\0'))
@@ -1904,53 +1907,55 @@  printf_positional (FILE *s, const CHAR_T *format, int readonly_format,
       union printf_arg the_arg;
       CHAR_T *string;		/* Pointer to argument string.  */
 
+      struct printf_spec * const pspec = &specs[nspecs_done];
+
       /* Fill variables from values in struct.  */
-      int alt = specs[nspecs_done].info.alt;
-      int space = specs[nspecs_done].info.space;
-      int left = specs[nspecs_done].info.left;
-      int showsign = specs[nspecs_done].info.showsign;
-      int group = specs[nspecs_done].info.group;
-      int is_long_double = specs[nspecs_done].info.is_long_double;
-      int is_short = specs[nspecs_done].info.is_short;
-      int is_char = specs[nspecs_done].info.is_char;
-      int is_long = specs[nspecs_done].info.is_long;
-      int width = specs[nspecs_done].info.width;
-      int prec = specs[nspecs_done].info.prec;
-      int use_outdigits = specs[nspecs_done].info.i18n;
-      char pad = specs[nspecs_done].info.pad;
-      CHAR_T spec = specs[nspecs_done].info.spec;
+      int alt = pspec->info.alt;
+      int space = pspec->info.space;
+      int left = pspec->info.left;
+      int showsign = pspec->info.showsign;
+      int group = pspec->info.group;
+      int is_long_double = pspec->info.is_long_double;
+      int is_short = pspec->info.is_short;
+      int is_char = pspec->info.is_char;
+      int is_long = pspec->info.is_long;
+      int width = pspec->info.width;
+      int prec = pspec->info.prec;
+      int use_outdigits = pspec->info.i18n;
+      char pad = pspec->info.pad;
+      CHAR_T spec = pspec->info.spec;
 
       CHAR_T *workend = work_buffer + WORK_BUFFER_SIZE;
 
       /* Fill in last information.  */
-      if (specs[nspecs_done].width_arg != -1)
+      if (pspec->width_arg != -1)
 	{
 	  /* Extract the field width from an argument.  */
-	  specs[nspecs_done].info.width =
-	    args_value[specs[nspecs_done].width_arg].pa_int;
+	  pspec->info.width =
+	    args_value[pspec->width_arg].pa_int;
 
-	  if (specs[nspecs_done].info.width < 0)
+	  if (pspec->info.width < 0)
 	    /* If the width value is negative left justification is
 	       selected and the value is taken as being positive.  */
 	    {
-	      specs[nspecs_done].info.width *= -1;
-	      left = specs[nspecs_done].info.left = 1;
+	      pspec->info.width *= -1;
+	      left = pspec->info.left = 1;
 	    }
-	  width = specs[nspecs_done].info.width;
+	  width = pspec->info.width;
 	}
 
-      if (specs[nspecs_done].prec_arg != -1)
+      if (pspec->prec_arg != -1)
 	{
 	  /* Extract the precision from an argument.  */
-	  specs[nspecs_done].info.prec =
-	    args_value[specs[nspecs_done].prec_arg].pa_int;
+	  pspec->info.prec =
+	    args_value[pspec->prec_arg].pa_int;
 
-	  if (specs[nspecs_done].info.prec < 0)
+	  if (pspec->info.prec < 0)
 	    /* If the precision is negative the precision is
 	       omitted.  */
-	    specs[nspecs_done].info.prec = -1;
+	    pspec->info.prec = -1;
 
-	  prec = specs[nspecs_done].info.prec;
+	  prec = pspec->info.prec;
 	}
 
       /* Process format specifiers.  */
@@ -1963,17 +1968,17 @@  printf_positional (FILE *s, const CHAR_T *format, int readonly_format,
 	      && __printf_function_table != NULL
 	      && __printf_function_table[(size_t) spec] != NULL)
 	    {
-	      const void **ptr = alloca (specs[nspecs_done].ndata_args
+	      const void **ptr = alloca (pspec->ndata_args
 					 * sizeof (const void *));
 
 	      /* Fill in an array of pointers to the argument values.  */
-	      for (unsigned int i = 0; i < specs[nspecs_done].ndata_args;
+	      for (unsigned int i = 0; i < pspec->ndata_args;
 		   ++i)
-		ptr[i] = &args_value[specs[nspecs_done].data_arg + i];
+		ptr[i] = &args_value[pspec->data_arg + i];
 
 	      /* Call the function.  */
 	      function_done = __printf_function_table[(size_t) spec]
-		(s, &specs[nspecs_done].info, ptr);
+		(s, &pspec->info, ptr);
 
 	      if (function_done != -2)
 		{
@@ -1993,23 +1998,23 @@  printf_positional (FILE *s, const CHAR_T *format, int readonly_format,
 
 	  JUMP (spec, step4_jumps);
 
-	  process_arg ((&specs[nspecs_done]));
-	  process_string_arg ((&specs[nspecs_done]));
+	  process_arg (pspec);
+	  process_string_arg (pspec);
 
 	  LABEL (form_unknown):
 	  {
 	    unsigned int i;
 	    const void **ptr;
 
-	    ptr = alloca (specs[nspecs_done].ndata_args
+	    ptr = alloca (pspec->ndata_args
 			  * sizeof (const void *));
 
 	    /* Fill in an array of pointers to the argument values.  */
-	    for (i = 0; i < specs[nspecs_done].ndata_args; ++i)
-	      ptr[i] = &args_value[specs[nspecs_done].data_arg + i];
+	    for (i = 0; i < pspec->ndata_args; ++i)
+	      ptr[i] = &args_value[pspec->data_arg + i];
 
 	    /* Call the function.  */
-	    function_done = printf_unknown (s, &specs[nspecs_done].info,
+	    function_done = printf_unknown (s, &pspec->info,
 					    ptr);
 
 	    /* If an error occurred we don't have information about #
@@ -2027,9 +2032,7 @@  printf_positional (FILE *s, const CHAR_T *format, int readonly_format,
 	}
 
       /* Write the following constant string.  */
-      outstring (specs[nspecs_done].end_of_fmt,
-		 specs[nspecs_done].next_fmt
-		 - specs[nspecs_done].end_of_fmt);
+      outstring (pspec->end_of_fmt, pspec->next_fmt - pspec->end_of_fmt);
     }
  all_done:
   scratch_buffer_free (&argsbuf);