To: vim_dev@googlegroups.com Subject: Patch 7.4.2142 Fcc: outbox From: Bram Moolenaar Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ------------ Patch 7.4.2142 Problem: Leaking memory when redefining a function. Solution: Don't increment the function reference count when it's found by name. Don't remove the wrong function from the hashtab. More reference counting fixes. Files: src/structs.h, src/userfunc.c *** ../vim-7.4.2141/src/structs.h 2016-08-01 16:35:56.472496015 +0200 --- src/structs.h 2016-08-01 19:53:46.630962268 +0200 *************** *** 1327,1333 **** #endif scid_T uf_script_ID; /* ID of script where function was defined, used for s: variables */ ! int uf_refcount; /* for numbered function: reference count */ funccall_T *uf_scoped; /* l: local variables for closure */ char_u uf_name[1]; /* name of function (actually longer); can start with 123_ ( is K_SPECIAL --- 1327,1333 ---- #endif scid_T uf_script_ID; /* ID of script where function was defined, used for s: variables */ ! int uf_refcount; /* reference count, see func_name_refcount() */ funccall_T *uf_scoped; /* l: local variables for closure */ char_u uf_name[1]; /* name of function (actually longer); can start with 123_ ( is K_SPECIAL *************** *** 1365,1373 **** funccall_T *caller; /* calling function or NULL */ /* for closure */ ! int fc_refcount; int fc_copyID; /* for garbage collection */ ! garray_T fc_funcs; /* list of ufunc_T* which refer this */ }; /* --- 1365,1375 ---- funccall_T *caller; /* calling function or NULL */ /* for closure */ ! int fc_refcount; /* number of user functions that reference this ! * funccal */ int fc_copyID; /* for garbage collection */ ! garray_T fc_funcs; /* list of ufunc_T* which keep a reference to ! * "func" */ }; /* *** ../vim-7.4.2141/src/userfunc.c 2016-08-01 17:10:13.593214865 +0200 --- src/userfunc.c 2016-08-01 19:58:18.616522721 +0200 *************** *** 15,25 **** #if defined(FEAT_EVAL) || defined(PROTO) /* function flags */ ! #define FC_ABORT 1 /* abort function on error */ ! #define FC_RANGE 2 /* function accepts range */ ! #define FC_DICT 4 /* Dict function, uses "self" */ ! #define FC_CLOSURE 8 /* closure, uses outer scope variables */ ! #define FC_DELETED 16 /* :delfunction used while uf_refcount > 0 */ /* From user function to hashitem and back. */ #define UF2HIKEY(fp) ((fp)->uf_name) --- 15,26 ---- #if defined(FEAT_EVAL) || defined(PROTO) /* function flags */ ! #define FC_ABORT 0x01 /* abort function on error */ ! #define FC_RANGE 0x02 /* function accepts range */ ! #define FC_DICT 0x04 /* Dict function, uses "self" */ ! #define FC_CLOSURE 0x08 /* closure, uses outer scope variables */ ! #define FC_DELETED 0x10 /* :delfunction used while uf_refcount > 0 */ ! #define FC_REMOVED 0x20 /* function redefined while uf_refcount > 0 */ /* From user function to hashitem and back. */ #define UF2HIKEY(fp) ((fp)->uf_name) *************** *** 178,191 **** static int register_closure(ufunc_T *fp) { ! funccal_unref(fp->uf_scoped, NULL); fp->uf_scoped = current_funccal; current_funccal->fc_refcount++; if (ga_grow(¤t_funccal->fc_funcs, 1) == FAIL) return FAIL; ((ufunc_T **)current_funccal->fc_funcs.ga_data) [current_funccal->fc_funcs.ga_len++] = fp; - func_ptr_ref(current_funccal->func); return OK; } --- 179,196 ---- static int register_closure(ufunc_T *fp) { ! if (fp->uf_scoped == current_funccal) ! /* no change */ ! return OK; ! funccal_unref(fp->uf_scoped, fp); fp->uf_scoped = current_funccal; current_funccal->fc_refcount++; + func_ptr_ref(current_funccal->func); + if (ga_grow(¤t_funccal->fc_funcs, 1) == FAIL) return FAIL; ((ufunc_T **)current_funccal->fc_funcs.ga_data) [current_funccal->fc_funcs.ga_len++] = fp; return OK; } *************** *** 1024,1083 **** /* * Unreference "fc": decrement the reference count and free it when it ! * becomes zero. If "fp" is not NULL, "fp" is detached from "fc". */ static void funccal_unref(funccall_T *fc, ufunc_T *fp) { funccall_T **pfc; int i; - int freed = FALSE; if (fc == NULL) return; ! if (--fc->fc_refcount <= 0) ! { for (pfc = &previous_funccal; *pfc != NULL; pfc = &(*pfc)->caller) { if (fc == *pfc) { ! if (fc->l_varlist.lv_refcount == DO_NOT_FREE_CNT ! && fc->l_vars.dv_refcount == DO_NOT_FREE_CNT ! && fc->l_avars.dv_refcount == DO_NOT_FREE_CNT) ! { ! *pfc = fc->caller; ! free_funccal(fc, TRUE); ! freed = TRUE; ! } ! break; } } ! } ! if (!freed) { ! func_ptr_unref(fc->func); ! ! if (fp != NULL) ! for (i = 0; i < fc->fc_funcs.ga_len; ++i) ! { ! if (((ufunc_T **)(fc->fc_funcs.ga_data))[i] == fp) ! ((ufunc_T **)(fc->fc_funcs.ga_data))[i] = NULL; ! } } } /* * Remove the function from the function hashtable. If the function was * deleted while it still has references this was already done. */ ! static void func_remove(ufunc_T *fp) { hashitem_T *hi = hash_find(&func_hashtab, UF2HIKEY(fp)); if (!HASHITEM_EMPTY(hi)) hash_remove(&func_hashtab, hi); } /* --- 1029,1084 ---- /* * Unreference "fc": decrement the reference count and free it when it ! * becomes zero. "fp" is detached from "fc". */ static void funccal_unref(funccall_T *fc, ufunc_T *fp) { funccall_T **pfc; int i; if (fc == NULL) return; ! if (--fc->fc_refcount <= 0 ! && fc->l_varlist.lv_refcount == DO_NOT_FREE_CNT ! && fc->l_vars.dv_refcount == DO_NOT_FREE_CNT ! && fc->l_avars.dv_refcount == DO_NOT_FREE_CNT) for (pfc = &previous_funccal; *pfc != NULL; pfc = &(*pfc)->caller) { if (fc == *pfc) { ! *pfc = fc->caller; ! free_funccal(fc, TRUE); ! return; } } ! for (i = 0; i < fc->fc_funcs.ga_len; ++i) { ! if (((ufunc_T **)(fc->fc_funcs.ga_data))[i] == fp) ! { ! func_ptr_unref(fc->func); ! ((ufunc_T **)(fc->fc_funcs.ga_data))[i] = NULL; ! } } } /* * Remove the function from the function hashtable. If the function was * deleted while it still has references this was already done. + * Return TRUE if the entry was deleted, FALSE if it wasn't found. */ ! static int func_remove(ufunc_T *fp) { hashitem_T *hi = hash_find(&func_hashtab, UF2HIKEY(fp)); if (!HASHITEM_EMPTY(hi)) + { hash_remove(&func_hashtab, hi); + return TRUE; + } + return FALSE; } /* *************** *** 1094,1100 **** vim_free(fp->uf_tml_total); vim_free(fp->uf_tml_self); #endif ! func_remove(fp); funccal_unref(fp->uf_scoped, fp); --- 1095,1104 ---- vim_free(fp->uf_tml_total); vim_free(fp->uf_tml_self); #endif ! /* only remove it when not done already, otherwise we would remove a newer ! * version of the function */ ! if ((fp->uf_flags & (FC_DELETED | FC_REMOVED)) == 0) ! func_remove(fp); funccal_unref(fp->uf_scoped, fp); *************** *** 2158,2163 **** --- 2162,2168 ---- /* This function is referenced somewhere, don't redefine it but * create a new one. */ --fp->uf_refcount; + fp->uf_flags |= FC_REMOVED; fp = NULL; overwrite = TRUE; } *************** *** 2651,2656 **** --- 2656,2675 ---- #endif /* FEAT_CMDL_COMPL */ /* + * There are two kinds of function names: + * 1. ordinary names, function defined with :function + * 2. numbered functions and lambdas + * For the first we only count the name stored in func_hashtab as a reference, + * using function() does not count as a reference, because the function is + * looked up by name. + */ + static int + func_name_refcount(char_u *name) + { + return isdigit(*name) || *name == '<'; + } + + /* * ":delfunction {name}" */ void *************** *** 2705,2723 **** } else { ! /* Normal functions (not numbered functions and lambdas) have a * refcount of 1 for the entry in the hashtable. When deleting ! * them and the refcount is more than one, it should be kept. ! * Numbered functions and lambdas snould be kept if the refcount is * one or more. */ ! if (fp->uf_refcount > (isdigit(fp->uf_name[0]) ! || fp->uf_name[0] == '<' ? 0 : 1)) { /* Function is still referenced somewhere. Don't free it but * do remove it from the hashtable. */ ! func_remove(fp); fp->uf_flags |= FC_DELETED; - fp->uf_refcount--; } else func_free(fp); --- 2724,2741 ---- } else { ! /* A normal function (not a numbered function or lambda) has a * refcount of 1 for the entry in the hashtable. When deleting ! * it and the refcount is more than one, it should be kept. ! * A numbered function and lambda snould be kept if the refcount is * one or more. */ ! if (fp->uf_refcount > (func_name_refcount(fp->uf_name) ? 0 : 1)) { /* Function is still referenced somewhere. Don't free it but * do remove it from the hashtable. */ ! if (func_remove(fp)) ! fp->uf_refcount--; fp->uf_flags |= FC_DELETED; } else func_free(fp); *************** *** 2734,2740 **** { ufunc_T *fp = NULL; ! if (name == NULL) return; fp = find_func(name); if (fp == NULL && isdigit(*name)) --- 2752,2758 ---- { ufunc_T *fp = NULL; ! if (name == NULL || !func_name_refcount(name)) return; fp = find_func(name); if (fp == NULL && isdigit(*name)) *************** *** 2777,2783 **** { ufunc_T *fp; ! if (name == NULL) return; fp = find_func(name); if (fp != NULL) --- 2795,2801 ---- { ufunc_T *fp; ! if (name == NULL || !func_name_refcount(name)) return; fp = find_func(name); if (fp != NULL) *** ../vim-7.4.2141/src/version.c 2016-08-01 17:10:13.601214790 +0200 --- src/version.c 2016-08-01 20:44:36.543597750 +0200 *************** *** 765,766 **** --- 765,768 ---- { /* Add new patch number below this line */ + /**/ + 2142, /**/ -- (letter from Mark to Mike, about the film's probable certificate) I would like to get back to the Censor and agree to lose the shits, take the odd Jesus Christ out and lose Oh fuck off, but to retain 'fart in your general direction', 'castanets of your testicles' and 'oral sex' and ask him for an 'A' rating on that basis. "Monty Python and the Holy Grail" PYTHON (MONTY) PICTURES LTD /// Bram Moolenaar -- Bram@Moolenaar.net -- http://www.Moolenaar.net \\\ /// sponsor Vim, vote for features -- http://www.Vim.org/sponsor/ \\\ \\\ an exciting new programming language -- http://www.Zimbu.org /// \\\ help me help AIDS victims -- http://ICCF-Holland.org ///