diff mbox series

[2/2] vfprintf: Unify argument handling in process_arg

Message ID 1a90d7bf325cd6606bcb083ffc5cd528eefad219.1632294469.git.fweimer@redhat.com
State New
Headers show
Series vfprintf cleanups to avoid -Waddress warning | 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

Florian Weimer Sept. 22, 2021, 7:10 a.m. UTC
Instead of checking a pointer argument for NULL, use helper macros
defined differently in the non-positional and positional cases.
This avoids frequent conditional checks and a GCC 12 warning
about comparing pointers against NULL which cannot be NULL.
---
 stdio-common/vfprintf-internal.c | 206 +++++++++++++------------------
 1 file changed, 89 insertions(+), 117 deletions(-)

Comments

Andreas Schwab Sept. 22, 2021, 2:17 p.m. UTC | #1
On Sep 22 2021, Florian Weimer via Libc-alpha wrote:

> Instead of checking a pointer argument for NULL, use helper macros
> defined differently in the non-positional and positional cases.
> This avoids frequent conditional checks and a GCC 12 warning
> about comparing pointers against NULL which cannot be NULL.

Ok.

> @@ -1545,8 +1474,28 @@ 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));
> +#define PROCESS_ARG_INT() va_arg (ap, int)
> +#define PROCESS_ARG_LONG_INT() va_arg (ap, long int)
> +#define PROCESS_ARG_LONG_LONG_INT() va_arg (ap, long long int)
> +#define PROCESS_ARG_POINTER() va_arg (ap, void *)
> +#define PROCESS_ARG_STRING() va_arg (ap, const char *)
> +#define PROCESS_ARG_UNSIGNED_INT() va_arg (ap, unsigned int)
> +#define PROCESS_ARG_UNSIGNED_LONG_INT() va_arg (ap, unsigned long int)
> +#define PROCESS_ARG_UNSIGNED_LONG_LONG_INT() va_arg (ap, unsigned long long int)
> +#define PROCESS_ARG_WCHAR_T() va_arg (ap, wchar_t)
> +#define PROCESS_ARG_WSTRING() va_arg (ap, const wchar_t *)

I think it would be nicer to spell these macros in lower case.

Andreas.
Florian Weimer Sept. 23, 2021, 10:07 a.m. UTC | #2
* Andreas Schwab:

> On Sep 22 2021, Florian Weimer via Libc-alpha wrote:
>
>> Instead of checking a pointer argument for NULL, use helper macros
>> defined differently in the non-positional and positional cases.
>> This avoids frequent conditional checks and a GCC 12 warning
>> about comparing pointers against NULL which cannot be NULL.
>
> Ok.
>
>> @@ -1545,8 +1474,28 @@ 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));
>> +#define PROCESS_ARG_INT() va_arg (ap, int)
>> +#define PROCESS_ARG_LONG_INT() va_arg (ap, long int)
>> +#define PROCESS_ARG_LONG_LONG_INT() va_arg (ap, long long int)
>> +#define PROCESS_ARG_POINTER() va_arg (ap, void *)
>> +#define PROCESS_ARG_STRING() va_arg (ap, const char *)
>> +#define PROCESS_ARG_UNSIGNED_INT() va_arg (ap, unsigned int)
>> +#define PROCESS_ARG_UNSIGNED_LONG_INT() va_arg (ap, unsigned long int)
>> +#define PROCESS_ARG_UNSIGNED_LONG_LONG_INT() va_arg (ap, unsigned long long int)
>> +#define PROCESS_ARG_WCHAR_T() va_arg (ap, wchar_t)
>> +#define PROCESS_ARG_WSTRING() va_arg (ap, const wchar_t *)
>
> I think it would be nicer to spell these macros in lower case.

Fair enough, I've renamed the macros to lower case before pushing.

Thanks,
Florian
diff mbox series

Patch

diff --git a/stdio-common/vfprintf-internal.c b/stdio-common/vfprintf-internal.c
index 8e31a964d1..7a6aac2a14 100644
--- a/stdio-common/vfprintf-internal.c
+++ b/stdio-common/vfprintf-internal.c
@@ -650,8 +650,9 @@  static const uint8_t jump_table[] =
       REF (form_unknown)        /* for 'I' */				      \
     }
 
