Merging scratch/no-purespace to remove unexec and purespace

Merging scratch/no-purespace to remove unexec and purespace

[Stefan Kangas]

In August, we decided to remove the unexec build along with purespace.
The scratch/no-purespace branch removes both, and has been rebased on
top of a recent master.

We, the maintainers, believe that the scratch/no-purespace branch is
now ready to merge, and would appreciate any final feedback, testing,
and code reviews.  Specifically, the branch has been primarily tested
on GNU/Linux and macOS, so testing on other systems would be greatly
appreciated.

Unless we uncover any serious blocking issues, or significant or
unexpected objections from the community, we plan to merge the
branch on February 1, 2025.

During our last discussion, we identified some issues with using
pdumper on some legacy proprietary systems: MS-DOS, Windows 98, and
Solaris 10 Zone.  As we have explained previously, patches are welcome
for these issues, but we do not currently consider them as blockers
for this merge.

Thanks!

Stefan Kangas, on behalf of the Emacs maintainers

Re: Merging scratch/no-purespace to remove unexec and purespace

[Gerd Möllmann]

Stefan Kangas <stefankangas@gmail.com> writes:

> In August, we decided to remove the unexec build along with purespace.
> The scratch/no-purespace branch removes both, and has been rebased on
> top of a recent master.
>
> We, the maintainers, believe that the scratch/no-purespace branch is
> now ready to merge, and would appreciate any final feedback, testing,
> and code reviews.  Specifically, the branch has been primarily tested
> on GNU/Linux and macOS, so testing on other systems would be greatly
> appreciated.
>
> Unless we uncover any serious blocking issues, or significant or
> unexpected objections from the community, we plan to merge the
> branch on February 1, 2025.
>
> During our last discussion, we identified some issues with using
> pdumper on some legacy proprietary systems: MS-DOS, Windows 98, and
> Solaris 10 Zone.  As we have explained previously, patches are welcome
> for these issues, but we do not currently consider them as blockers
> for this merge.
>
> Thanks!
>
> Stefan Kangas, on behalf of the Emacs maintainers

Building on macOS with --enable-checking=all --with-native-compilation
gives assertion violations. It builds without the --enable-checking.

gmake -C ../lisp compile-first EMACS="../src/bootstrap-emacs"
  ELC+ELN  emacs-lisp/comp-common.elc
  ELC+ELN  emacs-lisp/comp-run.elc
  ELC+ELN  emacs-lisp/radix-tree.elc
  ELC+ELN  emacs-lisp/loaddefs-gen.elc
  ELC+ELN  emacs-lisp/macroexp.elc
  ELC+ELN  emacs-lisp/cconv.elc
  ELC+ELN  emacs-lisp/comp-cstr.elc
  ELC+ELN  emacs-lisp/bytecomp.elc
  ELC+ELN  emacs-lisp/byte-opt.elc
  ELC+ELN  emacs-lisp/comp.elc
gmake -C ../lisp autoloads EMACS="../src/bootstrap-emacs"
  ELC+ELN  ../lisp/cus-face.elc

comp.c:5322: Emacs fatal error: assertion failed: check_comp_unit_relocs (comp_u)
gmake[3]: *** [Makefile:285: ../lisp/cus-face.elc] Abort trap: 6
  ELC+ELN  international/titdic-cnv.elc

comp.c:5322: Emacs fatal error: assertion failed: check_comp_unit_relocs (comp_u)
Fatal error 6: Aborted
gmake[3]: *** [Makefile:330: international/titdic-cnv.elc] Abort trap: 6
gmake[2]: *** [Makefile:901: ../lisp/cus-face.elc] Error 2
gmake[2]: *** Waiting for unfinished jobs....
gmake[2]: *** [Makefile:961: ../lisp/loaddefs.el] Error 2
  ELC+ELN  ../lisp/abbrev.elc

comp.c:5322: Emacs fatal error: assertion failed: check_comp_unit_relocs (comp_u)
gmake[3]: *** [Makefile:285: ../lisp/abbrev.elc] Abort trap: 6
  ELC+ELN  ../lisp/button.elc

Re: Merging scratch/no-purespace to remove unexec and purespace

[Gerd Möllmann]

Gerd Möllmann <gerd.moellmann@gmail.com> writes:

