MPS symbols

MPS symbols

[Helmut Eller]

I have trouble to understand how symbols work with MPS.

A Lisp_Object that references a symbol has tag Lisp_Symbol (0) and the
other bits are an offset (in bytes) from lispsym.

How can MPS recognize ambiguous references to symbols?

Re: MPS symbols

[Gerd Möllmann]

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

> I have trouble to understand how symbols work with MPS.
>
> A Lisp_Object that references a symbol has tag Lisp_Symbol (0) and the
> other bits are an offset (in bytes) from lispsym.
>
> How can MPS recognize ambiguous references to symbols?

I hope I got that right (I think I have). Let me cite the scanner
function for simpliciry:

  scan_ambig (mps_ss_t ss, void *start, void *end, void *closure)
  {
    MPS_SCAN_BEGIN (ss)
    {
      for (mps_word_t *p = start; p < (mps_word_t *) end; ++p)
        {
          mps_word_t word = *p;
          mps_word_t tag = word & IGC_TAG_MASK;

          /* If the references in the object being scanned are
             ambiguous then MPS_FIX2() does not update the
             reference (because it can't know if it's a
             genuine reference). The MPS handles an ambiguous
             reference by pinning the block pointed to so that
             it cannot move. */
          mps_addr_t ref = (mps_addr_t) word;
          mps_res_t res = MPS_FIX12 (ss, &ref);
          if (res != MPS_RES_OK)
            return res;

Ambiguous referencess can be either pointers or Lisp_Objects. The above
lines handle the pointer case, and below is the Lisp_Object case, that
is when it looks kuje a Lisp_Object.

          switch (tag)
            {
            case Lisp_Int0:
            case Lisp_Int1:
            case Lisp_Type_Unused0:
              break;

            case Lisp_Symbol:
              {
                ptrdiff_t off = word ^ tag;
                ref = (mps_addr_t) ((char *) lispsym + off);
                res = MPS_FIX12 (ss, &ref);
                if (res != MPS_RES_OK)
                  return res;
              }
              break;

And anove is the symbol case. I.e. the word looks like Lisp_Object of a
symbol, we do the offset thing and rely on MPS_FIX12 to to the right ting.

            default:
              ref = (mps_addr_t) (word ^ tag);
              res = MPS_FIX12 (ss, &ref);
              if (res != MPS_RES_OK)
                return res;
              break;

And the above is basically fhe same as for symbols, only that we don't have
to do the offset dance.
            }
        }
    }
    MPS_SCAN_END (ss);
    return MPS_RES_OK;
  }

Re: MPS symbols

[Helmut Eller]

On Fri, Jun 28 2024, Gerd Möllmann wrote:

> Ambiguous referencess can be either pointers or Lisp_Objects. The above
> lines handle the pointer case, and below is the Lisp_Object case, that
> is when it looks kuje a Lisp_Object.

Ah, it tries both, the "raw" and the "decoded" pointers.  I think I
understand now (until I get confused the next time :-)).

Thanks for the explanation.

Re: MPS symbols

[Gerd Möllmann]

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

> On Fri, Jun 28 2024, Gerd Möllmann wrote:
>
>> Ambiguous referencess can be either pointers or Lisp_Objects. The above
>> lines handle the pointer case, and below is the Lisp_Object case, that
>> is when it looks kuje a Lisp_Object.
>
> Ah, it tries both, the "raw" and the "decoded" pointers.  I think I
> understand now (until I get confused the next time :-)).
>
> Thanks for the explanation.

Nich dafür :-).

BTW, I've added an igc.el to emacs-lisp that contains the igc-stats, and
something new for roots, igc-roots-stats or so. I thought that's useful
enough to have always available.

Re: MPS symbols

[Gerd Möllmann]

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

> Helmut Eller <eller.helmut@gmail.com> writes:
>
>> On Fri, Jun 28 2024, Gerd Möllmann wrote:
>>
>>> Ambiguous referencess can be either pointers or Lisp_Objects. The above
>>> lines handle the pointer case, and below is the Lisp_Object case, that
>>> is when it looks kuje a Lisp_Object.
>>
>> Ah, it tries both, the "raw" and the "decoded" pointers.  I think I
>> understand now (until I get confused the next time :-)).
>>
>> Thanks for the explanation.
>
> Nich dafür :-).
>
> BTW, I've added an igc.el to emacs-lisp that contains the igc-stats, and
> something new for roots, igc-roots-stats or so. I thought that's useful
> enough to have always available.

Forgot something. While looking at the scanner, I wondered if we could
remove the is_mps check from fix_lisp_obj now that we have the dump
in MPS...

