From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 1335 invoked by alias); 27 Jun 2017 18:45:57 -0000 Mailing-List: contact libc-alpha-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: libc-alpha-owner@sourceware.org Received: (qmail 1321 invoked by uid 89); 27 Jun 2017 18:45:56 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-26.4 required=5.0 tests=BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,RCVD_IN_DNSWL_NONE,RCVD_IN_SORBS_SPAM,SPF_PASS autolearn=ham version=3.3.2 spammy= X-HELO: mail-qk0-f175.google.com X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=amoD22b0IaocDGHgDQ5zA3zBZ2jlbzUNvmvzP+oCDe8=; b=tkH201+XM2ve2dBwncUhe9Ssh4vy9BP6YaVTe/YRa8FxwnAwLL9QKgXtuB7b6wSrCX Un6yQh2/cOLcQqyDO79aXzbythns/MpvK64e5OtArMsJ/W6dag4eF4ovWBSPeCEb1UXY U1J7VL4F3WzQI7UbvMcSQUUMmAUQp6prpM25zYYUWYchz74sfXyzslTLW8JrQnlV4un5 9DazQAD5ECCeYh6yOmItcswJVcOOFzWJYc1jCaYjUa0MnQe1bane9ilO6e8e94yTAjNi OO/7k/n4tocFm7JNJZ38r2sKF5fVYCrWwF8puJwfmNtT+aABEPelK6owh0XjRjlGDZ8K 0RuQ== X-Gm-Message-State: AKS2vOx2EVqeq+CD6KQW16qEGT3tqXYiFB4+vTfG/1Qes9kRStJZimRk p2fv7qvf1qPg7uuTige7pg== X-Received: by 10.55.20.36 with SMTP id e36mr8407431qkh.1.1498589153132; Tue, 27 Jun 2017 11:45:53 -0700 (PDT) Subject: Re: [PATCH] vfprintf: Allocate the user argument buffer on the heap To: libc-alpha@sourceware.org References: <20170619162016.3F506402AEC0E@oldenburg.str.redhat.com> From: Adhemerval Zanella Message-ID: <2be1d31c-cd05-71fe-5ab6-11e5371c1c09@linaro.org> Date: Tue, 27 Jun 2017 18:45:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.1.1 MIME-Version: 1.0 In-Reply-To: <20170619162016.3F506402AEC0E@oldenburg.str.redhat.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit X-SW-Source: 2017-06/txt/msg01424.txt.bz2 On 19/06/2017 13:20, Florian Weimer wrote: > 2017-06-19 Florian Weimer > > * 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; >