Re: Ping. Re: [PATCH v5] Prepare vfprintf to use __printf_fp/__printf_fphex with float128 arg

[Adhemerval Zanella <adhemerval dot zanella at linaro dot org>]

On 11/06/2019 09:03, Gabriel F. T. Gomes wrote:
> Hi, Adhemerval,
> 
>>> On Tue, May 21 2019, Gabriel F. T. Gomes wrote:
>>>>
>>>> Changes since v4:
>>>>
>>>>   - Replaced macros with inline functions.
> 
> While running some final tests, I noticed that this last change is
> incorrect.  On x86_64, powerpc32, and maybe others, it will fail to
> build (details below).
> 
>>>> +static void
>>>> +parse_float_va_arg (struct printf_info *info, union printf_arg *the_arg,
>>>> +		    int is_long_double, unsigned int mode_flags,
>>>> +		    va_list *ap)
>>>> +{
>>>> +#if __HAVE_FLOAT128_UNLIKE_LDBL
>>>> +  if (is_long_double && (mode_flags & PRINTF_LDBL_USES_FLOAT128) != 0)
>>>> +    {
>>>> +      info->is_binary128 = 1;
>>>> +      the_arg->pa_float128 = va_arg (*ap, _Float128);
>>>> +    }
>>>> +  else
>>>> +#endif
>>>> +    {
>>>> +      info->is_binary128 = 0;
>>>> +      if (is_long_double)
>>>> +	the_arg->pa_long_double = va_arg (*ap, long double);
>>>> +      else
>>>> +	the_arg->pa_double = va_arg (*ap, double);
>>>> +    }
>>>> +}
>>>> +
>>>>
>>>> [...]
>>>>
>>>> -	    if (is_long_double)						      \
>>>> -	      the_arg.pa_long_double = va_arg (ap, long double);	      \
>>>> -	    else							      \
>>>> -	      the_arg.pa_double = va_arg (ap, double);			      \
>>>> +	    parse_float_va_arg (&info, &the_arg, is_long_double,	      \
>>>> +				mode_flags, &ap);			      \
> 
> On x86_64 and powerpc32, I get the following errors with this version of
> the patch:
> 
>   vfprintf-internal.c: In function ‘__vfprintf_internal’:
>   vfprintf-internal.c:811:17: error: passing argument 5 of ‘parse_float_va_arg’ from incompatible pointer type [-Werror=incompatible-pointer-types]
>        mode_flags, &ap);         \
>                    ^
>   vfprintf-internal.c:1674:4: note: in expansion of macro ‘process_arg’
>       process_arg (((struct printf_spec *) NULL));
>       ^~~~~~~~~~~
>   vfprintf-internal.c:154:1: note: expected ‘__va_list_tag (*)[1]’ but argument is of type ‘__va_list_tag **’
>    parse_float_va_arg (struct printf_info *info, union printf_arg *the_arg,
>    ^~~~~~~~~~~~~~~~~~
> 
> After I got this, I tried to use 'va_copy' to correctly pass the va_list
> (and its state) to 'parse_float_va_arg', but that doesn't work, because
> 'va_arg' might have been called an unknown number of times with 'ap'.
> 
> I'm sorry I didn't notice this before (my tests with v5 were not good
> enough), but I think we should go back to v4 (except for the typo fix)
> and use the macros that correctly call 'va_arg' on the already
> initialized 'va_list'.
> 
> Does that sound reasonable to you?

Right, I forgot this regarding va_list handling. Indeed the last version lgtm, thanks.