To: vim_dev@googlegroups.com Subject: Patch 7.4.2143 Fcc: outbox From: Bram Moolenaar Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ------------ Patch 7.4.2143 Problem: A funccal is garbage collected while it can still be used. Solution: Set copyID in all referenced functions. Do not list lambda functions with ":function". Files: src/userfunc.c, src/proto/userfunc.pro, src/eval.c, src/testdir/test_lambda.vim *** ../vim-7.4.2142/src/userfunc.c 2016-08-01 20:46:20.610667730 +0200 --- src/userfunc.c 2016-08-01 22:38:13.459038004 +0200 *************** *** 65,71 **** # endif prof_self_cmp(const void *s1, const void *s2); #endif ! static void funccal_unref(funccall_T *fc, ufunc_T *fp); void func_init() --- 65,71 ---- # endif prof_self_cmp(const void *s1, const void *s2); #endif ! static void funccal_unref(funccall_T *fc, ufunc_T *fp, int force); void func_init() *************** *** 182,191 **** 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; --- 182,190 ---- if (fp->uf_scoped == current_funccal) /* no change */ return OK; ! funccal_unref(fp->uf_scoped, fp, FALSE); fp->uf_scoped = current_funccal; current_funccal->fc_refcount++; if (ga_grow(¤t_funccal->fc_funcs, 1) == FAIL) return FAIL; *************** *** 603,616 **** { ufunc_T *fp = ((ufunc_T **)(fc->fc_funcs.ga_data))[i]; ! if (fp != NULL) ! { ! /* Function may have been redefined and point to another ! * funccall_T, don't clear it then. */ ! if (fp->uf_scoped == fc) ! fp->uf_scoped = NULL; ! func_ptr_unref(fc->func); ! } } ga_clear(&fc->fc_funcs); --- 602,613 ---- { ufunc_T *fp = ((ufunc_T **)(fc->fc_funcs.ga_data))[i]; ! /* When garbage collecting a funccall_T may be freed before the ! * function that references it, clear its uf_scoped field. ! * The function may have been redefined and point to another ! * funccall_T, don't clear it then. */ ! if (fp != NULL && fp->uf_scoped == fc) ! fp->uf_scoped = NULL; } ga_clear(&fc->fc_funcs); *************** *** 1030,1038 **** /* * 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; --- 1027,1036 ---- /* * Unreference "fc": decrement the reference count and free it when it * becomes zero. "fp" is detached from "fc". + * When "force" is TRUE we are exiting. */ static void ! funccal_unref(funccall_T *fc, ufunc_T *fp, int force) { funccall_T **pfc; int i; *************** *** 1040,1049 **** 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) --- 1038,1047 ---- if (fc == NULL) return; ! if (--fc->fc_refcount <= 0 && (force || ( ! 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) *************** *** 1054,1066 **** } } 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; - } - } } /* --- 1052,1059 ---- *************** *** 1083,1091 **** /* * Free a function and remove it from the list of functions. */ static void ! func_free(ufunc_T *fp) { /* clear this function */ ga_clear_strings(&(fp->uf_args)); --- 1076,1085 ---- /* * Free a function and remove it from the list of functions. + * When "force" is TRUE we are exiting. */ static void ! func_free(ufunc_T *fp, int force) { /* clear this function */ ga_clear_strings(&(fp->uf_args)); *************** *** 1100,1106 **** if ((fp->uf_flags & (FC_DELETED | FC_REMOVED)) == 0) func_remove(fp); ! funccal_unref(fp->uf_scoped, fp); vim_free(fp); } --- 1094,1100 ---- if ((fp->uf_flags & (FC_DELETED | FC_REMOVED)) == 0) func_remove(fp); ! funccal_unref(fp->uf_scoped, fp, force); vim_free(fp); } *************** *** 1117,1123 **** for (hi = func_hashtab.ht_array; ; ++hi) if (!HASHITEM_EMPTY(hi)) { ! func_free(HI2UF(hi)); break; } hash_clear(&func_hashtab); --- 1111,1117 ---- for (hi = func_hashtab.ht_array; ; ++hi) if (!HASHITEM_EMPTY(hi)) { ! func_free(HI2UF(hi), TRUE); break; } hash_clear(&func_hashtab); *************** *** 1331,1337 **** if (--fp->uf_calls <= 0 && fp->uf_refcount <= 0) /* Function was unreferenced while being used, free it * now. */ ! func_free(fp); if (did_save_redo) restoreRedobuff(); restore_search_patterns(); --- 1325,1331 ---- if (--fp->uf_calls <= 0 && fp->uf_refcount <= 0) /* Function was unreferenced while being used, free it * now. */ ! func_free(fp, FALSE); if (did_save_redo) restoreRedobuff(); restore_search_patterns(); *************** *** 1675,1680 **** --- 1669,1688 ---- } /* + * 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 == '<'; + } + + /* * ":function" */ void *************** *** 1721,1727 **** { --todo; fp = HI2UF(hi); ! if (!isdigit(*fp->uf_name)) list_func_head(fp, FALSE); } } --- 1729,1735 ---- { --todo; fp = HI2UF(hi); ! if (!func_name_refcount(fp->uf_name)) list_func_head(fp, FALSE); } } *************** *** 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 --- 2664,2669 ---- *************** *** 2738,2744 **** fp->uf_flags |= FC_DELETED; } else ! func_free(fp); } } } --- 2732,2738 ---- fp->uf_flags |= FC_DELETED; } else ! func_free(fp, FALSE); } } } *************** *** 2767,2773 **** /* Only delete it when it's not being used. Otherwise it's done * when "uf_calls" becomes zero. */ if (fp->uf_calls == 0) ! func_free(fp); } } --- 2761,2767 ---- /* Only delete it when it's not being used. Otherwise it's done * when "uf_calls" becomes zero. */ if (fp->uf_calls == 0) ! func_free(fp, FALSE); } } *************** *** 2783,2789 **** /* Only delete it when it's not being used. Otherwise it's done * when "uf_calls" becomes zero. */ if (fp->uf_calls == 0) ! func_free(fp); } } --- 2777,2783 ---- /* Only delete it when it's not being used. Otherwise it's done * when "uf_calls" becomes zero. */ if (fp->uf_calls == 0) ! func_free(fp, FALSE); } } *************** *** 3676,3681 **** --- 3670,3690 ---- return abort; } + static int + set_ref_in_funccal(funccall_T *fc, int copyID) + { + int abort = FALSE; + + if (fc->fc_copyID != copyID) + { + fc->fc_copyID = copyID; + abort = abort || set_ref_in_ht(&fc->l_vars.dv_hashtab, copyID, NULL); + abort = abort || set_ref_in_ht(&fc->l_avars.dv_hashtab, copyID, NULL); + abort = abort || set_ref_in_func(NULL, fc->func, copyID); + } + return abort; + } + /* * Set "copyID" in all local vars and arguments in the call stack. */ *************** *** 3686,3695 **** funccall_T *fc; for (fc = current_funccal; fc != NULL; fc = fc->caller) { ! fc->fc_copyID = copyID; ! abort = abort || set_ref_in_ht(&fc->l_vars.dv_hashtab, copyID, NULL); ! abort = abort || set_ref_in_ht(&fc->l_avars.dv_hashtab, copyID, NULL); } return abort; } --- 3695,3725 ---- funccall_T *fc; for (fc = current_funccal; fc != NULL; fc = fc->caller) + abort = abort || set_ref_in_funccal(fc, copyID); + return abort; + } + + /* + * Set "copyID" in all functions available by name. + */ + int + set_ref_in_functions(int copyID) + { + int todo; + hashitem_T *hi = NULL; + int abort = FALSE; + ufunc_T *fp; + + todo = (int)func_hashtab.ht_used; + for (hi = func_hashtab.ht_array; todo > 0 && !got_int; ++hi) { ! if (!HASHITEM_EMPTY(hi)) ! { ! --todo; ! fp = HI2UF(hi); ! if (!func_name_refcount(fp->uf_name)) ! abort = abort || set_ref_in_func(NULL, fp, copyID); ! } } return abort; } *************** *** 3711,3719 **** /* * Mark all lists and dicts referenced through function "name" with "copyID". - * "list_stack" is used to add lists to be marked. Can be NULL. - * "ht_stack" is used to add hashtabs to be marked. Can be NULL. - * * Returns TRUE if setting references failed somehow. */ int --- 3741,3746 ---- *************** *** 3725,3730 **** --- 3752,3758 ---- char_u fname_buf[FLEN_FIXED + 1]; char_u *tofree = NULL; char_u *fname; + int abort = FALSE; if (name == NULL && fp_in == NULL) return FALSE; *************** *** 3737,3753 **** if (fp != NULL) { for (fc = fp->uf_scoped; fc != NULL; fc = fc->func->uf_scoped) ! { ! if (fc->fc_copyID != copyID) ! { ! fc->fc_copyID = copyID; ! set_ref_in_ht(&fc->l_vars.dv_hashtab, copyID, NULL); ! set_ref_in_ht(&fc->l_avars.dv_hashtab, copyID, NULL); ! } ! } } vim_free(tofree); ! return FALSE; } #endif /* FEAT_EVAL */ --- 3765,3774 ---- if (fp != NULL) { for (fc = fp->uf_scoped; fc != NULL; fc = fc->func->uf_scoped) ! abort = abort || set_ref_in_funccal(fc, copyID); } vim_free(tofree); ! return abort; } #endif /* FEAT_EVAL */ *** ../vim-7.4.2142/src/proto/userfunc.pro 2016-08-01 17:10:13.593214865 +0200 --- src/proto/userfunc.pro 2016-08-01 21:43:03.616306718 +0200 *************** *** 54,59 **** --- 54,60 ---- dictitem_T *find_var_in_scoped_ht(char_u *name, int no_autoload); int set_ref_in_previous_funccal(int copyID); int set_ref_in_call_stack(int copyID); + int set_ref_in_functions(int copyID); int set_ref_in_func_args(int copyID); int set_ref_in_func(char_u *name, ufunc_T *fp_in, int copyID); /* vim: set ft=c : */ *** ../vim-7.4.2142/src/eval.c 2016-08-01 17:10:13.601214790 +0200 --- src/eval.c 2016-08-01 22:20:55.712231023 +0200 *************** *** 5304,5309 **** --- 5304,5312 ---- /* function-local variables */ abort = abort || set_ref_in_call_stack(copyID); + /* named functions (matters for closures) */ + abort = abort || set_ref_in_functions(copyID); + /* function call arguments, if v:testing is set. */ abort = abort || set_ref_in_func_args(copyID); *** ../vim-7.4.2142/src/testdir/test_lambda.vim 2016-08-01 16:29:42.516009792 +0200 --- src/testdir/test_lambda.vim 2016-08-01 22:26:44.049146492 +0200 *************** *** 270,272 **** --- 270,286 ---- delfunc LambdaFoo delfunc LambdaBar endfunc + + func Test_named_function_closure() + func! Afoo() + let x = 14 + func! s:Abar() closure + return x + endfunc + call assert_equal(14, s:Abar()) + endfunc + call Afoo() + call assert_equal(14, s:Abar()) + call test_garbagecollect_now() + call assert_equal(14, s:Abar()) + endfunc *** ../vim-7.4.2142/src/version.c 2016-08-01 20:46:20.614667695 +0200 --- src/version.c 2016-08-01 20:57:19.548765670 +0200 *************** *** 765,766 **** --- 765,768 ---- { /* Add new patch number below this line */ + /**/ + 2143, /**/ -- A fool must search for a greater fool to find admiration. /// 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 ///