* [PATCH] vfprintf: Allocate the user argument buffer on the heap
@ 2017-06-19 16:20 Florian Weimer
2017-06-27 18:45 ` Adhemerval Zanella
0 siblings, 1 reply; 4+ messages in thread
From: Florian Weimer @ 2017-06-19 16:20 UTC (permalink / raw)
To: libc-alpha
2017-06-19 Florian Weimer <fweimer@redhat.com>
* stdio-common/vfprintf.c (allocate_user_args_buffer): New
function.
(printf_positional): Call it.
diff --git a/stdio-common/vfprintf.c b/stdio-common/vfprintf.c
index e0c6edf..b15c5d0 100644
--- a/stdio-common/vfprintf.c
+++ b/stdio-common/vfprintf.c
@@ -1618,6 +1618,26 @@ do_positional:
return done;
}
\f
+
+
+/* Called from printf_positional to determine the size of the user
+ argument area and allocate it, after discovering that one is
+ needed. This function returns NULL on allocation failure. */
+static void *
+allocate_user_args_buffer (size_t nargs, const int *args_size,
+ const int *args_type)
+{
+ assert (__printf_va_arg_table != NULL);
+ size_t size = 0;
+ for (size_t i = 0; i < nargs; ++i)
+ if ((args_type[i] & PA_FLAG_MASK) == 0
+ && args_type[i] >= PA_LAST
+ && __printf_va_arg_table[args_type[i] - PA_LAST] != NULL)
+ size += roundup (args_size[i], _Alignof (max_align_t));
+ assert (size > 0);
+ return malloc (size);
+}
+
static int
printf_positional (_IO_FILE *s, const CHAR_T *format, int readonly_format,
va_list ap, va_list *ap_savep, int done, int nspecs_done,
@@ -1636,6 +1656,12 @@ printf_positional (_IO_FILE *s, const CHAR_T *format, int readonly_format,
struct scratch_buffer argsbuf;
scratch_buffer_init (&argsbuf);
+ /* Allocation area for user argument data. Lazily allocated by
+ allocate_user_args_buffer. */
+ void *user_args_buffer = NULL;
+ /* Upcoming allocation within user_args_buffer. */
+ void *user_args_buffer_next = NULL;
+
/* Array with information about the needed arguments. This has to
be dynamically extensible. */
size_t nspecs = 0;
@@ -1796,7 +1822,34 @@ printf_positional (_IO_FILE *s, const CHAR_T *format, int readonly_format,
else if (__glibc_unlikely (__printf_va_arg_table != NULL)
&& __printf_va_arg_table[args_type[cnt] - PA_LAST] != NULL)
{
- args_value[cnt].pa_user = alloca (args_size[cnt]);
+ /* Allocate from user_args_buffer. */
+ size_t allocation_size = args_size[cnt];
+ void *allocation;
+ if (allocation_size == 0)
+ /* Nothing to allocate. */
+ allocation = NULL;
+ else
+ {
+ if (user_args_buffer == NULL)
+ {
+ /* First user argument. Allocate the complete
+ buffer. */
+ user_args_buffer = allocate_user_args_buffer
+ (nargs, args_size, args_type);
+ if (user_args_buffer == NULL)
+ {
+ done = -1;
+ goto all_done;
+ }
+ user_args_buffer_next = user_args_buffer;
+ }
+ allocation = user_args_buffer_next;
+ user_args_buffer_next
+ += roundup (allocation_size, _Alignof (max_align_t));
+ }
+ /* Install the allocated pointer and use the callback to
+ extract the argument. */
+ args_value[cnt].pa_user = allocation;
(*__printf_va_arg_table[args_type[cnt] - PA_LAST])
(args_value[cnt].pa_user, ap_savep);
}
@@ -1953,6 +2006,7 @@ printf_positional (_IO_FILE *s, const CHAR_T *format, int readonly_format,
- specs[nspecs_done].end_of_fmt);
}
all_done:
+ free (user_args_buffer);
scratch_buffer_free (&argsbuf);
scratch_buffer_free (&specsbuf);
return done;
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] vfprintf: Allocate the user argument buffer on the heap
2017-06-19 16:20 [PATCH] vfprintf: Allocate the user argument buffer on the heap Florian Weimer
@ 2017-06-27 18:45 ` Adhemerval Zanella
2017-06-27 19:13 ` Florian Weimer
0 siblings, 1 reply; 4+ messages in thread
From: Adhemerval Zanella @ 2017-06-27 18:45 UTC (permalink / raw)
To: libc-alpha
On 19/06/2017 13:20, Florian Weimer wrote:
> 2017-06-19 Florian Weimer <fweimer@redhat.com>
>
> * stdio-common/vfprintf.c (allocate_user_args_buffer): New
> function.
> (printf_positional): Call it.
>
> diff --git a/stdio-common/vfprintf.c b/stdio-common/vfprintf.c
> index e0c6edf..b15c5d0 100644
> --- a/stdio-common/vfprintf.c
> +++ b/stdio-common/vfprintf.c
> @@ -1618,6 +1618,26 @@ do_positional:
> return done;
> }
> \f
> +
> +
> +/* Called from printf_positional to determine the size of the user
> + argument area and allocate it, after discovering that one is
> + needed. This function returns NULL on allocation failure. */
> +static void *
> +allocate_user_args_buffer (size_t nargs, const int *args_size,
> + const int *args_type)
> +{
> + assert (__printf_va_arg_table != NULL);
> + size_t size = 0;
> + for (size_t i = 0; i < nargs; ++i)
> + if ((args_type[i] & PA_FLAG_MASK) == 0
> + && args_type[i] >= PA_LAST
> + && __printf_va_arg_table[args_type[i] - PA_LAST] != NULL)
> + size += roundup (args_size[i], _Alignof (max_align_t));
> + assert (size > 0);
> + return malloc (size);
> +}
> +
> static int
> printf_positional (_IO_FILE *s, const CHAR_T *format, int readonly_format,
> va_list ap, va_list *ap_savep, int done, int nspecs_done,
> @@ -1636,6 +1656,12 @@ printf_positional (_IO_FILE *s, const CHAR_T *format, int readonly_format,
> struct scratch_buffer argsbuf;
> scratch_buffer_init (&argsbuf);
>
> + /* Allocation area for user argument data. Lazily allocated by
> + allocate_user_args_buffer. */
> + void *user_args_buffer = NULL;
> + /* Upcoming allocation within user_args_buffer. */
> + void *user_args_buffer_next = NULL;
> +
> /* Array with information about the needed arguments. This has to
> be dynamically extensible. */
> size_t nspecs = 0;
> @@ -1796,7 +1822,34 @@ printf_positional (_IO_FILE *s, const CHAR_T *format, int readonly_format,
> else if (__glibc_unlikely (__printf_va_arg_table != NULL)
> && __printf_va_arg_table[args_type[cnt] - PA_LAST] != NULL)
> {
> - args_value[cnt].pa_user = alloca (args_size[cnt]);
> + /* Allocate from user_args_buffer. */
> + size_t allocation_size = args_size[cnt];
> + void *allocation;
> + if (allocation_size == 0)
> + /* Nothing to allocate. */
> + allocation = NULL;
> + else
> + {
> + if (user_args_buffer == NULL)
> + {
> + /* First user argument. Allocate the complete
> + buffer. */
> + user_args_buffer = allocate_user_args_buffer
> + (nargs, args_size, args_type);
> + if (user_args_buffer == NULL)
> + {
> + done = -1;
> + goto all_done;
> + }
> + user_args_buffer_next = user_args_buffer;
> + }
> + allocation = user_args_buffer_next;
> + user_args_buffer_next
> + += roundup (allocation_size, _Alignof (max_align_t));
> + }
> + /* Install the allocated pointer and use the callback to
> + extract the argument. */
> + args_value[cnt].pa_user = allocation;
> (*__printf_va_arg_table[args_type[cnt] - PA_LAST])
> (args_value[cnt].pa_user, ap_savep);
I am trying to convince myself it is worth to add all this complexity
to allocate for user defined types, but I am failing to understand why
can we just simplify it to a malloc using 'args_size[cnt]' (as the alloca
is already using it). And why do we need to keep track of the buffer
allocation after the callback track? Could we just free it after the
call?
> }
> @@ -1953,6 +2006,7 @@ printf_positional (_IO_FILE *s, const CHAR_T *format, int readonly_format,
> - specs[nspecs_done].end_of_fmt);
> }
> all_done:
> + free (user_args_buffer);
> scratch_buffer_free (&argsbuf);
> scratch_buffer_free (&specsbuf);
> return done;
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] vfprintf: Allocate the user argument buffer on the heap
2017-06-27 18:45 ` Adhemerval Zanella
@ 2017-06-27 19:13 ` Florian Weimer
2017-06-27 20:04 ` Adhemerval Zanella
0 siblings, 1 reply; 4+ messages in thread
From: Florian Weimer @ 2017-06-27 19:13 UTC (permalink / raw)
To: Adhemerval Zanella; +Cc: libc-alpha
* Adhemerval Zanella:
>> - args_value[cnt].pa_user = alloca (args_size[cnt]);
>> + /* Allocate from user_args_buffer. */
>> + size_t allocation_size = args_size[cnt];
>> + void *allocation;
>> + if (allocation_size == 0)
>> + /* Nothing to allocate. */
>> + allocation = NULL;
>> + else
>> + {
>> + if (user_args_buffer == NULL)
>> + {
>> + /* First user argument. Allocate the complete
>> + buffer. */
>> + user_args_buffer = allocate_user_args_buffer
>> + (nargs, args_size, args_type);
>> + if (user_args_buffer == NULL)
>> + {
>> + done = -1;
>> + goto all_done;
>> + }
>> + user_args_buffer_next = user_args_buffer;
>> + }
>> + allocation = user_args_buffer_next;
>> + user_args_buffer_next
>> + += roundup (allocation_size, _Alignof (max_align_t));
>> + }
>> + /* Install the allocated pointer and use the callback to
>> + extract the argument. */
>> + args_value[cnt].pa_user = allocation;
>> (*__printf_va_arg_table[args_type[cnt] - PA_LAST])
>> (args_value[cnt].pa_user, ap_savep);
>
> I am trying to convince myself it is worth to add all this complexity
> to allocate for user defined types, but I am failing to understand why
> can we just simplify it to a malloc using 'args_size[cnt]' (as the alloca
> is already using it). And why do we need to keep track of the buffer
> allocation after the callback track? Could we just free it after the
> call?
We need to delay the deallocation until the string has been formatted
because the data is later passed to the formatting function.
We could use separate malloc allocations and a second pass through the
argument array to free the user allocations (if any). This might be
simpler, but I would have to write it down to be certain.
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] vfprintf: Allocate the user argument buffer on the heap
2017-06-27 19:13 ` Florian Weimer
@ 2017-06-27 20:04 ` Adhemerval Zanella
0 siblings, 0 replies; 4+ messages in thread
From: Adhemerval Zanella @ 2017-06-27 20:04 UTC (permalink / raw)
To: Florian Weimer; +Cc: libc-alpha
On 27/06/2017 16:13, Florian Weimer wrote:
> * Adhemerval Zanella:
>
>>> - args_value[cnt].pa_user = alloca (args_size[cnt]);
>>> + /* Allocate from user_args_buffer. */
>>> + size_t allocation_size = args_size[cnt];
>>> + void *allocation;
>>> + if (allocation_size == 0)
>>> + /* Nothing to allocate. */
>>> + allocation = NULL;
>>> + else
>>> + {
>>> + if (user_args_buffer == NULL)
>>> + {
>>> + /* First user argument. Allocate the complete
>>> + buffer. */
>>> + user_args_buffer = allocate_user_args_buffer
>>> + (nargs, args_size, args_type);
>>> + if (user_args_buffer == NULL)
>>> + {
>>> + done = -1;
>>> + goto all_done;
>>> + }
>>> + user_args_buffer_next = user_args_buffer;
>>> + }
>>> + allocation = user_args_buffer_next;
>>> + user_args_buffer_next
>>> + += roundup (allocation_size, _Alignof (max_align_t));
>>> + }
>>> + /* Install the allocated pointer and use the callback to
>>> + extract the argument. */
>>> + args_value[cnt].pa_user = allocation;
>>> (*__printf_va_arg_table[args_type[cnt] - PA_LAST])
>>> (args_value[cnt].pa_user, ap_savep);
>>
>> I am trying to convince myself it is worth to add all this complexity
>> to allocate for user defined types, but I am failing to understand why
>> can we just simplify it to a malloc using 'args_size[cnt]' (as the alloca
>> is already using it). And why do we need to keep track of the buffer
>> allocation after the callback track? Could we just free it after the
>> call?
>
> We need to delay the deallocation until the string has been formatted
> because the data is later passed to the formatting function.
Ack.
>
> We could use separate malloc allocations and a second pass through the
> argument array to free the user allocations (if any). This might be
> simpler, but I would have to write it down to be certain.
If you could simplify it as cost of a slight worse performance/memory
utilization for this specific code path (user provided hooks) I think
it would be better. This code is already somewhat complex and convoluted.
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2017-06-27 20:04 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-06-19 16:20 [PATCH] vfprintf: Allocate the user argument buffer on the heap Florian Weimer
2017-06-27 18:45 ` Adhemerval Zanella
2017-06-27 19:13 ` Florian Weimer
2017-06-27 20:04 ` Adhemerval Zanella
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).