bug#2403: And the guilty is: Garbage Collecting leak

bug#2403: And the guilty is: Garbage Collecting leak

[Pierre Poissinger]

[-- Attachment #1: Type: text/plain, Size: 513 bytes --]

Hi,

Took time to trace/debug with the help of Pascal Stenuit: The bug and
pattern seems to be a leak in GC due to a missing GCUNPRO...
This result in a unclean gcprolist in AIX (I do think the bug is "all
arch", just badly visible in AIX and some specific GNU/Linux arch -
other archs will just leak)...

Here come the patch - applied to a clean today checkout and passed the
bootstrap of "characters" just fine

Regs,
Pierre
--
>>> horsemen = ['war', 'pestilence', 'famine']
>>> horsemen.append('Powerbuilder')

[-- Attachment #2: emacs-fix-missing-gc.patch --]
[-- Type: application/octet-stream, Size: 242 bytes --]

diff --git a/src/charset.c b/src/charset.c
index 2781275..50f1ba5 100644
--- a/src/charset.c
+++ b/src/charset.c
@@ -728,6 +728,8 @@ map_charset_for_dump (c_function, function, arg, from, to)
 	}
       c++;
     }
+  
+  UNGCPRO;
 }
 
 void

bug#2403: And the guilty is: Garbage Collecting leak

[Stefan Monnier]

> Took time to trace/debug with the help of Pascal Stenuit: The bug and
> pattern seems to be a leak in GC due to a missing GCUNPRO...

Than you very much, installed.

> This result in a unclean gcprolist in AIX (I do think the bug is "all
> arch", just badly visible in AIX and some specific GNU/Linux arch -
> other archs will just leak)...

Other arches don't use GCPROs at all, instead they conservatively scan
the whole stack.  They do that by setting

   #define GC_MARK_STACK GC_MAKE_GCPROS_NOOPS

in the corresponding emacs/src/s/*.h.  This is the preferred way to
function, so if you can test it and confirm that it works as well under
AIX, we could set it for that OS as well.


        Stefan

bug#2403: And the guilty is: Garbage Collecting leak

[Pierre Poissinger]

[-- Attachment #1: Type: text/plain, Size: 1416 bytes --]

> Than you very much, installed.
You are welcome

> Other arches don't use GCPROs at all, instead they conservatively scan
> the whole stack.  They do that by setting
>
>   #define GC_MARK_STACK GC_MAKE_GCPROS_NOOPS
>
> in the corresponding emacs/src/s/*.h.  This is the preferred way to
> function, so if you can test it and confirm that it works as well under
> AIX, we could set it for that OS as well.
Oups - I actually didn't look so far - readed that same bug hit
GNU/Linux on Super7 so didn't look further...
Since I was in a good mood (Emacs 23 working on my boxes) I took the
liberty to also define GC_SETJMP_WORKS since I saw it next to
GC_MARK_STACK [and ok, I felt lucky...]

So far so good - full build/bootstrapping went fine from latest git
master - basic testing don't show regressions - will see tomorrow if
something fishy pops during work, otherwise looks fine.

In case of need: the compiler used is a gcc-2.9-aix51 on a good old
aix 5.2 - didn't have the courage to try the outdated xlc...

Notes:
* To be able to build emacs with X support (I was really in a good
mood): Had to correct 2 files with "old compilers are grumpy" errors
[0001-gcc-2.9-aix51-020209-compile-fix.patch]
* The actual change I made to src/s/aix-4-2.h
[0002-GC-test-for-AIX.patch]

Regs,
Pierre
-- 
>>> horsemen = ['war', 'pestilence', 'famine']
>>> horsemen.append('Powerbuilder')

[-- Attachment #2: 0001-gcc-2.9-aix51-020209-compile-fix.patch --]
[-- Type: text/x-patch, Size: 1488 bytes --]

From b543f5c64a2456722b91c29016027dc958d39677 Mon Sep 17 00:00:00 2001
From: Pierre Poissinger <pierre.poissinger@gmail.com>
Date: Mon, 30 Mar 2009 17:10:35 -0400
Subject: [PATCH 1/2] gcc 2.9-aix51-020209 compile fix

---
 src/frame.c |    3 ++-
 src/xterm.c |    3 ++-
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/src/frame.c b/src/frame.c
index 1a11021..6408571 100644
--- a/src/frame.c
+++ b/src/frame.c
@@ -3370,6 +3370,7 @@ x_set_font (f, arg, oldval)
   Lisp_Object frame;
   int fontset = -1;
   Lisp_Object font_object;
+  Lisp_Object lval;
 
   /* Set the frame parameter back to the old value because we may
      fail to use ARG as the new parameter value.  */
