Message ID | 20170619162016.3F506402AEC0E@oldenburg.str.redhat.com |
---|---|
State | New |
Headers | show |
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; > } > > + > + > +/* 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; >
* 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.
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.
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; } + + +/* 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;