> Stefan Kangas <stefankangas@gmail.com> writes:
>
>> In August, we decided to remove the unexec build along with purespace.
>> The scratch/no-purespace branch removes both, and has been rebased on
>> top of a recent master.
>>
>> We, the maintainers, believe that the scratch/no-purespace branch is
>> now ready to merge, and would appreciate any final feedback, testing,
>> and code reviews.  Specifically, the branch has been primarily tested
>> on GNU/Linux and macOS, so testing on other systems would be greatly
>> appreciated.
>>
>> Unless we uncover any serious blocking issues, or significant or
>> unexpected objections from the community, we plan to merge the
>> branch on February 1, 2025.
>>
>> During our last discussion, we identified some issues with using
>> pdumper on some legacy proprietary systems: MS-DOS, Windows 98, and
>> Solaris 10 Zone.  As we have explained previously, patches are welcome
>> for these issues, but we do not currently consider them as blockers
>> for this merge.
>>
>> Thanks!
>>
>> Stefan Kangas, on behalf of the Emacs maintainers
>
> Building on macOS with --enable-checking=all --with-native-compilation
> gives assertion violations. It builds without the --enable-checking.
>
> gmake -C ../lisp compile-first EMACS="../src/bootstrap-emacs"
>   ELC+ELN  emacs-lisp/comp-common.elc
>   ELC+ELN  emacs-lisp/comp-run.elc
>   ELC+ELN  emacs-lisp/radix-tree.elc
>   ELC+ELN  emacs-lisp/loaddefs-gen.elc
>   ELC+ELN  emacs-lisp/macroexp.elc
>   ELC+ELN  emacs-lisp/cconv.elc
>   ELC+ELN  emacs-lisp/comp-cstr.elc
>   ELC+ELN  emacs-lisp/bytecomp.elc
>   ELC+ELN  emacs-lisp/byte-opt.elc
>   ELC+ELN  emacs-lisp/comp.elc
> gmake -C ../lisp autoloads EMACS="../src/bootstrap-emacs"
>   ELC+ELN  ../lisp/cus-face.elc
>
> comp.c:5322: Emacs fatal error: assertion failed: check_comp_unit_relocs (comp_u)
> gmake[3]: *** [Makefile:285: ../lisp/cus-face.elc] Abort trap: 6
>   ELC+ELN  international/titdic-cnv.elc
>
> comp.c:5322: Emacs fatal error: assertion failed: check_comp_unit_relocs (comp_u)
> Fatal error 6: Aborted
> gmake[3]: *** [Makefile:330: international/titdic-cnv.elc] Abort trap: 6
> gmake[2]: *** [Makefile:901: ../lisp/cus-face.elc] Error 2
> gmake[2]: *** Waiting for unfinished jobs....
> gmake[2]: *** [Makefile:961: ../lisp/loaddefs.el] Error 2
>   ELC+ELN  ../lisp/abbrev.elc
>
> comp.c:5322: Emacs fatal error: assertion failed: check_comp_unit_relocs (comp_u)
> gmake[3]: *** [Makefile:285: ../lisp/abbrev.elc] Abort trap: 6
>   ELC+ELN  ../lisp/button.elc

I think check_comp_unit_relocs should be removed in the branch. What's
left of it the branch, checks in master if everything has been
put in purespace which should be there. IIUC correctly, of course.

Re: Merging scratch/no-purespace to remove unexec and purespace

[Gerd Möllmann]

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

Gerd Möllmann <gerd.moellmann@gmail.com> writes:

> I think check_comp_unit_relocs should be removed in the branch. What's
> left of it the branch, checks in master if everything has been
> put in purespace which should be there. IIUC correctly, of course.

Patch attached.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Remove-check_comp_unit_relocs.patch --]
[-- Type: text/x-patch, Size: 1722 bytes --]

From 66e73f65966e40af67338cfdf18f89549d687935 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Gerd=20M=C3=B6llmann?= <gerd@gnu.org>
Date: Tue, 17 Dec 2024 15:28:14 +0100
Subject: [PATCH] Remove check_comp_unit_relocs

* src/comp.c (check_comp_unit_relocs): Removed.
(load_comp_unit): Remove use.
---
 src/comp.c | 27 ---------------------------
 1 file changed, 27 deletions(-)

diff --git a/src/comp.c b/src/comp.c
index f91f7c57f45..2c7f2b704f6 100644
--- a/src/comp.c
+++ b/src/comp.c
@@ -5164,32 +5164,6 @@ load_static_obj (struct Lisp_Native_Comp_Unit *comp_u, const char *name)
 
 }
 