@@ -3427,7 +3428,7 @@ x_set_font (f, arg, oldval)
     return;
 
   
-  Lisp_Object lval = Fassq (Qfullscreen, f->param_alist);
+  lval = Fassq (Qfullscreen, f->param_alist);
   if (CONSP (lval)) lval = CDR (lval);
 
   x_new_font (f, font_object, fontset);
diff --git a/src/xterm.c b/src/xterm.c
index 76beb62..8444061 100644
--- a/src/xterm.c
+++ b/src/xterm.c
@@ -8675,6 +8675,7 @@ x_handle_net_wm_state (f, event)
   Display *dpy = FRAME_X_DISPLAY (f);
   unsigned char *tmp_data = NULL;
   Atom target_type = XA_ATOM;
+  Lisp_Object lval;
 
   BLOCK_INPUT;
   x_catch_errors (dpy);
@@ -8704,7 +8705,7 @@ x_handle_net_wm_state (f, event)
         value |= FULLSCREEN_BOTH;
     }
 
-  Lisp_Object lval = Qnil;
+  lval = Qnil;
   switch (value) 
     {
     case FULLSCREEN_WIDTH:
-- 
1.6.2.1


[-- Attachment #3: 0002-GC-test-for-AIX.patch --]
[-- Type: text/x-patch, Size: 722 bytes --]

From 8293f6bec069a4c00aa20202173990597ab84ef4 Mon Sep 17 00:00:00 2001
From: Pierre Poissinger <pierre.poissinger@gmail.com>
Date: Mon, 30 Mar 2009 17:12:12 -0400
Subject: [PATCH 2/2] GC test for AIX

---
 src/s/aix4-2.h |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/src/s/aix4-2.h b/src/s/aix4-2.h
index 89154ee..c6e806c 100644
--- a/src/s/aix4-2.h
+++ b/src/s/aix4-2.h
@@ -168,5 +168,9 @@ along with GNU Emacs.  If not, see <http://www.gnu.org/licenses/>.  */
 */
 #define BROKEN_GET_CURRENT_DIR_NAME 1
 
+/* test */
+#define GC_SETJMP_WORKS 1
+#define GC_MARK_STACK GC_MAKE_GCPROS_NOOPS
+
 /* arch-tag: 38fe75ea-6aef-42bd-8449-bc34d921a562
    (do not change this comment) */
-- 
1.6.2.1

bug#2403: And the guilty is: Garbage Collecting leak

[Ulrich Mueller]

> Other arches don't use GCPROs at all, instead they conservatively
> scan the whole stack.  They do that by setting

>    #define GC_MARK_STACK GC_MAKE_GCPROS_NOOPS

> in the corresponding emacs/src/s/*.h.  This is the preferred way to
> function, [...]

Since almost all other arches do it, I wonder if this shouldn't be
done for GNU/Linux on SuperH, too. That is, add "|| defined __sh__"
to the large #if statement in src/s/gnu-linux.h .

Ulrich

bug#2403: And the guilty is: Garbage Collecting leak

[Stefan Monnier]

> Since almost all other arches do it, I wonder if this shouldn't be
> done for GNU/Linux on SuperH, too. That is, add "|| defined __sh__"
> to the large #if statement in src/s/gnu-linux.h .

Yes, it's probably a good idea, but we first need to check.
If you have access to such an arch, please try it out.


        Stefan

bug#2403: And the guilty is: Garbage Collecting leak

[Raúl Porcel]

Doing what Ulrich recommended makes it not hang on SuperH. However it
fails with a Bus error later on.

I'll try some stuff and submit a new bug for that one.

This is the error i get now:
Compiling quail/quick-cns.el
Fatal error (7)make[2]: *** [quail/quick-cns.elc] Bus error
make[2]: Leaving directory `/root/emacs/leim'
make[1]: *** [leim] Error 2

Unaligned accesses are fatal on sh like on sparc. But as I said, i'll
file another bug.

Thanks Pierre for finding out this stuff :)