Re: [PATCH 1/3] posix: Remove alloca usage on regex set_regs

[Paul Eggert]

On 1/6/21 10:17 AM, Adhemerval Zanella wrote:
> It replaces the regmatch_t with a dynarray list.

regexec.c is shared with Gnulib, so some work needed to be done on the 
Gnulib side for this patch since Gnulib didn't have dynarray. Dynarray 
is something I've been meaning to add to Gnulib for some time, so I did 
that by installing the first attached patch into Gnulib. Could you 
please propagate the new Gnulib dynarray sources into glibc so that they 
stay in sync? As near as I can make out, the glibc dynarray files can 
now be identical to the new Gnulib files; if not, please let me know.


>   posix/regexec.c | 62 ++++++++++++++++++++++++-------------------------
>   1 file changed, 31 insertions(+), 31 deletions(-)
> ...
> @@ -1355,6 +1352,16 @@ pop_fail_stack (struct re_fail_stack_t *fs, Idx *pidx, 
> Idx nregs,
>     return fs->stack[num].node;
>   }
>   
> +
> +#define DYNARRAY_STRUCT  regmatch_list
> +#define DYNARRAY_ELEMENT regmatch_t
> +#define DYNARRAY_PREFIX  regmatch_list_
> +#include <malloc/dynarray-skeleton.c>
> +
> +static void update_regs (const re_dfa_t *dfa, regmatch_t *pmatch,
> +                        struct regmatch_list *prev_idx_match, Idx cur_node,
> +                        Idx cur_idx, Idx nmatch);
> +
>   /* Set the positions where the subexpressions are starts/ends to registers
>      PMATCH.
>      Note: We assume that pmatch[0] is already set, and
> @@ -1370,8 +1377,8 @@ set_regs (const regex_t *preg, const re_match_context_t 
> *mctx, size_t nmatch,
>     re_node_set eps_via_nodes;
>     struct re_fail_stack_t *fs;
>     struct re_fail_stack_t fs_body = { 0, 2, NULL };
> -  regmatch_t *prev_idx_match;
> -  bool prev_idx_match_malloced = false;
> +  struct regmatch_list prev_idx_match;
> +  regmatch_list_init (&prev_idx_match);
>   
>     DEBUG_ASSERT (nmatch > 1);
>     DEBUG_ASSERT (mctx->state_log != NULL);
> @@ -1388,23 +1395,18 @@ set_regs (const regex_t *preg, const re_match_context_t 
> *mctx, size_t nmatch,
>     cur_node = dfa->init_node;
>     re_node_set_init_empty (&eps_via_nodes);
>   
> -  if (__libc_use_alloca (nmatch * sizeof (regmatch_t)))
> -    prev_idx_match = (regmatch_t *) alloca (nmatch * sizeof (regmatch_t));
> -  else
> +  if (!regmatch_list_resize (&prev_idx_match, nmatch))
>       {
> -      prev_idx_match = re_malloc (regmatch_t, nmatch);
> -      if (prev_idx_match == NULL)
> -       {
> -         free_fail_stack_return (fs);
> -         return REG_ESPACE;
> -       }
> -      prev_idx_match_malloced = true;
> +      regmatch_list_free (&prev_idx_match);
> +      free_fail_stack_return (fs);
> +      return REG_ESPACE;
>       }

These three hunks are good, but you can omit most of the other hunks 
(and improve performance a bit) by inserting the following line after 
the 3rd hunk:

+  regmatch_t *prev_idx_match = regmatch_list_begin (&prev_match);

since the dynarray doesn't grow after that and this means you don't need 
to change the rest of the code to use prev_match rather than 
prev_idx_match. The only other hunks you need to retain are the ones 
replacing re_free with regmastch_list_free.

I've made this improvement to Gnulib by installing the second attached 
patch, so you should be able to copy Gnulib regexec.c to glibc without 
changing it.

0001-dynarray-new-module.patch
Description: Text Data

0002-regex-remove-alloca-usage-on-regex-set_regs.patch
Description: Text Data