Re: scratch/igc 90e80a9a53e 6/6: Adjust igc.c code to header changes

Re: scratch/igc 90e80a9a53e 6/6: Adjust igc.c code to header changes

[Eli Zaretskii]

Hi,

This commit, or the few ones preceding it, broke the build for me:

  Loading d:/gnu/git/emacs/feature/lisp/leim/leim-list.el (source)...
  Finding pointers to doc strings...
  Finding pointers to doc strings...done
  Dumping under the name bootstrap-emacs.pdmp
  Dumping fingerprint: a1fe182420f021f394b12a862c4306fde3b9547d4943b4f87b1e36e1e602a4f1

  Error: error ("Memory exhausted--use M-x save-some-buffers then exit and restart Emacs")
    dump-emacs-portable("d:/gnu/git/emacs/feature/src/bootstrap-emacs.pdmp")

The last commit that did build was 55670e0.

The backtrace from the call to memory_full is below.  The reason seems
to be the recent changes in igc_dump_finish_obj: the result is that
dump_igc_finish_obj wants to write a preposterously large amount of
zeros into the dump: 744MB in the backtrace below, but I've seen much
larger numbers there, like 1.2GB(!).  This causes dump_grow_buffer
requests 2GB from the OS, which rightfully fails.

  Dumping under the name bootstrap-emacs.pdmp
  Dumping fingerprint: a1fe182420f021f394b12a862c4306fde3b9547d4943b4f87b1e36e1e602a4f1

  Thread 1 hit Breakpoint 2, memory_full (nbytes=nbytes@entry=1073741824)
      at alloc.c:4532
  4532    {
  (gdb) bt
  #0  memory_full (nbytes=nbytes@entry=1073741824) at alloc.c:4532
  #1  0x0074a65e in xrealloc (block=0x3a320020, size=1073741824) at alloc.c:829
  #2  0x0074fb1d in dump_grow_buffer (ctx=0x724eb38, ctx=0x724eb38)
      at pdumper.c:627
  #3  dump_write (nbyte=<optimized out>, buf=<optimized out>,
      ctx=<optimized out>) at pdumper.c:798
  #4  dump_write (ctx=0x724eb38, buf=0x724e848, nbyte=8) at pdumper.c:792
  #5  0x0074fc61 in dump_write_zero (ctx=ctx@entry=0x724eb38, nbytes=744574048)
      at pdumper.c:876
  #6  0x0074fdca in dump_igc_finish_obj (ctx=ctx@entry=0x724eb38)
      at pdumper.c:937
  #7  0x0075319c in dump_object_finish (sz=<optimized out>, out=<optimized out>,
      ctx=<optimized out>) at pdumper.c:1000
  #8  dump_float (lfloat=<optimized out>, ctx=<optimized out>) at pdumper.c:2381
  #9  dump_object (ctx=0x724eb38, ctx@entry=0x724eb3c, object=XIL(0xcbf005))
      at pdumper.c:3306
  #10 0x0075571a in dump_drain_copied_objects (ctx=0x724eb3c) at pdumper.c:3518
  #11 Fdump_emacs_portable (filename=<optimized out>,
      track_referrers=<optimized out>) at pdumper.c:4502
  #12 0x0077ae26 in eval_sub (form=XIL(0xbb30653)) at eval.c:2630
  #13 0x0077ac31 in eval_sub (form=XIL(0xbb30593)) at eval.c:2578
  #14 0x0077afb3 in Fprogn (body=XIL(0xbb3084b)) at eval.c:452
  #15 0x0077c058 in Flet (args=<optimized out>) at lisp.h:1545
  #16 0x0077ac31 in eval_sub (form=XIL(0xbb304ab)) at eval.c:2578
  #17 0x0077c906 in Funwind_protect (args=XIL(0xbb3085b)) at lisp.h:1545
  #18 0x0077ac31 in eval_sub (form=XIL(0xbb3049b)) at eval.c:2578
  #19 0x0077afb3 in Fprogn (body=XIL(0)) at eval.c:452
  #20 0x0077c058 in Flet (args=<optimized out>) at lisp.h:1545
  #21 0x0077ac31 in eval_sub (form=XIL(0xbb3046b)) at eval.c:2578
  #22 0x0077afb3 in Fprogn (body=XIL(0xbb3131b)) at eval.c:452
  #23 0x0077c058 in Flet (args=<optimized out>) at lisp.h:1545
  #24 0x0077ac31 in eval_sub (form=XIL(0xbb2ff0b)) at eval.c:2578
  #25 0x0077ac31 in eval_sub (form=XIL(0xbb2feeb)) at eval.c:2578
  #26 0x0077afb3 in Fprogn (body=XIL(0)) at eval.c:452
  #27 0x0077ac31 in eval_sub (form=form@entry=XIL(0xbb2f9d3)) at eval.c:2578
  #28 0x007b6f24 in readevalloop (readcharfun=readcharfun@entry=XIL(0x60c0),
      infile0=infile0@entry=0x724f638,
      sourcename=sourcename@entry=XIL(0xa848184),
      printflag=printflag@entry=false, unibyte=unibyte@entry=XIL(0),
      readfun=readfun@entry=XIL(0), start=start@entry=XIL(0),
      end=<optimized out>, end@entry=XIL(0)) at lread.c:2537
  #29 0x007b797b in Fload (file=<optimized out>, noerror=XIL(0),
      nomessage=XIL(0), nosuffix=XIL(0), must_suffix=<optimized out>)
      at lisp.h:1213
  #30 0x0077adc3 in eval_sub (form=form@entry=XIL(0xa847e4b)) at eval.c:2641
  #31 0x0077cefd in Feval (form=XIL(0xa847e4b), lexical=lexical@entry=XIL(0x20))
      at eval.c:2486
  #32 0x006d5d51 in top_level_2 () at lisp.h:1213
  #33 0x0077527c in internal_condition_case (
      bfun=bfun@entry=0x6d5cf3 <top_level_2>, handlers=handlers@entry=XIL(0x60),
      hfun=hfun@entry=0x6df806 <cmd_error>) at eval.c:1633
  #34 0x006d6460 in top_level_1 (ignore=XIL(0)) at lisp.h:1213
  #35 0x00775196 in internal_catch (tag=tag@entry=XIL(0xc520),
      func=func@entry=0x6d6437 <top_level_1>, arg=arg@entry=XIL(0))
      at eval.c:1312
  #36 0x006d5b0f in command_loop () at lisp.h:1213
  #37 0x006df3c4 in recursive_edit_1 () at keyboard.c:765
  #38 0x006df6b2 in Frecursive_edit () at keyboard.c:848
  #39 0x00921485 in main (argc=<optimized out>, argv=<optimized out>)
      at emacs.c:2646

