avoid -Waddress in vfprintf-internal.c

avoid -Waddress in vfprintf-internal.c

[Martin Sebor]

[-- Attachment #1: Type: text/plain, Size: 728 bytes --]

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

[-- Attachment #2: glibc-28368.diff --]
[-- Type: text/x-patch, Size: 6357 bytes --]

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);

Re: avoid -Waddress in vfprintf-internal.c

[Florian Weimer]

* 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

Re: avoid -Waddress in vfprintf-internal.c

[Florian Weimer]

* 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

Re: avoid -Waddress in vfprintf-internal.c

[Martin Sebor]

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

Re: avoid -Waddress in vfprintf-internal.c

[Carlos O'Donell]

On 9/27/21 19:58, Martin Sebor via Libc-alpha wrote:
> 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).

Is this patch still required for any other reasons?

-- 
Cheers,
Carlos.

Re: avoid -Waddress in vfprintf-internal.c

[Martin Sebor]

On 10/19/21 10:26 AM, Carlos O'Donell wrote:
> On 9/27/21 19:58, Martin Sebor via Libc-alpha wrote:
>> 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).
> 
> Is this patch still required for any other reasons?

I don't think so.

Thanks
Martin