* bug#15802: 24.3.50; PUSH_HANDLER leaks memory?
@ 2013-11-04 18:32 Jan Djärv
2013-11-04 20:10 ` Stefan Monnier
0 siblings, 1 reply; 6+ messages in thread
From: Jan Djärv @ 2013-11-04 18:32 UTC (permalink / raw)
To: 15802
Hello.
While running leaks on OSX, it indicated that PUSH_HANDLER. I can't find anywhere where
handlerlist members are freed.
Consider handlerlist = NULL.
One PUSH_HANDLER will then allocate one struct handler that becomes handlerlist, next is NULL and nextfree is NULL.
In the code, for example internal_condition_case_1, the code simply does
handlerlist = handlerllist->next
thus loosing the allocated struct. There must be a corresponding xfree somewhere but I can't find it.
Jan D.
In GNU Emacs 24.3.50.1 (x86_64-apple-darwin13.0.0, NS apple-appkit-1265.00)
of 2013-11-04 on zeplin
Bzr revision: 114945 jan.h.d@swipnet.se-20131104175717-8j1fzo686naq33js
Windowing system distributor `Apple', version 10.3.1265
Configured using:
`configure --verbose --with-ns CFLAGS=-g3'
Important settings:
value of $LC_COLLATE: C
value of $LANG: sv_SE.UTF-8
locale-coding-system: utf-8-unix
default enable-multibyte-characters: t
Major mode: Lisp Interaction
Minor modes in effect:
tooltip-mode: t
mouse-wheel-mode: t
tool-bar-mode: t
menu-bar-mode: t
file-name-shadow-mode: t
global-font-lock-mode: t
font-lock-mode: t
blink-cursor-mode: t
auto-composition-mode: t
auto-encryption-mode: t
auto-compression-mode: t
line-number-mode: t
transient-mark-mode: t
Recent input:
<escape> x r e p o r t - e m <tab> <return>
Recent messages:
Unable to load color "unspecified-fg"
For information about GNU Emacs and the GNU system, type C-h C-a.
Unable to load color "unspecified-fg" [2 times]
Load-path shadows:
None found.
Features:
(shadow sort gnus-util mail-extr emacsbug message format-spec rfc822 mml
easymenu mml-sec mm-decode mm-bodies mm-encode mail-parse rfc2231
mailabbrev gmm-utils mailheader sendmail rfc2047 rfc2045 ietf-drums
mm-util mail-prsvr mail-utils time-date tooltip ediff-hook vc-hooks
lisp-float-type mwheel ns-win tool-bar dnd fontset image regexp-opt
fringe tabulated-list newcomment lisp-mode prog-mode register page
menu-bar rfn-eshadow timer select scroll-bar mouse jit-lock font-lock
syntax facemenu font-core frame cham georgian utf-8-lang misc-lang
vietnamese tibetan thai tai-viet lao korean japanese hebrew greek
romanian slovak czech european ethiopic indian cyrillic chinese
case-table epa-hook jka-cmpr-hook help simple abbrev minibuffer nadvice
loaddefs button faces cus-face macroexp files text-properties overlay
sha1 md5 base64 format env code-pages mule custom widget
hashtable-print-readable backquote make-network-process cocoa ns
multi-tty emacs)
^ permalink raw reply [flat|nested] 6+ messages in thread
* bug#15802: 24.3.50; PUSH_HANDLER leaks memory?
2013-11-04 18:32 bug#15802: 24.3.50; PUSH_HANDLER leaks memory? Jan Djärv
@ 2013-11-04 20:10 ` Stefan Monnier
2013-11-04 21:49 ` Jan Djärv
0 siblings, 1 reply; 6+ messages in thread
From: Stefan Monnier @ 2013-11-04 20:10 UTC (permalink / raw)
To: Jan Djärv; +Cc: 15802
> While running leaks on OSX, it indicated that PUSH_HANDLER. I can't find anywhere where
> handlerlist members are freed.
They're never freed.
Stefan
^ permalink raw reply [flat|nested] 6+ messages in thread
* bug#15802: 24.3.50; PUSH_HANDLER leaks memory?
2013-11-04 20:10 ` Stefan Monnier
@ 2013-11-04 21:49 ` Jan Djärv
2013-11-05 2:48 ` Stefan Monnier
0 siblings, 1 reply; 6+ messages in thread
From: Jan Djärv @ 2013-11-04 21:49 UTC (permalink / raw)
To: Stefan Monnier; +Cc: 15802
Hello.
4 nov 2013 kl. 21:10 skrev Stefan Monnier <monnier@iro.umontreal.ca>:
>> While running leaks on OSX, it indicated that PUSH_HANDLER. I can't find anywhere where
>> handlerlist members are freed.
>
> They're never freed.
>
That must be a leak then. If you start with a NULL handlerlist, add one, and then remove it, it is leaked. Then add one and remove, more leakage. And so on. Why are they never released?
Jan D.
^ permalink raw reply [flat|nested] 6+ messages in thread
* bug#15802: 24.3.50; PUSH_HANDLER leaks memory?
2013-11-04 21:49 ` Jan Djärv
@ 2013-11-05 2:48 ` Stefan Monnier
2013-11-05 6:53 ` Jan Djärv
0 siblings, 1 reply; 6+ messages in thread
From: Stefan Monnier @ 2013-11-05 2:48 UTC (permalink / raw)
To: Jan Djärv; +Cc: 15802
> That must be a leak then. If you start with a NULL handlerlist, add one,
> and then remove it, it is leaked. Then add one and remove, more leakage.
> And so on. Why are they never released?
You might be right that the NULL case is not handled.
Once we move past NULL, they are still not freed, but they're kept in
"nextfree" and hence reused.
Stefan
^ permalink raw reply [flat|nested] 6+ messages in thread
* bug#15802: 24.3.50; PUSH_HANDLER leaks memory?
2013-11-05 2:48 ` Stefan Monnier
@ 2013-11-05 6:53 ` Jan Djärv
2013-11-05 16:28 ` Stefan Monnier
0 siblings, 1 reply; 6+ messages in thread
From: Jan Djärv @ 2013-11-05 6:53 UTC (permalink / raw)
To: Stefan Monnier; +Cc: 15802
Hello.
5 nov 2013 kl. 03:48 skrev Stefan Monnier <monnier@iro.umontreal.ca>:
>> That must be a leak then. If you start with a NULL handlerlist, add one,
>> and then remove it, it is leaked. Then add one and remove, more leakage.
>> And so on. Why are they never released?
>
> You might be right that the NULL case is not handled.
> Once we move past NULL, they are still not freed, but they're kept in
> "nextfree" and hence reused.
The NULL case must be fairly common, i.e just one PUSH and then unwind.
Jan D.
^ permalink raw reply [flat|nested] 6+ messages in thread
* bug#15802: 24.3.50; PUSH_HANDLER leaks memory?
2013-11-05 6:53 ` Jan Djärv
@ 2013-11-05 16:28 ` Stefan Monnier
0 siblings, 0 replies; 6+ messages in thread
From: Stefan Monnier @ 2013-11-05 16:28 UTC (permalink / raw)
To: Jan Djärv; +Cc: 15802-done
> The NULL case must be fairly common, i.e just one PUSH and then unwind.
I'm not sure it's that common, since command_loop pushes a `top-level'
catcher, so as long as we stay within command_loop_2 it doesn't go back
to NULL.
This said we should eliminate the NULL case by putting
a "sentinel" at the beginning, so as to avoid this problem.
I've done that with the patch below,
Stefan
=== modified file 'src/ChangeLog'
--- src/ChangeLog 2013-11-05 09:00:52 +0000
+++ src/ChangeLog 2013-11-05 16:26:47 +0000
@@ -1,3 +1,12 @@
+2013-11-05 Stefan Monnier <monnier@iro.umontreal.ca>
+
+ * eval.c (handlerlist_sentinel): New variable (bug#15802).
+ (init_eval): Use it to ensure handlerlist is non-NULL.
+ (unwind_to_catch): Make sure we never set handlerlist to NULL.
+ (Fsignal): Adjust NULLness test of handlerlist.
+
+ * lisp.h (PUSH_HANDLER): Assume handlerlist is non-NULL.
+
2013-11-05 Xue Fuqiao <xfq.free@gmail.com>
* xdisp.c (syms_of_xdisp): Mention the active display table in doc
=== modified file 'src/eval.c'
--- src/eval.c 2013-10-29 14:46:23 +0000
+++ src/eval.c 2013-11-05 16:24:40 +0000
@@ -237,11 +237,22 @@
Vrun_hooks = Qnil;
}
+static struct handler handlerlist_sentinel;
+
void
init_eval (void)
{
specpdl_ptr = specpdl;
- handlerlist = NULL;
+ { /* Put a dummy catcher at top-level so that handlerlist is never NULL.
+ This is important since handlerlist->nextfree holds the freelist
+ which would otherwise leak every time we unwind back to top-level. */
+ struct handler *c;
+ handlerlist = handlerlist_sentinel.nextfree = &handlerlist_sentinel;
+ PUSH_HANDLER (c, Qunbound, CATCHER);
+ eassert (c == &handlerlist_sentinel);
+ handlerlist_sentinel.nextfree = NULL;
+ handlerlist_sentinel.next = NULL;
+ }
Vquit_flag = Qnil;
debug_on_next_call = 0;
lisp_eval_depth = 0;
@@ -1129,6 +1140,8 @@
{
bool last_time;
+ eassert (catch->next);
+
/* Save the value in the tag. */
catch->val = value;
@@ -1542,7 +1555,10 @@
}
else
{
- if (handlerlist != 0)
+ if (handlerlist != &handlerlist_sentinel)
+ /* FIXME: This will come right back here if there's no `top-level'
+ catcher. A better solution would be to abort here, and instead
+ add a catch-all condition handler so we never come here. */
Fthrow (Qtop_level, Qt);
}
=== modified file 'src/lisp.h'
--- src/lisp.h 2013-11-05 07:11:24 +0000
+++ src/lisp.h 2013-11-05 15:48:45 +0000
@@ -2873,13 +2873,12 @@
/* Fill in the components of c, and put it on the list. */
#define PUSH_HANDLER(c, tag_ch_val, handlertype) \
- if (handlerlist && handlerlist->nextfree) \
+ if (handlerlist->nextfree) \
(c) = handlerlist->nextfree; \
else \
{ \
(c) = xmalloc (sizeof (struct handler)); \
(c)->nextfree = NULL; \
- if (handlerlist) \
handlerlist->nextfree = (c); \
} \
(c)->type = (handlertype); \
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2013-11-05 16:28 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-11-04 18:32 bug#15802: 24.3.50; PUSH_HANDLER leaks memory? Jan Djärv
2013-11-04 20:10 ` Stefan Monnier
2013-11-04 21:49 ` Jan Djärv
2013-11-05 2:48 ` Stefan Monnier
2013-11-05 6:53 ` Jan Djärv
2013-11-05 16:28 ` Stefan Monnier
Code repositories for project(s) associated with this public inbox
https://git.savannah.gnu.org/cgit/emacs.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).