Re: scratch/igc 90e80a9a53e 6/6: Adjust igc.c code to header changes

[Pip Cet]

On Sunday, July 21st, 2024 at 10:47, Eli Zaretskii <eliz@gnu.org> wrote:
> This commit, or the few ones preceding it, broke the build for me:
> 
> Loading d:/gnu/git/emacs/feature/lisp/leim/leim-list.el (source)...
> Finding pointers to doc strings...
> Finding pointers to doc strings...done
> Dumping under the name bootstrap-emacs.pdmp
> Dumping fingerprint: a1fe182420f021f394b12a862c4306fde3b9547d4943b4f87b1e36e1e602a4f1
> 
> Error: error ("Memory exhausted--use M-x save-some-buffers then exit and restart Emacs")
> dump-emacs-portable("d:/gnu/git/emacs/feature/src/bootstrap-emacs.pdmp")
> 
> The last commit that did build was 55670e0.

Thanks for letting me know! It builds here on 32-bit and 64-bit GNU/Linux systems, so it's possible this is a Windows issue...

> The backtrace from the call to memory_full is below. The reason seems
> to be the recent changes in igc_dump_finish_obj: the result is that
> dump_igc_finish_obj wants to write a preposterously large amount of
> zeros into the dump: 744MB in the backtrace below, but I've seen much
> larger numbers there, like 1.2GB(!). This causes dump_grow_buffer
> requests 2GB from the OS, which rightfully fails.