-/* Return false when something is wrong or true otherwise.  */
-
-static bool
-check_comp_unit_relocs (struct Lisp_Native_Comp_Unit *comp_u)
-{
-  dynlib_handle_ptr handle = comp_u->handle;
-  Lisp_Object *data_relocs = dynlib_sym (handle, DATA_RELOC_SYM);
-
-  EMACS_INT d_vec_len = XFIXNUM (Flength (comp_u->data_vec));
-
-  for (ptrdiff_t i = 0; i < d_vec_len; i++)
-    {
-      Lisp_Object x = data_relocs[i];
-      if (EQ (x, Qlambda_fixup))
-	return false;
-      else if (NATIVE_COMP_FUNCTIONP (x))
-	{
-	  if (NILP (Fgethash (x, comp_u->lambda_gc_guard_h, Qnil)))
-	    return false;
-	}
-      else if (!EQ (x, AREF (comp_u->data_vec, i)))
-	return false;
-    }
-  return true;
-}
-
 static void
 unset_cu_load_ongoing (Lisp_Object comp_u)
 {
@@ -5319,7 +5293,6 @@ load_comp_unit (struct Lisp_Native_Comp_Unit *comp_u, bool loading_dump,
       /* Make sure data_ephemeral_vec still exists after top_level_run has run.
 	 Guard against sibling call optimization (or any other).  */
       data_ephemeral_vec = data_ephemeral_vec;
-      eassert (check_comp_unit_relocs (comp_u));
     }
 
   if (!recursive_load)
-- 
2.47.1

Re: Merging scratch/no-purespace to remove unexec and purespace

[Gerd Möllmann]

Gerd Möllmann <gerd.moellmann@gmail.com> writes:

> Gerd Möllmann <gerd.moellmann@gmail.com> writes:
>
>> I think check_comp_unit_relocs should be removed in the branch. What's
>> left of it the branch, checks in master if everything has been
>> put in purespace which should be there. IIUC correctly, of course.
>
> Patch attached.

Pushed. Complaints to me please.

Re: Merging scratch/no-purespace to remove unexec and purespace

[Eli Zaretskii]

> From: Gerd Möllmann <gerd.moellmann@gmail.com>
> Cc: emacs-devel@gnu.org,  Pip Cet <pipcet@protonmail.com>,  Stefan Monnier
>  <monnier@iro.umontreal.ca>
> Date: Tue, 17 Dec 2024 18:56:05 +0100
> 
> Gerd Möllmann <gerd.moellmann@gmail.com> writes:
> 
> > Gerd Möllmann <gerd.moellmann@gmail.com> writes:
> >
> >> I think check_comp_unit_relocs should be removed in the branch. What's
> >> left of it the branch, checks in master if everything has been
> >> put in purespace which should be there. IIUC correctly, of course.
> >
> > Patch attached.
> 
> Pushed. Complaints to me please.

I'd like at least Andrea to take a look and confirm.

Re: Merging scratch/no-purespace to remove unexec and purespace

[Gerd Möllmann]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Gerd Möllmann <gerd.moellmann@gmail.com>
>> Cc: emacs-devel@gnu.org,  Pip Cet <pipcet@protonmail.com>,  Stefan Monnier
>>  <monnier@iro.umontreal.ca>
>> Date: Tue, 17 Dec 2024 18:56:05 +0100
>> 
>> Gerd Möllmann <gerd.moellmann@gmail.com> writes:
>> 
>> > Gerd Möllmann <gerd.moellmann@gmail.com> writes:
>> >
>> >> I think check_comp_unit_relocs should be removed in the branch. What's
>> >> left of it the branch, checks in master if everything has been
>> >> put in purespace which should be there. IIUC correctly, of course.
>> >
>> > Patch attached.
>> 
>> Pushed. Complaints to me please.
>
> I'd like at least Andrea to take a look and confirm.

It's 81fc23b5d6a60ca4f3d269ab2c88eb9a850bac4c

Re: Merging scratch/no-purespace to remove unexec and purespace

[Helmut Eller]

On Tue, Dec 17 2024, Stefan Kangas wrote:

[...]
> We, the maintainers, believe that the scratch/no-purespace branch is
> now ready to merge, and would appreciate any final feedback, testing,
> and code reviews.  Specifically, the branch has been primarily tested
> on GNU/Linux and macOS, so testing on other systems would be greatly
> appreciated.

Do you have an estimate what removing purespace will cost in terms of GC
time?  I mean something like "1ms per collection".  Or perhaps a
suggestion how I could measure it?

Re: Merging scratch/no-purespace to remove unexec and purespace

[Stefan Monnier]

>> We, the maintainers, believe that the scratch/no-purespace branch is
>> now ready to merge, and would appreciate any final feedback, testing,
>> and code reviews.  Specifically, the branch has been primarily tested
>> on GNU/Linux and macOS, so testing on other systems would be greatly
>> appreciated.
>
> Do you have an estimate what removing purespace will cost in terms of GC
> time?  I mean something like "1ms per collection".  Or perhaps a
> suggestion how I could measure it?

In the pdump case it should have no effect at all, or maybe even
a slight *speedup*.

That's because the pdump already fails to take advantage of the
purespace (i.e. the GC traces through the purespace like the rest of
the heap).


        Stefan

Re: Merging scratch/no-purespace to remove unexec and purespace

[Stefan Kangas]

Gerd Möllmann <gerd.moellmann@gmail.com> writes:

> Gerd Möllmann <gerd.moellmann@gmail.com> writes:
>
>> Gerd Möllmann <gerd.moellmann@gmail.com> writes:
>>
>>> I think check_comp_unit_relocs should be removed in the branch. What's
>>> left of it the branch, checks in master if everything has been
>>> put in purespace which should be there. IIUC correctly, of course.
>>
>> Patch attached.
>
> Pushed. Complaints to me please.

Thanks!

Re: Merging scratch/no-purespace to remove unexec and purespace

[Po Lu]

Stefan Kangas <stefankangas@gmail.com> writes:

> In August, we decided to remove the unexec build along with purespace.
> The scratch/no-purespace branch removes both, and has been rebased on
> top of a recent master.
>
> We, the maintainers, believe that the scratch/no-purespace branch is
> now ready to merge, and would appreciate any final feedback, testing,
> and code reviews.  Specifically, the branch has been primarily tested
> on GNU/Linux and macOS, so testing on other systems would be greatly
> appreciated.
>
> Unless we uncover any serious blocking issues, or significant or
> unexpected objections from the community, we plan to merge the
> branch on February 1, 2025.
>
> During our last discussion, we identified some issues with using
> pdumper on some legacy proprietary systems: MS-DOS, Windows 98, and
> Solaris 10 Zone.  As we have explained previously, patches are welcome
> for these issues, but we do not currently consider them as blockers
> for this merge.

Please don't merge this until Emacs 30 is released, whether that be
before the 1st of February or after.

Re: Merging scratch/no-purespace to remove unexec and purespace

[Stefan Kangas]

Po Lu <luangruo@yahoo.com> writes:

> Stefan Kangas <stefankangas@gmail.com> writes:
>
>> In August, we decided to remove the unexec build along with purespace.
>> The scratch/no-purespace branch removes both, and has been rebased on
>> top of a recent master.
>>
>> We, the maintainers, believe that the scratch/no-purespace branch is
>> now ready to merge, and would appreciate any final feedback, testing,
>> and code reviews.  Specifically, the branch has been primarily tested
>> on GNU/Linux and macOS, so testing on other systems would be greatly
>> appreciated.
>>
>> Unless we uncover any serious blocking issues, or significant or
>> unexpected objections from the community, we plan to merge the
>> branch on February 1, 2025.
>>
>> During our last discussion, we identified some issues with using
>> pdumper on some legacy proprietary systems: MS-DOS, Windows 98, and
>> Solaris 10 Zone.  As we have explained previously, patches are welcome
>> for these issues, but we do not currently consider them as blockers
>> for this merge.
>
> Please don't merge this until Emacs 30 is released, whether that be
> before the 1st of February or after.

I'm guessing here, but I take that to mean:
1. You intend to work on resolving the problems with pdumper on one or
   more legacy proprietary systems.
2. You will not have much time before Emacs 30 is out, as you have
   previously indicated.

Is that a correct understanding?  If not, may I ask that you elaborate?

Re: Merging scratch/no-purespace to remove unexec and purespace

[Stefan Kangas]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>>> We, the maintainers, believe that the scratch/no-purespace branch is
>>> now ready to merge, and would appreciate any final feedback, testing,
>>> and code reviews.  Specifically, the branch has been primarily tested
>>> on GNU/Linux and macOS, so testing on other systems would be greatly
>>> appreciated.
>>
>> Do you have an estimate what removing purespace will cost in terms of GC
>> time?  I mean something like "1ms per collection".  Or perhaps a
>> suggestion how I could measure it?
>
> In the pdump case it should have no effect at all, or maybe even
> a slight *speedup*.
>
> That's because the pdump already fails to take advantage of the
> purespace (i.e. the GC traces through the purespace like the rest of
> the heap).

I'll note that the best solution to that is to have a generational GC
instead.  Simple, right?

It's not entirely unrelated though: among other things, one reason why
merging this would be good is that it would reportedly simplify the work
on the igc branch.  And indeed any GC-related work now or in future.

Re: Merging scratch/no-purespace to remove unexec and purespace

[Helmut Eller]

On Tue, Dec 17 2024, Stefan Monnier wrote:

>> Do you have an estimate what removing purespace will cost in terms of GC
>> time?  I mean something like "1ms per collection".  Or perhaps a
>> suggestion how I could measure it?
>
> In the pdump case it should have no effect at all, or maybe even
> a slight *speedup*.
>
> That's because the pdump already fails to take advantage of the
> purespace (i.e. the GC traces through the purespace like the rest of
> the heap).

So the pdumper copies objects from purespace to the dump like normal
objects; when loading the dump, purespace stays empty.

I had (wrongly) assumed that the pdumper creates a separate section for
pure objects.  Creating such a section sounds easy enough (hmm, maybe
not so easy because of hashtables).  Still not sure if it would be worth
the effort.

Helmut

Re: Merging scratch/no-purespace to remove unexec and purespace

[Helmut Eller]

On Tue, Dec 17 2024, Stefan Kangas wrote:

> Stefan Monnier <monnier@iro.umontreal.ca> writes:
[...]
>> That's because the pdump already fails to take advantage of the
>> purespace (i.e. the GC traces through the purespace like the rest of
>> the heap).
>
> I'll note that the best solution to that is to have a generational GC
> instead.  Simple, right?

A generational GC is definitely simpler.  Whether it's the "best"
solution is not so clear: A copying GC, like MPS, still needs to trace
and copy pure objects whenever the oldest generation is in the condemned
set.  Moving pure objects to a non-moving pool might be better.

> It's not entirely unrelated though: among other things, one reason why
> merging this would be good is that it would reportedly simplify the work
> on the igc branch.  And indeed any GC-related work now or in future.

Objects in purespace are immutable and immortal.  That's potentially
useful information for the GC.  Removing purespace also removes that
information.  Of course, if the pdumper already throws away this
information, then purespace just adds useless complexity.

Helmut

Re: Merging scratch/no-purespace to remove unexec and purespace

[Pip Cet via Emacs development discussions.]

"Helmut Eller" <eller.helmut@gmail.com> writes:

> On Tue, Dec 17 2024, Stefan Kangas wrote:
>
> [...]
>> We, the maintainers, believe that the scratch/no-purespace branch is
>> now ready to merge, and would appreciate any final feedback, testing,
>> and code reviews.  Specifically, the branch has been primarily tested
>> on GNU/Linux and macOS, so testing on other systems would be greatly
>> appreciated.
>
> Do you have an estimate what removing purespace will cost in terms of GC
> time?  I mean something like "1ms per collection".  Or perhaps a
> suggestion how I could measure it?

"Close to zero" is the best I can do at this time.  In particular, that
implies "not catastrophically worse", which is all that is relevant
right now, IMHO.

> I mean something like "1ms per collection".  Or perhaps a
> suggestion how I could measure it?

I think that all the previously-pure data should be available right away
after starting an Emacs session, so maybe something like:

perf record -e cycles ./src/emacs -Q --batch --eval '(dotimes (i 1000)
(garbage-collect)))'