Re: MPS symbols

[Gerd Möllmann]

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

> Forgot something. While looking at the scanner, I wondered if we could
> remove the is_mps check from fix_lisp_obj now that we have the dump
> in MPS...

Let's find out, pushed. Works for me :-)

Re: MPS symbols

[Helmut Eller]

On Fri, Jun 28 2024, Gerd Möllmann wrote:

> Gerd Möllmann <gerd.moellmann@gmail.com> writes:
>
>> Forgot something. While looking at the scanner, I wondered if we could
>> remove the is_mps check from fix_lisp_obj now that we have the dump
>> in MPS...
>
> Let's find out, pushed. Works for me :-)

I think this uncovered a bug.  Now this

  emacs -Q -eval '(progn (view-hello-file) (forward-line 7) (dotimes (_ 5)
  (redisplay) (igc--collect) (forward-line)))' -f kill-emacs

causes a SIGSEGV in fix_font,

      case FONT_OBJECT_MAX:
	{
	  struct font *f = (struct font *)v;
	  const Lisp_Object *type = &f->driver->type;
	  IGC_FIX12_OBJ (ss, igc_const_cast (Lisp_Object *, type));
	}

It turns out that f->driver is the variable xfont_driver and it is
actually const and the type field is a builtin symbol.  So it looks like
we can remove fix_font completely.

Re: MPS symbols

[Gerd Möllmann]

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

> On Fri, Jun 28 2024, Gerd Möllmann wrote:
>
>> Gerd Möllmann <gerd.moellmann@gmail.com> writes:
>>
>>> Forgot something. While looking at the scanner, I wondered if we could
>>> remove the is_mps check from fix_lisp_obj now that we have the dump
>>> in MPS...
>>
>> Let's find out, pushed. Works for me :-)
>
> I think this uncovered a bug.  Now this
>
>   emacs -Q -eval '(progn (view-hello-file) (forward-line 7) (dotimes (_ 5)
>   (redisplay) (igc--collect) (forward-line)))' -f kill-emacs
>
> causes a SIGSEGV in fix_font,
>
>       case FONT_OBJECT_MAX:
> 	{
> 	  struct font *f = (struct font *)v;
> 	  const Lisp_Object *type = &f->driver->type;
> 	  IGC_FIX12_OBJ (ss, igc_const_cast (Lisp_Object *, type));
> 	}
>
> It turns out that f->driver is the variable xfont_driver and it is
> actually const and the type field is a builtin symbol.  So it looks like
> we can remove fix_font completely.

Hm, okay. Can you see why is segfaults? I'm wondering because I'd have
thought that we'd crash all over the place if we couldn't handle
built-in symbols...

Re: MPS symbols

[Gerd Möllmann]

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

> Helmut Eller <eller.helmut@gmail.com> writes:
>
>> On Fri, Jun 28 2024, Gerd Möllmann wrote:
>>
>>> Gerd Möllmann <gerd.moellmann@gmail.com> writes:
>>>
>>>> Forgot something. While looking at the scanner, I wondered if we could
>>>> remove the is_mps check from fix_lisp_obj now that we have the dump
>>>> in MPS...
>>>
>>> Let's find out, pushed. Works for me :-)
>>
>> I think this uncovered a bug.  Now this
>>
>>   emacs -Q -eval '(progn (view-hello-file) (forward-line 7) (dotimes (_ 5)
>>   (redisplay) (igc--collect) (forward-line)))' -f kill-emacs
>>
>> causes a SIGSEGV in fix_font,
>>
>>       case FONT_OBJECT_MAX:
>> 	{
>> 	  struct font *f = (struct font *)v;
>> 	  const Lisp_Object *type = &f->driver->type;
>> 	  IGC_FIX12_OBJ (ss, igc_const_cast (Lisp_Object *, type));
>> 	}
>>
>> It turns out that f->driver is the variable xfont_driver and it is
>> actually const and the type field is a builtin symbol.  So it looks like
>> we can remove fix_font completely.
>
> Hm, okay. Can you see why is segfaults? I'm wondering because I'd have
> thought that we'd crash all over the place if we couldn't handle
> built-in symbols...

Please scratch that, I see it (macOS doesn't even have a definition for
struct font_driver...). I'll remove fix_font.

Re: MPS symbols

[Eli Zaretskii]

