| Message ID | 201712080916.vB89GxEG005501@skeeve.com |
|---|---|
| State | New |
| Headers | show |
| Series | None | expand |
On 12/08/2017 01:16 AM, Arnold Robbins wrote: > This patch changes several calls to malloc/free into re_malloc/re_free, > bringing consistency to the code. Thanks, that patch makes sense, but it misses three opportunities to bring consistency. regcomp.c has one call each to malloc and free, which should be consistent too. Also, regexec.c has a call to realloc that should be be changed to re_realloc. A minor formatting issue: one newly-introduced re_malloc call doesn't need to appear on the next line. (Possibly we should be adding consistency in the opposite way, by removing the macros re_free, re_malloc, and re_realloc, and simply using the underlying C functions. These macros are tricky since they are function-like but (aside from re_free) cannot be implemented as functions, and they don't buy much. But that'd be a bigger change.) I installed the attached patch into Gnulib; it contains the originally proposed patch 06/17 along with the abovementioned fixups. Something like this should be easily installable into glibc. From ce5d72b85e472ab8c6322b5960b11608d80ad360 Mon Sep 17 00:00:00 2001 From: Paul Eggert <eggert@cs.ucla.edu> Date: Tue, 19 Dec 2017 15:53:47 -0800 Subject: [PATCH] regex: use re_malloc etc. consistently Problem and original patch reported by Arnold Robbins in: https://sourceware.org/ml/libc-alpha/2017-12/msg00241.html * lib/regcomp.c (re_comp): * lib/regexec.c (push_fail_stack, build_trtable, match_ctx_clean): Use re_malloc/re_realloc/re_free instead of malloc/realloc/free. --- ChangeLog | 9 +++++++++ lib/regcomp.c | 4 ++-- lib/regexec.c | 19 +++++++++---------- 3 files changed, 20 insertions(+), 12 deletions(-) diff --git a/ChangeLog b/ChangeLog index 2c838329f..7d7b10aa2 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,12 @@ +2017-12-19 Paul Eggert <eggert@cs.ucla.edu> + + regex: use re_malloc etc. consistently + Problem and original patch reported by Arnold Robbins in: + https://sourceware.org/ml/libc-alpha/2017-12/msg00241.html + * lib/regcomp.c (re_comp): + * lib/regexec.c (push_fail_stack, build_trtable, match_ctx_clean): + Use re_malloc/re_realloc/re_free instead of malloc/realloc/free. + 2017-12-15 Tim Rühsen <tim.ruehsen@gmx.de> Paul Eggert <eggert@cs.ucla.edu> diff --git a/lib/regcomp.c b/lib/regcomp.c index 88539ad3e..b4bc37353 100644 --- a/lib/regcomp.c +++ b/lib/regcomp.c @@ -701,7 +701,7 @@ re_comp (const char *s) if (re_comp_buf.fastmap == NULL) { - re_comp_buf.fastmap = (char *) malloc (SBC_MAX); + re_comp_buf.fastmap = re_malloc (char, SBC_MAX); if (re_comp_buf.fastmap == NULL) return (char *) gettext (__re_error_msgid + __re_error_msgid_idx[(int) REG_ESPACE]); @@ -1197,7 +1197,7 @@ analyze (regex_t *preg) break; if (i == preg->re_nsub) { - free (dfa->subexp_map); + re_free (dfa->subexp_map); dfa->subexp_map = NULL; } } diff --git a/lib/regexec.c b/lib/regexec.c index 262384cfc..5d7ba2a33 100644 --- a/lib/regexec.c +++ b/lib/regexec.c @@ -1334,8 +1334,8 @@ push_fail_stack (struct re_fail_stack_t *fs, Idx str_idx, Idx dest_node, if (fs->num == fs->alloc) { struct re_fail_stack_ent_t *new_array; - new_array = realloc (fs->stack, (sizeof (struct re_fail_stack_ent_t) - * fs->alloc * 2)); + new_array = re_realloc (fs->stack, struct re_fail_stack_ent_t, + fs->alloc * 2); if (new_array == NULL) return REG_ESPACE; fs->alloc *= 2; @@ -3319,7 +3319,7 @@ build_trtable (const re_dfa_t *dfa, re_dfastate_t *state) if (BE (ndests <= 0, 0)) { if (dests_node_malloced) - free (dests_alloc); + re_free (dests_alloc); /* Return false in case of an error, true otherwise. */ if (ndests == 0) { @@ -3349,18 +3349,17 @@ build_trtable (const re_dfa_t *dfa, re_dfastate_t *state) alloca (ndests * 3 * sizeof (re_dfastate_t *)); else { - dest_states = (re_dfastate_t **) - malloc (ndests * 3 * sizeof (re_dfastate_t *)); + dest_states = re_malloc (re_dfastate_t *, ndests * 3); if (BE (dest_states == NULL, 0)) { out_free: if (dest_states_malloced) - free (dest_states); + re_free (dest_states); re_node_set_free (&follows); for (i = 0; i < ndests; ++i) re_node_set_free (dests_node + i); if (dests_node_malloced) - free (dests_alloc); + re_free (dests_alloc); return false; } dest_states_malloced = true; @@ -3491,14 +3490,14 @@ out_free: } if (dest_states_malloced) - free (dest_states); + re_free (dest_states); re_node_set_free (&follows); for (i = 0; i < ndests; ++i) re_node_set_free (dests_node + i); if (dests_node_malloced) - free (dests_alloc); + re_free (dests_alloc); return true; } @@ -4166,7 +4165,7 @@ match_ctx_clean (re_match_context_t *mctx) re_free (top->path->array); re_free (top->path); } - free (top); + re_free (top); } mctx->nsub_tops = 0;
Thanks, I have merged this in to gawk's version. Paul --- I think that you have permission to push the patches you approve to glibc. Please do so. Thanks, Arnold Paul Eggert <eggert@cs.ucla.edu> wrote: > On 12/08/2017 01:16 AM, Arnold Robbins wrote: > > This patch changes several calls to malloc/free into re_malloc/re_free, > > bringing consistency to the code. > > Thanks, that patch makes sense, but it misses three opportunities to > bring consistency. regcomp.c has one call each to malloc and free, which > should be consistent too. Also, regexec.c has a call to realloc that > should be be changed to re_realloc. A minor formatting issue: one > newly-introduced re_malloc call doesn't need to appear on the next line. > > (Possibly we should be adding consistency in the opposite way, by > removing the macros re_free, re_malloc, and re_realloc, and simply using > the underlying C functions. These macros are tricky since they are > function-like but (aside from re_free) cannot be implemented as > functions, and they don't buy much. But that'd be a bigger change.) > > I installed the attached patch into Gnulib; it contains the originally > proposed patch 06/17 along with the abovementioned fixups. Something > like this should be easily installable into glibc. >
arnold@skeeve.com wrote: > Paul --- I think that you have permission to push the patches you approve > to glibc. Please do so. OK, I just now did that for patch 04/17, using a variant of Eric Blake's original commit message for Gnulib (see attached). Carlos already installed patch 01/17. As discussed, 02, 03, 05, 06, and 07 need thought before they can go in, and the other patches I haven't reviewed yet. I'd like to finish reviewing them all before installing patches that need thought. From bff2701c1234446e4e873bee8894f6650a9dc3b3 Mon Sep 17 00:00:00 2001 From: Eric Blake <ebb9@byu.net> Date: Fri, 22 Dec 2017 07:57:25 -0800 Subject: [PATCH] Avoid gcc warnings on cygwin * posix/regex_internal.c (re_string_reconstruct) [!RE_ENABLE_I18N]: * posix/regexec.c (check_arrival_add_next_nodes) [!RE_ENABLE_I18N]: Avoid unused variable. --- ChangeLog | 7 +++++++ posix/regex_internal.c | 2 +- posix/regexec.c | 2 ++ 3 files changed, 10 insertions(+), 1 deletion(-) diff --git a/ChangeLog b/ChangeLog index 14950fe373..b296ec0ad6 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,10 @@ +2017-12-22 Eric Blake <ebb9@byu.net> + + Avoid gcc warnings on cygwin + * posix/regex_internal.c (re_string_reconstruct) [!RE_ENABLE_I18N]: + * posix/regexec.c (check_arrival_add_next_nodes) [!RE_ENABLE_I18N]: + Avoid unused variable. + 2017-12-22 Florian Weimer <fweimer@redhat.com> * io/Makefile (routines): Add copy_file_range. diff --git a/posix/regex_internal.c b/posix/regex_internal.c index a97f703bc7..705d6a4c73 100644 --- a/posix/regex_internal.c +++ b/posix/regex_internal.c @@ -681,10 +681,10 @@ re_string_reconstruct (re_string_t *pstr, int idx, int eflags) } else { +#ifdef RE_ENABLE_I18N /* No, skip all characters until IDX. */ int prev_valid_len = pstr->valid_len; -#ifdef RE_ENABLE_I18N if (BE (pstr->offsets_needed, 0)) { pstr->len = pstr->raw_len - idx + offset; diff --git a/posix/regexec.c b/posix/regexec.c index 95e31d3f80..4cf1eeafc5 100644 --- a/posix/regexec.c +++ b/posix/regexec.c @@ -2988,7 +2988,9 @@ check_arrival_add_next_nodes (re_match_context_t *mctx, int str_idx, const re_dfa_t *const dfa = mctx->dfa; int result; int cur_idx; +#ifdef RE_ENABLE_I18N reg_errcode_t err = REG_NOERROR; +#endif re_node_set union_set; re_node_set_init_empty (&union_set); for (cur_idx = 0; cur_idx < cur_nodes->nelem; ++cur_idx)
diff --git a/posix/regexec.c b/posix/regexec.c index a0d9736..2d2bc46 100644 --- a/posix/regexec.c +++ b/posix/regexec.c @@ -3296,7 +3296,7 @@ build_trtable (const re_dfa_t *dfa, re_dfastate_t *state) if (BE (ndests <= 0, 0)) { if (dests_node_malloced) - free (dests_alloc); + re_free (dests_alloc); /* Return 0 in case of an error, 1 otherwise. */ if (ndests == 0) { @@ -3328,18 +3328,18 @@ build_trtable (const re_dfa_t *dfa, re_dfastate_t *state) else #endif { - dest_states = (re_dfastate_t **) - malloc (ndests * 3 * sizeof (re_dfastate_t *)); + dest_states = + re_malloc (re_dfastate_t *, ndests * 3); if (BE (dest_states == NULL, 0)) { out_free: if (dest_states_malloced) - free (dest_states); + re_free (dest_states); re_node_set_free (&follows); for (i = 0; i < ndests; ++i) re_node_set_free (dests_node + i); if (dests_node_malloced) - free (dests_alloc); + re_free (dests_alloc); return 0; } dest_states_malloced = true; @@ -3470,14 +3470,14 @@ out_free: } if (dest_states_malloced) - free (dest_states); + re_free (dest_states); re_node_set_free (&follows); for (i = 0; i < ndests; ++i) re_node_set_free (dests_node + i); if (dests_node_malloced) - free (dests_alloc); + re_free (dests_alloc); return 1; } @@ -4141,7 +4141,7 @@ match_ctx_clean (re_match_context_t *mctx) re_free (top->path->array); re_free (top->path); } - free (top); + re_free (top); } mctx->nsub_tops = 0;