would be one data point.  Some effects would only be visible in GCs with
deep call stacks, or in large sessions, but that's hard to measure.

Pip

Re: Merging scratch/no-purespace to remove unexec and purespace

[Andrea Corallo]

Gerd Möllmann <gerd.moellmann@gmail.com> writes:

> Eli Zaretskii <eliz@gnu.org> writes:
>
>>> From: Gerd Möllmann <gerd.moellmann@gmail.com>
>>> Cc: emacs-devel@gnu.org,  Pip Cet <pipcet@protonmail.com>,  Stefan Monnier
>>>  <monnier@iro.umontreal.ca>
>>> Date: Tue, 17 Dec 2024 18:56:05 +0100
>>> 
>>> Gerd Möllmann <gerd.moellmann@gmail.com> writes:
>>> 
>>> > Gerd Möllmann <gerd.moellmann@gmail.com> writes:
>>> >
>>> >> I think check_comp_unit_relocs should be removed in the branch. What's
>>> >> left of it the branch, checks in master if everything has been
>>> >> put in purespace which should be there. IIUC correctly, of course.
>>> >
>>> > Patch attached.
>>> 
>>> Pushed. Complaints to me please.
>>
>> I'd like at least Andrea to take a look and confirm.
>
> It's 81fc23b5d6a60ca4f3d269ab2c88eb9a850bac4c