> From: Gerd Möllmann <gerd.moellmann@gmail.com>
> Cc: emacs-devel@gnu.org
> Date: Fri, 28 Jun 2024 10:37:53 +0200
> 
> BTW, I've added an igc.el to emacs-lisp that contains the igc-stats, and
> something new for roots, igc-roots-stats or so. I thought that's useful
> enough to have always available.

This leads to:

    ELC      emacs-lisp/igc.elc

  In end of data:
  emacs-lisp/igc.el:89:27: Warning: the function `igc-roots' is not known to be defined.

What is 'igc-roots'?

Re: MPS symbols

[Gerd Möllmann]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Gerd Möllmann <gerd.moellmann@gmail.com>
>> Cc: emacs-devel@gnu.org
>> Date: Fri, 28 Jun 2024 10:37:53 +0200
>> 
>> BTW, I've added an igc.el to emacs-lisp that contains the igc-stats, and
>> something new for roots, igc-roots-stats or so. I thought that's useful
>> enough to have always available.
>
> This leads to:
>
>     ELC      emacs-lisp/igc.elc
>
>   In end of data:
>   emacs-lisp/igc.el:89:27: Warning: the function `igc-roots' is not
>   known to be defined.

Should be fixed now.

> What is 'igc-roots'?

That one actually doesn't exist. I intended to make 'r' show root
statistics in igc-stats, but didn't complete that, and left the above
call.

igc--roots is a DEFUN in igc.c returning information about roots. In the
end, the plan is that every root has a label, and all are displayed in
igc-roots-stats, so that one can see what is going on wrt roots over
time.

Re: MPS symbols

[Helmut Eller]

On Fri, Jun 28 2024, Gerd Möllmann wrote:

> igc--roots is a DEFUN in igc.c returning information about roots. In the
> end, the plan is that every root has a label, and all are displayed in
> igc-roots-stats, so that one can see what is going on wrt roots over
> time.

Hm, I see some odd numbers in *igc roots*:

bc-stack                       ambig               1         4194304
pure                           ambig               1        41333328

40 MB pure space :-).  It looks like PURESIZE isn't the number of words
but the number of bytes.  Or so it seems in alloc.c:

EMACS_INT pure[(PURESIZE + sizeof (EMACS_INT) - 1) / sizeof (EMACS_INT)] = {1,};

4 MB bc-stack also seems a lot.  But it's less obviously wrong.

It helps to sometimes look at basic things :-)

Re: MPS symbols

[Gerd Möllmann]

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

> On Fri, Jun 28 2024, Gerd Möllmann wrote:
>
>> igc--roots is a DEFUN in igc.c returning information about roots. In the
>> end, the plan is that every root has a label, and all are displayed in
>> igc-roots-stats, so that one can see what is going on wrt roots over
>> time.
>
> Hm, I see some odd numbers in *igc roots*:
>
> bc-stack                       ambig               1         4194304
> pure                           ambig               1        41333328
>
> 40 MB pure space :-).  It looks like PURESIZE isn't the number of words
> but the number of bytes.  Or so it seems in alloc.c:
>
> EMACS_INT pure[(PURESIZE + sizeof (EMACS_INT) - 1) / sizeof
> (EMACS_INT)] = {1,};

Haven't seen that - mine has no pure space :-).

> 4 MB bc-stack also seems a lot.  But it's less obviously wrong.

It's actually large, at least in principle. Hence my attempt to figure
out a more reasonable end address in scan_bc

  #define BC_STACK_SIZE (512 * 1024 * sizeof (Lisp_Object))

> It helps to sometimes look at basic things :-)

👍 

Re: MPS symbols

[Gerd Möllmann]

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

>> EMACS_INT pure[(PURESIZE + sizeof (EMACS_INT) - 1) / sizeof
>> (EMACS_INT)] = {1,};
>
> Haven't seen that - mine has no pure space :-).

Should be fixed.

Re: MPS symbols

[Helmut Eller]

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

On Fri, Jun 28 2024, Gerd Möllmann wrote:

> Gerd Möllmann <gerd.moellmann@gmail.com> writes:
>
>>> EMACS_INT pure[(PURESIZE + sizeof (EMACS_INT) - 1) / sizeof
>>> (EMACS_INT)] = {1,};
>>
>> Haven't seen that - mine has no pure space :-).
>
> Should be fixed.

Not quite.  The size must be a multiple of the word size.  This seems to
work:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Fir-pure-root-size-again.patch --]
[-- Type: text/x-diff, Size: 1872 bytes --]

From acba908affb92090785fbf77360d5d84ce4b14f9 Mon Sep 17 00:00:00 2001
From: Helmut Eller <eller.helmut@gmail.com>
Date: Fri, 28 Jun 2024 17:32:08 +0200
Subject: [PATCH] Fir pure root size again

