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

[Adhemerval Zanella]

On 08/01/2021 17:14, Paul Eggert wrote:
> 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.

I will check and sync the differences.

> 
> 
>>   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.

Ok, I will check and sync with gnulib.