Hi Gerd,

looking at the commit now, why do you think 'check_comp_unit_relocs'
should be removed?  

Even if now the situation is simpler 'check_comp_unit_relocs' is still
performing some sanity checks like if lambdas are all been fixed-up and
present in 'comp_u->lambda_gc_guard_h'.

  Andrea

Re: Merging scratch/no-purespace to remove unexec and purespace

[Pip Cet via Emacs development discussions.]

"Helmut Eller" <eller.helmut@gmail.com> writes:

> On Tue, Dec 17 2024, Stefan Kangas wrote:
>
>> Stefan Monnier <monnier@iro.umontreal.ca> writes:
> [...]
>>> That's because the pdump already fails to take advantage of the
>>> purespace (i.e. the GC traces through the purespace like the rest of
>>> the heap).
>>
>> I'll note that the best solution to that is to have a generational GC
>> instead.  Simple, right?
>
> A generational GC is definitely simpler.  Whether it's the "best"
> solution is not so clear: A copying GC, like MPS, still needs to trace
> and copy pure objects whenever the oldest generation is in the condemned
> set.

>  Moving pure objects to a non-moving pool might be better.

I think we should rephrase that without presupposing the existence of
"pure" objects: Hinting to MPS that an object is expected to be
immutable and have a very long lifetime may have advantages, and that's
a potential reason for introducing (and maintaining) a hinting
mechanism.

On the other hand, it's very common for objects to have those properties
without us knowing in advance that they will, so it's important MPS
works well in the absence of such hints.

>> It's not entirely unrelated though: among other things, one reason why
>> merging this would be good is that it would reportedly simplify the work
>> on the igc branch.  And indeed any GC-related work now or in future.
>
> Objects in purespace are immutable and immortal.  That's potentially
> useful information for the GC.

I may be misunderstanding what you mean by "immutable", but the most
important property "pure" objects had was that they only referenced
other pure objects or static objects, so GC didn't need to look at them
(but, IIUC, this optimization never worked in pdumper builds).  This
required us to make such objects read-only (which caused problems) and
immortal.

Immortality was an undesirable, but necessary, side effect of the "pure"
optimization, not a feature.  IMHO, so was immutability, but some people
consider it a feature not to be able to modify certain objects.

For example, on the no-purespace branch, you can execute

(aset (symbol-name nil) 0 ?N)

and rename nil to Nil, which will make the rest of your Emacs session
unusable.

On master, this code should (and did at one point, IIRC) throw a
CHECK_IMPURE exception.  Right now, it segfaults, which demonstrates
that the current purespace code has suffered from some bit rot and
removing purespace will fix some bugs for that reason alone.

So the two major features of pure space are broken right now.  Fixing
that is an option, and it's the only way we'd ever see a fair comparison
of purespace and no-purespace performance, but I hope it's not going to
happen.

So what's left is a weak hint to the GC that this object is likely to
remain reachable, and not to be modified, but for MPS to set up a "pure"
space of such objects based on this hint seems to me to be an
unrealistic expectation.

> Removing purespace also removes that information.

Removing purespace does leave us with very few read-only objects.  We
can consider introducing a useful set of read-only objects (and agree on
what that even means) after purespace is removed.

However, I would be surprised if the lost information about whether an
object used to be pure turns out to be very useful.

> Of course, if the pdumper already throws away this
> information, then purespace just adds useless complexity.

I believe that's the case.  In fact, once we remove purespace, we can
look at improving how pdumper dumps are handled during GC.  Maybe we can
make it so the pdumper dump looks even more like an ordinary MPS segment
in memory, and then we don't need to treat pdumper objects specially.

Pip

Re: Merging scratch/no-purespace to remove unexec and purespace

[Gerd Möllmann]

Andrea Corallo <acorallo@gnu.org> writes:

> Gerd Möllmann <gerd.moellmann@gmail.com> writes:
>
>> Eli Zaretskii <eliz@gnu.org> writes:
>>
>>>> From: Gerd Möllmann <gerd.moellmann@gmail.com>
>>>> Cc: emacs-devel@gnu.org,  Pip Cet <pipcet@protonmail.com>,  Stefan Monnier
>>>>  <monnier@iro.umontreal.ca>
>>>> Date: Tue, 17 Dec 2024 18:56:05 +0100
>>>> 
>>>> Gerd Möllmann <gerd.moellmann@gmail.com> writes:
>>>> 
>>>> > Gerd Möllmann <gerd.moellmann@gmail.com> writes:
>>>> >
>>>> >> I think check_comp_unit_relocs should be removed in the branch. What's
>>>> >> left of it the branch, checks in master if everything has been
>>>> >> put in purespace which should be there. IIUC correctly, of course.
>>>> >
>>>> > Patch attached.
>>>> 
>>>> Pushed. Complaints to me please.
>>>
>>> I'd like at least Andrea to take a look and confirm.
>>
>> It's 81fc23b5d6a60ca4f3d269ab2c88eb9a850bac4c
>
> Hi Gerd,
>
> looking at the commit now, why do you think 'check_comp_unit_relocs'
> should be removed?  
>
> Even if now the situation is simpler 'check_comp_unit_relocs' is still
> performing some sanity checks like if lambdas are all been fixed-up and
> present in 'comp_u->lambda_gc_guard_h'.
>
>   Andrea

Hi Andrea.

The check you mention checks something that I don't see how it could
happen. (With the usual disclaimers, because it's been some time since I
was in that code for igc.)

