[01/14] Prepare vfprintf to use __printf_fp/__printf_fphex with float128 arg
Commit Message
On Tue, 18 Dec 2018, Florian Weimer wrote:
>* Gabriel F. T. Gomes:
>>
>> @@ -1887,7 +1930,12 @@ printf_positional (FILE *s, const CHAR_T *format, int readonly_format,
>> (args_value[cnt].pa_user, ap_savep);
>> }
>> else
>> - args_value[cnt].pa_long_double = 0.0;
>> + {
>> + args_value[cnt].pa_long_double = 0.0;
>> +#if __HAVE_FLOAT128_UNLIKE_LDBL
>> + args_value[cnt].pa_float128 = 0;
>> +#endif
>> + }
>
>This bit doesn't look right to me because args_value[cnt] is a union.
>You need to assign to the right member, or perhaps zero-initialize using
>memset.
Hmm, the original code doesn't seem to be dealing with anything particular
to the long double type. It seems that assigning to .pa_long_double,
rather than to other members, was an arbitrary decision. Do you know
why .pa_long_double was chosen?
So, if this code is just zero-initializing the memory, do you think I
should zero-initialize the whole of args_value/argsbuf.data when it gets
expanded (see below)? Then I could remove the else block entirely.
Like this:
Comments
* Gabriel F. T. Gomes:
> On Tue, 18 Dec 2018, Florian Weimer wrote:
>
>>* Gabriel F. T. Gomes:
>>>
>>> @@ -1887,7 +1930,12 @@ printf_positional (FILE *s, const CHAR_T *format, int readonly_format,
>>> (args_value[cnt].pa_user, ap_savep);
>>> }
>>> else
>>> - args_value[cnt].pa_long_double = 0.0;
>>> + {
>>> + args_value[cnt].pa_long_double = 0.0;
>>> +#if __HAVE_FLOAT128_UNLIKE_LDBL
>>> + args_value[cnt].pa_float128 = 0;
>>> +#endif
>>> + }
>>
>>This bit doesn't look right to me because args_value[cnt] is a union.
>>You need to assign to the right member, or perhaps zero-initialize using
>>memset.
>
> Hmm, the original code doesn't seem to be dealing with anything particular
> to the long double type. It seems that assigning to .pa_long_double,
> rather than to other members, was an arbitrary decision. Do you know
> why .pa_long_double was chosen?
Not really. My guess it was expected that it would be the largest
member.
> So, if this code is just zero-initializing the memory, do you think I
> should zero-initialize the whole of args_value/argsbuf.data when it gets
> expanded (see below)? Then I could remove the else block entirely.
>
> Like this:
>
> diff --git a/stdio-common/vfprintf-internal.c b/stdio-common/vfprintf-internal.c
> index 61769e0ce1..17f1bae796 100644
> --- a/stdio-common/vfprintf-internal.c
> +++ b/stdio-common/vfprintf-internal.c
> @@ -1789,10 +1789,11 @@ printf_positional (FILE *s, const CHAR_T *format, int readonly_format,
> if (!scratch_buffer_set_array_size (&argsbuf, nargs, bytes_per_arg))
> {
> done = -1;
> goto all_done;
> }
> + memset (argsbuf.data, 0, argsbuf.size);
> args_value = argsbuf.data;
> /* Set up the remaining two arrays to each point past the end of
> the prior array, since space for all three has been allocated
> now. */
> args_size = &args_value[nargs].pa_int;
> @@ -1884,12 +1885,10 @@ printf_positional (FILE *s, const CHAR_T *format, int readonly_format,
> {
> args_value[cnt].pa_user = alloca (args_size[cnt]);
> (*__printf_va_arg_table[args_type[cnt] - PA_LAST])
> (args_value[cnt].pa_user, ap_savep);
> }
> - else
> - args_value[cnt].pa_long_double = 0.0;
I would haved expected
memset (&args_value[cnt], 0, sizeof (args_value[cnt]));
under thelse branch.
Thanks,
Florian
@@ -1789,10 +1789,11 @@ printf_positional (FILE *s, const CHAR_T *format, int readonly_format,
if (!scratch_buffer_set_array_size (&argsbuf, nargs, bytes_per_arg))
{
done = -1;
goto all_done;
}
+ memset (argsbuf.data, 0, argsbuf.size);
args_value = argsbuf.data;
/* Set up the remaining two arrays to each point past the end of
the prior array, since space for all three has been allocated
now. */
args_size = &args_value[nargs].pa_int;
@@ -1884,12 +1885,10 @@ printf_positional (FILE *s, const CHAR_T *format, int readonly_format,
{
args_value[cnt].pa_user = alloca (args_size[cnt]);
(*__printf_va_arg_table[args_type[cnt] - PA_LAST])
(args_value[cnt].pa_user, ap_savep);
}
- else
- args_value[cnt].pa_long_double = 0.0;
break;
case -1:
/* Error case. Not all parameters appear in N$ format
strings. We have no way to determine their type. */
assert ((mode_flags & PRINTF_FORTIFY) != 0);