-
-#define process_arg(fspec)						      \
+/* Before invoking this macro, PROCESS_ARG_INT etc. macros have to be
+   defined to extract one argument of the appropriate type.  */
+#define process_arg()						              \
       /* Start real work.  We know about all flags and modifiers and	      \
 	 now process the wanted format specifier.  */			      \
     LABEL (form_percent):						      \
@@ -665,13 +666,7 @@  static const uint8_t jump_table[] =
 									      \
       if (is_longlong)							      \
 	{								      \
-	  long long int signed_number;					      \
-									      \
-	  if (fspec == NULL)						      \
-	    signed_number = va_arg (ap, long long int);			      \
-	  else								      \
-	    signed_number = args_value[fspec->data_arg].pa_long_long_int;     \
-									      \
+	  long long int signed_number = PROCESS_ARG_LONG_LONG_INT ();	      \
 	  is_negative = signed_number < 0;				      \
 	  number.longlong = is_negative ? (- signed_number) : signed_number;  \
 									      \
@@ -680,29 +675,14 @@  static const uint8_t jump_table[] =
       else								      \
 	{								      \
 	  long int signed_number;					      \
-									      \
-	  if (fspec == NULL)						      \
-	    {								      \
-	      if (is_long_num)						      \
-		signed_number = va_arg (ap, long int);			      \
-	      else if (is_char)						      \
-		signed_number = (signed char) va_arg (ap, unsigned int);      \
-	      else if (!is_short)					      \
-		signed_number = va_arg (ap, int);			      \
-	      else							      \
-		signed_number = (short int) va_arg (ap, unsigned int);	      \
-	    }								      \
+	  if (is_long_num)						      \
+	    signed_number = PROCESS_ARG_LONG_INT ();			      \
+	  else if (is_char)						      \
+	    signed_number = (signed char) PROCESS_ARG_UNSIGNED_INT ();	      \
+	  else if (!is_short)						      \
+	    signed_number = PROCESS_ARG_INT ();				      \
 	  else								      \
-	    if (is_long_num)						      \
-	      signed_number = args_value[fspec->data_arg].pa_long_int;	      \
-	    else if (is_char)						      \
-	      signed_number = (signed char)				      \
-		args_value[fspec->data_arg].pa_u_int;			      \
-	    else if (!is_short)						      \
-	      signed_number = args_value[fspec->data_arg].pa_int;	      \
-	    else							      \
-	      signed_number = (short int)				      \
-		args_value[fspec->data_arg].pa_u_int;			      \
+	    signed_number = (short int) PROCESS_ARG_UNSIGNED_INT ();	      \
 									      \
 	  is_negative = signed_number < 0;				      \
 	  number.word = is_negative ? (- signed_number) : signed_number;      \
@@ -737,10 +717,7 @@  static const uint8_t jump_table[] =
 									      \
       if (is_longlong)							      \
 	{								      \
-	  if (fspec == NULL)						      \
-	    number.longlong = va_arg (ap, unsigned long long int);	      \
-	  else								      \
-	    number.longlong = args_value[fspec->data_arg].pa_u_long_long_int; \
+	  number.longlong = PROCESS_ARG_UNSIGNED_LONG_LONG_INT ();	      \
 									      \
 	LABEL (longlong_number):					      \
 	  if (prec < 0)							      \
@@ -776,28 +753,14 @@  static const uint8_t jump_table[] =
 	}								      \
       else								      \
 	{								      \
-	  if (fspec == NULL)						      \
-	    {								      \
-	      if (is_long_num)						      \
-		number.word = va_arg (ap, unsigned long int);		      \
-	      else if (is_char)						      \
-		number.word = (unsigned char) va_arg (ap, unsigned int);      \
-	      else if (!is_short)					      \
-		number.word = va_arg (ap, unsigned int);		      \
-	      else							      \
-		number.word = (unsigned short int) va_arg (ap, unsigned int); \
-	    }								      \
+	  if (is_long_num)						      \
+	    number.word = PROCESS_ARG_UNSIGNED_LONG_INT ();		      \
+	  else if (is_char)						      \
+	    number.word = (unsigned char) PROCESS_ARG_UNSIGNED_INT ();	      \
+	  else if (!is_short)						      \
+	    number.word = PROCESS_ARG_UNSIGNED_INT ();			      \
 	  else								      \
-	    if (is_long_num)						      \
-	      number.word = args_value[fspec->data_arg].pa_u_long_int;	      \
-	    else if (is_char)						      \
-	      number.word = (unsigned char)				      \
-		args_value[fspec->data_arg].pa_u_int;			      \
-	    else if (!is_short)						      \
-	      number.word = args_value[fspec->data_arg].pa_u_int;	      \
-	    else							      \
-	      number.word = (unsigned short int)			      \
-		args_value[fspec->data_arg].pa_u_int;			      \
+	    number.word = (unsigned short int) PROCESS_ARG_UNSIGNED_INT ();   \
 									      \
 	LABEL (number):							      \
 	  if (prec < 0)							      \
@@ -917,11 +880,7 @@  static const uint8_t jump_table[] =
     LABEL (form_pointer):						      \
       /* Generic pointer.  */						      \
       {									      \
-	const void *ptr;						      \
-	if (fspec == NULL)						      \
-	  ptr = va_arg (ap, void *);					      \
-	else								      \
-	  ptr = args_value[fspec->data_arg].pa_pointer;			      \
+	const void *ptr = PROCESS_ARG_POINTER ();			      \
 	if (ptr != NULL)						      \
 	  {								      \
 	    /* If the pointer is not NULL, write it as a %#x spec.  */	      \
@@ -962,30 +921,17 @@  static const uint8_t jump_table[] =
 	    __libc_fatal ("*** %n in writable segment detected ***\n");	      \
 	}								      \
       /* Answer the count of characters written.  */			      \
-      if (fspec == NULL)						      \
-	{								      \
-	  if (is_longlong)						      \
-	    *(long long int *) va_arg (ap, void *) = done;		      \
-	  else if (is_long_num)						      \
-	    *(long int *) va_arg (ap, void *) = done;			      \
-	  else if (is_char)						      \
-	    *(char *) va_arg (ap, void *) = done;			      \
-	  else if (!is_short)						      \
-	    *(int *) va_arg (ap, void *) = done;			      \
-	  else								      \
-	    *(short int *) va_arg (ap, void *) = done;			      \
-	}								      \
+      void *ptrptr = PROCESS_ARG_POINTER ();				      \
+      if (is_longlong)							      \
+	*(long long int *) ptrptr = done;				      \
+      else if (is_long_num)						      \
+	*(long int *) ptrptr = done;					      \
+      else if (is_char)							      \
+	*(char *) ptrptr = done;					      \
+      else if (!is_short)						      \
+	*(int *) ptrptr = done;						      \
       else								      \
-	if (is_longlong)						      \
-	  *(long long int *) args_value[fspec->data_arg].pa_pointer = done;   \
-	else if (is_long_num)						      \
-	  *(long int *) args_value[fspec->data_arg].pa_pointer = done;	      \
-	else if (is_char)						      \
-	  *(char *) args_value[fspec->data_arg].pa_pointer = done;	      \
-	else if (!is_short)						      \
-	  *(int *) args_value[fspec->data_arg].pa_pointer = done;	      \
-	else								      \
-	  *(short int *) args_value[fspec->data_arg].pa_pointer = done;	      \
+	*(short int *) ptrptr = done;					      \
       break;								      \
 									      \
     LABEL (form_strerror):						      \
@@ -997,7 +943,7 @@  static const uint8_t jump_table[] =
       goto LABEL (print_string)
 
 #ifdef COMPILE_WPRINTF
-# define process_string_arg(fspec) \
+# define process_string_arg()						      \
     LABEL (form_character):						      \
       /* Character.  */							      \
       if (is_long)							      \
@@ -1005,11 +951,7 @@  static const uint8_t jump_table[] =
       --width;	/* Account for the character itself.  */		      \
       if (!left)							      \
 	PAD (L' ');							      \
-      if (fspec == NULL)						      \
-	outchar (__btowc ((unsigned char) va_arg (ap, int))); /* Promoted. */ \
-      else								      \
-	outchar (__btowc ((unsigned char)				      \
-			  args_value[fspec->data_arg].pa_int));		      \
+      outchar (__btowc ((unsigned char) PROCESS_ARG_INT ())); /* Promoted. */ \
       if (left)								      \
 	PAD (L' ');							      \
       break;								      \
@@ -1020,10 +962,7 @@  static const uint8_t jump_table[] =
 	--width;							      \
 	if (!left)							      \
 	  PAD (L' ');							      \
-	if (fspec == NULL)						      \
-	  outchar (va_arg (ap, wchar_t));				      \
-	else								      \
-	  outchar (args_value[fspec->data_arg].pa_wchar);		      \
+	outchar (PROCESS_ARG_WCHAR_T ());				      \
 	if (left)							      \
 	  PAD (L' ');							      \
       }									      \
@@ -1035,10 +974,7 @@  static const uint8_t jump_table[] =
 									      \
 	/* The string argument could in fact be `char *' or `wchar_t *'.      \
 	   But this should not make a difference here.  */		      \
-	if (fspec == NULL)						      \
-	  string = (CHAR_T *) va_arg (ap, const wchar_t *);		      \
-	else								      \
-	  string = (CHAR_T *) args_value[fspec->data_arg].pa_wstring;	      \
+	string = (CHAR_T *) PROCESS_ARG_WSTRING ();			      \
 									      \
 	/* Entry point for printing other strings.  */			      \
       LABEL (print_string):						      \
@@ -1090,7 +1026,7 @@  static const uint8_t jump_table[] =
       }									      \
       break;
 #else
-# define process_string_arg(fspec) \
+# define process_string_arg()						      \
     LABEL (form_character):						      \
       /* Character.  */							      \
       if (is_long)							      \
@@ -1098,10 +1034,7 @@  static const uint8_t jump_table[] =
       --width;	/* Account for the character itself.  */		      \
       if (!left)							      \
 	PAD (' ');							      \
-      if (fspec == NULL)						      \
-	outchar ((unsigned char) va_arg (ap, int)); /* Promoted.  */	      \
-      else								      \
-	outchar ((unsigned char) args_value[fspec->data_arg].pa_int);	      \
+      outchar ((unsigned char) PROCESS_ARG_INT ()); /* Promoted.  */	      \
       if (left)								      \
 	PAD (' ');							      \
       break;								      \
@@ -1114,9 +1047,7 @@  static const uint8_t jump_table[] =
 	size_t len;							      \
 									      \
 	memset (&mbstate, '\0', sizeof (mbstate_t));			      \
-	len = __wcrtomb (buf, (fspec == NULL ? va_arg (ap, wchar_t)	      \
-			       : args_value[fspec->data_arg].pa_wchar),	      \
-			 &mbstate);					      \
+	len = __wcrtomb (buf, PROCESS_ARG_WCHAR_T (), &mbstate);	      \
 	if (len == (size_t) -1)						      \
 	  {								      \
 	    /* Something went wrong during the conversion.  Bail out.  */     \
@@ -1138,10 +1069,7 @@  static const uint8_t jump_table[] =
 									      \
 	/* The string argument could in fact be `char *' or `wchar_t *'.      \
 	   But this should not make a difference here.  */		      \
-	if (fspec == NULL)						      \
-	  string = (char *) va_arg (ap, const char *);			      \
-	else								      \
-	  string = (char *) args_value[fspec->data_arg].pa_string;	      \
+	string = (char *) PROCESS_ARG_STRING ();			      \
 									      \
 	/* Entry point for printing other strings.  */			      \
       LABEL (print_string):						      \
@@ -1322,7 +1250,6 @@  vfprintf (FILE *s, const CHAR_T *format, va_list ap, unsigned int mode_flags)
       STEP0_3_TABLE;
       STEP4_TABLE;
 
-      union printf_arg *args_value;	/* This is not used here but ... */
       int is_negative;	/* Flag for negative number.  */
       union
       {
@@ -1337,7 +1264,9 @@  vfprintf (FILE *s, const CHAR_T *format, va_list ap, unsigned int mode_flags)
       int left = 0;	/* Left-justify output.  */
       int showsign = 0;	/* Always begin with plus or minus sign.  */
       int group = 0;	/* Print numbers according grouping rules.  */
-      int is_long_double = 0; /* Argument is long double/ long long int.  */
+      /* Argument is long double/long long int.  Only used if
+	 double/long double or long int/long long int are distinct.  */
+      int is_long_double __attribute__ ((unused)) = 0;
       int is_short = 0;	/* Argument is short int.  */
       int is_long = 0;	/* Argument is long int.  */
       int is_char = 0;	/* Argument is promoted (unsigned) char.  */
@@ -1545,8 +1474,28 @@  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));
+#define PROCESS_ARG_INT() va_arg (ap, int)
+#define PROCESS_ARG_LONG_INT() va_arg (ap, long int)
+#define PROCESS_ARG_LONG_LONG_INT() va_arg (ap, long long int)
+#define PROCESS_ARG_POINTER() va_arg (ap, void *)
+#define PROCESS_ARG_STRING() va_arg (ap, const char *)
+#define PROCESS_ARG_UNSIGNED_INT() va_arg (ap, unsigned int)
+#define PROCESS_ARG_UNSIGNED_LONG_INT() va_arg (ap, unsigned long int)
+#define PROCESS_ARG_UNSIGNED_LONG_LONG_INT() va_arg (ap, unsigned long long int)
+#define PROCESS_ARG_WCHAR_T() va_arg (ap, wchar_t)
+#define PROCESS_ARG_WSTRING() va_arg (ap, const wchar_t *)
+	  process_arg ();
+	  process_string_arg ();
+#undef PROCESS_ARG_INT
+#undef PROCESS_ARG_LONG_INT
+#undef PROCESS_ARG_LONG_LONG_INT
+#undef PROCESS_ARG_POINTER
+#undef PROCESS_ARG_STRING
+#undef PROCESS_ARG_UNSIGNED_INT
+#undef PROCESS_ARG_UNSIGNED_LONG_INT
+#undef PROCESS_ARG_UNSIGNED_LONG_LONG_INT
+#undef PROCESS_ARG_WCHAR_T
+#undef PROCESS_ARG_WSTRING
 
 	LABEL (form_float):
 	LABEL (form_floathex):
@@ -1850,7 +1799,8 @@  printf_positional (FILE *s, const CHAR_T *format, int readonly_format,
       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_long_double __attribute__ ((unused))
+	= 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;
@@ -1933,8 +1883,30 @@  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]));
+#define PROCESS_ARG_DATA args_value[specs[nspecs_done].data_arg]
+#define PROCESS_ARG_INT() PROCESS_ARG_DATA.pa_int
+#define PROCESS_ARG_LONG_INT() PROCESS_ARG_DATA.pa_long_int
+#define PROCESS_ARG_LONG_LONG_INT() PROCESS_ARG_DATA.pa_long_long_int
+#define PROCESS_ARG_POINTER() PROCESS_ARG_DATA.pa_pointer
+#define PROCESS_ARG_STRING() PROCESS_ARG_DATA.pa_string
+#define PROCESS_ARG_UNSIGNED_INT() PROCESS_ARG_DATA.pa_u_int
+#define PROCESS_ARG_UNSIGNED_LONG_INT() PROCESS_ARG_DATA.pa_u_long_int
+#define PROCESS_ARG_UNSIGNED_LONG_LONG_INT() PROCESS_ARG_DATA.pa_u_long_long_int
+#define PROCESS_ARG_WCHAR_T() PROCESS_ARG_DATA.pa_wchar
+#define PROCESS_ARG_WSTRING() PROCESS_ARG_DATA.pa_wstring
+	  process_arg ();
+	  process_string_arg ();
+#undef PROCESS_ARG_DATA
+#undef PROCESS_ARG_INT
+#undef PROCESS_ARG_LONG_INT
+#undef PROCESS_ARG_LONG_LONG_INT
+#undef PROCESS_ARG_POINTER
+#undef PROCESS_ARG_STRING
+#undef PROCESS_ARG_UNSIGNED_INT
+#undef PROCESS_ARG_UNSIGNED_LONG_INT
+#undef PROCESS_ARG_UNSIGNED_LONG_LONG_INT
+#undef PROCESS_ARG_WCHAR_T
+#undef PROCESS_ARG_WSTRING
 
 	  LABEL (form_float):
 	  LABEL (form_floathex):