When we comp--register-lambda the code putting the result of make_subr
into the hash table and putting it in data_relocs (in the branch) is
just a handful of lines apart. I thereforeo thought we could do without.

Re: Merging scratch/no-purespace to remove unexec and purespace

[Pip Cet via Emacs development discussions.]

Gerd Möllmann <gerd.moellmann@gmail.com> writes:

> Andrea Corallo <acorallo@gnu.org> writes:
>
>> Gerd Möllmann <gerd.moellmann@gmail.com> writes:
>>
>>> Eli Zaretskii <eliz@gnu.org> writes:
>>>
>>>>> From: Gerd Möllmann <gerd.moellmann@gmail.com>
>>>>> Cc: emacs-devel@gnu.org,  Pip Cet <pipcet@protonmail.com>,  Stefan Monnier
>>>>>  <monnier@iro.umontreal.ca>
>>>>> Date: Tue, 17 Dec 2024 18:56:05 +0100
>>>>>
>>>>> Gerd Möllmann <gerd.moellmann@gmail.com> writes:
>>>>>
>>>>> > Gerd Möllmann <gerd.moellmann@gmail.com> writes:
>>>>> >
>>>>> >> I think check_comp_unit_relocs should be removed in the branch. What's
>>>>> >> left of it the branch, checks in master if everything has been
>>>>> >> put in purespace which should be there. IIUC correctly, of course.
>>>>> >
>>>>> > Patch attached.
>>>>>
>>>>> Pushed. Complaints to me please.
>>>>
>>>> I'd like at least Andrea to take a look and confirm.
>>>
>>> It's 81fc23b5d6a60ca4f3d269ab2c88eb9a850bac4c
>>
>> Hi Gerd,
>>
>> looking at the commit now, why do you think 'check_comp_unit_relocs'
>> should be removed?
>>
>> Even if now the situation is simpler 'check_comp_unit_relocs' is still
>> performing some sanity checks like if lambdas are all been fixed-up and
>> present in 'comp_u->lambda_gc_guard_h'.
>>
>>   Andrea
>
> Hi Andrea.
>
> The check you mention checks something that I don't see how it could
> happen. (With the usual disclaimers, because it's been some time since I
> was in that code for igc.)

I'm confused, I thought you did hit the assertion?

AFAICT, the problem is simply that comp.el uses the symbol lambda-fixup.

So a relocation for that symbol is emitted.  But no fixup is, because
this isn't an actual lambda, it's merely the symbol.  The debug code
then sees "lambda-fixup", assumes it's a failed fixup, and asserts.

IOW, the old code happened not to run into this problem because
lambda-fixup was pure, and we never applied the sanity checks to the
pure section.

This "fix" appears to work:

