This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCH] DCIGETTEXT: Remove alloca support
- From: Adhemerval Zanella <adhemerval dot zanella at linaro dot org>
- To: libc-alpha at sourceware dot org
- Cc: Florian Weimer <fweimer at redhat dot com>
- Date: Wed, 21 Jun 2017 16:50:35 -0300
- Subject: Re: [PATCH] DCIGETTEXT: Remove alloca support
- Authentication-results: sourceware.org; auth=none
- References: <20170619161817.17415402AEC0E@oldenburg.str.redhat.com>
On 19/06/2017 13:18, Florian Weimer wrote:
> 2017-06-19 Florian Weimer <fweimer@redhat.com>
>
> * intl/dcigettext.c: Remove alloca support.
> (DCIGETTEXT): Use heap allocations for xdomainname, single_locale.
I think this could be another possible usage of char_array, but to safely
use it would require to slight change '_nl_find_domain' to avoid tamper
the 'locale' argument and making a copy otherwise (current approach seems
a bad idiom imho, where it allocates a buffer in some cases and redirect
to input argument). So I am not sure if it worth the trouble, the only
meaningful in this case is a slight better performance due less heap
usage for general cases.
So patch LGTM, but I can send a variant which uses char_array if you
prefer as well.
>
> diff --git a/intl/dcigettext.c b/intl/dcigettext.c
> index 0e79b1f..d778d19 100644
> --- a/intl/dcigettext.c
> +++ b/intl/dcigettext.c
> @@ -27,28 +27,6 @@
>
> #include <sys/types.h>
>
> -#ifdef __GNUC__
> -# define alloca __builtin_alloca
> -# define HAVE_ALLOCA 1
> -#else
> -# ifdef _MSC_VER
> -# include <malloc.h>
> -# define alloca _alloca
> -# else
> -# if defined HAVE_ALLOCA_H || defined _LIBC
> -# include <alloca.h>
> -# else
> -# ifdef _AIX
> - #pragma alloca
> -# else
> -# ifndef alloca
> -char *alloca ();
> -# endif
> -# endif
> -# endif
> -# endif
> -#endif
> -
> #include <errno.h>
> #ifndef errno
> extern int errno;
> @@ -374,45 +352,6 @@ static const char *get_output_charset (struct binding *domainbinding)
> #endif
>
>
> -/* For those losing systems which don't have `alloca' we have to add
> - some additional code emulating it. */
> -#ifdef HAVE_ALLOCA
> -/* Nothing has to be done. */
> -# define freea(p) /* nothing */
> -# define ADD_BLOCK(list, address) /* nothing */
> -# define FREE_BLOCKS(list) /* nothing */
> -#else
> -struct block_list
> -{
> - void *address;
> - struct block_list *next;
> -};
> -# define ADD_BLOCK(list, addr) \
> - do { \
> - struct block_list *newp = (struct block_list *) malloc (sizeof (*newp)); \
> - /* If we cannot get a free block we cannot add the new element to \
> - the list. */ \
> - if (newp != NULL) { \
> - newp->address = (addr); \
> - newp->next = (list); \
> - (list) = newp; \
> - } \
> - } while (0)
> -# define FREE_BLOCKS(list) \
> - do { \
> - while (list != NULL) { \
> - struct block_list *old = list; \
> - list = list->next; \
> - free (old->address); \
> - free (old); \
> - } \
> - } while (0)
> -# undef alloca
> -# define alloca(size) (malloc (size))
> -# define freea(p) free (p)
> -#endif /* have alloca */
> -
> -
> #ifdef _LIBC
> /* List of blocks allocated for translations. */
> typedef struct transmem_list
> @@ -488,17 +427,14 @@ DCIGETTEXT (const char *domainname, const char *msgid1, const char *msgid2,
> int plural, unsigned long int n, int category)
> #endif
> {
> -#ifndef HAVE_ALLOCA
> - struct block_list *block_list = NULL;
> -#endif
> struct loaded_l10nfile *domain;
> struct binding *binding;
> const char *categoryname;
> const char *categoryvalue;
> const char *dirname;
> char *xdirname = NULL;
> - char *xdomainname;
> - char *single_locale;
> + char *xdomainname = NULL;
> + char *single_locale = NULL;
> char *retval;
> size_t retlen;
> int saved_errno;
> @@ -507,7 +443,6 @@ DCIGETTEXT (const char *domainname, const char *msgid1, const char *msgid2,
> #if defined HAVE_PER_THREAD_LOCALE && !defined IN_LIBGLOCALE
> const char *localename;
> #endif
> - size_t domainname_len;
>
> /* If no real MSGID is given return NULL. */
> if (msgid1 == NULL)
> @@ -537,6 +472,7 @@ DCIGETTEXT (const char *domainname, const char *msgid1, const char *msgid2,
> definition left this undefined. */
> if (domainname == NULL)
> domainname = _nl_current_default_domain;
> + size_t domainname_len = strlen (domainname);
>
> /* OS/2 specific: backward compatibility with older libintl versions */
> #ifdef LC_MESSAGES_COMPAT
> @@ -652,19 +588,16 @@ DCIGETTEXT (const char *domainname, const char *msgid1, const char *msgid2,
> categoryvalue = guess_category_value (category, categoryname);
> #endif
>
> - domainname_len = strlen (domainname);
> - xdomainname = (char *) alloca (strlen (categoryname)
> - + domainname_len + 5);
> - ADD_BLOCK (block_list, xdomainname);
> -
> - stpcpy ((char *) mempcpy (stpcpy (stpcpy (xdomainname, categoryname), "/"),
> - domainname, domainname_len),
> - ".mo");
> -
> /* Creating working area. */
> - single_locale = (char *) alloca (strlen (categoryvalue) + 1);
> - ADD_BLOCK (block_list, single_locale);
> + single_locale = malloc (strlen (categoryvalue) + 1);
>
> + if (single_locale == NULL
> + || __asprintf (&xdomainname, "%s/%s.mo", categoryname, domainname) < 0)
> + {
> + free (single_locale);
> + free (xdirname);
> + return NULL;
> + }
>
> /* Search for the given string. This is a loop because we perhaps
> got an ordered list of languages to consider for the translation. */
> @@ -752,7 +685,8 @@ DCIGETTEXT (const char *domainname, const char *msgid1, const char *msgid2,
> /* Found the translation of MSGID1 in domain DOMAIN:
> starting at RETVAL, RETLEN bytes. */
> free (xdirname);
> - FREE_BLOCKS (block_list);
> + free (xdomainname);
> + free (single_locale);
> if (foundp == NULL)
> {
> /* Create a new entry and add it to the search tree. */
> @@ -836,7 +770,8 @@ DCIGETTEXT (const char *domainname, const char *msgid1, const char *msgid2,
> return_untranslated:
> /* Return the untranslated MSGID. */
> free (xdirname);
> - FREE_BLOCKS (block_list);
> + free (xdomainname);
> + free (single_locale);
> gl_rwlock_unlock (_nl_state_lock);
> #ifdef _LIBC
> __libc_rwlock_unlock (__libc_setlocale_lock);
>