That all points to an object with a broken MPS header (or none). The question is what kind of object.

> #8 dump_float (lfloat=<optimized out>, ctx=<optimized out>) at pdumper.c:2381
> 
> #9 dump_object (ctx=0x724eb38, ctx@entry=0x724eb3c, object=XIL(0xcbf005))
> at pdumper.c:3306

This section puzzles me. If object is a vectorlike (the type bits of 0xcbf005 are 5, so it should be), it should be dumped as a vectorlike, not a float.

(Also, ctx@entry shouldn't be different from ctx, should it?)

Pip

Re: scratch/igc 90e80a9a53e 6/6: Adjust igc.c code to header changes

[Eli Zaretskii]

> Date: Sun, 21 Jul 2024 11:42:41 +0000
> From: Pip Cet <pipcet@protonmail.com>
> Cc: emacs-devel@gnu.org
> 
> > Error: error ("Memory exhausted--use M-x save-some-buffers then exit and restart Emacs")
> > dump-emacs-portable("d:/gnu/git/emacs/feature/src/bootstrap-emacs.pdmp")
> > 
> > The last commit that did build was 55670e0.
> 
> Thanks for letting me know! It builds here on 32-bit and 64-bit GNU/Linux systems, so it's possible this is a Windows issue...

Maybe, maybe not.  I see nothing specific to Windows yet.

> > The backtrace from the call to memory_full is below. The reason seems
> > to be the recent changes in igc_dump_finish_obj: the result is that
> > dump_igc_finish_obj wants to write a preposterously large amount of
> > zeros into the dump: 744MB in the backtrace below, but I've seen much
> > larger numbers there, like 1.2GB(!). This causes dump_grow_buffer
> > requests 2GB from the OS, which rightfully fails.
> 
> That all points to an object with a broken MPS header (or none). The question is what kind of object.
> 
> > #8 dump_float (lfloat=<optimized out>, ctx=<optimized out>) at pdumper.c:2381
> > 
> > #9 dump_object (ctx=0x724eb38, ctx@entry=0x724eb3c, object=XIL(0xcbf005))
> > at pdumper.c:3306
> 
> This section puzzles me. If object is a vectorlike (the type bits of 0xcbf005 are 5, so it should be), it should be dumped as a vectorlike, not a float.

Does the below help?

  #4  dump_write (ctx=0x71ceb38, buf=0x71ce848, nbyte=8) at pdumper.c:792
  792     dump_write (struct dump_context *ctx, const void *buf, dump_off nbyte)
  (gdb) p ctx->igc_obj_dumped
  $1 = (void *) 0xc3f000 <Sinternal_show_cursor_p>

> (Also, ctx@entry shouldn't be different from ctx, should it?)

I think this is just GDB trying (and failing) to understand the
complexity of GCC's allocation of the place to hold the value:

  (gdb) fr 9
  #9  dump_object (ctx=0x71ceb38, ctx@entry=0x71ceb3c,
      object=0xc3f005 <Sinternal_show_cursor_p+5>) at pdumper.c:3306
  3306          offset = dump_float (ctx, XFLOAT (object));
  (gdb) info address ctx
  Symbol "ctx" is multi-location:
    Range 0x6d2df0-0x6d2e14: a variable in $eax
    Range 0x6d2e14-0x6d2f84: a variable in $ebx
    Range 0x6d2f84-0x6d2f88: a complex DWARF expression:
       0: DW_OP_GNU_entry_value
	 2: DW_OP_reg0 [$eax]
       3: DW_OP_stack_value

    Range 0x6d2f88-0x6d2f8b: a variable in $eax
    Range 0x6d2f8b-0x6d300a: a variable in $ebx
    Range 0x6d300a-0x6d300e: a complex DWARF expression:
       0: DW_OP_GNU_entry_value
	 2: DW_OP_reg0 [$eax]
       3: DW_OP_stack_value

    Range 0x6d300e-0x6d3bb5: a variable in $ebx
    Range 0x8a1c70-0x8a1c75: a variable in $ebx

From poking inside igc_dump_finish_obj, the value returned by obj_size
here:

	return base + obj_size (h);

is preposterously large.

Re: scratch/igc 90e80a9a53e 6/6: Adjust igc.c code to header changes

[Pip Cet]

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

On Sunday, July 21st, 2024 at 12:07, Eli Zaretskii <eliz@gnu.org> wrote:
> > Date: Sun, 21 Jul 2024 11:42:41 +0000
> 
> > From: Pip Cet pipcet@protonmail.com
> > Cc: emacs-devel@gnu.org
> > 
> > > Error: error ("Memory exhausted--use M-x save-some-buffers then exit and restart Emacs")
> > > dump-emacs-portable("d:/gnu/git/emacs/feature/src/bootstrap-emacs.pdmp")
> > > 
> > > The last commit that did build was 55670e0.
> > 
> > Thanks for letting me know! It builds here on 32-bit and 64-bit GNU/Linux systems, so it's possible this is a Windows issue...
> 
> Maybe, maybe not. I see nothing specific to Windows yet.

If it is what I think it is, I'm surprised I haven't seen it in my tests, as it's not specific to Windows at all.

> > > The backtrace from the call to memory_full is below. The reason seems
> > > to be the recent changes in igc_dump_finish_obj: the result is that
> > > dump_igc_finish_obj wants to write a preposterously large amount of
> > > zeros into the dump: 744MB in the backtrace below, but I've seen much
> > > larger numbers there, like 1.2GB(!). This causes dump_grow_buffer
> > > requests 2GB from the OS, which rightfully fails.
> > 
> > That all points to an object with a broken MPS header (or none). The question is what kind of object.
> > 
> > > #8 dump_float (lfloat=<optimized out>, ctx=<optimized out>) at pdumper.c:2381
> > > 
> > > #9 dump_object (ctx=0x724eb38, ctx@entry=0x724eb3c, object=XIL(0xcbf005))
> > > at pdumper.c:3306
> > 
> > This section puzzles me. If object is a vectorlike (the type bits of 0xcbf005 are 5, so it should be), it should be dumped as a vectorlike, not a float.
> 
> Does the below help?
> 
> #4 dump_write (ctx=0x71ceb38, buf=0x71ce848, nbyte=8) at pdumper.c:792
> 792 dump_write (struct dump_context *ctx, const void *buf, dump_off nbyte)
> (gdb) p ctx->igc_obj_dumped
> 
> $1 = (void *) 0xc3f000 <Sinternal_show_cursor_p>

Thanks, that helps a lot! It's an internal subr, and those don't store their proper size in the pseudovector header, so we don't dump them properly as we trust the pseudovector header.

I've managed to reproduce something similar here, and the attached patch helps, but it probably breaks GC in the !HAVE_MPS case.

> > (Also, ctx@entry shouldn't be different from ctx, should it?)
> 
> I think this is just GDB trying (and failing) to understand the
> complexity of GCC's allocation of the place to hold the value:

That makes sense, yes.

Thanks again
Pip

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-mps-subr.patch --]
[-- Type: text/x-patch; name=0001-mps-subr.patch, Size: 605 bytes --]

diff --git a/src/lisp.h b/src/lisp.h
index 5b555c62304..1eab8abd07b 100644
--- a/src/lisp.h
+++ b/src/lisp.h
@@ -3699,7 +3699,7 @@ CHECK_SUBR (Lisp_Object x)
 #define DEFUN(lname, fnname, sname, minargs, maxargs, intspec, doc)	\
   SUBR_SECTION_ATTRIBUTE						\
   static union Aligned_Lisp_Subr sname =				\
-    { {	{ GC_HEADER_INIT PVEC_SUBR << PSEUDOVECTOR_AREA_BITS },	\
+    { {	{ GC_HEADER_INIT PVECHEADERSIZE (PVEC_SUBR, sizeof (union Aligned_Lisp_Subr) / word_size, 0) }, \
         { .a ## maxargs = fnname },					\
 	minargs, maxargs, lname, {intspec}, lisp_h_Qnil}};		\
    Lisp_Object fnname

Re: scratch/igc 90e80a9a53e 6/6: Adjust igc.c code to header changes

[Eli Zaretskii]

> Date: Sun, 21 Jul 2024 14:11:56 +0000
> From: Pip Cet <pipcet@protonmail.com>
> Cc: emacs-devel@gnu.org
> 
> > Does the below help?
> > 
> > #4 dump_write (ctx=0x71ceb38, buf=0x71ce848, nbyte=8) at pdumper.c:792
> > 792 dump_write (struct dump_context *ctx, const void *buf, dump_off nbyte)
> > (gdb) p ctx->igc_obj_dumped
> > 
> > $1 = (void *) 0xc3f000 <Sinternal_show_cursor_p>
> 
> Thanks, that helps a lot! It's an internal subr, and those don't store their proper size in the pseudovector header, so we don't dump them properly as we trust the pseudovector header.
> 
> I've managed to reproduce something similar here, and the attached patch helps, but it probably breaks GC in the !HAVE_MPS case.

Why cannot you identify this kind of object during dumping, and avoid
applying whatever igc_dump_finish_obj does?  There's the PVEC_SUBR in
the struct, no?

Re: scratch/igc 90e80a9a53e 6/6: Adjust igc.c code to header changes

[Pip Cet]

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

On Sunday, July 21st, 2024 at 14:25, Eli Zaretskii <eliz@gnu.org> wrote:
> > Date: Sun, 21 Jul 2024 14:11:56 +0000
> 
> > From: Pip Cet pipcet@protonmail.com
> > Cc: emacs-devel@gnu.org
> > 
> > > Does the below help?
> > > 
> > > #4 dump_write (ctx=0x71ceb38, buf=0x71ce848, nbyte=8) at pdumper.c:792
> > > 792 dump_write (struct dump_context *ctx, const void *buf, dump_off nbyte)
> > > (gdb) p ctx->igc_obj_dumped
> > > 
> > > $1 = (void *) 0xc3f000 <Sinternal_show_cursor_p>
> > 
> > Thanks, that helps a lot! It's an internal subr, and those don't store their proper size in the pseudovector header, so we don't dump them properly as we trust the pseudovector header.
> > 
> > I've managed to reproduce something similar here, and the attached patch helps, but it probably breaks GC in the !HAVE_MPS case.
> 
> 
> Why cannot you identify this kind of object during dumping, and avoid
> applying whatever igc_dump_finish_obj does?

You mean you'd prefer to leave the pseudovector header for builtin subrs at its current value? We can do that, but I think the MPS header should still be valid. (It's not with the patch I had attached: that makes it claim an object size of 112 bytes when it's actually just 96 bytes, and that causes us to overwrite the first 16 bytes after the last subr; in my case, stdout... Anyway, next patch attached, which duplicates a bit of code unnecessarily.)

> There's the PVEC_SUBR in the struct, no?

Yes, there is.

Pip

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0002-mps-subr.patch --]
[-- Type: text/x-patch; name=0002-mps-subr.patch, Size: 3242 bytes --]

diff --git a/src/igc.c b/src/igc.c
index 6960e5267d2..7dd2715204a 100644
--- a/src/igc.c
+++ b/src/igc.c
@@ -619,6 +619,27 @@ set_header (struct igc_header *h, enum igc_obj_type type,
 static unsigned alloc_hash (void);
 static size_t igc_round (size_t nbytes, size_t align);
 
+#define COMMON_MULTIPLE(a, b) \
+  ((a) % (b) == 0 ? (a) : (b) % (a) == 0 ? (b) : (a) * (b))
+enum { LISP_ALIGNMENT = alignof (union { GCALIGNED_UNION_MEMBER }) };
+/* Vector size requests are a multiple of this.  */
+enum { roundup_size = COMMON_MULTIPLE (LISP_ALIGNMENT, word_size) };
+/* Round up X to nearest mult-of-ROUNDUP_SIZE --- use at compile time.  */
+#define vroundup_ct(x) ROUNDUP (x, roundup_size)
+/* Round up X to nearest mult-of-ROUNDUP_SIZE --- use at runtime.  */
+#define vroundup(x) (eassume ((x) >= 0), vroundup_ct (x))
+
+/* Memory footprint in bytes of a pseudovector other than a bool-vector.  */
+static ptrdiff_t
+pseudovector_nbytes (const struct vectorlike_header *hdr)
+{
+  eassert (!PSEUDOVECTOR_TYPEP (hdr, PVEC_BOOL_VECTOR));
+  ptrdiff_t nwords = ((hdr->size & PSEUDOVECTOR_SIZE_MASK)
+		      + ((hdr->size & PSEUDOVECTOR_REST_MASK)
+			 >> PSEUDOVECTOR_SIZE_BITS));
+  return vroundup (header_size + word_size * nwords);
+}
+
 /* Called throughout Emacs to initialize the GC header of an object
    which does not live in GC-managed memory, such as pure objects and
    builtin symbols.  */
@@ -641,11 +662,13 @@ set_header (struct igc_header *h, enum igc_obj_type type,
       break;
     case IGC_OBJ_VECTOR:
       {
+	ssize_t nbytes;
 	ptrdiff_t size = ((struct Lisp_Vector *)header)->header.size;
 	if (size & PSEUDOVECTOR_FLAG)
-	  size &= PSEUDOVECTOR_SIZE_MASK;
-	set_header (h, IGC_OBJ_VECTOR, sizeof (struct Lisp_Vector) +
-		    size * sizeof (Lisp_Object), alloc_hash ());
+	  nbytes = pseudovector_nbytes (&((struct Lisp_Vector *)header)->header);
+	else
+	  nbytes = size * sizeof (Lisp_Object) + header_size;
+	set_header (h, IGC_OBJ_VECTOR, nbytes, alloc_hash ());
 	break;
       }
     case IGC_OBJ_DUMPED_CHARSET_TABLE:
diff --git a/src/lisp.h b/src/lisp.h
index 5b555c62304..170490297e4 100644
--- a/src/lisp.h
+++ b/src/lisp.h
@@ -3699,7 +3699,8 @@ CHECK_SUBR (Lisp_Object x)
 #define DEFUN(lname, fnname, sname, minargs, maxargs, intspec, doc)	\
   SUBR_SECTION_ATTRIBUTE						\
   static union Aligned_Lisp_Subr sname =				\
-    { {	{ GC_HEADER_INIT PVEC_SUBR << PSEUDOVECTOR_AREA_BITS },	\
+    { {	{ GC_HEADER_INIT						\
+	  PVECHEADERSIZE (PVEC_SUBR, 0, VECSIZE (union Aligned_Lisp_Subr)) }, \
         { .a ## maxargs = fnname },					\
 	minargs, maxargs, lname, {intspec}, lisp_h_Qnil}};		\
    Lisp_Object fnname
diff --git a/src/sfntfont.c b/src/sfntfont.c
index 9bc3fb3415e..dd21fba4776 100644
--- a/src/sfntfont.c
+++ b/src/sfntfont.c
@@ -1990,7 +1990,8 @@ sfntfont_compare_font_entities (Lisp_Object a, Lisp_Object b)
 static union Aligned_Lisp_Subr Scompare_font_entities =
   {
     {
-      { GC_HEADER_INIT PSEUDOVECTOR_FLAG | (PVEC_SUBR << PSEUDOVECTOR_AREA_BITS), },
+      { GC_HEADER_INIT
+	PVECHEADERSIZE (PVEC_SUBR, 0, VECSIZE (union Aligned_Lisp_Subr)) },
       { .a2 = sfntfont_compare_font_entities, },
       2, 2, "sfntfont_compare_font_entities", {0}, lisp_h_Qnil,
     },

Re: scratch/igc 90e80a9a53e 6/6: Adjust igc.c code to header changes

[Eli Zaretskii]

> Date: Sun, 21 Jul 2024 16:20:57 +0000
> From: Pip Cet <pipcet@protonmail.com>
> Cc: emacs-devel@gnu.org
> 
> > Why cannot you identify this kind of object during dumping, and avoid
> > applying whatever igc_dump_finish_obj does?
> 
> You mean you'd prefer to leave the pseudovector header for builtin subrs at its current value? We can do that, but I think the MPS header should still be valid. (It's not with the patch I had attached: that makes it claim an object size of 112 bytes when it's actually just 96 bytes, and that causes us to overwrite the first 16 bytes after the last subr; in my case, stdout... Anyway, next patch attached, which duplicates a bit of code unnecessarily.)

Then why doesn't your patch do that?  And this new patch is even more
massive than the previous one?

In any case, if we need to change the definition of Aligned_Lisp_Subr,
let's do that only "#if HAVE_MPS", and leave the old code intact.

Re: scratch/igc 90e80a9a53e 6/6: Adjust igc.c code to header changes

[Pip Cet]

On Sunday, July 21st, 2024 at 16:27, Eli Zaretskii <eliz@gnu.org> wrote:
> > Date: Sun, 21 Jul 2024 16:20:57 +0000
> 
> > From: Pip Cet pipcet@protonmail.com
> > Cc: emacs-devel@gnu.org
> > 
> > > Why cannot you identify this kind of object during dumping, and avoid
> > > applying whatever igc_dump_finish_obj does?
> > 
> > You mean you'd prefer to leave the pseudovector header for builtin subrs at its current value? We can do that, but I think the MPS header should still be valid. (It's not with the patch I had attached: that makes it claim an object size of 112 bytes when it's actually just 96 bytes, and that causes us to overwrite the first 16 bytes after the last subr; in my case, stdout... Anyway, next patch attached, which duplicates a bit of code unnecessarily.)
> 
> 
> Then why doesn't your patch do that?

I don't understand why it should. The pseudovector headers of main_thread and the subroutines are, currently, simply wrong:

(gdb) p vectorlike_nbytes(&Scall_interactively)
$1 = 2415919112
(gdb) p vectorlike_nbytes(&main_thread)
$2 = 584
(gdb) p sizeof(main_thread)
$3 = 520
(gdb) p sizeof(Scall_interactively)
$4 = 88

> And this new patch is even more massive than the previous one?

It is "even more massive" than a one-liner because there were additional bugs to fix, yes. As demonstrated above, vectorlike_nbytes doesn't work on subroutines because they lack the pseudovector flag, so I had to duplicate some code from alloc.c to use pseudovector_nbytes instead.
 
> In any case, if we need to change the definition of Aligned_Lisp_Subr,
> let's do that only "#if HAVE_MPS", and leave the old code intact.

No objections there.

I've found two more bugs that popped up testing this in various configurations: not protecting Vinternal_interpreter_environment (fun to debug, as might be imagined) and using xfree rather than igc_xfree in sort.c. Both may have affected dumping, so I'll push all three fixes and then hopefully things will work again.

Pip