diff --git a/lisp/emacs-lisp/comp.el b/lisp/emacs-lisp/comp.el
index dbd14b2740d..5d364e77e66 100644
--- a/lisp/emacs-lisp/comp.el
+++ b/lisp/emacs-lisp/comp.el
@@ -3254,7 +3254,7 @@ comp--finalize-container
                  ;; from the corresponding m-var.
                  collect (if (gethash obj
                                       (comp-ctxt-byte-func-to-func-h comp-ctxt))
-                             'lambda-fixup
+                             (intern (concat "lambda" (make-string 1 ?-) "fixup"))
                            obj))))
 
 (defun comp--finalize-relocs ()

My suggestion is to fix the "sanity check" on the master branch, change
it to apply to pure relocs there, and restore the fixed check on
scratch/no-purespace afterwards.

Pip

Re: Merging scratch/no-purespace to remove unexec and purespace

[Pip Cet via Emacs development discussions.]

Pip Cet <pipcet@protonmail.com> writes:
> IOW, the old code happened not to run into this problem because
> lambda-fixup was pure, and we never applied the sanity checks to the
> pure section.

Just to be clear: the code on master is fine.  I misunderstood it when
modifying it for purespace removal, resulting in my bug which Gerd
discovered and fixed.  The code on no-purespace is also fine now, but
it's Andrea's call whether he wants some of the checking code restored,
and how.

> My suggestion is to fix the "sanity check" on the master branch, change
> it to apply to pure relocs there, and restore the fixed check on
> scratch/no-purespace afterwards.

Please ignore that. My suggestion is to EXTEND the sanity check on the
master branch to cover pure and impure relocs, and restore the EXTENDED
check before merging scratch/no-purespace.

There is no bug to fix on master.

Pip

Re: Merging scratch/no-purespace to remove unexec and purespace

[Gerd Möllmann]

Pip Cet <pipcet@protonmail.com> writes:

> Gerd Möllmann <gerd.moellmann@gmail.com> writes:
>
>> Andrea Corallo <acorallo@gnu.org> writes:
>>
>>> Gerd Möllmann <gerd.moellmann@gmail.com> writes:
>>>
>>>> Eli Zaretskii <eliz@gnu.org> writes:
>>>>
>>>>>> From: Gerd Möllmann <gerd.moellmann@gmail.com>
>>>>>> Cc: emacs-devel@gnu.org,  Pip Cet <pipcet@protonmail.com>,  Stefan Monnier
>>>>>>  <monnier@iro.umontreal.ca>
>>>>>> Date: Tue, 17 Dec 2024 18:56:05 +0100
>>>>>>
>>>>>> Gerd Möllmann <gerd.moellmann@gmail.com> writes:
>>>>>>
>>>>>> > Gerd Möllmann <gerd.moellmann@gmail.com> writes:
>>>>>> >
>>>>>> >> I think check_comp_unit_relocs should be removed in the branch. What's
>>>>>> >> left of it the branch, checks in master if everything has been
>>>>>> >> put in purespace which should be there. IIUC correctly, of course.
>>>>>> >
>>>>>> > Patch attached.
>>>>>>
>>>>>> Pushed. Complaints to me please.
>>>>>
>>>>> I'd like at least Andrea to take a look and confirm.
>>>>
>>>> It's 81fc23b5d6a60ca4f3d269ab2c88eb9a850bac4c
>>>
>>> Hi Gerd,
>>>
>>> looking at the commit now, why do you think 'check_comp_unit_relocs'
>>> should be removed?
>>>
>>> Even if now the situation is simpler 'check_comp_unit_relocs' is still
>>> performing some sanity checks like if lambdas are all been fixed-up and
>>> present in 'comp_u->lambda_gc_guard_h'.
>>>
>>>   Andrea
>>
>> Hi Andrea.
>>
>> The check you mention checks something that I don't see how it could
>> happen. (With the usual disclaimers, because it's been some time since I
>> was in that code for igc.)
>
> I'm confused, I thought you did hit the assertion?

Correct, building with --enable-checking failed. What confuses?

Re: Merging scratch/no-purespace to remove unexec and purespace

[Stefan Monnier]

> Please don't merge this until Emacs 30 is released, whether that be
> before the 1st of February or after.

Why?


        Stefan

Re: Merging scratch/no-purespace to remove unexec and purespace

[Andrea Corallo]

Pip Cet <pipcet@protonmail.com> writes:

> Pip Cet <pipcet@protonmail.com> writes:
>> IOW, the old code happened not to run into this problem because
>> lambda-fixup was pure, and we never applied the sanity checks to the
>> pure section.
>
> Just to be clear: the code on master is fine.  I misunderstood it when
> modifying it for purespace removal, resulting in my bug which Gerd
> discovered and fixed.  The code on no-purespace is also fine now, but
> it's Andrea's call whether he wants some of the checking code restored,
> and how.
>
>> My suggestion is to fix the "sanity check" on the master branch, change
>> it to apply to pure relocs there, and restore the fixed check on
>> scratch/no-purespace afterwards.
>
> Please ignore that. My suggestion is to EXTEND the sanity check on the
> master branch to cover pure and impure relocs, and restore the EXTENDED
> check before merging scratch/no-purespace.
>
> There is no bug to fix on master.

Right your analysis is correct, the new code in the branch just made the
symbol 'fixup-lambda' not compilable.

I restored the check and applied a variant of your fix with a comment
around.  scratch/no-purespace work for me now.

On master I don't think I see what we should do and the motivation for.

Re: Merging scratch/no-purespace to remove unexec and purespace

[Pip Cet via Emacs development discussions.]

"Andrea Corallo" <acorallo@gnu.org> writes:

> Pip Cet <pipcet@protonmail.com> writes:
>
>> Pip Cet <pipcet@protonmail.com> writes:
>>> IOW, the old code happened not to run into this problem because
>>> lambda-fixup was pure, and we never applied the sanity checks to the
>>> pure section.
>>
>> Just to be clear: the code on master is fine.  I misunderstood it when
>> modifying it for purespace removal, resulting in my bug which Gerd
>> discovered and fixed.  The code on no-purespace is also fine now, but
>> it's Andrea's call whether he wants some of the checking code restored,
>> and how.
>>
>>> My suggestion is to fix the "sanity check" on the master branch, change
>>> it to apply to pure relocs there, and restore the fixed check on
>>> scratch/no-purespace afterwards.
>>
>> Please ignore that. My suggestion is to EXTEND the sanity check on the
>> master branch to cover pure and impure relocs, and restore the EXTENDED
>> check before merging scratch/no-purespace.
>>
>> There is no bug to fix on master.
>
> Right your analysis is correct, the new code in the branch just made the
> symbol 'fixup-lambda' not compilable.
>
> I restored the check and applied a variant of your fix with a comment
> around.  scratch/no-purespace work for me now.

Just to summarize this:
There's now a forbidden symbol, --lambda-fixup.  If you use this symbol
in your code and compile the code with nativecomp, that may appear to work,
but loading the resulting object file into another Emacs will crash that
Emacs, if that Emacs was built with checks enabled.

> On master I don't think I see what we should do and the motivation for.

The scratch/no-purespace branch now tests things more rigorously than
the master branch does: master performs three checks on all impure
relocations and a single check on pure ones, but scratch/no-purespace
performs all three checks on all relocations.

That means when we merge scratch/no-purespace, and hit one of the new
assertions, it may be (and was, in Gerd's case) because the test would
have failed on the master branch but was never performed there, and that
would be unrelated to purespace removal.

What I think we should do doesn't really matter, but it seems quite
obvious to me that we should make the code on the master branch perform
all three checks on all relocations, as the code on no-purespace does.

Pip

Re: Merging scratch/no-purespace to remove unexec and purespace

[Andrea Corallo]

Pip Cet <pipcet@protonmail.com> writes:

> "Andrea Corallo" <acorallo@gnu.org> writes:
>
>> Pip Cet <pipcet@protonmail.com> writes:
>>
>>> Pip Cet <pipcet@protonmail.com> writes:
>>>> IOW, the old code happened not to run into this problem because
>>>> lambda-fixup was pure, and we never applied the sanity checks to the
>>>> pure section.
>>>
>>> Just to be clear: the code on master is fine.  I misunderstood it when
>>> modifying it for purespace removal, resulting in my bug which Gerd
>>> discovered and fixed.  The code on no-purespace is also fine now, but
>>> it's Andrea's call whether he wants some of the checking code restored,
>>> and how.
>>>
>>>> My suggestion is to fix the "sanity check" on the master branch, change
>>>> it to apply to pure relocs there, and restore the fixed check on
>>>> scratch/no-purespace afterwards.
>>>
>>> Please ignore that. My suggestion is to EXTEND the sanity check on the
>>> master branch to cover pure and impure relocs, and restore the EXTENDED
>>> check before merging scratch/no-purespace.
>>>
>>> There is no bug to fix on master.
>>
>> Right your analysis is correct, the new code in the branch just made the
>> symbol 'fixup-lambda' not compilable.
>>
>> I restored the check and applied a variant of your fix with a comment
>> around.  scratch/no-purespace work for me now.
>
> Just to summarize this:
> There's now a forbidden symbol, --lambda-fixup.  If you use this symbol
> in your code and compile the code with nativecomp, that may appear to work,
> but loading the resulting object file into another Emacs will crash that
> Emacs, if that Emacs was built with checks enabled.

Correct, maybe we should use 'comp--lambda-fixup' so it's even more
evidently private to the compiler, not sure if would be worth using a
different kind of placeholder object.

>> On master I don't think I see what we should do and the motivation for.
>
> The scratch/no-purespace branch now tests things more rigorously than
> the master branch does: master performs three checks on all impure
> relocations and a single check on pure ones, but scratch/no-purespace
> performs all three checks on all relocations.
>
> That means when we merge scratch/no-purespace, and hit one of the new
> assertions, it may be (and was, in Gerd's case) because the test would
> have failed on the master branch but was never performed there, and that
> would be unrelated to purespace removal.
>
> What I think we should do doesn't really matter, but it seems quite
> obvious to me that we should make the code on the master branch perform
> all three checks on all relocations, as the code on no-purespace does.

master doesn't have forbidden symbols that can't be stored in data_vec
so why should we test for that?

Re: Merging scratch/no-purespace to remove unexec and purespace

[Stefan Kangas]

Pip Cet <pipcet@protonmail.com> writes:

> What I think we should do doesn't really matter, but it seems quite
> obvious to me that we should make the code on the master branch
> perform all three checks on all relocations, as the code on
> no-purespace does.

Maybe.  But won't we get those checks with no additional effort once we
merge no-purespace, and if so, can't it wait until then?

Re: Merging scratch/no-purespace to remove unexec and purespace

[Pip Cet via Emacs development discussions.]

"Stefan Kangas" <stefankangas@gmail.com> writes:

> Pip Cet <pipcet@protonmail.com> writes:
>
>> What I think we should do doesn't really matter, but it seems quite
>> obvious to me that we should make the code on the master branch
>> perform all three checks on all relocations, as the code on
>> no-purespace does.
>
> Maybe.  But won't we get those checks with no additional effort once we
> merge no-purespace,

Yes, we will.  (And the forbidden symbol; even if the forbidden symbol
doesn't cause trouble, which I think it will, it's simply very poor
programming practice to do things that way, particularly since the
crash may happen a long time after the compilation.  But, again, what I
think obviously doesn't matter here.  I'll just remember that
--enable-checking causes false positive crashes and shouldn't be used).

That's a problem, because if we run into problems there, we'll have no
way of knowing whether the problem was present on the pre-merge master
(where we didn't check) or not.  But, again, as it's specific to
--enable-checking, we can simply stop using that.

> and if so, can't it wait until then?

Of course, but changing two things at a time makes debugging harder.
(And IIUC, we won't rename the symbol on master until we merge, so
that's three changes which can cause trouble with the nativecomp code,
all introduced at the same time).

Pip

Re: Merging scratch/no-purespace to remove unexec and purespace

[Stefan Kangas]

Pip Cet <pipcet@protonmail.com> writes:

> "Stefan Kangas" <stefankangas@gmail.com> writes:
>
>> Pip Cet <pipcet@protonmail.com> writes:
>>
>>> What I think we should do doesn't really matter, but it seems quite
>>> obvious to me that we should make the code on the master branch
>>> perform all three checks on all relocations, as the code on
>>> no-purespace does.
>>
>> Maybe.  But won't we get those checks with no additional effort once we
>> merge no-purespace,
>
> Yes, we will.  (And the forbidden symbol; even if the forbidden symbol
> doesn't cause trouble, which I think it will, it's simply very poor
> programming practice to do things that way, particularly since the
> crash may happen a long time after the compilation.  But, again, what I
> think obviously doesn't matter here.  I'll just remember that
> --enable-checking causes false positive crashes and shouldn't be used).

I don't think the existence of one symbol that will crash Emacs in some
situations means that --enable-checking should be completely avoided.
It's still quite useful, and we're fine as long as we avoid using that
one symbol, right?

OTOH and IMHO, it would be preferable if that symbol could not crash
Emacs.  Can we come up with a good way to fix that, while preserving the
check that Andrea wants to keep?

> That's a problem, because if we run into problems there, we'll have no
> way of knowing whether the problem was present on the pre-merge master
> (where we didn't check) or not.  But, again, as it's specific to
> --enable-checking, we can simply stop using that.
>
>> and if so, can't it wait until then?
>
> Of course, but changing two things at a time makes debugging harder.
> (And IIUC, we won't rename the symbol on master until we merge, so
> that's three changes which can cause trouble with the nativecomp code,
> all introduced at the same time).

I still don't think I understand your argument here, sorry.

The scratch/no-purespace branch contains several different changes, all
of which have had to pass through review, testing and verification.

Why is it particularly important to "backport" this change to master, in
advance of the merge, but not any of the other changes on that branch?
What am I missing?