* src/puresize.h (root): Specify array bounds.
* src/igc.c (root_create_pure): Use the size of the type.
* src/array.c (root): Simplify.
---
 src/alloc.c    | 2 +-
 src/igc.c      | 4 ++--
 src/puresize.h | 3 ++-
 3 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/src/alloc.c b/src/alloc.c
index 80701ae1b38..31cb86e6d0e 100644
--- a/src/alloc.c
+++ b/src/alloc.c
@@ -394,7 +394,7 @@ #define SPARE_MEMORY (1 << 14)
    space (pure), on some systems.  We have not implemented the
    remapping on more recent systems because this is less important
    nowadays than in the days of small memories and timesharing.  */
-EMACS_INT pure[(PURESIZE + sizeof (EMACS_INT) - 1) / sizeof (EMACS_INT)] = {1,};
+EMACS_INT pure[] = {1,};
 
 #define PUREBEG (char *) pure
 
diff --git a/src/igc.c b/src/igc.c
index aa7c6150aeb..77dd20d64e3 100644
--- a/src/igc.c
+++ b/src/igc.c
@@ -2359,8 +2359,8 @@ root_create_charset_table (struct igc *gc)
 static void
 root_create_pure (struct igc *gc)
 {
-  char *start = (char *) &pure[0];
-  char *end = start + PURESIZE;
+  void *start = &pure;
+  void *end = &pure + 1;
   root_create (gc, start, end, mps_rank_ambig (), scan_pure, NULL, true,
 	       "pure");
 }
diff --git a/src/puresize.h b/src/puresize.h
index 4f15dcb6665..a09e60504a8 100644
--- a/src/puresize.h
+++ b/src/puresize.h
@@ -79,7 +79,8 @@ #define PURESIZE  (BASE_PURESIZE * PURESIZE_RATIO * PURESIZE_CHECKING_RATIO)
 
 extern AVOID pure_write_error (Lisp_Object);
 
-extern EMACS_INT pure[];
+extern EMACS_INT pure[(PURESIZE + sizeof (EMACS_INT) - 1)
+		      / sizeof (EMACS_INT)];
 
 /* The puresize_h_* macros are private to this include file.  */
 
-- 
2.39.2

Re: MPS symbols

[Eli Zaretskii]

> From: Helmut Eller <eller.helmut@gmail.com>
> Cc: Eli Zaretskii <eliz@gnu.org>,  emacs-devel@gnu.org
> Date: Fri, 28 Jun 2024 17:41:42 +0200
> 
> Not quite.  The size must be a multiple of the word size.  This seems to
> work:
> 
> 
> >From acba908affb92090785fbf77360d5d84ce4b14f9 Mon Sep 17 00:00:00 2001
> From: Helmut Eller <eller.helmut@gmail.com>
> Date: Fri, 28 Jun 2024 17:32:08 +0200
> Subject: [PATCH] Fir pure root size again
> 
> * src/puresize.h (root): Specify array bounds.
> * src/igc.c (root_create_pure): Use the size of the type.
> * src/array.c (root): Simplify.
> ---
>  src/alloc.c    | 2 +-
>  src/igc.c      | 4 ++--
>  src/puresize.h | 3 ++-
>  3 files changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/src/alloc.c b/src/alloc.c
> index 80701ae1b38..31cb86e6d0e 100644
> --- a/src/alloc.c
> +++ b/src/alloc.c
> @@ -394,7 +394,7 @@ #define SPARE_MEMORY (1 << 14)
>     space (pure), on some systems.  We have not implemented the
>     remapping on more recent systems because this is less important
>     nowadays than in the days of small memories and timesharing.  */
> -EMACS_INT pure[(PURESIZE + sizeof (EMACS_INT) - 1) / sizeof (EMACS_INT)] = {1,};
> +EMACS_INT pure[] = {1,};
>  
>  #define PUREBEG (char *) pure
>  
> diff --git a/src/igc.c b/src/igc.c
> index aa7c6150aeb..77dd20d64e3 100644
> --- a/src/igc.c
> +++ b/src/igc.c
> @@ -2359,8 +2359,8 @@ root_create_charset_table (struct igc *gc)
>  static void
>  root_create_pure (struct igc *gc)
>  {
> -  char *start = (char *) &pure[0];
> -  char *end = start + PURESIZE;
> +  void *start = &pure;
> +  void *end = &pure + 1;
>    root_create (gc, start, end, mps_rank_ambig (), scan_pure, NULL, true,
>  	       "pure");
>  }
> diff --git a/src/puresize.h b/src/puresize.h
> index 4f15dcb6665..a09e60504a8 100644
> --- a/src/puresize.h
> +++ b/src/puresize.h
> @@ -79,7 +79,8 @@ #define PURESIZE  (BASE_PURESIZE * PURESIZE_RATIO * PURESIZE_CHECKING_RATIO)
>  
>  extern AVOID pure_write_error (Lisp_Object);
>  
> -extern EMACS_INT pure[];
> +extern EMACS_INT pure[(PURESIZE + sizeof (EMACS_INT) - 1)
> +		      / sizeof (EMACS_INT)];

I don't understand why you moved the definition of puresize[] from
alloc.c to puresize.h.  If we need the same definition in two source
files, and you don't want to repeat it more than once, just do the
arithmetics in puresize.h and leave the definition simple, like this:

  EMACS_INT pure[pure_size];

where pure_size is computed in puresize.h.

Having dimension in an extern declaration in a header file is at least
unusual, if not unportable.

Re: MPS symbols

[Helmut Eller]

On Fri, Jun 28 2024, Eli Zaretskii wrote:

>> -extern EMACS_INT pure[];
>> +extern EMACS_INT pure[(PURESIZE + sizeof (EMACS_INT) - 1)
>> +		      / sizeof (EMACS_INT)];
>
> I don't understand why you moved the definition of puresize[] from
> alloc.c to puresize.h.  If we need the same definition in two source
> files, and you don't want to repeat it more than once, just do the
> arithmetics in puresize.h and leave the definition simple, like this:
>
>   EMACS_INT pure[pure_size];
>
> where pure_size is computed in puresize.h.
>
> Having dimension in an extern declaration in a header file is at least
> unusual, if not unportable.

I guess it's a matter of taste.  If the array type includes the length,
then the compiler knows the size and sizeof(pure) works.  That's all.

Re: MPS symbols

[Eli Zaretskii]

> From: Helmut Eller <eller.helmut@gmail.com>
> Cc: gerd.moellmann@gmail.com,  emacs-devel@gnu.org
> Date: Fri, 28 Jun 2024 18:19:23 +0200
> 
> On Fri, Jun 28 2024, Eli Zaretskii wrote:
> 
> >> -extern EMACS_INT pure[];
> >> +extern EMACS_INT pure[(PURESIZE + sizeof (EMACS_INT) - 1)
> >> +		      / sizeof (EMACS_INT)];
> >
> > I don't understand why you moved the definition of puresize[] from
> > alloc.c to puresize.h.  If we need the same definition in two source
> > files, and you don't want to repeat it more than once, just do the
> > arithmetics in puresize.h and leave the definition simple, like this:
> >
> >   EMACS_INT pure[pure_size];
> >
> > where pure_size is computed in puresize.h.
> >
> > Having dimension in an extern declaration in a header file is at least
> > unusual, if not unportable.
> 
> I guess it's a matter of taste.  If the array type includes the length,
> then the compiler knows the size and sizeof(pure) works.  That's all.

The dimension is part of the definition, not of the declaration.  You
rely on the linker to supply the dimension, but that's not how things
are done in C.

Re: MPS symbols

[Gerd Möllmann]

Eli Zaretskii <eliz@gnu.org> writes:

> Having dimension in an extern declaration in a header file is at least
> unusual, if not unportable.

If it's okay for you, I'll leave pushing what's necessary to you.

Re: MPS symbols

[Eli Zaretskii]

> From: Gerd Möllmann <gerd.moellmann@gmail.com>
> Cc: Helmut Eller <eller.helmut@gmail.com>,  emacs-devel@gnu.org
> Date: Fri, 28 Jun 2024 18:20:13 +0200
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > Having dimension in an extern declaration in a header file is at least
> > unusual, if not unportable.
> 
> If it's okay for you, I'll leave pushing what's necessary to you.

Done, please take a look.

Re: MPS symbols

[Gerd Möllmann]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Gerd Möllmann <gerd.moellmann@gmail.com>
>> Cc: Helmut Eller <eller.helmut@gmail.com>,  emacs-devel@gnu.org
>> Date: Fri, 28 Jun 2024 18:20:13 +0200
>> 
>> Eli Zaretskii <eliz@gnu.org> writes:
>> 
>> > Having dimension in an extern declaration in a header file is at least
>> > unusual, if not unportable.
>> 
>> If it's okay for you, I'll leave pushing what's necessary to you.
>
> Done, please take a look.

Looks good, thanks!