MPS: face-cache

MPS: face-cache

[Gerd Möllmann]

FYI: Just found this while randomly debugging

  static uintptr_t
  lface_hash (Lisp_Object *v)
  {
    return (hash_string_case_insensitive (v[LFACE_FAMILY_INDEX])
            ^ hash_string_case_insensitive (v[LFACE_FOUNDRY_INDEX])
            ^ hash_string_case_insensitive (v[LFACE_FOREGROUND_INDEX])
            ^ hash_string_case_insensitive (v[LFACE_BACKGROUND_INDEX])
            ^ XHASH (v[LFACE_WEIGHT_INDEX])
            ^ XHASH (v[LFACE_SLANT_INDEX])
            ^ XHASH (v[LFACE_SWIDTH_INDEX])
            ^ XHASH (v[LFACE_HEIGHT_INDEX]));
  }

XHASH is address dependent, so it can't work. That should be igc_hash
for MPS.

Not today, though. Good night!

Re: MPS: face-cache

[Gerd Möllmann]

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

> FYI: Just found this while randomly debugging
>
>   static uintptr_t
>   lface_hash (Lisp_Object *v)
>   {
>     return (hash_string_case_insensitive (v[LFACE_FAMILY_INDEX])
>             ^ hash_string_case_insensitive (v[LFACE_FOUNDRY_INDEX])
>             ^ hash_string_case_insensitive (v[LFACE_FOREGROUND_INDEX])
>             ^ hash_string_case_insensitive (v[LFACE_BACKGROUND_INDEX])
>             ^ XHASH (v[LFACE_WEIGHT_INDEX])
>             ^ XHASH (v[LFACE_SLANT_INDEX])
>             ^ XHASH (v[LFACE_SWIDTH_INDEX])
>             ^ XHASH (v[LFACE_HEIGHT_INDEX]));
>   }
>
> XHASH is address dependent, so it can't work. That should be igc_hash
> for MPS.
>
> Not today, though. Good night!

Moin moin.

Did that now, but it's still crashing. My impression, which might of
course be completely wrong, is that we are losing faces somehow, i.e.
something is wrong with the cache.

Re: MPS: face-cache

[Gerd Möllmann]

The branch builds now with --enable-checking=all, except that the string
bytes check (HC_CHECK_STRING_BYZES) cannot be done because that relies
on data structures of the old GC.

Re: MPS: face-cache

[Eli Zaretskii]

> From: Gerd Möllmann <gerd.moellmann@gmail.com>
> Cc: Helmut Eller <eller.helmut@gmail.com>,  emacs-devel missing value
>  <emacs-devel@gnu.org>
> Date: Sun, 28 Apr 2024 07:06:45 +0200
> 
> Did that now, but it's still crashing. My impression, which might of
> course be completely wrong, is that we are losing faces somehow, i.e.
> something is wrong with the cache.

Could be.

But there's also FRAME_FONTSET, e.g. for w32:

  #define FRAME_FONTSET(f) ((f)->output_data.w32->fontset)

Are we handling it correctly vis-a-vis GC?  fix_frame doesn't do
anything with it, AFAICT.  Should it?

When Emacs is looking for a font, it consults the fontset.  In one
case, I got a crash with Helmut's recipe, like this:

  Thread 1 received signal SIGSEGV, Segmentation fault.
  parse_str_as_multibyte (
      str=0xc045000 <error: Cannot access memory at address 0xc045000>,
      str@entry=0xaa1a880 "", len=len@entry=211856320,
      nchars=nchars@entry=0x6df89cc, nbytes=nbytes@entry=0x6df89c8)
      at character.c:555
  555               int n = multibyte_length (str, NULL, false, false);
  (gdb) bt
  #0  parse_str_as_multibyte (
      str=0xc045000 <error: Cannot access memory at address 0xc045000>,
      str@entry=0xaa1a880 "", len=len@entry=211856320,
      nchars=nchars@entry=0x6df89cc, nbytes=nbytes@entry=0x6df89c8)
      at character.c:555
  #1  0x003141a1 in font_intern_prop (str=0xaa1a880 "", len=211856320,
      force_symbol=force_symbol@entry=true) at font.c:326
  #2  0x0031d387 in font_find_for_lface (f=f@entry=0xaa1c478,
      attrs=attrs@entry=0x1dc263b0, spec=0xcb8ebed, c=c@entry=21457)
      at lisp.h:751
  #3  0x003e7c01 in fontset_find_font (fontset=0xb41812d, c=c@entry=21457,
      face=face@entry=0x1dc263b0, charset_id=charset_id@entry=42,
      fallback=fallback@entry=false) at lisp.h:751
  #4  0x003e8475 in fontset_font (fontset=fontset@entry=0xb3fc765,
      c=c@entry=21457, face=face@entry=0x1dc263b0, id=42) at lisp.h:751
  #5  0x003e9232 in face_for_char (f=0xaa1c478, face=face@entry=0x1dc263b0,
      c=21457, pos=pos@entry=788, object=0x0) at fontset.c:1031
  #6  0x001a79d4 in FACE_FOR_CHAR (object=<optimized out>, pos=788,
      character=<optimized out>, face=0x1dc263b0, f=<optimized out>)
      at dispextern.h:1959
  #7  get_next_display_element (it=it@entry=0x6dfa6b0) at xdisp.c:8499
  #8  0x001b0d96 in display_line (it=it@entry=0x6dfa6b0,
      cursor_vpos=cursor_vpos@entry=1) at xdisp.c:25298
  #9  0x001b7611 in try_window (window=<optimized out>, window@entry=0xc8007b5,
      pos=..., flags=<optimized out>, flags@entry=1) at xdisp.c:21139
  #10 0x001d9947 in redisplay_window (window=window@entry=0xc8007b5,
      just_this_one_p=just_this_one_p@entry=false) at xdisp.c:20533
  #11 0x001dcb44 in redisplay_window_0 (window=0xc8007b5) at xdisp.c:18018
  #12 0x002f29db in internal_condition_case_1 (
      bfun=bfun@entry=0x1dcb0f <redisplay_window_0>, arg=arg@entry=0xc8007b5,
      handlers=0x1a3a10c3, hfun=hfun@entry=0x186297 <redisplay_window_error>)
      at eval.c:1568
  #13 0x00181eed in redisplay_windows (window=0xc8007b5) at xdisp.c:17987
  #14 0x001c0f5f in redisplay_internal () at xdisp.c:17387
  #15 0x001c24b3 in redisplay () at xdisp.c:16565
  #16 0x002661c8 in read_char (commandflag=<optimized out>, commandflag@entry=1,
      map=<optimized out>, map@entry=0xa820f7b, prev_event=<optimized out>,
      used_mouse_menu=<optimized out>, used_mouse_menu@entry=0x6dff7fb,
      end_time=<optimized out>, end_time@entry=0x0) at keyboard.c:2678
  #17 0x002697fd in read_key_sequence (keybuf=keybuf@entry=0x6dff8d8,
      prompt=prompt@entry=0x0,
      dont_downcase_last=dont_downcase_last@entry=false,
      can_return_switch_frame=<optimized out>,
      can_return_switch_frame@entry=true, fix_current_buffer=<optimized out>,
      fix_current_buffer@entry=true, prevent_redisplay=<optimized out>,
      prevent_redisplay@entry=false, disable_text_conversion_p=false)
      at keyboard.c:10727
  #18 0x0026b38b in command_loop_1 () at lisp.h:1179
  #19 0x002f293d in internal_condition_case (
      bfun=bfun@entry=0x26b1d0 <command_loop_1>, handlers=handlers@entry=0x48,
      hfun=hfun@entry=0x25e516 <cmd_error>) at eval.c:1544
  #20 0x0025482b in command_loop_2 (handlers=0x48) at keyboard.c:1168
  #21 0x002f2857 in internal_catch (tag=tag@entry=0x87a8,
      func=func@entry=0x25480b <command_loop_2>, arg=arg@entry=0x48)
      at eval.c:1224
  #22 0x002547cb in command_loop () at lisp.h:1179
  #23 0x0025e0d1 in recursive_edit_1 () at keyboard.c:754
  #24 0x0025e3c1 in Frecursive_edit () at keyboard.c:837
  #25 0x00497d5f in main (argc=<optimized out>, argv=<optimized out>)
      at emacs.c:2626

Note that the string length in parse_str_as_multibyte is completely
bogus, which led me look into what we do with fontsets, because frame
#4 calls fontset_font.  The default fontset, Vdefault_fontset, is
staticpro'd, but that's all I found.

Re: MPS: face-cache

[Gerd Möllmann]

Eli Zaretskii <eliz@gnu.org> writes:

> But there's also FRAME_FONTSET, e.g. for w32:
>
>   #define FRAME_FONTSET(f) ((f)->output_data.w32->fontset)
>
> Are we handling it correctly vis-a-vis GC?  fix_frame doesn't do
> anything with it, AFAICT.  Should it?
>

Yeah, I think I see something similar here, but I don't understand it.

In my case, it looks like a string which is created by Ffont_xlfd_name
is later garbled. The string is used as the name of a fontset. Fontsets
are PVEC_CHAR_TABLE, which, AFAIU, are treated like normal vectors
during old GC. It's a bit confusing to me... Fix_char_table does that
for the new GC in a similar way.

All the fontsets are held in an array Vfontset_table which is
staticpro'd. An index in that array is the "fontset id" which is passed
around. At least that's my understanding - that we mainly pass these
integer ids around.

On NS for example, I believe it's ns_output that contains a fontset id.

So, in summary, I don't understand how a fontset, i.e. the char
table/vector would not be traces - the staticpro'd array should make
that sure.

> Note that the string length in parse_str_as_multibyte is completely
> bogus, which led me look into what we do with fontsets, because frame
> #4 calls fontset_font.  The default fontset, Vdefault_fontset, is
> staticpro'd, but that's all I found.

Yeah, same here. String is garbage at some point, fontsets are somehow
involved, etc.

Re: MPS: face-cache

[Eli Zaretskii]

> From: Gerd Möllmann <gerd.moellmann@gmail.com>
> Cc: eller.helmut@gmail.com,  emacs-devel@gnu.org
> Date: Sun, 28 Apr 2024 08:28:39 +0200
> 
> In my case, it looks like a string which is created by Ffont_xlfd_name
> is later garbled. The string is used as the name of a fontset. Fontsets
> are PVEC_CHAR_TABLE, which, AFAIU, are treated like normal vectors
> during old GC. It's a bit confusing to me... Fix_char_table does that
> for the new GC in a similar way.

Char-tables have the extras[] array:

  struct Lisp_Char_Table
    {
    [...]
      /* These hold additional data.  It is a vector.  */
      Lisp_Object extras[FLEXIBLE_ARRAY_MEMBER];

Does fix_char_table handle that?

Re: MPS: face-cache

[Gerd Möllmann]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Gerd Möllmann <gerd.moellmann@gmail.com>
>> Cc: eller.helmut@gmail.com,  emacs-devel@gnu.org
>> Date: Sun, 28 Apr 2024 08:28:39 +0200
>> 
>> In my case, it looks like a string which is created by Ffont_xlfd_name
>> is later garbled. The string is used as the name of a fontset. Fontsets
>> are PVEC_CHAR_TABLE, which, AFAIU, are treated like normal vectors
>> during old GC. It's a bit confusing to me... Fix_char_table does that
>> for the new GC in a similar way.
>
> Char-tables have the extras[] array:
>
>   struct Lisp_Char_Table
>     {
>     [...]
>       /* These hold additional data.  It is a vector.  */
>       Lisp_Object extras[FLEXIBLE_ARRAY_MEMBER];
>
> Does fix_char_table handle that?

I think so, but I find it a bit confusing.

If you take a look at mark_char_table (alloc.c:7122), I see that the
function treats char tables as struct Lisp_Vector, i.e. there is an
implicit assumption there that marking v->contents when looking at it as
Lisp_Vector catches bot the contents and extra contents when looking at
it as a Lisp_Char_Table.

WDT?

Re: MPS: face-cache

[Eli Zaretskii]

> From: Gerd Möllmann <gerd.moellmann@gmail.com>
> Cc: eller.helmut@gmail.com,  emacs-devel@gnu.org
> Date: Sun, 28 Apr 2024 08:28:39 +0200
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > But there's also FRAME_FONTSET, e.g. for w32:
> >
> >   #define FRAME_FONTSET(f) ((f)->output_data.w32->fontset)
> >
> > Are we handling it correctly vis-a-vis GC?  fix_frame doesn't do
> > anything with it, AFAICT.  Should it?
> >
> 
> Yeah, I think I see something similar here, but I don't understand it.
> 
> In my case, it looks like a string which is created by Ffont_xlfd_name
> is later garbled. The string is used as the name of a fontset. Fontsets
> are PVEC_CHAR_TABLE, which, AFAIU, are treated like normal vectors
> during old GC. It's a bit confusing to me... Fix_char_table does that
> for the new GC in a similar way.

Maybe it all works because the pseudo-vector's SIZE includes the
extras?

Re: MPS: face-cache

[Gerd Möllmann]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Gerd Möllmann <gerd.moellmann@gmail.com>
>> Cc: eller.helmut@gmail.com,  emacs-devel@gnu.org
>> Date: Sun, 28 Apr 2024 08:28:39 +0200
>> 
>> Eli Zaretskii <eliz@gnu.org> writes:
>> 
>> > But there's also FRAME_FONTSET, e.g. for w32:
>> >
>> >   #define FRAME_FONTSET(f) ((f)->output_data.w32->fontset)
>> >
>> > Are we handling it correctly vis-a-vis GC?  fix_frame doesn't do
>> > anything with it, AFAICT.  Should it?
>> >
>> 
>> Yeah, I think I see something similar here, but I don't understand it.
>> 
>> In my case, it looks like a string which is created by Ffont_xlfd_name
>> is later garbled. The string is used as the name of a fontset. Fontsets
>> are PVEC_CHAR_TABLE, which, AFAIU, are treated like normal vectors
>> during old GC. It's a bit confusing to me... Fix_char_table does that
>> for the new GC in a similar way.
>
> Maybe it all works because the pseudo-vector's SIZE includes the
> extras?

Must be so. In make-fontset, make-vector is used to make a normal vector
which is then at the end made pseudovector. I guess somehow that all
fits together.

Re: MPS: face-cache

[Helmut Eller]

> Did that now, but it's still crashing. My impression, which might of
> course be completely wrong, is that we are losing faces somehow, i.e.
> something is wrong with the cache.

I think the problem is in realize_non_ascii_face.  The xmalloc
should be replaced with igc_make_face.

Re: MPS: face-cache

[Gerd Möllmann]

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

>> Did that now, but it's still crashing. My impression, which might of
>> course be completely wrong, is that we are losing faces somehow, i.e.
>> something is wrong with the cache.
>
> I think the problem is in realize_non_ascii_face.  The xmalloc
> should be replaced with igc_make_face.

Wow! I'm an idiot. All three thumbs up! Did you already try it?

Re: MPS: face-cache

[Helmut Eller]

On Sun, Apr 28 2024, Gerd Möllmann wrote:

> Wow! I'm an idiot. All three thumbs up! Did you already try it?

Yes. My test case is this

set args -Q -eval '(progn (view-hello-file) (while t (redisplay) (igc--collect) (forward-line)))'

It goes now a few lines further before it hits the next issue.

Re: MPS: face-cache

[Gerd Möllmann]

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

> On Sun, Apr 28 2024, Gerd Möllmann wrote:
>
>> Wow! I'm an idiot. All three thumbs up! Did you already try it?
>
> Yes. My test case is this
>
> set args -Q -eval '(progn (view-hello-file) (while t (redisplay) (igc--collect) (forward-line)))'
>
> It goes now a few lines further before it hits the next issue.

Hm, yeah. I've oused that, BTW.

Re: MPS: face-cache

[Gerd Möllmann]

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

> Helmut Eller <eller.helmut@gmail.com> writes:
>
>> On Sun, Apr 28 2024, Gerd Möllmann wrote:
>>
>>> Wow! I'm an idiot. All three thumbs up! Did you already try it?
>>
>> Yes. My test case is this
>>
>> set args -Q -eval '(progn (view-hello-file) (while t (redisplay) (igc--collect) (forward-line)))'
>>
>> It goes now a few lines further before it hits the next issue.
>
> Hm, yeah. I've oused that, BTW.

Ok, it's an assertion in font_has_char for me.

(lldb) p font
(Lisp_Object) 0x000000010320c8b5 (struct Lisp_Vector *) $7 = 0x000000010320c8b0
(lldb) p *(struct igc_header *) ((char *) $7 - 8)
(struct igc_header)  (obj_type = IGC_OBJ_FWD, pvec_type = PVEC_FONT, hash = 174276, nwords = 16)

The header says the object gas been moved (IGC_OBJ_FWD), which should
mean that the reference from where font comes hasn't been traced
(otherwise it wouldn't point to the tombstone.

Re: MPS: face-cache

[Gerd Möllmann]

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

> Gerd Möllmann <gerd.moellmann@gmail.com> writes:
>
>> Helmut Eller <eller.helmut@gmail.com> writes:
>>
>>> On Sun, Apr 28 2024, Gerd Möllmann wrote:
>>>
>>>> Wow! I'm an idiot. All three thumbs up! Did you already try it?
>>>
>>> Yes. My test case is this
>>>
>>> set args -Q -eval '(progn (view-hello-file) (while t (redisplay) (igc--collect) (forward-line)))'
>>>
>>> It goes now a few lines further before it hits the next issue.
>>
>> Hm, yeah. I've oused that, BTW.
>
> Ok, it's an assertion in font_has_char for me.
>
> (lldb) p font
> (Lisp_Object) 0x000000010320c8b5 (struct Lisp_Vector *) $7 = 0x000000010320c8b0
> (lldb) p *(struct igc_header *) ((char *) $7 - 8)
> (struct igc_header)  (obj_type = IGC_OBJ_FWD, pvec_type = PVEC_FONT, hash = 174276, nwords = 16)
>
> The header says the object gas been moved (IGC_OBJ_FWD), which should
> mean that the reference from where font comes hasn't been traced
> (otherwise it wouldn't point to the tombstone.

Someone knows something about this in struct frame?

  /* List of font-drivers available on the frame.  */
  struct font_driver_list *font_driver_list;

Looks to me that the font objecz could come from there. Alas, I don't
remember anything about font drivers. I don't think we had that in the
good old times.

Re: MPS: face-cache

[Eli Zaretskii]

> From: Gerd Möllmann <gerd.moellmann@gmail.com>
> Cc: Eli Zaretskii <eliz@gnu.org>,  emacs-devel missing value
>  <emacs-devel@gnu.org>
> Date: Sun, 28 Apr 2024 09:22:16 +0200
> 
> Someone knows something about this in struct frame?
> 
>   /* List of font-drivers available on the frame.  */
>   struct font_driver_list *font_driver_list;

It's a list of C structs, AFAICT.

> Looks to me that the font objecz could come from there.

Can you tell how that could happen, with pointers to the code?

> Alas, I don't remember anything about font drivers. I don't think we
> had that in the good old times.

Yes, they were added in Emacs 23.

Re: MPS: face-cache

[Gerd Möllmann]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Gerd Möllmann <gerd.moellmann@gmail.com>
>> Cc: Eli Zaretskii <eliz@gnu.org>,  emacs-devel missing value
>>  <emacs-devel@gnu.org>
>> Date: Sun, 28 Apr 2024 09:22:16 +0200
>> 
>> Someone knows something about this in struct frame?
>> 
>>   /* List of font-drivers available on the frame.  */
>>   struct font_driver_list *font_driver_list;
>
> It's a list of C structs, AFAICT.

Yes, looks like an interface, like the redisplay one.

struct font_driver
{
  /* Symbol indicating the type of the font-driver.  */
  Lisp_Object type;

  /* True iff the font's foundry, family, and adstyle names are case
     sensitive.  */
  bool case_sensitive;

  /* Return a cache of font-entities on frame F.  The cache must be a
     cons whose cdr part is the actual cache area.  */
  Lisp_Object (*get_cache) (struct frame *f);
  ...

So, somewhere is a cache, that's interesting. Where is it?


>> Looks to me that the font objecz could come from there.
>
> Can you tell how that could happen, with pointers to the code?

Oh, that's a bit difficult. I went frames up, read the code to
understand where the font object might come from. Then in functions
called fromt here, and landed in something retrieving Lisp_Objects from
these drivers. One can probably find that that by looking where the
frame's driver list is used.

>> Alas, I don't remember anything about font drivers. I don't think we
>> had that in the good old times.
>
> Yes, they were added in Emacs 23.

Ok. Thanks.

Re: MPS: face-cache

[Eli Zaretskii]

> From: Gerd Möllmann <gerd.moellmann@gmail.com>
> Cc: eller.helmut@gmail.com,  emacs-devel@gnu.org
> Date: Sun, 28 Apr 2024 09:38:49 +0200
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> Someone knows something about this in struct frame?
> >> 
> >>   /* List of font-drivers available on the frame.  */
> >>   struct font_driver_list *font_driver_list;
> >
> > It's a list of C structs, AFAICT.
> 
> Yes, looks like an interface, like the redisplay one.
> 
> struct font_driver
> {
>   /* Symbol indicating the type of the font-driver.  */
>   Lisp_Object type;
> 
>   /* True iff the font's foundry, family, and adstyle names are case
>      sensitive.  */
>   bool case_sensitive;
> 
>   /* Return a cache of font-entities on frame F.  The cache must be a
>      cons whose cdr part is the actual cache area.  */
>   Lisp_Object (*get_cache) (struct frame *f);
>   ...
> 
> So, somewhere is a cache, that's interesting. Where is it?

It's in dpyinfo:

  Lisp_Object
  xfont_get_cache (struct frame *f)
  {
    Display_Info *dpyinfo = FRAME_DISPLAY_INFO (f);

    return (dpyinfo->name_list_element);
  }

That name_list_element is the cache, of this form:

   (DRIVER-TYPE NUM-FRAMES FONT-CACHE-DATA ...)

On w32, dpyinfo is just a single global variable, so I protected the
cache with staticpro.  But on platforms that support several displays
in the same session, something else should be done to protect the
cache for each instance of dpyinfo.

> >> Looks to me that the font objecz could come from there.
> >
> > Can you tell how that could happen, with pointers to the code?
> 
> Oh, that's a bit difficult. I went frames up, read the code to
> understand where the font object might come from. Then in functions
> called fromt here, and landed in something retrieving Lisp_Objects from
> these drivers. One can probably find that that by looking where the
> frame's driver list is used.

I looked, but couldn't find it.  All I see is that the driver's
_type_ (a symbol) is used in font objects.  Otherwise, what I see is
that we invoke driver's methods, like 'list', 'match', and 'has_char',
but do not use the driver struct itself in dealing with fonts.

Re: MPS: face-cache

[Gerd Möllmann]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Gerd Möllmann <gerd.moellmann@gmail.com>
>> Cc: eller.helmut@gmail.com,  emacs-devel@gnu.org
>> Date: Sun, 28 Apr 2024 09:38:49 +0200
>> 
>> Eli Zaretskii <eliz@gnu.org> writes:
>> 
>> >> Someone knows something about this in struct frame?
>> >> 
>> >>   /* List of font-drivers available on the frame.  */
>> >>   struct font_driver_list *font_driver_list;
>> >
>> > It's a list of C structs, AFAICT.
>> 
>> Yes, looks like an interface, like the redisplay one.
>> 
>> struct font_driver
>> {
>>   /* Symbol indicating the type of the font-driver.  */
>>   Lisp_Object type;
>> 
>>   /* True iff the font's foundry, family, and adstyle names are case
>>      sensitive.  */
>>   bool case_sensitive;
>> 
>>   /* Return a cache of font-entities on frame F.  The cache must be a
>>      cons whose cdr part is the actual cache area.  */
>>   Lisp_Object (*get_cache) (struct frame *f);
>>   ...
>> 
>> So, somewhere is a cache, that's interesting. Where is it?
>
> It's in dpyinfo:
>
>   Lisp_Object
>   xfont_get_cache (struct frame *f)
>   {
>     Display_Info *dpyinfo = FRAME_DISPLAY_INFO (f);
>
>     return (dpyinfo->name_list_element);
>   }
>
> That name_list_element is the cache, of this form:
>
>    (DRIVER-TYPE NUM-FRAMES FONT-CACHE-DATA ...)
>
> On w32, dpyinfo is just a single global variable, so I protected the
> cache with staticpro.  But on platforms that support several displays
> in the same session, something else should be done to protect the
> cache for each instance of dpyinfo.

Thansk.

>
>> >> Looks to me that the font objecz could come from there.
>> >
>> > Can you tell how that could happen, with pointers to the code?
>> 
>> Oh, that's a bit difficult. I went frames up, read the code to
>> understand where the font object might come from. Then in functions
>> called fromt here, and landed in something retrieving Lisp_Objects from
>> these drivers. One can probably find that that by looking where the
>> frame's driver list is used.
>
> I looked, but couldn't find it.  All I see is that the driver's
> _type_ (a symbol) is used in font objects.  Otherwise, what I see is
> that we invoke driver's methods, like 'list', 'match', and 'has_char',
> but do not use the driver struct itself in dealing with fonts.

Eglot/clangd shows me it's used in font.c, for example

Lisp_Object
font_list_entities (struct frame *f, Lisp_Object spec)
{
  struct font_driver_list *driver_list = f->font_driver_list;

Not sure, but I think that's also one function is landed in when reading...

Re: MPS: face-cache

[Eli Zaretskii]

> From: Gerd Möllmann <gerd.moellmann@gmail.com>
> Cc: eller.helmut@gmail.com,  emacs-devel@gnu.org
> Date: Sun, 28 Apr 2024 10:05:03 +0200
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > I looked, but couldn't find it.  All I see is that the driver's
> > _type_ (a symbol) is used in font objects.  Otherwise, what I see is
> > that we invoke driver's methods, like 'list', 'match', and 'has_char',
> > but do not use the driver struct itself in dealing with fonts.
> 
> Eglot/clangd shows me it's used in font.c, for example
> 
> Lisp_Object
> font_list_entities (struct frame *f, Lisp_Object spec)
> {
>   struct font_driver_list *driver_list = f->font_driver_list;

That function calls the 'list' method of each of the registered font
drivers to produce fonts that match a given spec:

  for (; driver_list; driver_list = driver_list->next)
    if (driver_list->on
	&& (NILP (ftype) || EQ (driver_list->driver->type, ftype)))
      {
	Lisp_Object cache = font_get_cache (f, driver_list->driver);

	ASET (scratch_font_spec, FONT_TYPE_INDEX, driver_list->driver->type);
	val = assoc_no_quit (scratch_font_spec, XCDR (cache));
	if (CONSP (val))
	  val = XCDR (val);
	else
	  {
	    Lisp_Object copy;

	    val = (driver_list->driver->list) (f, scratch_font_spec);
	    /* We put zero_vector in the font-cache to indicate that
	       no fonts matching SPEC were found on the system.
	       Failure to have this indication in the font cache can
	       cause severe performance degradation in some rare
	       cases, see bug#21028.  */
	    if (NILP (val))
	      val = zero_vector;
	    else
	      val = Fvconcat (1, &val);
	    copy = copy_font_spec (scratch_font_spec);
	    ASET (copy, FONT_TYPE_INDEX, driver_list->driver->type);
	    XSETCDR (cache, Fcons (Fcons (copy, val), XCDR (cache)));
	  }

Then it filters the fonts according to some criteria.

So my reading of this is that the 'list' method can create font
entities, but the list of drivers itself is not used for the font
object, only the driver's 'type' (as symbol) is inserted into the
FONT_TYPE_INDEX slot of the font entities.  IOW, the font objects come
from calls to font driver methods, not from the list of drivers.

Re: MPS: face-cache

[Gerd Möllmann]

Eli Zaretskii <eliz@gnu.org> writes:

> IOW, the font objects come from calls to font driver methods, not from
> the list of drivers.

Yeah, that's what I meant, sorry for not being clear.

Re: MPS: face-cache

[Po Lu]

Eli Zaretskii <eliz@gnu.org> writes:

> It's in dpyinfo:
>
>   Lisp_Object
>   xfont_get_cache (struct frame *f)
>   {
>     Display_Info *dpyinfo = FRAME_DISPLAY_INFO (f);
>
>     return (dpyinfo->name_list_element);
>   }

Not always; see ftfont_get_cache in ftfont.c, for a counterexample.

Re: MPS: face-cache

[Eli Zaretskii]

> From: Po Lu <luangruo@yahoo.com>
> Cc: Gerd Möllmann <gerd.moellmann@gmail.com>,
>   eller.helmut@gmail.com,
>   emacs-devel@gnu.org
> Date: Sun, 28 Apr 2024 16:23:32 +0800
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > It's in dpyinfo:
> >
> >   Lisp_Object
> >   xfont_get_cache (struct frame *f)
> >   {
> >     Display_Info *dpyinfo = FRAME_DISPLAY_INFO (f);
> >
> >     return (dpyinfo->name_list_element);
> >   }
> 
> Not always; see ftfont_get_cache in ftfont.c, for a counterexample.

ftfont's freetype_font_cache is already staticpro'd, so we should be
okay there.

Re: MPS: face-cache

[Po Lu]

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

> Eli Zaretskii <eliz@gnu.org> writes:
>
>>> From: Gerd Möllmann <gerd.moellmann@gmail.com>
>>> Cc: Eli Zaretskii <eliz@gnu.org>,  emacs-devel missing value
>>>  <emacs-devel@gnu.org>
>>> Date: Sun, 28 Apr 2024 09:22:16 +0200
>>> 
>>> Someone knows something about this in struct frame?
>>> 
>>>   /* List of font-drivers available on the frame.  */
>>>   struct font_driver_list *font_driver_list;
>>
>> It's a list of C structs, AFAICT.
>
> Yes, looks like an interface, like the redisplay one.
>
> struct font_driver
> {
>   /* Symbol indicating the type of the font-driver.  */
>   Lisp_Object type;
>
>   /* True iff the font's foundry, family, and adstyle names are case
>      sensitive.  */
>   bool case_sensitive;
>
>   /* Return a cache of font-entities on frame F.  The cache must be a
>      cons whose cdr part is the actual cache area.  */
>   Lisp_Object (*get_cache) (struct frame *f);
>   ...
>
> So, somewhere is a cache, that's interesting. Where is it?

It's defined by the font backend, generally to either an alist within
the display connection info or a staticpro'd variable specific to the
font driver.

Re: MPS: face-cache

[Gerd Möllmann]

Po Lu <luangruo@yahoo.com> writes:

> Gerd Möllmann <gerd.moellmann@gmail.com> writes:
>
>> Eli Zaretskii <eliz@gnu.org> writes:
>>
>>>> From: Gerd Möllmann <gerd.moellmann@gmail.com>
>>>> Cc: Eli Zaretskii <eliz@gnu.org>,  emacs-devel missing value
>>>>  <emacs-devel@gnu.org>
>>>> Date: Sun, 28 Apr 2024 09:22:16 +0200
>>>> 
>>>> Someone knows something about this in struct frame?
>>>> 
>>>>   /* List of font-drivers available on the frame.  */
>>>>   struct font_driver_list *font_driver_list;
>>>
>>> It's a list of C structs, AFAICT.
>>
>> Yes, looks like an interface, like the redisplay one.
>>
>> struct font_driver
>> {
>>   /* Symbol indicating the type of the font-driver.  */
>>   Lisp_Object type;
>>
>>   /* True iff the font's foundry, family, and adstyle names are case
>>      sensitive.  */
>>   bool case_sensitive;
>>
>>   /* Return a cache of font-entities on frame F.  The cache must be a
>>      cons whose cdr part is the actual cache area.  */
>>   Lisp_Object (*get_cache) (struct frame *f);
>>   ...
>>
>> So, somewhere is a cache, that's interesting. Where is it?
>
> It's defined by the font backend, generally to either an alist within
> the display connection info or a staticpro'd variable specific to the
> font driver.

Thansk. I think I found the cache for NS

/* Initialized in ns_initialize_display_info ().  */
struct ns_display_info
{
  /* Chain of all ns_display_info structures.  */
  struct ns_display_info *next;

  /* The generic display parameters corresponding to this NS display.  */
  struct terminal *terminal;

  /* This is a cons cell of the form (NAME . FONT-LIST-CACHE).  */
  Lisp_Object name_list_element;

Why clangd also shows me somethign in macfont, I still have to find out.

Almost there... :-)

Re: MPS: face-cache

[Gerd Möllmann]

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

> /* Initialized in ns_initialize_display_info ().  */
> struct ns_display_info
> {
>   /* Chain of all ns_display_info structures.  */
>   struct ns_display_info *next;
>
>   /* The generic display parameters corresponding to this NS display.  */
>   struct terminal *terminal;
>
>   /* This is a cons cell of the form (NAME . FONT-LIST-CACHE).  */
>   Lisp_Object name_list_element;
>
> Why clangd also shows me somethign in macfont, I still have to find
> out.

It seems to be the same for macfont. I mean... there are certain
complexity requirements, after all ... 🤷 

Re: MPS: face-cache

[Po Lu]

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

> Why clangd also shows me somethign in macfont, I still have to find out.

Emacs doesn't build nsfont on Mac OS, which is actually the font driver
for the GNUstep builds.  It appears that macfont_family_cache is the
cache enabled on Mac OS.

Re: MPS: face-cache

[Gerd Möllmann]

Po Lu <luangruo@yahoo.com> writes:

> Gerd Möllmann <gerd.moellmann@gmail.com> writes:
>
>> Why clangd also shows me somethign in macfont, I still have to find out.
>
> Emacs doesn't build nsfont on Mac OS, which is actually the font driver
> for the GNUstep builds.  It appears that macfont_family_cache is the
> cache enabled on Mac OS.

First! :-)

Re: MPS: face-cache

[Gerd Möllmann]

Po Lu <luangruo@yahoo.com> writes:

> Gerd Möllmann <gerd.moellmann@gmail.com> writes:
>
>> Why clangd also shows me somethign in macfont, I still have to find out.
>
> Emacs doesn't build nsfont on Mac OS, which is actually the font driver
> for the GNUstep builds.  It appears that macfont_family_cache is the
> cache enabled on Mac OS.

So, Helmut's test case seems to work for me now, and pushed.

The conditional compilation in igc.c might be broken, though, don't
know. I'll leave that for the experts (tm).

Re: MPS: face-cache

[Gerd Möllmann]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Gerd Möllmann <gerd.moellmann@gmail.com>
>> Cc: Eli Zaretskii <eliz@gnu.org>,  emacs-devel missing value
>>  <emacs-devel@gnu.org>
>> Date: Sun, 28 Apr 2024 09:22:16 +0200
>> 
>> Someone knows something about this in struct frame?
>> 
>>   /* List of font-drivers available on the frame.  */
>>   struct font_driver_list *font_driver_list;
>
> It's a list of C structs, AFAICT.
>
>> Looks to me that the font objecz could come from there.
>
> Can you tell how that could happen, with pointers to the code?
>
>> Alas, I don't remember anything about font drivers. I don't think we
>> had that in the good old times.
>
> Yes, they were added in Emacs 23.

I could find the one(s)? for macOS by searching where
register_font_driver is called.

Which shows me macfont_driver and nsfont_driver on my MB. Hope I can
understand that code :-/.

Anyway - the drivers on macOS seem to be static vars, and the type
symbol is a DEFSYM, so there's nothing to there, I'd say, as long as
everyone uses DEFSYM for their drivers.

Re: MPS: face-cache

[Eli Zaretskii]

> From: Gerd Möllmann <gerd.moellmann@gmail.com>
> Cc: eller.helmut@gmail.com,  emacs-devel@gnu.org
> Date: Sun, 28 Apr 2024 09:55:13 +0200
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> From: Gerd Möllmann <gerd.moellmann@gmail.com>
> >> Cc: Eli Zaretskii <eliz@gnu.org>,  emacs-devel missing value
> >>  <emacs-devel@gnu.org>
> >> Date: Sun, 28 Apr 2024 09:22:16 +0200
> >> 
> >> Someone knows something about this in struct frame?
> >> 
> >>   /* List of font-drivers available on the frame.  */
> >>   struct font_driver_list *font_driver_list;
> >
> > It's a list of C structs, AFAICT.
> >
> >> Looks to me that the font objecz could come from there.
> >
> > Can you tell how that could happen, with pointers to the code?
> >
> >> Alas, I don't remember anything about font drivers. I don't think we
> >> had that in the good old times.
> >
> > Yes, they were added in Emacs 23.
> 
> I could find the one(s)? for macOS by searching where
> register_font_driver is called.
> 
> Which shows me macfont_driver and nsfont_driver on my MB. Hope I can
> understand that code :-/.

I think macOS only supports its native drivers (but I'm not sure).
Look for calls to register_font_driver.

Re: MPS: face-cache

[Helmut Eller]

> Looks to me that the font objecz could come from there.

In font.h is this comment
   FONT-ENTITY

	Pseudo vector (length FONT_ENTITY_MAX) of fully instantiated
	font properties that a font-driver returns upon a request of
	FONT-SPEC.

	Note: Only the method `list' and `match' of a font-driver can
	create this object, and it should never be modified by Lisp.

Is the "should never be modified" part a problem for the GC?

Re: MPS: face-cache

[Gerd Möllmann]

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

>> Looks to me that the font objecz could come from there.
>
> In font.h is this comment
>    FONT-ENTITY
>
> 	Pseudo vector (length FONT_ENTITY_MAX) of fully instantiated
> 	font properties that a font-driver returns upon a request of
> 	FONT-SPEC.
>
> 	Note: Only the method `list' and `match' of a font-driver can
> 	create this object, and it should never be modified by Lisp.
>
> Is the "should never be modified" part a problem for the GC?

Don't know, but it says "modified by Lisp", which I'd interpret as
meaning users should not tinker with them.

Re: MPS: face-cache

[Helmut Eller]

> Someone knows something about this in struct frame?

I have autocmp_chars in the backtrace.  So I looked at struct
composition.  That has a Lisp_Object key and a void* font field.  Can we
allocate this struct with igc_xzalloc_ambig or does this need more work?

Re: MPS: face-cache

[Gerd Möllmann]

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

>> Someone knows something about this in struct frame?
>
> I have autocmp_chars in the backtrace.  So I looked at struct
> composition.  That has a Lisp_Object key and a void* font field.  Can we
> allocate this struct with igc_xzalloc_ambig or does this need more work?

If it's now allocated with malloc, I think using igc_xzalloc_ambig is
exactly the right thing.

Re: MPS: face-cache

[Helmut Eller]

>> I have autocmp_chars in the backtrace.  So I looked at struct
>> composition.  That has a Lisp_Object key and a void* font field.  Can we
>> allocate this struct with igc_xzalloc_ambig or does this need more work?
>
> If it's now allocated with malloc, I think using igc_xzalloc_ambig is
> exactly the right thing.

Yes, it is allocated with xmalloc. But switching to igc_xzalloc_ambig
doesn't fix anything.

Re: MPS: face-cache

[Gerd Möllmann]

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

>>> I have autocmp_chars in the backtrace.  So I looked at struct
>>> composition.  That has a Lisp_Object key and a void* font field.  Can we
>>> allocate this struct with igc_xzalloc_ambig or does this need more work?
>>
>> If it's now allocated with malloc, I think using igc_xzalloc_ambig is
>> exactly the right thing.
>
> Yes, it is allocated with xmalloc. But switching to igc_xzalloc_ambig
> doesn't fix anything.

Hm.

(BTW - forgot to say - igc_xfree instead of xfree too, but that
probably has nothing to do with it.)

Is that the same test case as before? I don't seems to get that.

Anyway. Eli can probably tell us how struct composition is used. If it
has to do with character composition, I think Eli once told that is
also something "new" (for me).

In what way does it fail? Can we look at the void * and check what it
is? p is_mps(x) for example and p *(struct igc_header *)
client_to_base(x), maybe?

Re: MPS: face-cache

[Helmut Eller]

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

On Sun, Apr 28 2024, Gerd Möllmann wrote:

> Is that the same test case as before? I don't seems to get that.

This one:
(progn (view-hello-file) (forward-line 30) (dotimes (_ 5) (redisplay)
(igc--collect) (forward-line))

the one you just fixed.  I just thought that your fix is only for macOS
but it works for me too.  With minor modifications:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: x.diff --]
[-- Type: text/x-diff, Size: 558 bytes --]

diff --git a/src/igc.c b/src/igc.c
index 518ad47e5a8..97f55f0fe1a 100644
--- a/src/igc.c
+++ b/src/igc.c
@@ -1332,14 +1332,8 @@ fix_frame (mps_ss_t ss, struct frame *f)
 	if (*font_ptr)
 	  IGC_FIX12_RAW (ss, font_ptr);
 
-# ifdef HAVE_NS
-	font_ptr = &f->output_data.ns->font;
-	if (*font_ptr)
-	  IGC_FIX12_RAW (ss, font_ptr);
-
-	struct ns_display_info *d = f->output_data.ns->display_info;
-	IGC_FIX12_OBJ (ss, &d->name_list_element);
-# endif
+	Lisp_Object *nle = &FRAME_DISPLAY_INFO(f)->name_list_element;
+	IGC_FIX12_OBJ (ss, nle);
       }
 #endif
 

Re: MPS: face-cache

[Gerd Möllmann]

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

> On Sun, Apr 28 2024, Gerd Möllmann wrote:
>
>> Is that the same test case as before? I don't seems to get that.
>
> This one:
> (progn (view-hello-file) (forward-line 30) (dotimes (_ 5) (redisplay)
> (igc--collect) (forward-line))
>
> the one you just fixed.  I just thought that your fix is only for macOS
> but it works for me too.  With minor modifications:

Uff, I healing by Handauflegen :-).

If you want I can push that change, with the slight modification that I
have a font in the output_data, too.

Eerie :-)

Re: MPS: face-cache

[Helmut Eller]

On Sun, Apr 28 2024, Gerd Möllmann wrote:

> If you want I can push that change, with the slight modification that I
> have a font in the output_data, too.
>
> Eerie :-)

You mean that f->output_data.ns->font is a different font than one
returned by FRAME_FONT?

Wathever, you will know the right fix.

Re: MPS: face-cache

[Gerd Möllmann]

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

> On Sun, Apr 28 2024, Gerd Möllmann wrote:
>
>> If you want I can push that change, with the slight modification that I
>> have a font in the output_data, too.
>>
>> Eerie :-)
>
> You mean that f->output_data.ns->font is a different font than one
> returned by FRAME_FONT?

Oh, you're right! I thought FRAME_FONT would return a frame member.

Pushed.

Re: MPS: face-cache

[Eli Zaretskii]

> From: Helmut Eller <eller.helmut@gmail.com>
> Cc: Eli Zaretskii <eliz@gnu.org>,  emacs-devel missing value
>  <emacs-devel@gnu.org>
> Date: Sun, 28 Apr 2024 11:26:38 +0200
> 
> --- a/src/igc.c
> +++ b/src/igc.c
> @@ -1332,14 +1332,8 @@ fix_frame (mps_ss_t ss, struct frame *f)
>  	if (*font_ptr)
>  	  IGC_FIX12_RAW (ss, font_ptr);
>  
> -# ifdef HAVE_NS
> -	font_ptr = &f->output_data.ns->font;
> -	if (*font_ptr)
> -	  IGC_FIX12_RAW (ss, font_ptr);
> -
> -	struct ns_display_info *d = f->output_data.ns->display_info;
> -	IGC_FIX12_OBJ (ss, &d->name_list_element);
> -# endif
> +	Lisp_Object *nle = &FRAME_DISPLAY_INFO(f)->name_list_element;
> +	IGC_FIX12_OBJ (ss, nle);

In the w32 build, name_list_element is staticpro'd (because there's
only one FRAME_DISPLAY_INFO, and it's in a global variable), so I
guess the above is not needed on MS-Windows?

Re: MPS: face-cache

[Gerd Möllmann]

Eli Zaretskii <eliz@gnu.org> writes:

 > +	Lisp_Object *nle = &FRAME_DISPLAY_INFO(f)->name_list_element;
>> +	IGC_FIX12_OBJ (ss, nle);
>
> In the w32 build, name_list_element is staticpro'd (because there's
> only one FRAME_DISPLAY_INFO, and it's in a global variable), so I
> guess the above is not needed on MS-Windows?

Yes, but also doesn't do harm.

Re: MPS: face-cache

[Eli Zaretskii]

> From: Gerd Möllmann <gerd.moellmann@gmail.com>
> Cc: Eli Zaretskii <eliz@gnu.org>,  emacs-devel missing value
>  <emacs-devel@gnu.org>
> Date: Sun, 28 Apr 2024 11:14:59 +0200
> 
> Anyway. Eli can probably tell us how struct composition is used. If it
> has to do with character composition, I think Eli once told that is
> also something "new" (for me).

Yes, it's used for character compositions.

But I don't see the 'font' field of 'struct composition' used
anywhere.  I only see it being set to NULL and not accessed anywhere
else.  The font we use for composed characters is obtained "by other
means", via the lispy LGSTRING object.

As for 'key', it is indeed used to look up known compositions.  Each
composition, once it's computed, is cached in a hash table, to avoid
computing it again (because that can be expensive):

  /* Check if the same composition has already been registered or not
     by consulting composition_hash_table.  The key for this table is
     COMPONENTS (converted to a vector COMPONENTS-VEC) or, if it is
     nil, vector of characters in the composition range.  */
  if (FIXNUMP (components))
    key = make_vector (1, components);
  else if (STRINGP (components) || CONSP (components))
    key = Fvconcat (1, &components);
  else if (VECTORP (components))
    key = components;
  else if (NILP (components))
    {
      key = make_uninit_vector (nchars);
      if (STRINGP (string))
	for (ptrdiff_t i = 0; i < nchars; i++)
	  {
	    ch = fetch_string_char_advance (string, &charpos, &bytepos);
	    ASET (key, i, make_fixnum (ch));
	  }
      else
	for (ptrdiff_t i = 0; i < nchars; i++)
	  {
	    ch = fetch_char_advance (&charpos, &bytepos);
	    ASET (key, i, make_fixnum (ch));
	  }
    }
  else
    goto invalid_composition;

  hash_hash_t hash_code;
  hash_index = hash_lookup_get_hash (hash_table, key, &hash_code);
  if (hash_index >= 0)
    {
      /* We have already registered the same composition.  Change PROP
	 from Form-A above to Form-B while replacing COMPONENTS with
	 COMPONENTS-VEC stored in the hash table.  We can directly
	 modify the cons cell of PROP because it is not shared.  */
      key = HASH_KEY (hash_table, hash_index);
      id = HASH_VALUE (hash_table, hash_index);
      XSETCAR (prop, id);
      XSETCDR (prop, Fcons (make_fixnum (nchars), Fcons (key, XCDR (prop))));
      return XFIXNUM (id);
    }

Re: MPS: face-cache

[Gerd Möllmann]

Eli Zaretskii <eliz@gnu.org> writes:

>   hash_index = hash_lookup_get_hash (hash_table, key, &hash_code);

And hash_table above is composition_hash_table which is staticpro'd. So
I think that works OOTB, Nice!

Re: MPS: face-cache

[Gerd Möllmann]

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

> Eli Zaretskii <eliz@gnu.org> writes:
>
>>   hash_index = hash_lookup_get_hash (hash_table, key, &hash_code);
>
> And hash_table above is composition_hash_table which is staticpro'd. So
> I think that works OOTB, Nice!

I take that back. The reference to key in struct composition is of
course a separate reference from what's in the hash table, so it should
either be traced, and be in an ambiguous root so that it can't move.

Re: MPS: face-cache

[Helmut Eller]

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

> I take that back. The reference to key in struct composition is of
> course a separate reference from what's in the hash table, so it should
> either be traced, and be in an ambiguous root so that it can't move.

If it wasn't clear before, the composition stuff didn't trigger any test
case to fail.  I know, because I tried the change below.

I general, how do we deal with such cases where one could fix something
"preventively" because it could potentially cause problems later or do
we do let it sit there until an actual test case shows up?



[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: c.diff --]
[-- Type: text/x-diff, Size: 780 bytes --]

diff --git a/src/composite.c b/src/composite.c
index f6b4cb55a57..e3c4c162122 100644
--- a/src/composite.c
+++ b/src/composite.c
@@ -35,7 +35,9 @@
 #include "frame.h"
 #include "dispextern.h"
 #include "termhooks.h"
-
+#ifdef HAVE_MPS
+#include "igc.h"
+#endif
 
 /* Emacs uses special text property `composition' to support character
    composition.  A sequence of characters that have the same (i.e. eq)
@@ -318,7 +320,12 @@ get_composition_id (ptrdiff_t charpos, ptrdiff_t bytepos, ptrdiff_t nchars,
     memory_full (SIZE_MAX);
 
   /* Register the composition in composition_table.  */
+#ifdef HAS_MPC
+  /* FIXME: maybe trace exactly? */
+  cmp = igc_xzalloc_ambig (sizeof *cmp);
+#else
   cmp = xmalloc (sizeof *cmp);
+#endif
 
   cmp->method = method;
   cmp->key = key;

Re: MPS: face-cache

[Eli Zaretskii]

> From: Helmut Eller <eller.helmut@gmail.com>
> Cc: Eli Zaretskii <eliz@gnu.org>,  emacs-devel@gnu.org
> Date: Sun, 28 Apr 2024 14:59:39 +0200
> 
> I general, how do we deal with such cases where one could fix something
> "preventively" because it could potentially cause problems later or do
> we do let it sit there until an actual test case shows up?

Ideally, a test case in test/src/composition-tests.el that triggers
the problem would be nice.

Re: MPS: face-cache

[Gerd Möllmann]

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

>> I take that back. The reference to key in struct composition is of
>> course a separate reference from what's in the hash table, so it should
>> either be traced, and be in an ambiguous root so that it can't move.
>
> If it wasn't clear before, the composition stuff didn't trigger any test
> case to fail.  I know, because I tried the change below.
>
> I general, how do we deal with such cases where one could fix something
> "preventively" because it could potentially cause problems later or do
> we do let it sit there until an actual test case shows up?

I can't give a good answer to that.

My personal approach is sometimes that in areas which I'm almost sure to
never remember existing, I try to get that out of the way, so that I
don't have to remember. Or I do that "correctly" because I think it's
very likely to fall on my feet soon. Other times, I prioritize making
progress fast, and I don't give a sh*t. And so on.

But I'm fine with both approaches. A FIXME suffices.

Re: MPS: face-cache

[Gerd Möllmann]

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

> But I'm fine with both approaches. A FIXME suffices.

So, I've pushed it. Thanks Helmut!

(And a wishlist entry for piem-am :-). A mode in which it doesn't try to
commit, because Emacs commit hook doesn't allow empty commit messages.)

Re: MPS: face-cache

[Helmut Eller]

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

> But I'm fine with both approaches. A FIXME suffices.

Here are some more small fixes.  I don't have test cases for them.  The
mere fact that a structure has a field of type Lisp_Object should be
argument enough to write the code to trace it.  Having tests would of
course be better.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Weak-refs-don-t-need-finalization.patch --]
[-- Type: text/x-diff, Size: 962 bytes --]

From 376151cfa7d203677a1919e1830a4cee6d7fdc79 Mon Sep 17 00:00:00 2001
From: Helmut Eller <eller.helmut@gmail.com>
Date: Mon, 29 Apr 2024 08:25:29 +0200
Subject: [PATCH] Weak refs don't need finalization

* src/igc.c (Figc_make_weak_ref): Don't call maybe_finalize.
---
 src/igc.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/src/igc.c b/src/igc.c
index 9882a08208e..3b2d1452082 100644
--- a/src/igc.c
+++ b/src/igc.c
@@ -2808,9 +2808,8 @@ DEFUN ("igc-make-weak-ref", Figc_make_weak_ref, Sigc_make_weak_ref, 1, 1, 0,
 {
   const enum pvec_type type = PVEC_WEAK_REF;
   struct Lisp_Weak_Ref *wref = alloc (sizeof *wref, IGC_OBJ_WEAK, type);
-  int nwords_lisp = VECSIZE(struct Lisp_Weak_Ref);
+  int nwords_lisp = VECSIZE (struct Lisp_Weak_Ref);
   XSETPVECTYPESIZE (wref, type, nwords_lisp, 0);
-  maybe_finalize (wref, type);
   wref->ref = target;
   Lisp_Object obj = make_lisp_ptr (wref, Lisp_Vectorlike);
   return obj;
-- 
2.39.2


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: 0001-Next-attempt-at-fix_font.patch --]
[-- Type: text/x-diff, Size: 1799 bytes --]

From dda69f1af496437067a5140c5f9c413aabfd112b Mon Sep 17 00:00:00 2001
From: Helmut Eller <eller.helmut@gmail.com>
Date: Mon, 29 Apr 2024 08:49:40 +0200
Subject: [PATCH] Next attempt at fix_font

* src/igc.c (fix_font): This time, dispatch on the length.
  (fix_vector): Call it.
---
 src/igc.c | 30 ++++++++++++++++++++++++++++--
 1 file changed, 28 insertions(+), 2 deletions(-)

diff --git a/src/igc.c b/src/igc.c
index 3b2d1452082..37866f7be8a 100644
--- a/src/igc.c
+++ b/src/igc.c
@@ -1622,6 +1622,31 @@ fix_obarray (mps_ss_t ss, struct Lisp_Obarray *o)
 }
 #endif
 
+static mps_res_t
+fix_font (mps_ss_t ss, struct Lisp_Vector *v)
+{
+  MPS_SCAN_BEGIN (ss)
+  {
+    IGC_FIX_CALL_FN (ss, struct Lisp_Vector, v, fix_vectorlike);
+    /* See font.h for the magic numbers. */
+    switch (v->header.size & PSEUDOVECTOR_SIZE_MASK)
+      {
+      case FONT_SPEC_MAX:
+      case FONT_ENTITY_MAX:
+	break;
+      case FONT_OBJECT_MAX:
+	struct font *f = (struct font *)v;
+	Lisp_Object const *type = &f->driver->type;
+	IGC_FIX12_OBJ (ss, (Lisp_Object *)type);
+	break;
+      default:
+	emacs_abort ();
+      }
+  }
+  MPS_SCAN_END (ss);
+  return MPS_RES_OK;
+}
+
 /* Note that there is a small window after committing a vectorlike
    allocation where the object is zeroed, and so the vector header is
    also zero.  This doesn't have an adverse effect. */
@@ -1725,9 +1750,10 @@ fix_vector (mps_ss_t ss, struct Lisp_Vector *v)
 #endif
 	break;
 
-	/* NB: PVEC_FONT objects come in different varieties having different
-	   sizes. The only part they have in common are the Lisp_Object members. */
       case PVEC_FONT:
+	IGC_FIX_CALL_FN (ss, struct Lisp_Vector, v, fix_font);
+	break;
+
       case PVEC_NORMAL_VECTOR:
       case PVEC_SYMBOL_WITH_POS:
       case PVEC_PROCESS:
-- 
2.39.2


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #4: 0001-Trace-glyph-matrices-in-windows.patch --]
[-- Type: text/x-diff, Size: 2007 bytes --]

From 206dbf10225c8a23c54d2d33ad6c16b9c9f04709 Mon Sep 17 00:00:00 2001
From: Helmut Eller <eller.helmut@gmail.com>
Date: Mon, 29 Apr 2024 09:03:16 +0200
Subject: [PATCH] Trace glyph matrices in windows

* src/igc.c (fix_glyph_matrix): New.
  (fix_window): Call it.
---
 src/igc.c | 34 ++++++++++++++++++++++++++++++++--
 1 file changed, 32 insertions(+), 2 deletions(-)

diff --git a/src/igc.c b/src/igc.c
index 37866f7be8a..b26b19d6c53 100644
--- a/src/igc.c
+++ b/src/igc.c
@@ -1309,6 +1309,34 @@ fix_buffer (mps_ss_t ss, struct buffer *b)
   return MPS_RES_OK;
 }
 
+static mps_res_t
+fix_glyph_matrix (mps_ss_t ss, struct glyph_matrix *matrix)
+{
+  MPS_SCAN_BEGIN (ss)
+  {
+    struct glyph_row *row = matrix->rows;
+    struct glyph_row *end = row + matrix->nrows;
+
+    for (; row < end; ++row)
+      if (row->enabled_p)
+	{
+	  for (int area = LEFT_MARGIN_AREA; area < LAST_AREA; ++area)
+	    {
+	      struct glyph *glyph = row->glyphs[area];
+	      struct glyph *end_glyph = glyph + row->used[area];
+	      for (; glyph < end_glyph; ++glyph)
+		{
+		  Lisp_Object *obj_ptr = &glyph->object;
+		  if (STRINGP (*obj_ptr))
+		    IGC_FIX12_OBJ (ss, obj_ptr);
+		}
+	    }
+	}
+  }
+  MPS_SCAN_END (ss);
+  return MPS_RES_OK;
+}
+
 static mps_res_t
 fix_frame (mps_ss_t ss, struct frame *f)
 {
@@ -1335,7 +1363,6 @@ fix_frame (mps_ss_t ss, struct frame *f)
 	IGC_FIX12_OBJ (ss, nle);
       }
 #endif
-
   }
   MPS_SCAN_END (ss);
   return MPS_RES_OK;
@@ -1346,8 +1373,11 @@ fix_window (mps_ss_t ss, struct window *w)
 {
   MPS_SCAN_BEGIN (ss)
   {
-    /* FIXME: matrices... */
     IGC_FIX_CALL_FN (ss, struct Lisp_Vector, w, fix_vectorlike);
+    if (w->current_matrix)
+      IGC_FIX_CALL (ss, fix_glyph_matrix (ss, w->current_matrix));
+    if (w->desired_matrix)
+      IGC_FIX_CALL (ss, fix_glyph_matrix (ss, w->desired_matrix));
 
     /* FIXME: window.h syas the following two are "marked specially", so
        they are not seen by fix_vectorlike. That's of course a no-go
-- 
2.39.2

Re: MPS: face-cache

[Gerd Möllmann]

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

>> But I'm fine with both approaches. A FIXME suffices.
>
> Here are some more small fixes.  I don't have test cases for them.  The
> mere fact that a structure has a field of type Lisp_Object should be
> argument enough to write the code to trace it.

Right.

Should be in now.

Could you please check that everything is okay? Should be, just to
reassure me that I did it right using Magit.

Re: MPS: face-cache

[Helmut Eller]

> Could you please check that everything is okay? Should be, just to
> reassure me that I did it right using Magit.

LGTM

Re: MPS: face-cache

[Gerd Möllmann]

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

>> Could you please check that everything is okay? Should be, just to
>> reassure me that I did it right using Magit.
>
> LGTM

Thanks

Re: MPS: face-cache

[Eli Zaretskii]

> From: Helmut Eller <eller.helmut@gmail.com>
> Cc: Eli Zaretskii <eliz@gnu.org>,  emacs-devel@gnu.org
> Date: Mon, 29 Apr 2024 10:03:17 +0200
> 
> > Could you please check that everything is okay? Should be, just to
> > reassure me that I did it right using Magit.
> 
> LGTM

Are you both using clang or something?  The change didn't compiled for
me with GCC 9.2, because the case in a switch needed braces:

  igc.c: In function 'fix_font':
  igc.c:1671:2: error: a label can only be part of a statement and a declaration is not a statement
   1671 |  struct font *f = (struct font *)v;
	|  ^~~~~~
  igc.c:1672:2: error: expected expression before 'Lisp_Object'
   1672 |  Lisp_Object const *type = &f->driver->type;
	|  ^~~~~~~~~~~

Re: MPS: face-cache

[Gerd Möllmann]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Helmut Eller <eller.helmut@gmail.com>
>> Cc: Eli Zaretskii <eliz@gnu.org>,  emacs-devel@gnu.org
>> Date: Mon, 29 Apr 2024 10:03:17 +0200
>> 
>> > Could you please check that everything is okay? Should be, just to
>> > reassure me that I did it right using Magit.
>> 
>> LGTM
>
> Are you both using clang or something?  The change didn't compiled for
> me with GCC 9.2, because the case in a switch needed braces:
>
>   igc.c: In function 'fix_font':
>   igc.c:1671:2: error: a label can only be part of a statement and a declaration is not a statement
>    1671 |  struct font *f = (struct font *)v;
> 	|  ^~~~~~
>   igc.c:1672:2: error: expected expression before 'Lisp_Object'
>    1672 |  Lisp_Object const *type = &f->driver->type;
> 	|  ^~~~~~~~~~~

I didn't compile, because I'm debugging ATM.

Re: MPS: face-cache

[Helmut Eller]

> Are you both using clang or something?  The change didn't compiled for
> me with GCC 9.2, because the case in a switch needed braces:

I use GCC (Debian 12.2.0-14) 12.2.0

Should I add some compiler flags?  This is the output of make V=1:

gcc -c  -Demacs  -I. -I. -I../lib -I../lib   -isystem
/usr/include/gtk-3.0 -isystem /usr/include/pango-1.0 -isystem
/usr/include/glib-2.0 -isystem
/usr/lib/x86_64-linux-gnu/glib-2.0/include -isystem
/usr/include/harfbuzz -isystem /usr/include/freetype2 -isystem
/usr/include/libpng16 -isystem /usr/include/libmount -isystem
/usr/include/blkid -isystem /usr/include/fribidi -isystem
/usr/include/cairo -isystem /usr/include/pixman-1 -isystem
/usr/include/gdk-pixbuf-2.0 -isystem /usr/include/x86_64-linux-gnu
-isystem /usr/include/webp -isystem /usr/include/gio-unix-2.0 -isystem
/usr/include/atk-1.0 -isystem /usr/include/at-spi2-atk/2.0 -isystem
/usr/include/at-spi-2.0 -isystem /usr/include/dbus-1.0 -isystem
/usr/lib/x86_64-linux-gnu/dbus-1.0/include -pthread      -isystem
/usr/include/libpng16  -isystem /usr/include/libxml2   -isystem
/usr/include/dbus-1.0 -isystem
/usr/lib/x86_64-linux-gnu/dbus-1.0/include       -isystem
/usr/include/webp    -isystem /usr/include/glib-2.0 -isystem
/usr/lib/x86_64-linux-gnu/glib-2.0/include -pthread -isystem
/usr/include/libmount -isystem /usr/include/blkid  -isystem
/usr/include/glib-2.0 -isystem
/usr/lib/x86_64-linux-gnu/glib-2.0/include  -isystem
/usr/include/freetype2 -isystem /usr/include/libpng16  -isystem
/usr/include/freetype2 -isystem /usr/include/libpng16  -isystem
/usr/include/harfbuzz -isystem /usr/include/freetype2 -isystem
/usr/include/libpng16 -isystem /usr/include/glib-2.0 -isystem
/usr/lib/x86_64-linux-gnu/glib-2.0/include    -MMD -MF deps/igc.d -MP
-isystem /usr/include/p11-kit-1   -isystem /usr/include/cairo -isystem
/usr/include/glib-2.0 -isystem
/usr/lib/x86_64-linux-gnu/glib-2.0/include -isystem
/usr/include/pixman-1 -isystem /usr/include/freetype2 -isystem
/usr/include/libpng16             -Wall -Warith-conversion -Wdate-time
-Wdisabled-optimization -Wdouble-promotion -Wduplicated-cond -Wextra
-Wformat-signedness -Winit-self -Winvalid-pch -Wlogical-op
-Wmissing-declarations -Wmissing-include-dirs -Wmissing-prototypes
-Wnested-externs -Wnull-dereference -Wold-style-definition -Wopenmp-simd
-Wpacked -Wpointer-arith -Wstrict-prototypes -Wsuggest-attribute=format
-Wsuggest-attribute=noreturn -Wsuggest-final-methods
-Wsuggest-final-types -Wtrampolines -Wuninitialized -Wunknown-pragmas
-Wunused-macros -Wvariadic-macros -Wvector-operation-performance
-Wwrite-strings -Warray-bounds=2 -Wattribute-alias=2 -Wformat=2
-Wformat-truncation=2 -Wimplicit-fallthrough=5 -Wshift-overflow=2
-Wuse-after-free=3 -Wvla-larger-than=4031 -Wredundant-decls
-Wno-missing-field-initializers -Wno-override-init -Wno-sign-compare
-Wno-type-limits -Wno-unused-parameter -Wno-format-nonliteral
-Wno-bidi-chars -g -O0 -I/scratch/emacs/mps-install/include
-L/scratch/emacs/mps-install/lib  igc.c

Re: MPS: face-cache

[Eli Zaretskii]

> From: Helmut Eller <eller.helmut@gmail.com>
> Cc: gerd.moellmann@gmail.com,  emacs-devel@gnu.org
> Date: Mon, 29 Apr 2024 10:54:57 +0200
> 
> > Are you both using clang or something?  The change didn't compiled for
> > me with GCC 9.2, because the case in a switch needed braces:
> 
> I use GCC (Debian 12.2.0-14) 12.2.0
> 
> Should I add some compiler flags?  This is the output of make V=1:

I don't know.  Maybe GCC 12 lifts that restriction or something.  I
clearly remember that if a case includes declarations, it must be in
braces.

This is an error, not a warning, so warning switches shouldn't matter.

Re: MPS: face-cache

[Gerd Möllmann]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Helmut Eller <eller.helmut@gmail.com>
>> Cc: gerd.moellmann@gmail.com,  emacs-devel@gnu.org
>> Date: Mon, 29 Apr 2024 10:54:57 +0200
>> 
>> > Are you both using clang or something?  The change didn't compiled for
>> > me with GCC 9.2, because the case in a switch needed braces:
>> 
>> I use GCC (Debian 12.2.0-14) 12.2.0
>> 
>> Should I add some compiler flags?  This is the output of make V=1:
>
> I don't know.  Maybe GCC 12 lifts that restriction or something.  I
> clearly remember that if a case includes declarations, it must be in
> braces.
>
> This is an error, not a warning, so warning switches shouldn't matter.

Clang said it's C23.

Re: MPS: face-cache

[Eli Zaretskii]

> From: Gerd Möllmann <gerd.moellmann@gmail.com>
> Cc: Helmut Eller <eller.helmut@gmail.com>,  emacs-devel@gnu.org
> Date: Mon, 29 Apr 2024 13:18:47 +0200
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> From: Helmut Eller <eller.helmut@gmail.com>
> >> Cc: gerd.moellmann@gmail.com,  emacs-devel@gnu.org
> >> Date: Mon, 29 Apr 2024 10:54:57 +0200
> >> 
> >> > Are you both using clang or something?  The change didn't compiled for
> >> > me with GCC 9.2, because the case in a switch needed braces:
> >> 
> >> I use GCC (Debian 12.2.0-14) 12.2.0
> >> 
> >> Should I add some compiler flags?  This is the output of make V=1:
> >
> > I don't know.  Maybe GCC 12 lifts that restriction or something.  I
> > clearly remember that if a case includes declarations, it must be in
> > braces.
> >
> > This is an error, not a warning, so warning switches shouldn't matter.
> 
> Clang said it's C23.

If only C23 allows this, then we shouldn't use that because we don't
compile at that STDC level.

Re: MPS: face-cache

[Helmut Eller]

> I don't know.  Maybe GCC 12 lifts that restriction or something.  I
> clearly remember that if a case includes declarations, it must be in
> braces.

Here doc[*] for  GCC 12 says:

 GNU dialect of ISO C17. This is the default for C code.

https://gcc.gnu.org/onlinedocs/gcc-12.1.0/gcc/C-Dialect-Options.html#C-Dialect-Options

Re: MPS: face-cache

[Eli Zaretskii]

> From: Helmut Eller <eller.helmut@gmail.com>
> Cc: gerd.moellmann@gmail.com,  emacs-devel@gnu.org
> Date: Mon, 29 Apr 2024 15:07:35 +0200
> 
> > I don't know.  Maybe GCC 12 lifts that restriction or something.  I
> > clearly remember that if a case includes declarations, it must be in
> > braces.
> 
> Here doc[*] for  GCC 12 says:
> 
>  GNU dialect of ISO C17. This is the default for C code.
> 
> https://gcc.gnu.org/onlinedocs/gcc-12.1.0/gcc/C-Dialect-Options.html#C-Dialect-Options

Strange, the default for GCC 9.2 is also gnu17.

Re: MPS: face-cache

[Helmut Eller]

> Strange, the default for GCC 9.2 is also gnu17.

I don't even get any warnings for igc.c with --std=gnu99.  With -std=c17
there are so many errors it doesn't even parse through lisp.h.

MPS uses: -std=c89 -pedantic  :-)

Re: MPS: face-cache

[Eli Zaretskii]

> From: Helmut Eller <eller.helmut@gmail.com>
> Cc: gerd.moellmann@gmail.com,  emacs-devel@gnu.org
> Date: Mon, 29 Apr 2024 15:38:57 +0200
> 
> > Strange, the default for GCC 9.2 is also gnu17.
> 
> I don't even get any warnings for igc.c with --std=gnu99.  With -std=c17
> there are so many errors it doesn't even parse through lisp.h.

The -std=SOMETHING makes no sense in any real-world program, because
many system header files will omit useful declarations and prototypes
when they see that.

> MPS uses: -std=c89 -pedantic  :-)

And it caused me trouble when building it for MinGW, since 'alloca'
was not declared.  I needed to switch to -std=gnu89 instead.

Re: MPS: face-cache

[Helmut Eller]

On Mon, Apr 29 2024, Eli Zaretskii wrote:

>> Here doc[*] for  GCC 12 says:
>> 
>>  GNU dialect of ISO C17. This is the default for C code.
>> 
>> https://gcc.gnu.org/onlinedocs/gcc-12.1.0/gcc/C-Dialect-Options.html#C-Dialect-Options
>
> Strange, the default for GCC 9.2 is also gnu17.

They changed the definition of gnu17 between GCC 10 and 11, compare:
https://gcc.gnu.org/onlinedocs/gcc-10.5.0/gcc/Mixed-Declarations.html
https://gcc.gnu.org/onlinedocs/gcc-11.4.0/gcc/Mixed-Labels-and-Declarations.html

Good thing that I still have gcc-10 installed.  Those kind of things
wouldn't happen with --std=c17.

Re: MPS: face-cache

[Eli Zaretskii]

> From: Helmut Eller <eller.helmut@gmail.com>
> Cc: gerd.moellmann@gmail.com,  emacs-devel@gnu.org
> Date: Mon, 29 Apr 2024 17:19:32 +0200
> 
> 
> On Mon, Apr 29 2024, Eli Zaretskii wrote:
> 
> >> Here doc[*] for  GCC 12 says:
> >> 
> >>  GNU dialect of ISO C17. This is the default for C code.
> >> 
> >> https://gcc.gnu.org/onlinedocs/gcc-12.1.0/gcc/C-Dialect-Options.html#C-Dialect-Options
> >
> > Strange, the default for GCC 9.2 is also gnu17.
> 
> They changed the definition of gnu17 between GCC 10 and 11, compare:
> https://gcc.gnu.org/onlinedocs/gcc-10.5.0/gcc/Mixed-Declarations.html
> https://gcc.gnu.org/onlinedocs/gcc-11.4.0/gcc/Mixed-Labels-and-Declarations.html

I trust the compiler more than I trust the documentation:

  cpp -dM nul | fgrep STDC
  #define __STDC_HOSTED__ 1
  #define __STDC_UTF_16__ 1
  #define __STDC_VERSION__ 201710L  <<<<<<<<<<<<<<<<<<<<<<
  #define __GNUC_STDC_INLINE__ 1
  #define __STDC_UTF_32__ 1
  #define __STDC__ 1

Doesn't that mean the default is c17?

In any case, the GCC Info manual I have, which is for GCC 9.2.0, says:

     'gnu17'
     'gnu18'
          GNU dialect of ISO C17.  This is the default for C code.

Re: MPS: face-cache

[Eli Zaretskii]

> Date: Mon, 29 Apr 2024 18:32:02 +0300
> From: Eli Zaretskii <eliz@gnu.org>
> Cc: gerd.moellmann@gmail.com, emacs-devel@gnu.org
> 
> > They changed the definition of gnu17 between GCC 10 and 11, compare:
> > https://gcc.gnu.org/onlinedocs/gcc-10.5.0/gcc/Mixed-Declarations.html
> > https://gcc.gnu.org/onlinedocs/gcc-11.4.0/gcc/Mixed-Labels-and-Declarations.html
> 
> I trust the compiler more than I trust the documentation:
> 
>   cpp -dM nul | fgrep STDC
>   #define __STDC_HOSTED__ 1
>   #define __STDC_UTF_16__ 1
>   #define __STDC_VERSION__ 201710L  <<<<<<<<<<<<<<<<<<<<<<
>   #define __GNUC_STDC_INLINE__ 1
>   #define __STDC_UTF_32__ 1
>   #define __STDC__ 1
> 
> Doesn't that mean the default is c17?
> 
> In any case, the GCC Info manual I have, which is for GCC 9.2.0, says:
> 
>      'gnu17'
>      'gnu18'
>           GNU dialect of ISO C17.  This is the default for C code.

Of course, if the meaning of gnu17 was changed, all of the above
doesn't matter anyway.  And we in Emacs are nowhere near those late
standards anyway, because we still support very old versions of GCC.

Re: MPS: face-cache

[Eli Zaretskii]

> From: Helmut Eller <eller.helmut@gmail.com>
> Cc: Eli Zaretskii <eliz@gnu.org>,  emacs-devel@gnu.org
> Date: Mon, 29 Apr 2024 09:15:00 +0200
> 
> +static mps_res_t
> +fix_glyph_matrix (mps_ss_t ss, struct glyph_matrix *matrix)
> +{
> +  MPS_SCAN_BEGIN (ss)
> +  {
> +    struct glyph_row *row = matrix->rows;
> +    struct glyph_row *end = row + matrix->nrows;
> +
> +    for (; row < end; ++row)
> +      if (row->enabled_p)
> +	{
> +	  for (int area = LEFT_MARGIN_AREA; area < LAST_AREA; ++area)
> +	    {
> +	      struct glyph *glyph = row->glyphs[area];
> +	      struct glyph *end_glyph = glyph + row->used[area];
> +	      for (; glyph < end_glyph; ++glyph)
> +		{
> +		  Lisp_Object *obj_ptr = &glyph->object;
> +		  if (STRINGP (*obj_ptr))
> +		    IGC_FIX12_OBJ (ss, obj_ptr);
> +		}
> +	    }
> +	}
> +  }
> +  MPS_SCAN_END (ss);
> +  return MPS_RES_OK;
> +}

A glyph matrix also has a pointer to the buffer displayed by the
window.  Should we do something about that pointer?

Re: MPS: face-cache

[Gerd Möllmann]

Eli Zaretskii <eliz@gnu.org> writes:

> A glyph matrix also has a pointer to the buffer displayed by the
> window.  Should we do something about that pointer?

Yes!

Re: MPS: face-cache

[Po Lu]

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

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

Labels cannot refer to declarations in all but the newest revisions of
ISO C, only statements.

Re: MPS: face-cache

[Gerd Möllmann]

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

>> But I'm fine with both approaches. A FIXME suffices.
>
> Here are some more small fixes.  I don't have test cases for them.  The
> mere fact that a structure has a field of type Lisp_Object should be
> argument enough to write the code to trace it.  Having tests would of
> course be better.

Hallo Helmut,

i wonder - now that we trace glyph matrices, could we get rid of the
ambig roots created in dispnew.c? like

#ifdef HAVE_MPS
		igc_xnrealloc_ambig (row->glyphs[LEFT_MARGIN_AREA],
			       dim.width, sizeof (struct glyph));
#else
		xnrealloc (row->glyphs[LEFT_MARGIN_AREA],
			   dim.width, sizeof (struct glyph));
#endif

Re: MPS: face-cache

[Helmut Eller]

On Mon, Apr 29 2024, Gerd Möllmann wrote:

> Hallo Helmut,

Gudn Abend,

> i wonder - now that we trace glyph matrices, could we get rid of the
> ambig roots created in dispnew.c? like
>
> #ifdef HAVE_MPS
> 		igc_xnrealloc_ambig (row->glyphs[LEFT_MARGIN_AREA],
> 			       dim.width, sizeof (struct glyph));
> #else
> 		xnrealloc (row->glyphs[LEFT_MARGIN_AREA],
> 			   dim.width, sizeof (struct glyph));
> #endif

You mean like reverting back to xnrealloc?  That's a good way to crash
immediately at startup :-)

There also are glyph matrices in struct frame that aren't traced yet,
but adding code to fix_frame didn't help.  Even struct tty_menu_state
has glyph matrices; though probably not used in GUI mode.  Either way, a
useful test case.

Re: MPS: face-cache

[Gerd Möllmann]

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

> On Mon, Apr 29 2024, Gerd Möllmann wrote:
>
>> Hallo Helmut,
>
> Gudn Abend,

Einen Guten

>
>> i wonder - now that we trace glyph matrices, could we get rid of the
>> ambig roots created in dispnew.c? like
>>
>> #ifdef HAVE_MPS
>> 		igc_xnrealloc_ambig (row->glyphs[LEFT_MARGIN_AREA],
>> 			       dim.width, sizeof (struct glyph));
>> #else
>> 		xnrealloc (row->glyphs[LEFT_MARGIN_AREA],
>> 			   dim.width, sizeof (struct glyph));
>> #endif
>
> You mean like reverting back to xnrealloc?  That's a good way to crash
> immediately at startup :-)
>
> There also are glyph matrices in struct frame that aren't traced yet,
> but adding code to fix_frame didn't help.  Even struct tty_menu_state
> has glyph matrices; though probably not used in GUI mode.  Either way, a
> useful test case.

Maybe the frame matrix is actually responsible for the crashes.

Windows on tty frames, don't manage their own memory but use parts of a
frame matrix which the frame malloc's. The reason for that is that the
"drawing" code for ttys does optimizations for the frame/tty at once.
Theoretically the output for one window could reuse parts of another
window. That was, BTW the way pre-21 Emacs did things also for GUI
frames.

With proportional fonts etc. such a frame-based redisplay makes no sense,
so I made windows on GUI Frames manage their own memory and so on, which
also made is possible to update single windows and not all windows on a
frame. And so on.

On startup, I believe we have initially a tty frame (invisble). 

Re: MPS: face-cache

[Helmut Eller]

> Maybe the frame matrix is actually responsible for the crashes.

Actually the problem was that I forgot to revert the corresponding
igc_xfree call.  I only looked at the code and without debugger.
Stupidly inefficient.  With the correct xfree in place nothing more than
usual crashes.  So I guess it works for the glyph matrices in windows.
For glyph matrices in frames it's a bit different.  Those have a glyph
pool which must be traced too.  Are nrows, ncolumns in glyph_pool of any
relevance, or can we just loop over the nglyphs there?

Re: MPS: face-cache

[Gerd Möllmann]

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

>> Maybe the frame matrix is actually responsible for the crashes.
>
> Actually the problem was that I forgot to revert the corresponding
> igc_xfree call.  I only looked at the code and without debugger.
> Stupidly inefficient.  

Maybe it was simply too late in the evening, at least for me it was :-).

> With the correct xfree in place nothing more than usual crashes. So I
> guess it works for the glyph matrices in windows. For glyph matrices
> in frames it's a bit different. Those have a glyph pool which must be
> traced too. Are nrows, ncolumns in glyph_pool of any relevance, or can
> we just loop over the nglyphs there?

I think it would suffice if we only trace the windows mactrices + check
that the glyph pool of the frame is not made a root, because it need not
be a root.

Reason is that the window matrices on tty frames are sub-allocated from
the pool in such a way that all matrices taken together form the frame
matrix. So, when we trace window matrices, all windows taken together
trace the frame matrices.

WDYT Eli?

MPS: staticpro everything

[Helmut Eller]

I will go through all the files and staticpro all globals that are
missing.  I'll go through the files in alphabetical order, skipping over
those files that aren't compiled on Debian.  You see a good reason not
to do that?

Re: MPS: staticpro everything

[Gerd Möllmann]

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

> I will go through all the files and staticpro all globals that are
> missing.  I'll go through the files in alphabetical order, skipping over
> those files that aren't compiled on Debian.  You see a good reason not
> to do that?

I don't see one. Thanks for doing this!

Re: MPS: staticpro everything

[Helmut Eller]

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

On Tue, Apr 30 2024, Gerd Möllmann wrote:

>> I will go through all the files and staticpro all globals that are
>> missing.  I'll go through the files in alphabetical order, skipping over
>> those files that aren't compiled on Debian.  You see a good reason not
>> to do that?
>
> I don't see one. Thanks for doing this!

Today I came as far as frame.c.  For the first few, I hadn't thought
about the #ifdef HAVE_MPS, so I went back and added it.  Now there are
almost twice as many patches :(.  I thought it's better to send
something now instead of everything at the end to get feedback.

I also have collected some notes so far:


[-- Attachment #2: globals.org --]
[-- Type: text/plain, Size: 644 bytes --]

* Tricky cases

These probably need special tracing code:

bidi.c: bidi_cache
buffer.c: overlay_heads overlay_tails
dispnew.c: frame_matrix_frame
fns.c: hash_table_user_tests
font.c: font_driver_list

* Allocate ambigiously

charset.c: temp_charset_work

* Probably don't need to be roots, but double-check

buffer.c: buffer_local_flags
dispnew.c: row_entry_pool row_table old_lines new_lines

* Weird

emacs.c: empty_unibyte_string empty_multibyte_string staticpro'ed in alloc.c
frame.c:
  + selected_frame staticpro'ed in init_frame_once
  + last_nonminibuf_frame registered in init_frame_once_for_pdumper

* Skipped files

cygw32.c
dosfns.c

[-- Attachment #3: Type: text/plain, Size: 24 bytes --]


And here the patches:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #4: 0001-Staticpro-Vcharset_non_preferred_head.patch --]
[-- Type: text/x-diff, Size: 717 bytes --]

From 879d75032b97b5690c794d44f972a00113666d5d Mon Sep 17 00:00:00 2001
From: Helmut Eller <eller.helmut@gmail.com>
Date: Tue, 30 Apr 2024 10:48:39 +0200
Subject: [PATCH] Staticpro Vcharset_non_preferred_head

* src/charset (syms_of_charset):
---
 src/charset.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/src/charset.c b/src/charset.c
index 0acf2bde702..6fa085a7183 100644
--- a/src/charset.c
+++ b/src/charset.c
@@ -2373,6 +2373,9 @@ syms_of_charset (void)
   staticpro (&Vcharset_ordered_list);
   Vcharset_ordered_list = Qnil;
 
+  staticpro (&Vcharset_non_preferred_head);
+  Vcharset_non_preferred_head = Qnil;
+
   staticpro (&Viso_2022_charset_list);
   Viso_2022_charset_list = Qnil;
 
-- 
2.39.2


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #5: 0001-Add-missing-ifdef-HAVE_MPS.patch --]
[-- Type: text/x-diff, Size: 732 bytes --]

From 833ef9da200f1bbff623cd1ed943e6a2ef0e4097 Mon Sep 17 00:00:00 2001
From: Helmut Eller <eller.helmut@gmail.com>
Date: Tue, 30 Apr 2024 20:33:30 +0200
Subject: [PATCH] Add missing #ifdef HAVE_MPS

* src/charset.c (syms_of_charset):
---
 src/charset.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/src/charset.c b/src/charset.c
index 6fa085a7183..6635afe938d 100644
--- a/src/charset.c
+++ b/src/charset.c
@@ -2373,8 +2373,10 @@ syms_of_charset (void)
   staticpro (&Vcharset_ordered_list);
   Vcharset_ordered_list = Qnil;
 
+#ifdef HAVE_MPS
   staticpro (&Vcharset_non_preferred_head);
   Vcharset_non_preferred_head = Qnil;
+#endif
 
   staticpro (&Viso_2022_charset_list);
   Viso_2022_charset_list = Qnil;
-- 
2.39.2


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #6: 0001-Staticpro-coding_categories.patch --]
[-- Type: text/x-diff, Size: 1042 bytes --]

From 44f686b800b7b0dc1998de82e437f8f8168f200e Mon Sep 17 00:00:00 2001
From: Helmut Eller <eller.helmut@gmail.com>
Date: Tue, 30 Apr 2024 11:04:54 +0200
Subject: [PATCH] Staticpro coding_categories

* src/coding.c (syms_of_coding):
---
 src/coding.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/src/coding.c b/src/coding.c
index b21f2ecf00a..80f48f675d5 100644
--- a/src/coding.c
+++ b/src/coding.c
@@ -11848,6 +11848,16 @@ syms_of_coding (void)
 
   Vcoding_category_table = make_nil_vector (coding_category_max);
   staticpro (&Vcoding_category_table);
+  for (size_t i; i < ARRAYELTS (coding_categories); i++)
+    {
+      struct coding_system* cs = &coding_categories[i];
+      Lisp_Object *src = &cs->src_object;
+      *src = Qnil;
+      staticpro (src);
+      Lisp_Object *dst = &cs->dst_object;
+      *dst = Qnil;
+      staticpro (dst);
+    }
   /* Followings are target of code detection.  */
   ASET (Vcoding_category_table, coding_category_iso_7,
 	intern_c_string ("coding-category-iso-7"));
-- 
2.39.2


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #7: 0001-Staticpro-coding_categories-only-if-HAVE_MPS.patch --]
[-- Type: text/x-diff, Size: 989 bytes --]

From 85f697feb55bfeec55c8cad4f2696a889b653ab4 Mon Sep 17 00:00:00 2001
From: Helmut Eller <eller.helmut@gmail.com>
Date: Tue, 30 Apr 2024 20:36:04 +0200
Subject: [PATCH] Staticpro coding_categories only if HAVE_MPS

* src/coding.c (syms_of_coding):
---
 src/coding.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/src/coding.c b/src/coding.c
index 80f48f675d5..05f5be81bc5 100644
--- a/src/coding.c
+++ b/src/coding.c
@@ -11848,6 +11848,7 @@ syms_of_coding (void)
 
   Vcoding_category_table = make_nil_vector (coding_category_max);
   staticpro (&Vcoding_category_table);
+#ifdef HAVE_MPS
   for (size_t i; i < ARRAYELTS (coding_categories); i++)
     {
       struct coding_system* cs = &coding_categories[i];
@@ -11858,6 +11859,7 @@ syms_of_coding (void)
       *dst = Qnil;
       staticpro (dst);
     }
+#endif
   /* Followings are target of code detection.  */
   ASET (Vcoding_category_table, coding_category_iso_7,
 	intern_c_string ("coding-category-iso-7"));
-- 
2.39.2


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #8: 0001-Staticpro-space_glyph.patch --]
[-- Type: text/x-diff, Size: 678 bytes --]

From 2cf885bd2cd3fbf19a7994b5fe530de36ba2e4cc Mon Sep 17 00:00:00 2001
From: Helmut Eller <eller.helmut@gmail.com>
Date: Tue, 30 Apr 2024 17:47:25 +0200
Subject: [PATCH] Staticpro space_glyph

* src/dispnew.c (syms_of_display):
---
 src/dispnew.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/src/dispnew.c b/src/dispnew.c
index 0f5063c047f..9f71e9ec761 100644
--- a/src/dispnew.c
+++ b/src/dispnew.c
@@ -6841,6 +6841,8 @@ syms_of_display (void)
   defsubr (&Sdump_redisplay_history);
 #endif
 
+  space_glyph.object = Qnil;
+  staticpro (&space_glyph.object);
   frame_and_buffer_state = make_vector (20, Qlambda);
   staticpro (&frame_and_buffer_state);
 
-- 
2.39.2


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #9: 0001-Staticpro-space_glyph.object-only-if-HAVE_MPS.patch --]
[-- Type: text/x-diff, Size: 728 bytes --]

From e64430a3af77226b0ff8f8814645355201625381 Mon Sep 17 00:00:00 2001
From: Helmut Eller <eller.helmut@gmail.com>
Date: Tue, 30 Apr 2024 20:38:01 +0200
Subject: [PATCH] Staticpro space_glyph.object only if HAVE_MPS

* src/dispnew.c (syms_of_display):
---
 src/dispnew.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/src/dispnew.c b/src/dispnew.c
index 9f71e9ec761..7bb78c3b214 100644
--- a/src/dispnew.c
+++ b/src/dispnew.c
@@ -6841,8 +6841,10 @@ syms_of_display (void)
   defsubr (&Sdump_redisplay_history);
 #endif
 
+#ifdef HAVE_MPS
   space_glyph.object = Qnil;
   staticpro (&space_glyph.object);
+#endif
   frame_and_buffer_state = make_vector (20, Qlambda);
   staticpro (&frame_and_buffer_state);
 
-- 
2.39.2


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #10: 0001-Staticpro-cached_system_name.patch --]
[-- Type: text/x-diff, Size: 791 bytes --]

From f475129ddaceb3bc487d28a32e79731de6119293 Mon Sep 17 00:00:00 2001
From: Helmut Eller <eller.helmut@gmail.com>
Date: Tue, 30 Apr 2024 18:13:57 +0200
Subject: [PATCH] Staticpro cached_system_name

* src/editfns.s (syms_of_editfns): Also initialize labeled_restrictions.
---
 src/editfns.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/src/editfns.c b/src/editfns.c
index cd4507aedcc..3e0e5856185 100644
--- a/src/editfns.c
+++ b/src/editfns.c
@@ -4778,6 +4778,9 @@ syms_of_editfns (void)
   DEFSYM (Qwall, "wall");
   DEFSYM (Qpropertize, "propertize");
 
+  cached_system_name = Qnil;
+  staticpro (&cached_system_name);
+  labeled_restrictions = Qnil;
   staticpro (&labeled_restrictions);
 
   DEFVAR_LISP ("inhibit-field-text-motion", Vinhibit_field_text_motion,
-- 
2.39.2


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #11: 0001-Staticpro-cached_system_name-only-if-HAVE_MPS.patch --]
[-- Type: text/x-diff, Size: 795 bytes --]

From e0cbb1eb7d00e94bbf96cf6a8e45c5285a598838 Mon Sep 17 00:00:00 2001
From: Helmut Eller <eller.helmut@gmail.com>
Date: Tue, 30 Apr 2024 20:40:10 +0200
Subject: [PATCH] Staticpro cached_system_name only if HAVE_MPS

* src/editfns.c (syms_of_editfns):
---
 src/editfns.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/src/editfns.c b/src/editfns.c
index 3e0e5856185..fc297896119 100644
--- a/src/editfns.c
+++ b/src/editfns.c
@@ -4778,9 +4778,11 @@ syms_of_editfns (void)
   DEFSYM (Qwall, "wall");
   DEFSYM (Qpropertize, "propertize");
 
+#ifdef HAVE_MPS
   cached_system_name = Qnil;
   staticpro (&cached_system_name);
   labeled_restrictions = Qnil;
+#endif
   staticpro (&labeled_restrictions);
 
   DEFVAR_LISP ("inhibit-field-text-motion", Vinhibit_field_text_motion,
-- 
2.39.2


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #12: 0001-Staticpro-old_selected_frame.patch --]
[-- Type: text/x-diff, Size: 736 bytes --]

From 8a164eacfe71be00f16eb059c5d8365586b837d8 Mon Sep 17 00:00:00 2001
From: Helmut Eller <eller.helmut@gmail.com>
Date: Tue, 30 Apr 2024 20:27:05 +0200
Subject: [PATCH] Staticpro old_selected_frame

* src/frame.c (syms_of_frame):
---
 src/frame.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/src/frame.c b/src/frame.c
index 42fb8eccb46..a2e78d27c75 100644
--- a/src/frame.c
+++ b/src/frame.c
@@ -6266,6 +6266,11 @@ init_frame_once_for_pdumper (void)
 void
 syms_of_frame (void)
 {
+#ifdef HAVE_MPS
+  old_selected_frame = Qnil;
+  staticpro (&old_selected_frame);
+#endif
+
   DEFSYM (Qframep, "framep");
   DEFSYM (Qframe_live_p, "frame-live-p");
   DEFSYM (Qframe_windows_min_size, "frame-windows-min-size");
-- 
2.39.2

Re: MPS: staticpro everything

[Gerd Möllmann]

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

> Today I came as far as frame.c.  For the first few, I hadn't thought
> about the #ifdef HAVE_MPS, so I went back and added it.  Now there are
> almost twice as many patches :(.  I thought it's better to send
> something now instead of everything at the end to get feedback.

Thanks!

Committed plus one commit from me for a warning from clang.
I'll take a closer look tomorrow, It's late again.

> I also have collected some notes so far:

Danke und einen geruhsamen Abend!

Re: MPS: staticpro everything

[Eli Zaretskii]

> From: Helmut Eller <eller.helmut@gmail.com>
> Cc: Eli Zaretskii <eliz@gnu.org>,  emacs-devel@gnu.org
> Date: Tue, 30 Apr 2024 20:57:16 +0200
> 
> These probably need special tracing code:
> 
> bidi.c: bidi_cache

Can you tell why?  The bidi cache is allocated using xmalloc, and has
a single Lisp string object in each of its slots.  The Lisp string
doesn't need any special handling, AFAIU, or does it?

> buffer.c: overlay_heads overlay_tails
> dispnew.c: frame_matrix_frame
> fns.c: hash_table_user_tests
> font.c: font_driver_list

I thought we decided that font_driver_list didn't need any special
handling, as it's a simple C linked list?

Thanks.

Re: MPS: staticpro everything

[Gerd Möllmann]

Eli Zaretskii <eliz@gnu.org> writes:

>> font.c: font_driver_list
>
> I thought we decided that font_driver_list didn't need any special
> handling, as it's a simple C linked list?

Hm, I can't find that change in font.c.

(@Helmut: forgot to tell - with M-x piem-am the number of patches you
send is not a problem. It handles all patches in one swoop. Very nice, I
can recommend it, thanks to the author(s).)

Re: MPS: staticpro everything

[Helmut Eller]

>>> font.c: font_driver_list
>>
>> I thought we decided that font_driver_list didn't need any special
>> handling, as it's a simple C linked list?
>
> Hm, I can't find that change in font.c.

It's not a patch.  It's in the notes.

Re: MPS: staticpro everything

[Gerd Möllmann]

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

> It's not a patch.  It's in the notes.

Ah, thanks!

Re: MPS: staticpro everything

[Eli Zaretskii]

> From: Gerd Möllmann <gerd.moellmann@gmail.com>
> Cc: Helmut Eller <eller.helmut@gmail.com>,  emacs-devel@gnu.org
> Date: Wed, 01 May 2024 08:47:40 +0200
> 
> (@Helmut: forgot to tell - with M-x piem-am the number of patches you
> send is not a problem. It handles all patches in one swoop. Very nice, I
> can recommend it, thanks to the author(s).)

So does "M-| git am", FWIW.

Re: MPS: staticpro everything

[Helmut Eller]

On Tue, Apr 30 2024, Eli Zaretskii wrote:

>> These probably need special tracing code:
>> 
>> bidi.c: bidi_cache
>
> Can you tell why?  The bidi cache is allocated using xmalloc, and has
> a single Lisp string object in each of its slots.  The Lisp string
> doesn't need any special handling, AFAIU, or does it?

The struct bidi_it has a pointer to a window.  I think something needs
to be done about that.  I didn't know what, so I made a note "for
later".  I don't see the string you're talking about but I don't
actually understand what this code does.  I just looked at the types.

>> buffer.c: overlay_heads overlay_tails
>> dispnew.c: frame_matrix_frame
>> fns.c: hash_table_user_tests
>> font.c: font_driver_list
>
> I thought we decided that font_driver_list didn't need any special
> handling, as it's a simple C linked list?

Then maybe a comment would be appropriate.  Usually, any root needs to
be traced.

Re: MPS: staticpro everything

[Eli Zaretskii]

> From: Helmut Eller <eller.helmut@gmail.com>
> Cc: gerd.moellmann@gmail.com,  emacs-devel@gnu.org
> Date: Wed, 01 May 2024 08:53:31 +0200
> 
> On Tue, Apr 30 2024, Eli Zaretskii wrote:
> 
> >> These probably need special tracing code:
> >> 
> >> bidi.c: bidi_cache
> >
> > Can you tell why?  The bidi cache is allocated using xmalloc, and has
> > a single Lisp string object in each of its slots.  The Lisp string
> > doesn't need any special handling, AFAIU, or does it?
> 
> The struct bidi_it has a pointer to a window.

'struct bidi_it' is a member of 'struct it', the iterator object we
use to perform display layout of windows.  'struct it' includes
pointers to the window being displayed and also to the frame of that
window:

  struct it
  {
    /* The window in which we iterate over current_buffer (or a string).  */
    Lisp_Object window;
    struct window *w;

    /* The window's frame.  */
    struct frame *f;

It also has a few other pointers.  And yet, you didn't include 'struct
it' in your list, and I wonder why the difference?  (The pointer to
the window is the same pointer that is copied in 'struct bidi_it'.)

This goes back to my other questions about MPS GC: which pointers to
what objects should be audited for possible need of "doing something"?
I think we need to discuss this right now, because otherwise we might
be wasting our time and energy on something that is simply unworkable
in Emacs, at least unless we rewrite most of our C code where it
accesses Lisp objects.

Emacs has a lot of code that is mostly straightforward when the
"normal" C runtime is used, with its mostly unmovable allocated
memory, but it could become very tricky if allocated memory can move
from under our feet at any given moment.  No one of us remembers or
even knows about all the places where we did some C trickery that can
become unworkable when some basic assumptions are invalidated.  So
even to think about these issues, so we could bring our combined
knowledge and experience to bear, requires that the issues are well
understood and clear, and their implications on Emacs object system
are also clear.  Only given such understanding can each one of us
consider the parts of the code he or she is familiar with, and think
whether any of them need adjustments.

> I think something needs to be done about that.  I didn't know what,
> so I made a note "for later".  I don't see the string you're talking
> about but I don't actually understand what this code does.  I just
> looked at the types.

I do understand what the code does, but I need to understand better
why having a pointer to a 'struct window' could be a reason "to do
something" about it.

> >> buffer.c: overlay_heads overlay_tails
> >> dispnew.c: frame_matrix_frame
> >> fns.c: hash_table_user_tests
> >> font.c: font_driver_list
> >
> > I thought we decided that font_driver_list didn't need any special
> > handling, as it's a simple C linked list?
> 
> Then maybe a comment would be appropriate.  Usually, any root needs to
> be traced.

What is "a root" in this context?  I though only roots of Lisp objects
are alluded to by this term, and font_driver_list is not a Lisp
object.

Re: MPS: staticpro everything

[Gerd Möllmann]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Helmut Eller <eller.helmut@gmail.com>
>> Cc: gerd.moellmann@gmail.com,  emacs-devel@gnu.org
>> Date: Wed, 01 May 2024 08:53:31 +0200
>> 
>> On Tue, Apr 30 2024, Eli Zaretskii wrote:
>> 
>> >> These probably need special tracing code:
>> >> 
>> >> bidi.c: bidi_cache
>> >
>> > Can you tell why?  The bidi cache is allocated using xmalloc, and has
>> > a single Lisp string object in each of its slots.  The Lisp string
>> > doesn't need any special handling, AFAIU, or does it?
>> 
>> The struct bidi_it has a pointer to a window.
>
> 'struct bidi_it' is a member of 'struct it', the iterator object we
> use to perform display layout of windows.  'struct it' includes
> pointers to the window being displayed and also to the frame of that
> window:
>
>   struct it
>   {
>     /* The window in which we iterate over current_buffer (or a string).  */
>     Lisp_Object window;
>     struct window *w;
>
>     /* The window's frame.  */
>     struct frame *f;
>
> It also has a few other pointers.  And yet, you didn't include 'struct
> it' in your list, and I wonder why the difference?  (The pointer to
> the window is the same pointer that is copied in 'struct bidi_it'.)

I think struct it always lives on the stack. In that case we don't need
to do anything (see my other mail).

Re: MPS: staticpro everything

[Eli Zaretskii]

> From: Gerd Möllmann <gerd.moellmann@gmail.com>
> Cc: Helmut Eller <eller.helmut@gmail.com>,  emacs-devel@gnu.org
> Date: Wed, 01 May 2024 15:48:25 +0200
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > 'struct bidi_it' is a member of 'struct it', the iterator object we
> > use to perform display layout of windows.  'struct it' includes
> > pointers to the window being displayed and also to the frame of that
> > window:
> >
> >   struct it
> >   {
> >     /* The window in which we iterate over current_buffer (or a string).  */
> >     Lisp_Object window;
> >     struct window *w;
> >
> >     /* The window's frame.  */
> >     struct frame *f;
> >
> > It also has a few other pointers.  And yet, you didn't include 'struct
> > it' in your list, and I wonder why the difference?  (The pointer to
> > the window is the same pointer that is copied in 'struct bidi_it'.)
> 
> I think struct it always lives on the stack. In that case we don't need
> to do anything (see my other mail).

What a relief!

Re: MPS: staticpro everything

[Helmut Eller]

>> The struct bidi_it has a pointer to a window.
>
> 'struct bidi_it' is a member of 'struct it', the iterator object we
> use to perform display layout of windows.  'struct it' includes
> pointers to the window being displayed and also to the frame of that
> window:
>
>   struct it
>   {
>     /* The window in which we iterate over current_buffer (or a string).  */
>     Lisp_Object window;
>     struct window *w;
>
>     /* The window's frame.  */
>     struct frame *f;
>
> It also has a few other pointers.  And yet, you didn't include 'struct
> it' in your list, and I wonder why the difference? 

I don't know.  I only looked at the global variables.  Maybe there isn't
one of type struct it.  Or maybe I overlooked it.

> This goes back to my other questions about MPS GC: which pointers to
> what objects should be audited for possible need of "doing something"?

For all exact references to objects in MPS pools we need to do
something.  We need to "fix" them when our scan method is called.  If we
want to store exact references in global variables or in objects that
are not normally scanned than we must register them as roots.

> I think we need to discuss this right now, because otherwise wae might
> be wasting our time and energy on something that is simply unworkable
> in Emacs, at least unless we rewrite most of our C code where it
> accesses Lisp objects.

Right.  When making experiments we must be prepared that they fail.  So
we should keep the cost of failure in check.

[..]
>> I think something needs to be done about that.  I didn't know what,
>> so I made a note "for later".  I don't see the string you're talking
>> about but I don't actually understand what this code does.  I just
>> looked at the types.
>
> I do understand what the code does, but I need to understand better
> why having a pointer to a 'struct window' could be a reason "to do
> something" about it.

Struct window starts with a union vectorlike_header header.  That's a
clear sign that its managed by MPS. 

>> >> buffer.c: overlay_heads overlay_tails
>> >> dispnew.c: frame_matrix_frame
>> >> fns.c: hash_table_user_tests
>> >> font.c: font_driver_list
>> >
>> > I thought we decided that font_driver_list didn't need any special
>> > handling, as it's a simple C linked list?
>> 
>> Then maybe a comment would be appropriate.  Usually, any root needs to
>> be traced.
>
> What is "a root" in this context?  I though only roots of Lisp objects
> are alluded to by this term, and font_driver_list is not a Lisp
> object.

I think it means this: A root holds a reference to a MPS managed object
but the cell where the reference is stored, is itself not managed my
MPS.

Re: MPS: staticpro everything

[Gerd Möllmann]

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

>> What is "a root" in this context?  I though only roots of Lisp objects
>> are alluded to by this term, and font_driver_list is not a Lisp
>> object.
>
> I think it means this: A root holds a reference to a MPS managed object
> but the cell where the reference is stored, is itself not managed my
> MPS.

In the old GC, staticvec, lisp_sym and others are roots. I think one
could say that all the special mark_xy methods (specpdl, bc stack, ...)
are also examples of roots.

Re: MPS: staticpro everything

[Eli Zaretskii]

> From: Helmut Eller <eller.helmut@gmail.com>
> Cc: gerd.moellmann@gmail.com,  emacs-devel@gnu.org
> Date: Wed, 01 May 2024 16:08:30 +0200
> 
> >> The struct bidi_it has a pointer to a window.
> >
> > 'struct bidi_it' is a member of 'struct it', the iterator object we
> > use to perform display layout of windows.  'struct it' includes
> > pointers to the window being displayed and also to the frame of that
> > window:
> >
> >   struct it
> >   {
> >     /* The window in which we iterate over current_buffer (or a string).  */
> >     Lisp_Object window;
> >     struct window *w;
> >
> >     /* The window's frame.  */
> >     struct frame *f;
> >
> > It also has a few other pointers.  And yet, you didn't include 'struct
> > it' in your list, and I wonder why the difference? 
> 
> I don't know.  I only looked at the global variables.  Maybe there isn't
> one of type struct it.  Or maybe I overlooked it.

So the problem is that bidi_cache is global, and it holds bidi_it
objects, which have a pointer to 'struct window'?

> > This goes back to my other questions about MPS GC: which pointers to
> > what objects should be audited for possible need of "doing something"?
> 
> For all exact references to objects in MPS pools we need to do
> something.  We need to "fix" them when our scan method is called.  If we
> want to store exact references in global variables or in objects that
> are not normally scanned than we must register them as roots.

Sorry, I don't think I understand what is "exact reference".

> >> Then maybe a comment would be appropriate.  Usually, any root needs to
> >> be traced.
> >
> > What is "a root" in this context?  I though only roots of Lisp objects
> > are alluded to by this term, and font_driver_list is not a Lisp
> > object.
> 
> I think it means this: A root holds a reference to a MPS managed object
> but the cell where the reference is stored, is itself not managed my
> MPS.

Thanks, I need to think about the meaning of this.

Re: MPS: staticpro everything

[Helmut Eller]

>> I don't know.  I only looked at the global variables.  Maybe there isn't
>> one of type struct it.  Or maybe I overlooked it.
>
> So the problem is that bidi_cache is global, and it holds bidi_it
> objects, which have a pointer to 'struct window'?

Exactly.  Somebody could follow the path bidi_cache -> bidi_it ->
window.  And if window moves, we must update bidi_it.  But bidi_it
objects are normally not scanned, because they aren't allocated in an
MPS pool (I think).  So we need to do it some other way.  E.g. by
registering bidi_cache as root with mps_root_create and then our special
scan code follows the path to bidi_cache -> bidi_it to update the
reference to window.

Another solution would be to always allocate bidi_it objects with
igc_xzalloc_ambig.  Then MPS is forced to keep window in place.  But the
bidi_it structure is rather large so this might be somewhat costly,
because MPS must then scan all fields in bidi_it conservatively.

>> > This goes back to my other questions about MPS GC: which pointers to
>> > what objects should be audited for possible need of "doing something"?
>> 
>> For all exact references to objects in MPS pools we need to do
>> something.  We need to update them when our scan method is called.
>> If we want to store exact references in global variables or in
>> objects that are not normally scanned than we must register them as
>> roots.
>
> Sorry, I don't think I understand what is "exact reference".

In MPS we classify references either as exact or ambiguous.  For exact
references we are certain that they point to an MPS managed object.  For
ambiguous references we aren't sure: it could be a reference to an
object or it could just be an integer.  When MPS scans the stack, it
treats all bit patterns there as ambiguous references.

Re: MPS: staticpro everything

[Eli Zaretskii]

> From: Helmut Eller <eller.helmut@gmail.com>
> Cc: gerd.moellmann@gmail.com,  emacs-devel@gnu.org
> Date: Wed, 01 May 2024 18:06:19 +0200
> 
> >> I don't know.  I only looked at the global variables.  Maybe there isn't
> >> one of type struct it.  Or maybe I overlooked it.
> >
> > So the problem is that bidi_cache is global, and it holds bidi_it
> > objects, which have a pointer to 'struct window'?
> 
> Exactly.  Somebody could follow the path bidi_cache -> bidi_it ->
> window.  And if window moves, we must update bidi_it.  But bidi_it
> objects are normally not scanned, because they aren't allocated in an
> MPS pool (I think).  So we need to do it some other way.  E.g. by
> registering bidi_cache as root with mps_root_create and then our special
> scan code follows the path to bidi_cache -> bidi_it to update the
> reference to window.
> 
> Another solution would be to always allocate bidi_it objects with
> igc_xzalloc_ambig.  Then MPS is forced to keep window in place.  But the
> bidi_it structure is rather large so this might be somewhat costly,
> because MPS must then scan all fields in bidi_it conservatively.

Thanks.  I actually happen to dislike both of these alternatives.  The
first one is too costly, especially if bidi_cache happens to be large
(see below).  That cost is mostly unnecessary, since almost all
members of 'struct bidi_it' don't need any special handling, just the
window pointer needs it.

The second solution is problematic because it complicates the handling
of the parent 'struct it' iterator object, for example when we need to
copy it (see SAVE_IT and RESTORE_IT): currently, the iterator holds
the entire bidi_it structure, whereas under this second alternative it
will hold only a pointer, which will need to be followed and its
target copied as well.

So I'd like to find a better alternative.

Let me first describe shortly how the bidi_cache is used during
redisplay.  The purpose of the cache is to avoid expensive
reprocessing of characters when we need to change the direction of
scanning buffer or string text due to reordering.  Specifically, if
the buffer text is

     abcdUVWXYZefgh

where lower-case characters indicate left-to-right (LTR) text and
upper-case characters indicate right-to-left (RTL) text, then we
originally scan the buffer forward, but when we get to U, we need to
find the end of the RTL text and scan back from there (because on
display this should show as "abcdZYXWVUefgh").  What bidi.c does is
quickly scan from U to Z, cache the results by copying to the cache
one bidi_it struct for each RTL character we scanned, and then deliver
the RTL characters to the caller in display code in the reverse order,
using the information in bidi_cache.  When the cached states are
exhausted, bidi.c jumps to e and continues the forward scan.  At this
point, all the cached states are no longer needed, so we "reset" the
cache: mark all of its slots as "free" (so they could be filled with
information next time we need to cache).

bidi_cache is malloc'ed, and if we need to enlarge it, we realloc it
as needed.  In some pathological cases the cache could become very
large.  For example, imagine a very long text line which has a single
LTR character at the beginning, then a long stretch of RTL characters,
and a single LTR character at the end.  In such cases we need the
cache to hold one bidi_it struct for each RTL character we process,
and that could be quite a lot of memory.  So when we "reset" the
cache, we also check whether it is larger than some threshold, and if
so, we also reallocate it to take less memory (on the hope that such
pathological cases are rare).

I hope the above provides a clear overview of what the cache does.

Given this, what would be the best way of making sure the 'struct
window' pointer in each bidi_it remains valid?  The simplest idea that
comes to my mind is to arrange for the window on which redisplay works
to be unmovable.  Would that solve the problem?  If so, we could do
that in init_iterator, which starts the display iteration process, and
I can suggest a patch if you tell me how to do that with MPS.  (I
actually think that making the window being redisplayed unmovable is a
Good Thing for other reasons, since it frees us from worrying about
passing the 'struct window' pointer around, which we do liberally.)

TIA

Re: MPS: staticpro everything

[Gerd Möllmann]

Eli Zaretskii <eliz@gnu.org> writes:

> I hope the above provides a clear overview of what the cache does.

Yes from me.

> Given this, what would be the best way of making sure the 'struct
> window' pointer in each bidi_it remains valid?  The simplest idea that
> comes to my mind is to arrange for the window on which redisplay works
> to be unmovable.  Would that solve the problem?  If so, we could do
> that in init_iterator, which starts the display iteration process, and
> I can suggest a patch if you tell me how to do that with MPS.  (I
> actually think that making the window being redisplayed unmovable is a
> Good Thing for other reasons, since it frees us from worrying about
> passing the 'struct window' pointer around, which we do liberally.)

I would propose to use igc_xzalloc_ambig + igc_free in the MPS case
instead of xmalloc/xfree. There are also realloc, and palloc variants,
if you use that.

This is the same as the normal malloc/free, but it also adds an ambig
root, so that anything references from the malloc'd memory becomes
immovable / doesn't die. It's the same as if that memory would be part
of the control stack.

Re: MPS: staticpro everything

[Eli Zaretskii]

> From: Gerd Möllmann <gerd.moellmann@gmail.com>
> Cc: Helmut Eller <eller.helmut@gmail.com>,  emacs-devel@gnu.org
> Date: Thu, 02 May 2024 11:04:43 +0200
> 
> > Given this, what would be the best way of making sure the 'struct
> > window' pointer in each bidi_it remains valid?  The simplest idea that
> > comes to my mind is to arrange for the window on which redisplay works
> > to be unmovable.  Would that solve the problem?  If so, we could do
> > that in init_iterator, which starts the display iteration process, and
> > I can suggest a patch if you tell me how to do that with MPS.  (I
> > actually think that making the window being redisplayed unmovable is a
> > Good Thing for other reasons, since it frees us from worrying about
> > passing the 'struct window' pointer around, which we do liberally.)
> 
> I would propose to use igc_xzalloc_ambig + igc_free in the MPS case
> instead of xmalloc/xfree. There are also realloc, and palloc variants,
> if you use that.
> 
> This is the same as the normal malloc/free, but it also adds an ambig
> root, so that anything references from the malloc'd memory becomes
> immovable / doesn't die. It's the same as if that memory would be part
> of the control stack.

But doesn't that mean MPS will need to trace too much?

Also, does the above mean my idea of making the window unmovable is
not good?  If so, can you explain why not?

Re: MPS: staticpro everything

[Gerd Möllmann]

Eli Zaretskii <eliz@gnu.org> writes:

>> I would propose to use igc_xzalloc_ambig + igc_free in the MPS case
>> instead of xmalloc/xfree. There are also realloc, and palloc variants,
>> if you use that.
>> 
>> This is the same as the normal malloc/free, but it also adds an ambig
>> root, so that anything references from the malloc'd memory becomes
>> immovable / doesn't die. It's the same as if that memory would be part
>> of the control stack.
>
> But doesn't that mean MPS will need to trace too much?

Yes, admitted :-). We could optimize that later...

> Also, does the above mean my idea of making the window unmovable is
> not good?  If so, can you explain why not?

I'm sorry, I don't remember. What was the idea again?

Re: MPS: staticpro everything

[Eli Zaretskii]

> From: Gerd Möllmann <gerd.moellmann@gmail.com>
> Cc: eller.helmut@gmail.com,  emacs-devel@gnu.org
> Date: Thu, 02 May 2024 14:01:18 +0200
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> I would propose to use igc_xzalloc_ambig + igc_free in the MPS case
> >> instead of xmalloc/xfree. There are also realloc, and palloc variants,
> >> if you use that.
> >> 
> >> This is the same as the normal malloc/free, but it also adds an ambig
> >> root, so that anything references from the malloc'd memory becomes
> >> immovable / doesn't die. It's the same as if that memory would be part
> >> of the control stack.
> >
> > But doesn't that mean MPS will need to trace too much?
> 
> Yes, admitted :-). We could optimize that later...

It also means (AFAIU) that every cache slot that is "freed" by moving
bidi_cache_idx back will need to have the 'struct window *' member be
set to NULL, to prevent MPS from tracing garbage.  Right?  I'd like to
avoid these complications, if only for performance reasons.

> > Also, does the above mean my idea of making the window unmovable is
> > not good?  If so, can you explain why not?
> 
> I'm sorry, I don't remember. What was the idea again?

Make it so the window being processed by redisplay_window is
unmovable.  That is the window whose pointer bidi_it uses.  But I also
think doing this will be useful for other reasons, since the display
code manipulates the it->w pointer quite liberally, and who knows what
we do with it?  If the window cannot move while it is being
redisplayed, we can stop bothering about all those uses of its
pointer.

Re: MPS: staticpro everything

[Gerd Möllmann]

Eli Zaretskii <eliz@gnu.org> writes:

>> > But doesn't that mean MPS will need to trace too much?
>> 
>> Yes, admitted :-). We could optimize that later...
>
> It also means (AFAIU) that every cache slot that is "freed" by moving
> bidi_cache_idx back will need to have the 'struct window *' member be
> set to NULL, to prevent MPS from tracing garbage.  Right?  I'd like to
> avoid these complications, if only for performance reasons.

Right, that's true.

>> > Also, does the above mean my idea of making the window unmovable is
>> > not good?  If so, can you explain why not?
>> 
>> I'm sorry, I don't remember. What was the idea again?
>
> Make it so the window being processed by redisplay_window is
> unmovable.  That is the window whose pointer bidi_it uses.  But I also
> think doing this will be useful for other reasons, since the display
> code manipulates the it->w pointer quite liberally, and who knows what
> we do with it?  If the window cannot move while it is being
> redisplayed, we can stop bothering about all those uses of its
> pointer.

Yes.

I now wonder: If struct it always lives on the stack (I think that's still
true?), then it->w is also on the stack, and thus it->w is immovable by
fact fact. Right?

Re: MPS: staticpro everything

[Po Lu]

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

> I now wonder: If struct it always lives on the stack (I think that's still
> true?), then it->w is also on the stack, and thus it->w is immovable by
> fact fact. Right?

You'll excuse me if I'm late to the party, but what becomes of strings
in cached bidi_its when MPS is in control?

Re: MPS: staticpro everything

[Gerd Möllmann]

Po Lu <luangruo@yahoo.com> writes:

> Gerd Möllmann <gerd.moellmann@gmail.com> writes:
>
>> I now wonder: If struct it always lives on the stack (I think that's still
>> true?), then it->w is also on the stack, and thus it->w is immovable by
>> fact fact. Right?
>
> You'll excuse me if I'm late to the party, but what becomes of strings
> in cached bidi_its when MPS is in control?

bidi_cache is a global variable pointing to a malloc'd array of bidi_it.
So, by itself, nothing is protected. MPS doesn't know anything about
malloc'd memory.

For some reason, this works with the old GC, without the need for a
special mark function, analogous to mark_specpdl, say. Why that is I
have no idea. Something else apparently keeps everything in the cache
alive.

(As I mentioned, using igc_xzalloc_ambig would do the trick. With the
disadvantages Eli mentioned - scanning more than needed.)

Re: MPS: staticpro everything

[Eli Zaretskii]

> From: Gerd Möllmann <gerd.moellmann@gmail.com>
> Cc: Eli Zaretskii <eliz@gnu.org>,  eller.helmut@gmail.com,  emacs-devel@gnu.org
> Date: Thu, 02 May 2024 16:01:42 +0200
> 
> Po Lu <luangruo@yahoo.com> writes:
> 
> > Gerd Möllmann <gerd.moellmann@gmail.com> writes:
> >
> >> I now wonder: If struct it always lives on the stack (I think that's still
> >> true?), then it->w is also on the stack, and thus it->w is immovable by
> >> fact fact. Right?
> >
> > You'll excuse me if I'm late to the party, but what becomes of strings
> > in cached bidi_its when MPS is in control?
> 
> bidi_cache is a global variable pointing to a malloc'd array of bidi_it.
> So, by itself, nothing is protected. MPS doesn't know anything about
> malloc'd memory.

That's true.  But the string Po Lu is alluding to is stored in the
cache as a Lisp object, not as a 'char *' pointer to the string's
data.  So as long as the string is alive (i.e. referenced from other
places), it should not be moved, right?

> For some reason, this works with the old GC, without the need for a
> special mark function, analogous to mark_specpdl, say. Why that is I
> have no idea. Something else apparently keeps everything in the cache
> alive.

That string is not just an arbitrary string, it's a string that the
display iterator is iterating.  So its reference is also in
it->string, which is on the C stack, and that reference keeps it alive
for as long as the string is being processed.  Once we are done
iterating the string, the parts of the cache that hold the states
related to that iteration are thrown away, because they are no longer
needed.

> (As I mentioned, using igc_xzalloc_ambig would do the trick. With the
> disadvantages Eli mentioned - scanning more than needed.)

I'd like to avoid that, of course.  We have only a single pointer to
'struct window', so scanning the entire cache is just waste of CPU
cycles.

Re: MPS: staticpro everything

[Gerd Möllmann]

Eli Zaretskii <eliz@gnu.org> writes:

>> bidi_cache is a global variable pointing to a malloc'd array of bidi_it.
>> So, by itself, nothing is protected. MPS doesn't know anything about
>> malloc'd memory.
>
> That's true.  But the string Po Lu is alluding to is stored in the
> cache as a Lisp object, not as a 'char *' pointer to the string's
> data.  So as long as the string is alive (i.e. referenced from other
> places), it should not be moved, right?

That's not the case. MPS doesn't know anything about the malloc'd block,
so it doesn't know anything about what it contains, it will never see
the char *, and nothing keeps it alive or prevents moving.

The situation is different if we make MPS know the block, by making it a
root (igc_xzalloc_ambig). MPS then scans the block, we see the char *,
and because we scan ambiguously, this keeps the string alive and it
precents moving.

(Other references outside of the malloc'd block can do the same of
course. I'm just talking about the one malloc'd block.)

>> For some reason, this works with the old GC, without the need for a
>> special mark function, analogous to mark_specpdl, say. Why that is I
>> have no idea. Something else apparently keeps everything in the cache
>> alive.
>
> That string is not just an arbitrary string, it's a string that the
> display iterator is iterating.  So its reference is also in
> it->string, which is on the C stack, and that reference keeps it alive
> for as long as the string is being processed.  Once we are done
> iterating the string, the parts of the cache that hold the states
> related to that iteration are thrown away, because they are no longer
> needed.

That would the "other references" I mention above that can have the same
effect of making things immovable, keeping things alive.

Re: MPS: staticpro everything

[Eli Zaretskii]

> From: Gerd Möllmann <gerd.moellmann@gmail.com>
> Cc: luangruo@yahoo.com,  eller.helmut@gmail.com,  emacs-devel@gnu.org
> Date: Thu, 02 May 2024 17:37:05 +0200
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> bidi_cache is a global variable pointing to a malloc'd array of bidi_it.
> >> So, by itself, nothing is protected. MPS doesn't know anything about
> >> malloc'd memory.
> >
> > That's true.  But the string Po Lu is alluding to is stored in the
> > cache as a Lisp object, not as a 'char *' pointer to the string's
> > data.  So as long as the string is alive (i.e. referenced from other
> > places), it should not be moved, right?
> 
> That's not the case. MPS doesn't know anything about the malloc'd block,
> so it doesn't know anything about what it contains, it will never see
> the char *, and nothing keeps it alive or prevents moving.

I'm confused: what 'char *'?  There's no 'char *' in bidi_it;
instead, there's a Lisp_Object:

  struct bidi_string_data {
    Lisp_Object lstring;	/* Lisp string to reorder, or nil */
    const unsigned char *s;	/* string data, or NULL if reordering buffer */
    ptrdiff_t schars;		/* the number of characters in the string,
				   excluding the terminating null */
    ptrdiff_t bufpos;		/* buffer position of lstring, or 0 if N/A */
    bool_bf from_disp_str : 1;	/* True means the string comes from a
				   display property */
    bool_bf unibyte : 1;	/* True means the string is unibyte */
  };

  struct bidi_it {
    [...]
    struct bidi_string_data string;	/* string to reorder */

Translation:

  . bidi_it has a sub-struct bidi_string_data
  . bidi_string_data has 2 fields: lstring and s
    - either one or the other of these two can be non-NULL/nil, but
      not both
    - if lstring is non-nil, it is a Lisp string which is being
      iterated by the caller, and is the same as it->string in the
      caller
    - if s is non-NULL, it is a C string being iterated by the caller,
      and is the same as it->s in the caller

(For those who forgot or didn't know what "iterating a C string" means
here: the Emacs display can display a C string, defined with the
normal C 'char *' way.  We use this, for example, for displaying
certain parts of the mode line, such as lots_of_dashes[].)

So, if you thought that 'char *s" above can ever point to the data of
lstring, that is not so.  When the caller iterates over a Lisp string,
bidi.c accesses lstring like so (where 'string' is a pointer to
bidi_it->string, i.e. a pointer to 'struct bidi_string_data'):

      else if (STRINGP (string->lstring))
	{
	  if (!string->unibyte)
	    {
	      int len;
	      ch = string_char_and_length (SDATA (string->lstring) + bytepos,
					   &len);
	      *ch_len = len;
	    }
	  else
	    {
	      ch = UNIBYTE_TO_CHAR (SREF (string->lstring, bytepos));
	      *ch_len = 1;
	    }

IOW, we maintain the string position and access the string using that
position.

(It was you, Gerd, years ago who explained to me why bidi.c cannot use
a 'char *' to the data of a Lisp string, because GC can relocate
string data, rendering the pointer invalid.  That's why the code gets
passed the Lisp string, not a pointer to its data, and that's why it
accesses it like above.)

When I said that the Lisp string "should not be moved", I meant the
string referenced in bidi_it->string.lstring.

The bidi_cache holds instances of 'struct bidi_it', and that includes
the Lisp string in bidi_it->string.lstring.  But since the same Lisp
string is referenced from it->string, where it is on the stack, AFAIU
the string pointed by it->string should not be moved by MPS.  And as
long as it is not moved all its instances in bidi_cache are okay.
Right?  Or did I misunderstand something?
    
> >> For some reason, this works with the old GC, without the need for a
> >> special mark function, analogous to mark_specpdl, say. Why that is I
> >> have no idea. Something else apparently keeps everything in the cache
> >> alive.
> >
> > That string is not just an arbitrary string, it's a string that the
> > display iterator is iterating.  So its reference is also in
> > it->string, which is on the C stack, and that reference keeps it alive
> > for as long as the string is being processed.  Once we are done
> > iterating the string, the parts of the cache that hold the states
> > related to that iteration are thrown away, because they are no longer
> > needed.
> 
> That would the "other references" I mention above that can have the same
> effect of making things immovable, keeping things alive.

Right.  But I see nothing except those "other references" in this
story.  Or what did I miss?

Re: MPS: staticpro everything

[Gerd Möllmann]

Eli Zaretskii <eliz@gnu.org> writes:

>> That's not the case. MPS doesn't know anything about the malloc'd block,
>> so it doesn't know anything about what it contains, it will never see
>> the char *, and nothing keeps it alive or prevents moving.
>
> I'm confused: what 'char *'?  There's no 'char *' in bidi_it;
> instead, there's a Lisp_Object:

I got probably confused about who said what. Good new is that it makes
not much difference. What's ultimately used for scanning ambigously is

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;

	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;

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

As one can see, this handles with normal pointers and tagged pointers,
aka Lisp_Object.

>
>   struct bidi_string_data {
>     Lisp_Object lstring;	/* Lisp string to reorder, or nil */
>     const unsigned char *s;	/* string data, or NULL if reordering buffer */
>     ptrdiff_t schars;		/* the number of characters in the string,
> 				   excluding the terminating null */
>     ptrdiff_t bufpos;		/* buffer position of lstring, or 0 if N/A */
>     bool_bf from_disp_str : 1;	/* True means the string comes from a
> 				   display property */
>     bool_bf unibyte : 1;	/* True means the string is unibyte */
>   };
>
>   struct bidi_it {
>     [...]
>     struct bidi_string_data string;	/* string to reorder */
>
> Translation:
>
>   . bidi_it has a sub-struct bidi_string_data
>   . bidi_string_data has 2 fields: lstring and s
>     - either one or the other of these two can be non-NULL/nil, but
>       not both
>     - if lstring is non-nil, it is a Lisp string which is being
>       iterated by the caller, and is the same as it->string in the
>       caller
>     - if s is non-NULL, it is a C string being iterated by the caller,
>       and is the same as it->s in the caller
>
> (For those who forgot or didn't know what "iterating a C string" means
> here: the Emacs display can display a C string, defined with the
> normal C 'char *' way.  We use this, for example, for displaying
> certain parts of the mode line, such as lots_of_dashes[].)
>
> So, if you thought that 'char *s" above can ever point to the data of
> lstring, that is not so.  When the caller iterates over a Lisp string,
> bidi.c accesses lstring like so (where 'string' is a pointer to
> bidi_it->string, i.e. a pointer to 'struct bidi_string_data'):
>
>       else if (STRINGP (string->lstring))
> 	{
> 	  if (!string->unibyte)
> 	    {
> 	      int len;
> 	      ch = string_char_and_length (SDATA (string->lstring) + bytepos,
> 					   &len);
> 	      *ch_len = len;
> 	    }
> 	  else
> 	    {
> 	      ch = UNIBYTE_TO_CHAR (SREF (string->lstring, bytepos));
> 	      *ch_len = 1;
> 	    }
>
> IOW, we maintain the string position and access the string using that
> position.

That looks okay, as long as we trace string->lstring. We need to, so
that XSTRING (string->lstring)->u.s.data can be updated by us when the
string data moves, which is managed by MPS (IGC_OBJ_STRING_DATA).

> (It was you, Gerd, years ago who explained to me why bidi.c cannot use
> a 'char *' to the data of a Lisp string, because GC can relocate
> string data, rendering the pointer invalid.  That's why the code gets
> passed the Lisp string, not a pointer to its data, and that's why it
> accesses it like above.)

:-). Yeah, we couldn't pin objects in the old GC like MPS can.

> When I said that the Lisp string "should not be moved", I meant the
> string referenced in bidi_it->string.lstring.

The string data "abc...", yes.

> The bidi_cache holds instances of 'struct bidi_it', and that includes
> the Lisp string in bidi_it->string.lstring.  But since the same Lisp
> string is referenced from it->string, where it is on the stack, AFAIU
> the string pointed by it->string should not be moved by MPS.  And as
> long as it is not moved all its instances in bidi_cache are okay.
> Right?  Or did I misunderstand something?

Well.... Just kidding :-).

What's on the stack strictly speaking is a Lisp_Object for the string.
That is as if a struct Lisp_String * is on the stack. That makes the
struct Lisp_String immovable. But the members of that struct Lisp_String
are not on the stack. In particular the char * u.s.data is not on the
stack. So u.s.data is not immovable.

Everything just looking at that single Lisp_Object on the stack.

Now what redeems us:

If we do anything with u.s.data in out code, u.s.data will be loaded
into a register or into an automatic variable/temporary on the stack!
And that amkes it immovable for the time it is used!

Only thing we must make sure is that the initial u.s.data when do stuff
with it is correct. But that should also be the case in our case: The
string is kept alive by the reference on the stack, and with u.s.data we
the GC/MPS can do whatever we like, as long as no one looks. Where
"looks" means imposes other restrictons on us.

Bottom line: I think we're good.

>> >> For some reason, this works with the old GC, without the need for a
>> >> special mark function, analogous to mark_specpdl, say. Why that is I
>> >> have no idea. Something else apparently keeps everything in the cache
>> >> alive.
>> >
>> > That string is not just an arbitrary string, it's a string that the
>> > display iterator is iterating.  So its reference is also in
>> > it->string, which is on the C stack, and that reference keeps it alive
>> > for as long as the string is being processed.  Once we are done
>> > iterating the string, the parts of the cache that hold the states
>> > related to that iteration are thrown away, because they are no longer
>> > needed.
>>
>> That would the "other references" I mention above that can have the same
>> effect of making things immovable, keeping things alive.
>
> Right.  But I see nothing except those "other references" in this
> story.  Or what did I miss?

Looks like we are good 👍

Re: MPS: staticpro everything

[Eli Zaretskii]

> From: Gerd Möllmann <gerd.moellmann@gmail.com>
> Cc: luangruo@yahoo.com,  eller.helmut@gmail.com,  emacs-devel@gnu.org
> Date: Thu, 02 May 2024 21:08:04 +0200
> 
> That looks okay, as long as we trace string->lstring. We need to, so
> that XSTRING (string->lstring)->u.s.data can be updated by us when the
> string data moves, which is managed by MPS (IGC_OBJ_STRING_DATA).

If that is the problem, it is not specific to bidi.c.  The same
happens in xdisp.c, because we have it->string, where 'string' is a
Lisp string which we are iterating, the same string that is stored in
it->bidi_it.string.lstring.

> What's on the stack strictly speaking is a Lisp_Object for the string.
> That is as if a struct Lisp_String * is on the stack. That makes the
> struct Lisp_String immovable. But the members of that struct Lisp_String
> are not on the stack. In particular the char * u.s.data is not on the
> stack. So u.s.data is not immovable.
> 
> Everything just looking at that single Lisp_Object on the stack.
> 
> Now what redeems us:
> 
> If we do anything with u.s.data in out code, u.s.data will be loaded
> into a register or into an automatic variable/temporary on the stack!
> And that amkes it immovable for the time it is used!

We access a lot to u.s.size and u.s.size_byte.  We access u.s.data
less frequently.

Let me make this less abstract.  The body of the basic display
iteration loop looks like this, if we remove all the code that handles
layout decisions:

   loop
     get_next_display_element
     PRODUCE_GLYPHS
     set_iterator_to_next
   end loop

We only access u.s.data in get_next_display_element and in
PRODUCE_GLYPHS, but not in set_iterator_to_next.  Callers of this loop
may or may not access u.s.data of the string which is being iterated.
Moreover, access to u.s.data is in many cases by calling a function,
such as string_char_and_length, passing it it->string or somesuch.

The iterator object is, of course, common to all of the above, and is
in a stack-based variable set up by the caller of the loop, but the
iterator object only holds the Lisp_Object for the string, as
mentioned above.

So are we okay or aren't we?

Re: MPS: staticpro everything

[Gerd Möllmann]

Eli Zaretskii <eliz@gnu.org> writes:

> Let me make this less abstract.  

That's good.

> The body of the basic display iteration loop looks like this, if we
> remove all the code that handles layout decisions:
>
     struct it it;
>    loop
>      get_next_display_element
>      PRODUCE_GLYPHS
>      set_iterator_to_next
>    end loop
>
> We only access u.s.data in get_next_display_element and in
> PRODUCE_GLYPHS, but not in set_iterator_to_next.  Callers of this loop
> may or may not access u.s.data of the string which is being iterated.
> Moreover, access to u.s.data is in many cases by calling a function,
> such as string_char_and_length, passing it it->string or somesuch.
>
> The iterator object is, of course, common to all of the above, and is
> in a stack-based variable set up by the caller of the loop, but the
> iterator object only holds the Lisp_Object for the string, as
> mentioned above.
>
> So are we okay or aren't we?

We are safe.

I added a line for the struct it in your code. That is on the stack,
so its members, whatever they are - Lisp_Object, char *, and whatever
else keeps whatever they reference alive, and, because the stack is an
ambig root, immovable.

That leaves the u.s.data, which is an object that is not on the stack
but only referenced from a string that is on the stack.

That always works because to do anything with u.s.data, the CPU must
load that pointer into a register or somewhere on the stack. By that
fact the same applies to the string data that applies to the string
above.

Re: MPS: staticpro everything

[Helmut Eller]

> That leaves the u.s.data, which is an object that is not on the stack
> but only referenced from a string that is on the stack.
>
> That always works because to do anything with u.s.data, the CPU must
> load that pointer into a register or somewhere on the stack. By that
> fact the same applies to the string data that applies to the string
> above.

Is it so easy?  Somebody could load &u.s.data into a register but then
quickly stow it away in a malloced struct or a "temporary global
variable".

Re: MPS: staticpro everything

[Eli Zaretskii]

> From: Helmut Eller <eller.helmut@gmail.com>
> Cc: Eli Zaretskii <eliz@gnu.org>,  luangruo@yahoo.com,  emacs-devel@gnu.org
> Date: Fri, 03 May 2024 13:18:32 +0200
> 
> > That leaves the u.s.data, which is an object that is not on the stack
> > but only referenced from a string that is on the stack.
> >
> > That always works because to do anything with u.s.data, the CPU must
> > load that pointer into a register or somewhere on the stack. By that
> > fact the same applies to the string data that applies to the string
> > above.
> 
> Is it so easy?  Somebody could load &u.s.data into a register but then
> quickly stow it away in a malloced struct or a "temporary global
> variable".

I also don't really understand this.  AFAIU, it is not enough to have
u.s.data pointer in a register, we should also be sure MPS will update
the pointer with the new address if and when the string's data is
moved.  But how do we know MPS will have the opportunity to do such an
update? the Lisp thread could use the (outdated) pointer before MPS
has the chance to update it in the register.  Or what am I missing?

Re: MPS: staticpro everything

[Gerd Möllmann]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Helmut Eller <eller.helmut@gmail.com>
>> Cc: Eli Zaretskii <eliz@gnu.org>,  luangruo@yahoo.com,  emacs-devel@gnu.org
>> Date: Fri, 03 May 2024 13:18:32 +0200
>> 
>> > That leaves the u.s.data, which is an object that is not on the stack
>> > but only referenced from a string that is on the stack.
>> >
>> > That always works because to do anything with u.s.data, the CPU must
>> > load that pointer into a register or somewhere on the stack. By that
>> > fact the same applies to the string data that applies to the string
>> > above.
>> 
>> Is it so easy?  Somebody could load &u.s.data into a register but then
>> quickly stow it away in a malloced struct or a "temporary global
>> variable".
>
> I also don't really understand this.  AFAIU, it is not enough to have
> u.s.data pointer in a register, we should also be sure MPS will update
> the pointer with the new address if and when the string's data is
> moved.  But how do we know MPS will have the opportunity to do such an
> update? the Lisp thread could use the (outdated) pointer before MPS
> has the chance to update it in the register.  Or what am I missing?

I think you're overlooking that registers and stack are ambiguous roots.
The presence of u.s.data in a register or on the stack _prevents_ it
from moving.

Re: MPS: staticpro everything

[Eli Zaretskii]

> From: Gerd Möllmann <gerd.moellmann@gmail.com>
> Cc: Helmut Eller <eller.helmut@gmail.com>,  luangruo@yahoo.com,
>   emacs-devel@gnu.org
> Date: Fri, 03 May 2024 13:49:51 +0200
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> From: Helmut Eller <eller.helmut@gmail.com>
> >> Cc: Eli Zaretskii <eliz@gnu.org>,  luangruo@yahoo.com,  emacs-devel@gnu.org
> >> Date: Fri, 03 May 2024 13:18:32 +0200
> >> 
> >> > That leaves the u.s.data, which is an object that is not on the stack
> >> > but only referenced from a string that is on the stack.
> >> >
> >> > That always works because to do anything with u.s.data, the CPU must
> >> > load that pointer into a register or somewhere on the stack. By that
> >> > fact the same applies to the string data that applies to the string
> >> > above.
> >> 
> >> Is it so easy?  Somebody could load &u.s.data into a register but then
> >> quickly stow it away in a malloced struct or a "temporary global
> >> variable".
> >
> > I also don't really understand this.  AFAIU, it is not enough to have
> > u.s.data pointer in a register, we should also be sure MPS will update
> > the pointer with the new address if and when the string's data is
> > moved.  But how do we know MPS will have the opportunity to do such an
> > update? the Lisp thread could use the (outdated) pointer before MPS
> > has the chance to update it in the register.  Or what am I missing?
> 
> I think you're overlooking that registers and stack are ambiguous roots.
> The presence of u.s.data in a register or on the stack _prevents_ it
> from moving.

That doesn't resolve the difficulty, it just turns the table.  Now we
could have a race condition between the Lisp thread loading a pointer
into a register or pushing it onto the C stack, and the MPS thread
taking a notice that the pointer is in a register or on the stack.  If
the pointer was neither in a register nor on the stack when MPS
started GC, it could decide to move the object, while the Lisp thread
concurrently loads the pointer into a register.  So we might think the
object pointed to is unmovable while it isn't.

What am I missing now?

Re: MPS: staticpro everything

[Gerd Möllmann]

Eli Zaretskii <eliz@gnu.org> writes:

>> I think you're overlooking that registers and stack are ambiguous roots.
>> The presence of u.s.data in a register or on the stack _prevents_ it
>> from moving.
>
> That doesn't resolve the difficulty, it just turns the table.  Now we
> could have a race condition between the Lisp thread loading a pointer
> into a register or pushing it onto the C stack, and the MPS thread
> taking a notice that the pointer is in a register or on the stack.  If
> the pointer was neither in a register nor on the stack when MPS
> started GC, it could decide to move the object, while the Lisp thread
> concurrently loads the pointer into a register.  So we might think the
> object pointed to is unmovable while it isn't.
>
> What am I missing now?

Here barriers come into play. Let's say, for simplicity, that we are
talking about an object on a VM page P.

In its thread, MPS decides that it wants to do an increment of work on
P, say it wants to move objects, without the chance that MPS and client
threads interfere.

So, MPS puts a read barrier on P (say with mprotect), so that other
threads are interrupted by a signal when they read from P. MPS puts a
write barrier on P so that the same happens when a thread tries to
modifiy P.

Does that help?

Re: MPS: staticpro everything

[Eli Zaretskii]

> From: Gerd Möllmann <gerd.moellmann@gmail.com>
> Cc: eller.helmut@gmail.com,  luangruo@yahoo.com,  emacs-devel@gnu.org
> Date: Fri, 03 May 2024 14:21:26 +0200
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> I think you're overlooking that registers and stack are ambiguous roots.
> >> The presence of u.s.data in a register or on the stack _prevents_ it
> >> from moving.
> >
> > That doesn't resolve the difficulty, it just turns the table.  Now we
> > could have a race condition between the Lisp thread loading a pointer
> > into a register or pushing it onto the C stack, and the MPS thread
> > taking a notice that the pointer is in a register or on the stack.  If
> > the pointer was neither in a register nor on the stack when MPS
> > started GC, it could decide to move the object, while the Lisp thread
> > concurrently loads the pointer into a register.  So we might think the
> > object pointed to is unmovable while it isn't.
> >
> > What am I missing now?
> 
> Here barriers come into play. Let's say, for simplicity, that we are
> talking about an object on a VM page P.
> 
> In its thread, MPS decides that it wants to do an increment of work on
> P, say it wants to move objects, without the chance that MPS and client
> threads interfere.
> 
> So, MPS puts a read barrier on P (say with mprotect), so that other
> threads are interrupted by a signal when they read from P. MPS puts a
> write barrier on P so that the same happens when a thread tries to
> modifiy P.
> 
> Does that help?

I understand how this is supposed to work, and I even see in MPS the
Windows code to implement this (protw3.c).  My problem is that I don't
see any evidence for these exceptions under GDB.

GDB has 2 Windows-specific settings to show debug information about
exceptions and debug-related events: "set debugexceptions" and "set
debugevents".  I turned both ON, but all I see is kernel events for
creation and exiting of threads.  Nothing more, and no exception
events anywhere in sight.  So I'm confused.

Re: MPS: staticpro everything

[Gerd Möllmann]

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

>> That leaves the u.s.data, which is an object that is not on the stack
>> but only referenced from a string that is on the stack.
>>
>> That always works because to do anything with u.s.data, the CPU must
>> load that pointer into a register or somewhere on the stack. By that
>> fact the same applies to the string data that applies to the string
>> above.
>
> Is it so easy?  Somebody could load &u.s.data into a register but then
> quickly stow it away in a malloced struct or a "temporary global
> variable".

Let's say S is the string in question. &S->u.s.data is an interior
pointer into the string, right? So, in essence, we're storing a
reference reference to the string in malloc'd memory. We know that that
is not safe.

Re: MPS: staticpro everything

[Eli Zaretskii]

> From: Gerd Möllmann <gerd.moellmann@gmail.com>
> Cc: eller.helmut@gmail.com,  emacs-devel@gnu.org
> Date: Thu, 02 May 2024 14:30:01 +0200
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> > But doesn't that mean MPS will need to trace too much?
> >> 
> >> Yes, admitted :-). We could optimize that later...
> >
> > It also means (AFAIU) that every cache slot that is "freed" by moving
> > bidi_cache_idx back will need to have the 'struct window *' member be
> > set to NULL, to prevent MPS from tracing garbage.  Right?  I'd like to
> > avoid these complications, if only for performance reasons.
> 
> Right, that's true.

So it's another complication.  Right now, the function that "frees"
the cache is very simple and fast:

  static void
  bidi_cache_reset_to (int n)
  {
    bidi_cache_idx = bidi_cache_start + n;
    bidi_cache_last_idx = -1;
  }
  static void
  bidi_cache_reset (void)
  {
    bidi_cache_reset_to (0);
  }

If we go the igc_xzalloc_ambig route, then not only will we be wasting
CPU cycles on zeroing-out of the cache each time we (re)allocate it,
but we will also need to waste CPU cycles on zeroing them out in the
above functions.  If that can be avoided, I'd prefer to.

> >> I'm sorry, I don't remember. What was the idea again?
> >
> > Make it so the window being processed by redisplay_window is
> > unmovable.  That is the window whose pointer bidi_it uses.  But I also
> > think doing this will be useful for other reasons, since the display
> > code manipulates the it->w pointer quite liberally, and who knows what
> > we do with it?  If the window cannot move while it is being
> > redisplayed, we can stop bothering about all those uses of its
> > pointer.
> 
> Yes.

That should be very easy to do, then.  Can you tell me how I make a
struct unmovable by MPS, when I have a pointer to the struct in hand?

> I now wonder: If struct it always lives on the stack (I think that's still
> true?), then it->w is also on the stack, and thus it->w is immovable by
> fact fact. Right?

Yes.  So you are saying there's no problem, neither with it->w, nor
with it->bidi_it.w, and thus the cached bidi_it objects are also safe
in this aspect?

Re: MPS: staticpro everything

[Gerd Möllmann]

Eli Zaretskii <eliz@gnu.org> writes:

> That should be very easy to do, then.  Can you tell me how I make a
> struct unmovable by MPS, when I have a pointer to the struct in hand?

The only API MPS provides for that is creating a ambig root for an
address range that contains the struct. (Even easier would be to place
the struct on the stack because the stack is already such a root.)

>> I now wonder: If struct it always lives on the stack (I think that's still
>> true?), then it->w is also on the stack, and thus it->w is immovable by
>> fact fact. Right?
>
> Yes.  So you are saying there's no problem, neither with it->w, nor
> with it->bidi_it.w, and thus the cached bidi_it objects are also safe
> in this aspect?

I think so, but I have limited understanding of what's going on. If
there is always some struct it on the stack that has the same it->w as
what struct bidi_it then I think we are safe.

Re: MPS: staticpro everything

[Eli Zaretskii]

> From: Gerd Möllmann <gerd.moellmann@gmail.com>
> Cc: eller.helmut@gmail.com,  emacs-devel@gnu.org
> Date: Thu, 02 May 2024 17:55:43 +0200
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> I now wonder: If struct it always lives on the stack (I think that's still
> >> true?), then it->w is also on the stack, and thus it->w is immovable by
> >> fact fact. Right?
> >
> > Yes.  So you are saying there's no problem, neither with it->w, nor
> > with it->bidi_it.w, and thus the cached bidi_it objects are also safe
> > in this aspect?
> 
> I think so, but I have limited understanding of what's going on. If
> there is always some struct it on the stack that has the same it->w as
> what struct bidi_it then I think we are safe.

That's what happens.

A typical redisplay cycle looks like this:

  int
  try_window (Lisp_Object window, struct text_pos pos, int flags)
  {
    struct window *w = XWINDOW (window);
    struct it it;
    [...]
    start_display (&it, w, pos);
    [...]
    /* Display all lines of W.  */
    while (it.current_y < it.last_visible_y)
      {
	[...]
	if (display_line (&it, cursor_vpos))
	  last_text_row = it.glyph_row - 1;
	[...]
      }

And all the "action", including calls to bidi.c, happens inside
display_line.

The call to start_display above initializes its IT argument by setting
it->w to the 2nd argument W.  It also initializes it->bidi_it, and
sets it->bidi_it.w to the same argument W.

IOW, the top-level caller of any display iteration uses a stack-based
variable of the type 'struct it', of which 'struct bidi_it' is a
field.

So, to summarize: we have concluded that the 'struct window' to which
it->w and it->bidi_it.w point will not be moved in memory as long as
struct it is on the C stack, and therefore the bidi_it.w pointers
stored in bidi_cache will be valid.  Is that right?

Re: MPS: staticpro everything

[Gerd Möllmann]

Eli Zaretskii <eliz@gnu.org> writes:

> So, to summarize: we have concluded that the 'struct window' to which
> it->w and it->bidi_it.w point will not be moved in memory as long as
> struct it is on the C stack, and therefore the bidi_it.w pointers
> stored in bidi_cache will be valid.  Is that right?

I think it is 👍

Re: MPS: staticpro everything

[Helmut Eller]

On Thu, May 02 2024, Eli Zaretskii wrote:

> I hope the above provides a clear overview of what the cache does.

Yes.  I wonder though, why there is a pointer from the cache to the
window a why it is not the other way around.  BTW, there is also a
Lisp_Object in bidi_string_data, and apparently one of those borrowed
char*.

I also have put the "staticpro everything" task on hold.  I think we
learned that protecting more globals than is done for the old GC is
rarely needed and may cause more harm than good.  Those cases were it's
actually needed, will show up during testing.

Re: MPS: staticpro everything

[Gerd Möllmann]

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

> On Thu, May 02 2024, Eli Zaretskii wrote:
>
> I also have put the "staticpro everything" task on hold.  I think we
> learned that protecting more globals than is done for the old GC is
> rarely needed and may cause more harm than good.  Those cases were it's
> actually needed, will show up during testing.

I think so too.

BTW, I sent the last mail an Emacs build from scratch/igc, without
native compilation, but with my init file). It didn't survive very long
though :-). I guess I'Ll try to do more in that direction.

Re: MPS: staticpro everything

[Helmut Eller]

> BTW, I sent the last mail an Emacs build from scratch/igc, without
> native compilation, but with my init file). It didn't survive very long
> though :-). I guess I'Ll try to do more in that direction.

(add-hook 'post-command-hook 'igc--collect) helps to crash it sooner.
For me, that always seems to involve some mouse interaction.

Re: MPS: staticpro everything

[Eli Zaretskii]

> From: Helmut Eller <eller.helmut@gmail.com>
> Cc: Eli Zaretskii <eliz@gnu.org>,  emacs-devel@gnu.org
> Date: Thu, 02 May 2024 18:03:17 +0200
> 
> > BTW, I sent the last mail an Emacs build from scratch/igc, without
> > native compilation, but with my init file). It didn't survive very long
> > though :-). I guess I'Ll try to do more in that direction.
> 
> (add-hook 'post-command-hook 'igc--collect) helps to crash it sooner.
> For me, that always seems to involve some mouse interaction.

Could be expose_widow/expose_frame and its ilk, a.k.a. "back-door
entry" into redisplay.

Re: MPS: staticpro everything

[Gerd Möllmann]

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

>> BTW, I sent the last mail an Emacs build from scratch/igc, without
>> native compilation, but with my init file). It didn't survive very long
>> though :-). I guess I'Ll try to do more in that direction.
>
> (add-hook 'post-command-hook 'igc--collect) helps to crash it sooner.
> For me, that always seems to involve some mouse interaction.

Thanks!

Re: MPS: staticpro everything

[Eli Zaretskii]

> From: Helmut Eller <eller.helmut@gmail.com>
> Cc: Eli Zaretskii <eliz@gnu.org>,  emacs-devel@gnu.org
> Date: Thu, 02 May 2024 18:03:17 +0200
> 
> > BTW, I sent the last mail an Emacs build from scratch/igc, without
> > native compilation, but with my init file). It didn't survive very long
> > though :-). I guess I'Ll try to do more in that direction.
> 
> (add-hook 'post-command-hook 'igc--collect) helps to crash it sooner.

Confirmed.

> For me, that always seems to involve some mouse interaction.

Maybe, not sure.  But anyway, I had one crash like this:

  keyboard.c:4269: Emacs fatal error: assertion failed: FRAMEP (frame)

  Thread 1 received signal SIGTRAP, Trace/breakpoint trap.
  0x75f45693 in KERNELBASE!DebugBreak () from C:\WINDOWS\SysWOW64\KernelBase.dll
  (gdb) thread 1
  [Switching to thread 1 (Thread 148160.0x23404)]
  #0  0x75f45693 in KERNELBASE!DebugBreak ()
     from C:\WINDOWS\SysWOW64\KernelBase.dll
  (gdb) bt
  #0  0x75f45693 in KERNELBASE!DebugBreak ()
     from C:\WINDOWS\SysWOW64\KernelBase.dll
  #1  0x00bc1bb6 in emacs_abort () at w32fns.c:11262
  #2  0x00a742b0 in terminate_due_to_signal (sig=sig@entry=22,
      backtrace_limit=backtrace_limit@entry=2147483647) at emacs.c:480
  #3  0x00ae8a00 in die (
      msg=msg@entry=0x105899d <GCTYPEBITS+53> "FRAMEP (frame)",
      file=file@entry=0x1058992 <GCTYPEBITS+42> "keyboard.c",
      line=line@entry=4269) at alloc.c:8337
  #4  0x00a864ec in kbd_buffer_get_event (end_time=0x0,
      used_mouse_menu=0x72bf7fb, kbp=<synthetic pointer>) at keyboard.c:4269

kbd_buffer_get_event does this where it aborted:

	  /* If this event is on a different frame, return a
	     switch-frame this time, and leave the event in the queue
	     for next time.  */
	  Lisp_Object frame;
	  Lisp_Object focus;

	  frame = event->ie.frame_or_window;
	  if (CONSP (frame))
	    frame = XCAR (frame);
	  else if (WINDOWP (frame))
	    frame = WINDOW_FRAME (XWINDOW (frame));

	  focus = FRAME_FOCUS_FRAME (XFRAME (frame)); <<<<<<<<<<<<<<<<

So we abort because of assertion violation in XFRAME, meaning that
'frame' is not a valid frame object.

This frame comes from the input event, where we have:

  struct input_event
  {
    /* What kind of event was this?  */
    ENUM_BF (event_kind) kind : EVENT_KIND_WIDTH;

    /* Used in scroll back click events.  */
    ENUM_BF (scroll_bar_part) part : 16;

    /* For an ASCII_KEYSTROKE_EVENT and MULTIBYTE_CHAR_KEYSTROKE_EVENT,
       this is the character.
       For a NON_ASCII_KEYSTROKE_EVENT, this is the keysym code.
       For a mouse event, this is the button number.  */
    unsigned code;

    /* See enum below for interpretation.  */
    unsigned modifiers;

    /* One would prefer C integers, but HELP_EVENT uses these to
       record frame or window object and a help form, respectively.  */
    Lisp_Object x, y;

    /* Usually a time as reported by window system-specific event loop.
       For a HELP_EVENT, this is the position within the object (stored
       in ARG below) where the help was found.  */
    Time timestamp;

    /* This field is copied into a vector while the event is in
       the queue, so that garbage collections won't kill it.  */
    Lisp_Object frame_or_window;

    /* This additional argument is used in attempt to avoid extra consing
       when building events.  Unfortunately some events have to pass much
       more data than it's reasonable to pack directly into this structure.  */
    Lisp_Object arg;

    /* The name of the device from which this event originated.

       It can either be a string, or Qt, which means to use the name
       "Virtual core pointer" for all events other than keystroke
       events, and "Virtual core keyboard" for those.  */
    Lisp_Object device;
  };

The input events are stored in kbd_buffer:

  union buffered_input_event kbd_buffer[KBD_BUFFER_SIZE];

In init_keyboard we do this:

  void
  init_keyboard (void)
  {
  #ifdef HAVE_MPS
    igc_root_create_ambig (kbd_buffer, (char *) kbd_buffer + ARRAYELTS (kbd_buffer));
  #endif

But is that enough to protect all the Lisp objects in each element of
kbd_buffer?  Evidently, in the above crash the frame object was moved,
without updating the events in kbd_buffer that reference it.  I'd
expect the frame to be unmovable when there's a reference to it in the
kbd_buffer, because there could be quite a lot of events there waiting
for processing.

Re: MPS: staticpro everything

[Eli Zaretskii]

> Date: Thu, 02 May 2024 21:46:08 +0300
> From: Eli Zaretskii <eliz@gnu.org>
> Cc: gerd.moellmann@gmail.com, emacs-devel@gnu.org
> 
> In init_keyboard we do this:
> 
>   void
>   init_keyboard (void)
>   {
>   #ifdef HAVE_MPS
>     igc_root_create_ambig (kbd_buffer, (char *) kbd_buffer + ARRAYELTS (kbd_buffer));
>   #endif

Hmm... shouldn't that be

   (char *) (kbd_buffer + ARRAYELTS (kbd_buffer))

instead?  Or maybe we don't need the cast at all?  But anyway, casting
before the addition looks wrong: kbd_buffer elements are not single-byte.

Re: MPS: staticpro everything

[Gerd Möllmann]

Eli Zaretskii <eliz@gnu.org> writes:

>> Date: Thu, 02 May 2024 21:46:08 +0300
>> From: Eli Zaretskii <eliz@gnu.org>
>> Cc: gerd.moellmann@gmail.com, emacs-devel@gnu.org
>> 
>> In init_keyboard we do this:
>> 
>>   void
>>   init_keyboard (void)
>>   {
>>   #ifdef HAVE_MPS
>>     igc_root_create_ambig (kbd_buffer, (char *) kbd_buffer + ARRAYELTS (kbd_buffer));
>>   #endif
>
> Hmm... shouldn't that be
>
>    (char *) (kbd_buffer + ARRAYELTS (kbd_buffer))
>
> instead?  Or maybe we don't need the cast at all?  But anyway, casting
> before the addition looks wrong: kbd_buffer elements are not single-byte.

Ooops, he foudn it already...

Re: MPS: staticpro everything

[Gerd Möllmann]

Eli Zaretskii <eliz@gnu.org> writes:

>> Date: Thu, 02 May 2024 21:46:08 +0300
>> From: Eli Zaretskii <eliz@gnu.org>
>> Cc: gerd.moellmann@gmail.com, emacs-devel@gnu.org
>> 
>> In init_keyboard we do this:
>> 
>>   void
>>   init_keyboard (void)
>>   {
>>   #ifdef HAVE_MPS
>>     igc_root_create_ambig (kbd_buffer, (char *) kbd_buffer + ARRAYELTS (kbd_buffer));
>>   #endif
>
> Hmm... shouldn't that be
>
>    (char *) (kbd_buffer + ARRAYELTS (kbd_buffer))
>
> instead?  Or maybe we don't need the cast at all?  But anyway, casting
> before the addition looks wrong: kbd_buffer elements are not single-byte.

Yeah, without the cast please :-). 

Re: MPS: staticpro everything

[Eli Zaretskii]

> From: Gerd Möllmann <gerd.moellmann@gmail.com>
> Cc: eller.helmut@gmail.com,  emacs-devel@gnu.org
> Date: Thu, 02 May 2024 21:51:43 +0200
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> Date: Thu, 02 May 2024 21:46:08 +0300
> >> From: Eli Zaretskii <eliz@gnu.org>
> >> Cc: gerd.moellmann@gmail.com, emacs-devel@gnu.org
> >> 
> >> In init_keyboard we do this:
> >> 
> >>   void
> >>   init_keyboard (void)
> >>   {
> >>   #ifdef HAVE_MPS
> >>     igc_root_create_ambig (kbd_buffer, (char *) kbd_buffer + ARRAYELTS (kbd_buffer));
> >>   #endif
> >
> > Hmm... shouldn't that be
> >
> >    (char *) (kbd_buffer + ARRAYELTS (kbd_buffer))
> >
> > instead?  Or maybe we don't need the cast at all?  But anyway, casting
> > before the addition looks wrong: kbd_buffer elements are not single-byte.
> 
> Yeah, without the cast please :-). 

Done.

Re: MPS: staticpro everything

[Gerd Möllmann]

Eli Zaretskii <eliz@gnu.org> writes:

>> Yeah, without the cast please :-). 
>
> Done.

Thanks. Still wondering how that happened...

Re: MPS: staticpro everything

[Gerd Möllmann]

Eli Zaretskii <eliz@gnu.org> writes:

> In init_keyboard we do this:
>
>   void
>   init_keyboard (void)
>   {
>   #ifdef HAVE_MPS
>     igc_root_create_ambig (kbd_buffer, (char *) kbd_buffer + ARRAYELTS (kbd_buffer));
>   #endif
>
> But is that enough to protect all the Lisp objects in each element of
> kbd_buffer?  Evidently, in the above crash the frame object was moved,
> without updating the events in kbd_buffer that reference it.  I'd
> expect the frame to be unmovable when there's a reference to it in the
> kbd_buffer, because there could be quite a lot of events there waiting
> for processing.

Yeah, the ambig root should make the frame immovable, that would be my
expectation as well. Hm, is the end address correct? Why would I use a
(char *) if it is already one :-(.

Re: MPS: staticpro everything

[Eli Zaretskii]

> From: Helmut Eller <eller.helmut@gmail.com>
> Cc: Eli Zaretskii <eliz@gnu.org>,  emacs-devel@gnu.org
> Date: Thu, 02 May 2024 18:03:17 +0200
> 
> > BTW, I sent the last mail an Emacs build from scratch/igc, without
> > native compilation, but with my init file). It didn't survive very long
> > though :-). I guess I'Ll try to do more in that direction.
> 
> (add-hook 'post-command-hook 'igc--collect) helps to crash it sooner.

Another crash I had was like this:

  Thread 1 received signal SIGSEGV, Segmentation fault.
  count_size_as_multibyte (
      str=0xb855020 "àÜA\ve-2: visit this file in other window",
      len=len@entry=197397672) at character.c:675
  675         nonascii += str[i] >> 7;
  (gdb) bt
  #0  count_size_as_multibyte (
      str=0xb855020 "àÜA\ve-2: visit this file in other window",
      len=len@entry=197397672) at character.c:675
  #1  0x00aab348 in insert_from_string_1 (string=<optimized out>, pos=0,
      pos_byte=0, nchars=197397672, nbytes=197397672, inherit=false,
      before_markers=false) at insdel.c:1017
  #2  0x00aabedd in insert_from_string (string=0xb807b0c, pos=pos@entry=0,
      pos_byte=pos_byte@entry=0, length=197397672, length_byte=197397672,
      inherit=inherit@entry=false) at insdel.c:974
  #3  0x00b05962 in general_insert_function (insert_func=0xaabb63 <insert>,
      insert_from_string_func=0xaabe6a <insert_from_string>,
      inherit=inherit@entry=false, nargs=nargs@entry=1,
      args=args@entry=0x1a99f04c) at editfns.c:1338
  #4  0x00b05ab5 in Finsert (nargs=1, args=0x1a99f04c) at editfns.c:1374
  #5  0x00b65737 in exec_byte_code (fun=0x103e065 <Smake_closure+5>,
      fun@entry=0x1a5d8dcd, args_template=513, nargs=2, nargs@entry=1,
      args=0x1a99f050, args@entry=0x72bf46c) at bytecode.c:1420
  #6  0x00b188b8 in funcall_lambda (fun=0x1a5d8dcd, nargs=nargs@entry=1,
      arg_vector=arg_vector@entry=0x72bf46c) at eval.c:3200
  #7  0x00b18dd4 in funcall_general (fun=<optimized out>,
      numargs=numargs@entry=1, args=args@entry=0x72bf46c) at eval.c:2994
  #8  0x00b1455b in Ffuncall (args=0x72bf468, nargs=2) at eval.c:3032
  #9  Ffuncall (nargs=nargs@entry=2, args=args@entry=0x72bf468) at eval.c:3006
  #10 0x00a766a7 in help_echo_substitute_command_keys (help=help@entry=0xb807b0c)
      at lisp.h:1179
  #11 0x00a7eaea in show_help_echo (help=help@entry=0xb807b0c,
      window=<optimized out>, window@entry=0xb0025fd, object=<optimized out>,
      object@entry=0xaa1248d, pos=<optimized out>) at keyboard.c:2275

In this case, the help-echo string is bogus (it has huge size), so I
suspect the string was moved or GCed.  Any suggestions or ideas?

Unfortunately, these crashes are not reproducible.

Re: MPS: staticpro everything

[Gerd Möllmann]

Eli Zaretskii <eliz@gnu.org> writes:

>   #11 0x00a7eaea in show_help_echo (help=help@entry=0xb807b0c,
>       window=<optimized out>, window@entry=0xb0025fd, object=<optimized out>,
>       object@entry=0xaa1248d, pos=<optimized out>) at keyboard.c:2275
>
> In this case, the help-echo string is bogus (it has huge size), so I
> suspect the string was moved or GCed.  Any suggestions or ideas?

You could look at the igc_header or igc_fwd of the string.
(client_to_base (p) to get to the header, then look *(struct igc_header
*) that. If that shows IGC_OBJ_FWD, it's a tombstone. *(struct igc_fwd
*) shows the new address.

MPS: GUI test automation (was: MPS: staticpro everything)

[Helmut Eller]

On Thu, May 02 2024, Eli Zaretskii wrote:

> Unfortunately, these crashes are not reproducible.

Yes, that's a problem.  Two days ago I found out that Dogtail[*] is
apparently the state of the art for GUI testing in the GTK world.  It's
a Python library that lets you find widgets and "click" on them an so
on.  I've used that to make one crash reproducible (almost, sometimes
Emacs crashes to soon for another reason).

I bed there are similar libraries for Windows and macOS.  Maybe it's
time to find and use them.


[*]https://gitlab.com/dogtail/dogtail

Re: MPS: GUI test automation (was: MPS: staticpro everything)

[Eli Zaretskii]

> From: Helmut Eller <eller.helmut@gmail.com>
> Cc: gerd.moellmann@gmail.com,  emacs-devel@gnu.org
> Date: Fri, 03 May 2024 08:26:24 +0200
> 
> On Thu, May 02 2024, Eli Zaretskii wrote:
> 
> > Unfortunately, these crashes are not reproducible.
> 
> Yes, that's a problem.  Two days ago I found out that Dogtail[*] is
> apparently the state of the art for GUI testing in the GTK world.  It's
> a Python library that lets you find widgets and "click" on them an so
> on.  I've used that to make one crash reproducible (almost, sometimes
> Emacs crashes to soon for another reason).
> 
> I bed there are similar libraries for Windows and macOS.  Maybe it's
> time to find and use them.

I'll try to look when I have time.

However, when I say "not reproducible", I mean that even if I do
exactly the same gestures, I cannot get a crash in a new session.
Since the original crash happened very soon after starting a session,
there's no problem for me to remember what I did, and repeat it.

And I always run under GDB, so ASLR should not be an issue.

Re: MPS: GUI test automation

[Gerd Möllmann]

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

> On Thu, May 02 2024, Eli Zaretskii wrote:
>
> I bed there are similar libraries for Windows and macOS.  Maybe it's
> time to find and use them.

Couldn't find a free one for macOS that does Cocoa. That was a week or
two ago.

Re: MPS: staticpro everything

[Eli Zaretskii]

> From: Helmut Eller <eller.helmut@gmail.com>
> Cc: gerd.moellmann@gmail.com,  emacs-devel@gnu.org
> Date: Thu, 02 May 2024 17:09:36 +0200
> 
> On Thu, May 02 2024, Eli Zaretskii wrote:
> 
> > I hope the above provides a clear overview of what the cache does.
> 
> Yes.  I wonder though, why there is a pointer from the cache to the
> window a why it is not the other way around.

At the lowest level of bidi.c, it accesses buffer or string text to
fetch characters.  But if the buffer/string position being accessed
has a display string or overlay string covering it, bidi.c needs to
know about that, because display strings and overlays are treated
specially by the reordering algorithms (I can tell the details if
needed, but for now I will omit them).  And overlays can be
window-specific.  So the code which searches for overlays, which is
called from bidi.c, needs to know the window.

This explains why strict bidi_it has a reference to the window: so
that code in bidi.c could use it when necessary.  Why this reference
to the window is in the cache is simply because we cache the iterator
states by copying struct bidi_it into the cache almost in its
entirety.  I guess if really necessary, we could avoid copying some
parts of struct bidi_it, like the pointer to the window, because the
window doesn't change during the iteration, so it doesn't depend on
the iterator state.  But bidi.c implements copying into the cache with
memcpy, and at least at the time it was very important to me to keep
the code as efficient as possible, which meant the speed of memcpy
trumped any other considerations.

> BTW, there is also a Lisp_Object in bidi_string_data, and apparently
> one of those borrowed char*.

Right, I hope I explained what those are in my other message.

> I also have put the "staticpro everything" task on hold.  I think we
> learned that protecting more globals than is done for the old GC is
> rarely needed and may cause more harm than good.  Those cases were it's
> actually needed, will show up during testing.

Fingers crossed.

Re: MPS: staticpro everything

[Gerd Möllmann]

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

> On Tue, Apr 30 2024, Gerd Möllmann wrote:
>
> And here the patches:

Moin Helmut.

I'm looking at struct coding_system and the staticpro's. And I'm
what to do with coding_system::source.

  struct coding_system
  {
    const unsigned char *source;

Clang shows me that

  coding_set_source (struct coding_system *coding)
  {
    if (BUFFERP (coding->src_object))
      {
        struct buffer *buf = XBUFFER (coding->src_object);

        if (coding->src_pos < 0)
          coding->source = BUF_GAP_END_ADDR (buf) + coding->src_pos_byte;
        else
          coding->source = BUF_BYTE_ADDRESS (buf, coding->src_pos_byte);
      }
    else if (STRINGP (coding->src_object))
      {
        coding->source = SDATA (coding->src_object) + coding->src_pos_byte;

I'm not sure what is with the buffer case, but the string case means
that source can point to string data, which is in our MPS leaf_pool. I
think we cvould use an exact scan function for that purpose, analogous
to scan_specpdl. WSYT?

Re: MPS: staticpro everything

[Helmut Eller]

On Wed, May 01 2024, Gerd Möllmann wrote:

> Moin Helmut.
>
> I'm looking at struct coding_system and the staticpro's. And I'm
> what to do with coding_system::source.
>
>   struct coding_system
>   {
>     const unsigned char *source;
>
> Clang shows me that
>
>   coding_set_source (struct coding_system *coding)
>   {
>     if (BUFFERP (coding->src_object))
>       {
>         struct buffer *buf = XBUFFER (coding->src_object);
>
>         if (coding->src_pos < 0)
>           coding->source = BUF_GAP_END_ADDR (buf) + coding->src_pos_byte;
>         else
>           coding->source = BUF_BYTE_ADDRESS (buf, coding->src_pos_byte);
>       }
>     else if (STRINGP (coding->src_object))
>       {
>         coding->source = SDATA (coding->src_object) + coding->src_pos_byte;
>
> I'm not sure what is with the buffer case, but the string case means
> that source can point to string data, which is in our MPS leaf_pool. I
> think we cvould use an exact scan function for that purpose, analogous
> to scan_specpdl. WSYT?

Yes, I think would work.

But I just realized that now that about any char* needs to be looked
upon very carefully to decide if it could point to the leaf_pool.
That's a problem.  Maybe a show-stopper kind-of problem.

Re: MPS: staticpro everything

[Gerd Möllmann]

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

>> I'm not sure what is with the buffer case, but the string case means
>> that source can point to string data, which is in our MPS leaf_pool. I
>> think we cvould use an exact scan function for that purpose, analogous
>> to scan_specpdl. WSYT?
>
> Yes, I think would work.

With an additional complication: the string is in MPS, of course, and to
get to its u.s.data pointer we would need to access it, say to compute a
new source pointer in case the string has moved. I'm not sure such an
access is allowed in a root scan function, because of barriers :-/.

Had something like that with an ASIZE in fix_comp_unit yesterday, which
resulted in an access violation.

Maybe one could replace char *source with an index? Or passing the src
around instead of the char*? I don't know how it is used, though.

> But I just realized that now that about any char* needs to be looked
> upon very carefully to decide if it could point to the leaf_pool.
> That's a problem.  Maybe a show-stopper kind-of problem.

Could be. I wonder how to find out how many of these cases there are.

Re: MPS: staticpro everything

[Gerd Möllmann]

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

> Helmut Eller <eller.helmut@gmail.com> writes:
>
>>> I'm not sure what is with the buffer case, but the string case means
>>> that source can point to string data, which is in our MPS leaf_pool. I
>>> think we cvould use an exact scan function for that purpose, analogous
>>> to scan_specpdl. WSYT?
>>
>> Yes, I think would work.
>
> With an additional complication: the string is in MPS, of course, and to
> get to its u.s.data pointer we would need to access it, say to compute a
> new source pointer in case the string has moved. I'm not sure such an
> access is allowed in a root scan function, because of barriers :-/.
>
> Had something like that with an ASIZE in fix_comp_unit yesterday, which
> resulted in an access violation.

The docs

  https://memory-pool-system.readthedocs.io/en/latest/topic/format.html#cautions

say

  Format methods must not:

  call library code;
  access MPS-managed memory in pools that protect their contents;

I'd conservativly extend that to root scanning functions as well. So, we
can't get the data pointer from the string. That would require surgery.

A cheaper alternative would be to make that whole thing an ambig root,
so that the string data referenced from char *source fields (interior
pointers) become immovable.

Re: MPS: staticpro everything

[Helmut Eller]

> A cheaper alternative would be to make that whole thing an ambig root,
> so that the string data referenced from char *source fields (interior
> pointers) become immovable.

Maybe we should move strings to the LO (Leaf Object) pool which doesn't
move blocks.

Re: MPS: staticpro everything

[Gerd Möllmann]

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

>> A cheaper alternative would be to make that whole thing an ambig root,
>> so that the string data referenced from char *source fields (interior
>> pointers) become immovable.
>
> Maybe we should move strings to the LO (Leaf Object) pool which doesn't
> move blocks.

Yes, that would might be an alternative.

  If LO is used to allocate large numbers of small objects, the garbage
  collection performance will degrade. For leaf objects that can move and
  be protected, it is better to use AMCZ (Automatic Mostly-Copying
  Zero-rank) instead.

What large and small means in that context is any ones guess :-).

Re: MPS: staticpro everything

[Helmut Eller]

>>> A cheaper alternative would be to make that whole thing an ambig root,
>>> so that the string data referenced from char *source fields (interior
>>> pointers) become immovable.
>>
>> Maybe we should move strings to the LO (Leaf Object) pool which doesn't
>> move blocks.
>
> Yes, that would might be an alternative.
>
>   If LO is used to allocate large numbers of small objects, the garbage
>   collection performance will degrade. For leaf objects that can move and
>   be protected, it is better to use AMCZ (Automatic Mostly-Copying
>   Zero-rank) instead.
>
> What large and small means in that context is any ones guess :-).

It does indeed look like the LO pool is intended for larger buffers and
the AMCZ for strings.  So let's make it an ambiguous root.  Which is
probably what we always should do "when in doubt".

Re: MPS: staticpro everything

[Eli Zaretskii]

> From: Gerd Möllmann <gerd.moellmann@gmail.com>
> Cc: Eli Zaretskii <eliz@gnu.org>,  emacs-devel@gnu.org
> Date: Wed, 01 May 2024 10:48:05 +0200
> 
> Helmut Eller <eller.helmut@gmail.com> writes:
> 
> >> A cheaper alternative would be to make that whole thing an ambig root,
> >> so that the string data referenced from char *source fields (interior
> >> pointers) become immovable.
> >
> > Maybe we should move strings to the LO (Leaf Object) pool which doesn't
> > move blocks.
> 
> Yes, that would might be an alternative.
> 
>   If LO is used to allocate large numbers of small objects, the garbage
>   collection performance will degrade. For leaf objects that can move and
>   be protected, it is better to use AMCZ (Automatic Mostly-Copying
>   Zero-rank) instead.
> 
> What large and small means in that context is any ones guess :-).

Lisp strings in Emacs can be very large, though.  Example: any call to
buffer-string.  Another example: strings in the kill-ring obtained by
copying or killing large buffers.

Re: MPS: staticpro everything

[Helmut Eller]

> A cheaper alternative would be to make that whole thing an ambig root,
> so that the string data referenced from char *source fields (interior
> pointers) become immovable.

Instead of the whole thing, we could only register
&coding_system[i].source as ambiguous root.

Re: MPS: staticpro everything

[Gerd Möllmann]

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

>> A cheaper alternative would be to make that whole thing an ambig root,
>> so that the string data referenced from char *source fields (interior
>> pointers) become immovable.
>
> Instead of the whole thing, we could only register
> &coding_system[i].source as ambiguous root.

True.

The question might then become what's more expensive - a higher number
of roots or a larger area to scan.

Re: MPS: staticpro everything

[Eli Zaretskii]

> From: Gerd Möllmann <gerd.moellmann@gmail.com>
> Cc: Eli Zaretskii <eliz@gnu.org>,  emacs-devel@gnu.org
> Date: Wed, 01 May 2024 09:46:25 +0200
> 
> Helmut Eller <eller.helmut@gmail.com> writes:
> 
> >> I'm not sure what is with the buffer case, but the string case means
> >> that source can point to string data, which is in our MPS leaf_pool. I
> >> think we cvould use an exact scan function for that purpose, analogous
> >> to scan_specpdl. WSYT?
> >
> > Yes, I think would work.
> 
> With an additional complication: the string is in MPS, of course, and to
> get to its u.s.data pointer we would need to access it, say to compute a
> new source pointer in case the string has moved. I'm not sure such an
> access is allowed in a root scan function, because of barriers :-/.

Can a string move while coding.c uses a pointer to it?  With the old
GC, it couldn't, or else we couldn't be using the pointer to its data.
coding_set_source is basically a small utility function, almost a
macro, that is called at the beginning of en/decoding processing, and
the stuff it sets up is used during that processing; it never leaks
outside of the caller of coding_set_source.  So I wonder whether we
indeed need to do something about this (and similarly about the buffer
text branch there), or maybe we see a problem where there is none?

I already asked more serious question related to this, so I won't
repeat them here.

> Maybe one could replace char *source with an index? Or passing the src
> around instead of the char*? I don't know how it is used, though.

That way lies madness: we will have to basically abandon any C-level
access via pointers to members of struct's that back Lisp objects.
E.g., we have gobs of functions that access windows via pointers to
'struct window'.

> > But I just realized that now that about any char* needs to be looked
> > upon very carefully to decide if it could point to the leaf_pool.
> > That's a problem.  Maybe a show-stopper kind-of problem.
> 
> Could be. I wonder how to find out how many of these cases there are.

They are all over the place, IIUC.  Scary.

Re: MPS: staticpro everything

[Gerd Möllmann]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Gerd Möllmann <gerd.moellmann@gmail.com>
>> Cc: Eli Zaretskii <eliz@gnu.org>,  emacs-devel@gnu.org
>> Date: Wed, 01 May 2024 09:46:25 +0200
>> 
>> Helmut Eller <eller.helmut@gmail.com> writes:
>> 
>> >> I'm not sure what is with the buffer case, but the string case means
>> >> that source can point to string data, which is in our MPS leaf_pool. I
>> >> think we cvould use an exact scan function for that purpose, analogous
>> >> to scan_specpdl. WSYT?
>> >
>> > Yes, I think would work.
>> 
>> With an additional complication: the string is in MPS, of course, and to
>> get to its u.s.data pointer we would need to access it, say to compute a
>> new source pointer in case the string has moved. I'm not sure such an
>> access is allowed in a root scan function, because of barriers :-/.
>
> Can a string move while coding.c uses a pointer to it?  With the old
> GC, it couldn't, or else we couldn't be using the pointer to its data.
> coding_set_source is basically a small utility function, almost a
> macro, that is called at the beginning of en/decoding processing, and
> the stuff it sets up is used during that processing; it never leaks
> outside of the caller of coding_set_source.  So I wonder whether we
> indeed need to do something about this (and similarly about the buffer
> text branch there), or maybe we see a problem where there is none?
>
> I already asked more serious question related to this, so I won't
> repeat them here.

Would you agree to delay these questions after reading my other mail?
I hope that mail answers the underlying question, so that this one
might become redundant.

>> Maybe one could replace char *source with an index? Or passing the src
>> around instead of the char*? I don't know how it is used, though.
>
> That way lies madness: we will have to basically abandon any C-level
> access via pointers to members of struct's that back Lisp objects.
> E.g., we have gobs of functions that access windows via pointers to
> 'struct window'.
>
>> > But I just realized that now that about any char* needs to be looked
>> > upon very carefully to decide if it could point to the leaf_pool.
>> > That's a problem.  Maybe a show-stopper kind-of problem.
>> 
>> Could be. I wonder how to find out how many of these cases there are.
>
> They are all over the place, IIUC.  Scary.

It's not bad. See the other mail.

(And with the clang Python API one can do really interesting things, I
see. If only the docs weren't so terrible ;-))

Re: MPS GC and its implications (was: staticpro everything)

[Eli Zaretskii]

> From: Gerd Möllmann <gerd.moellmann@gmail.com>
> Cc: Eli Zaretskii <eliz@gnu.org>,  emacs-devel@gnu.org
> Date: Wed, 01 May 2024 08:05:27 +0200
> 
>   coding_set_source (struct coding_system *coding)
>   {
>     if (BUFFERP (coding->src_object))
>       {
>         struct buffer *buf = XBUFFER (coding->src_object);
> 
>         if (coding->src_pos < 0)
>           coding->source = BUF_GAP_END_ADDR (buf) + coding->src_pos_byte;
>         else
>           coding->source = BUF_BYTE_ADDRESS (buf, coding->src_pos_byte);
>       }
>     else if (STRINGP (coding->src_object))
>       {
>         coding->source = SDATA (coding->src_object) + coding->src_pos_byte;
> 
> I'm not sure what is with the buffer case, but the string case means
> that source can point to string data, which is in our MPS leaf_pool. I
> think we cvould use an exact scan function for that purpose, analogous
> to scan_specpdl. WSYT?

If this is indeed needed, we are in trouble.  When we take a C pointer
to buffer or string text and use it in the following code, it's
because there's a hidden assumption that GC cannot happen during the
time the C pointer is being used.  If this assumption is not true with
MPS, nothing in Emacs will work reliably.

Is this indeed a problem?  Didn't Gerd just explain to Andrea that
every object referenced from the stack or even from a register will be
automatically "fixed" if GC happens and decides to move the objects?
If so, why doesn't that solve the problem here?

More generally, what happens with text of Emacs buffers and strings
when GC runs?  I know what the old GC did, but what does the new GC
do?

A related question, no less important, is: when can MPS GC happen, and
whether there's any reasonable way of being sure that it cannot
possibly happen while certain code fragments run?  For example, the
old GC could only be triggered by a call to funcall or eval (and a
small number of similar APIs); as long as the C code didn't cause any
of these calls, we could assume GC will not happen, and so any C
pointers to Lisp objects were safe to use.  What is the equivalent of
that with MPS?  Since MPS is a moving GC, this becomes much more
important; with old GC we only had 2 object types that could move
during GC: string data and buffer text.  Now every Lisp object is in
danger of being moved by GC, AFAIU.

Am I missing something?

I think we need to have a clear picture just what are the issues with
this, because it could indeed mean a show-stopper for Emacs.  So I
suggest that we stop for a moment and discuss these aspects of MPS.

(And please don't say "read the docs": I find the documentation of MPS
on these issues rather impenetrable; it looks like it was meant for
people who already know a lot about GC strategies and techniques.
E.g., they have 2 chapters on design: "Old Design" which is said to be
outdated at places (but no indications of where those outdated places
are), and "Design" which is described as "intended for developers of
the MPS".  IOW, there's no design description that can be relied
upon.)

Re: MPS GC and its implications

[Gerd Möllmann]

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

Eli Zaretskii <eliz@gnu.org> writes:

>> I'm not sure what is with the buffer case, but the string case means
>> that source can point to string data, which is in our MPS leaf_pool. I
>> think we cvould use an exact scan function for that purpose, analogous
>> to scan_specpdl. WSYT?
>
> If this is indeed needed, we are in trouble.  When we take a C pointer
> to buffer or string text and use it in the following code, it's
> because there's a hidden assumption that GC cannot happen during the
> time the C pointer is being used.  If this assumption is not true with
> MPS, nothing in Emacs will work reliably.

With the old GC and taking a char * from string.data we must inhibiti GC
because GC can move string data in memory (compact_small_strings in
alloc.c).

That's pretty similar to the MPS case, except that ambig scanning of
registers and control stacks means that string data automatically
becomes immovable. So, there's no need to inhibit GC because of moving
strings.

> Is this indeed a problem?  Didn't Gerd just explain to Andrea that
> every object referenced from the stack or even from a register will be
> automatically "fixed" if GC happens and decides to move the objects?
> If so, why doesn't that solve the problem here?

In this case, we have a string object and a char * in the struct. The
char * points into the string data of the string. Unless it is guranteed
that the char * is also alwqys in a register or on the stack, the above
mechanism doesn't work, and I don't see that guaranteed. Might be, of
course, but I don't see how.

Solution is to force ambig tracing, and therefore immovability, by
adding an ambig root.

> More generally, what happens with text of Emacs buffers and strings
> when GC runs?  I know what the old GC did, but what does the new GC
> do?

For buffer text I'm pretty sure nothing happens, but I'm meanwhile a bit
cautious because of features I potentially don't know. For me, buffer
text is still either from mmap or some malloc variant. If that's still
the case, MPS does nothing affecting buffer text.

Strings and string data are two objects in MPS memory. What happens to
them is what happens with other such objects, like conses and so on.
They are traced, moved in memory and so on. If something of that is
unclear, please let me know. I'm not sure what to write ATM.

> A related question, no less important, is: when can MPS GC happen, and
> whether there's any reasonable way of being sure that it cannot
> possibly happen while certain code fragments run?  

By default MPS runs concurrently, so always.

We can sort of turn off MMPS GC by "parking the arena" temporarily (the
igc_park_arena I mentioned in another mail). Example usage: pdumping.
Pdumper copies binary representation of Lisp objects to a memory area
that is later written to a file. If the Lisp objects changes their
binary representation in between that would not work. I think.

> For example, the old GC could only be triggered by a call to funcall
> or eval (and a small number of similar APIs); as long as the C code
> didn't cause any of these calls, we could assume GC will not happen,
> and so any C pointers to Lisp objects were safe to use. What is the
> equivalent of that with MPS? Since MPS is a moving GC, this becomes
> much more important; with old GC we only had 2 object types that could
> move during GC: string data and buffer text. Now every Lisp object is
> in danger of being moved by GC, AFAIU.

> Am I missing something?

I think the only thing you missed is why the automatic ambig scanning of
registers and control stack is not sufficient in Helmut's case (the
struct containing the char * etc.).

I hope I got that explained. Questions welcome.

> I think we need to have a clear picture just what are the issues with
> this, because it could indeed mean a show-stopper for Emacs.  So I
> suggest that we stop for a moment and discuss these aspects of MPS.

Sure. That's what my experiment originally was about.

> (And please don't say "read the docs": 

Read the docs! ;-)

> I find the documentation of MPS on these issues rather impenetrable;
> it looks like it was meant for people who already know a lot about GC
> strategies and techniques. E.g., they have 2 chapters on design: "Old
> Design" which is said to be outdated at places (but no indications of
> where those outdated places are), and "Design" which is described as
> "intended for developers of the MPS". IOW, there's no design
> description that can be relied upon.)

Yeah. I think one has keep in mind that MPS was commercial from 1997 or
so til they made it open source. Some links are probably company
internal stuff.

I have an old paper about MPS that I'm attaching. Don't remember anymore
what they write, but maybe it's of some use, even after 20+ years.


[-- Attachment #2: MPS paper 2002 --]
[-- Type: application/pdf, Size: 158873 bytes --]

Re: MPS GC and its implications

[Eli Zaretskii]

> From: Gerd Möllmann <gerd.moellmann@gmail.com>
> Cc: eller.helmut@gmail.com,  emacs-devel@gnu.org
> Date: Wed, 01 May 2024 15:39:48 +0200
> 
> > If this is indeed needed, we are in trouble.  When we take a C pointer
> > to buffer or string text and use it in the following code, it's
> > because there's a hidden assumption that GC cannot happen during the
> > time the C pointer is being used.  If this assumption is not true with
> > MPS, nothing in Emacs will work reliably.
> 
> With the old GC and taking a char * from string.data we must inhibiti GC
> because GC can move string data in memory (compact_small_strings in
> alloc.c).
> 
> That's pretty similar to the MPS case, except that ambig scanning of
> registers and control stacks means that string data automatically
> becomes immovable. So, there's no need to inhibit GC because of moving
> strings.

I guess you mean "in general" here?  Because right below you seem to
say that sometimes either inhibiting GC or some other protection _is_
needed?

> > Is this indeed a problem?  Didn't Gerd just explain to Andrea that
> > every object referenced from the stack or even from a register will be
> > automatically "fixed" if GC happens and decides to move the objects?
> > If so, why doesn't that solve the problem here?
> 
> In this case, we have a string object and a char * in the struct. The
> char * points into the string data of the string. Unless it is guranteed
> that the char * is also alwqys in a register or on the stack, the above
> mechanism doesn't work, and I don't see that guaranteed. Might be, of
> course, but I don't see how.

First, I don't understand why the above mechanism doesn't work.  What
exactly are the conditions which ensure that "ambig scanning of
registers and the C stack" will work?

Anyway: 'struct coding_system' is passed as argument to the functions
involved in the en/decoding, and so is on the stack or in a register.
The string object is a member of that struct, and the 'char *' is a
pointer into the data of that Lisp string object.  Does that answer
your question about whether 'char *' is on the stack or not?  If not,
maybe I don't understand what you mean by "'char *' is on the stack".
Since it's a pointer, what should be on the stack: the pointer or the
memory to which it points?

Similarly for buffer text: the buffer object is a member of 'struct
coding_system', which is on the stack, and the 'char *' points to the
text of that buffer.

> Solution is to force ambig tracing, and therefore immovability, by
> adding an ambig root.

What is an "ambig root" and how to add it?

> > More generally, what happens with text of Emacs buffers and strings
> > when GC runs?  I know what the old GC did, but what does the new GC
> > do?
> 
> For buffer text I'm pretty sure nothing happens, but I'm meanwhile a bit
> cautious because of features I potentially don't know. For me, buffer
> text is still either from mmap or some malloc variant. If that's still
> the case, MPS does nothing affecting buffer text.

So the relocation of buffer text that the old GC did is no longer done
by MPS?

> > A related question, no less important, is: when can MPS GC happen, and
> > whether there's any reasonable way of being sure that it cannot
> > possibly happen while certain code fragments run?  
> 
> By default MPS runs concurrently, so always.

Then what are the rules to make sure a C pointer to a struct that
backs a Lisp object (for example, 'struct window' which backs a window
object) is safe to have around and use in our usual style, viz.:

  struct window *w = XWINDOW (some_window);
  ...
  /* use the w pointer as a normal C pointer: dereference it, access
     and set window fields via w->foo, pass w to functions, etc. */

Is it enough to make sure w is on the stack?

What if I do something like this:

 static struct window *my_special_window = NULL;
 void foo (Lisp_Object window)
 {
   my_special_window = XWINDOW (window);
   ...
 }
 void bar (void)
 {
   if (my_special_window != NULL)
     /* do something with my_special_window */
 }

IOW, is storing a pointer like above in a temporary global variable
safe?

> I think the only thing you missed is why the automatic ambig scanning of
> registers and control stack is not sufficient in Helmut's case (the
> struct containing the char * etc.).

Why is it not safe?  I still don't think I understand that.

> I have an old paper about MPS that I'm attaching. Don't remember anymore
> what they write, but maybe it's of some use, even after 20+ years.

Thanks, will try to find some time for that.

Re: MPS GC and its implications

[Gerd Möllmann]

Eli Zaretskii <eliz@gnu.org> writes:

>> With the old GC and taking a char * from string.data we must inhibiti GC
>> because GC can move string data in memory (compact_small_strings in
>> alloc.c).
>>
>> That's pretty similar to the MPS case, except that ambig scanning of
>> registers and control stacks means that string data automatically
>> becomes immovable. So, there's no need to inhibit GC because of moving
>> strings.
>
> I guess you mean "in general" here?  Because right below you seem to
> say that sometimes either inhibiting GC or some other protection _is_
> needed?

It works if some word on the stack or in a register exists that we
recognize as an ambig reference in the igc scan_ambig function.

Which means it would not work when we deliberately "disguise" a
reference so that scan_ambig doesn't recognize it. I don't think that we
are doing that anywhere in Emacs.

>> > Is this indeed a problem?  Didn't Gerd just explain to Andrea that
>> > every object referenced from the stack or even from a register will be
>> > automatically "fixed" if GC happens and decides to move the objects?
>> > If so, why doesn't that solve the problem here?
>>
>> In this case, we have a string object and a char * in the struct. The
>> char * points into the string data of the string. Unless it is guranteed
>> that the char * is also alwqys in a register or on the stack, the above
>> mechanism doesn't work, and I don't see that guaranteed. Might be, of
>> course, but I don't see how.
>
> First, I don't understand why the above mechanism doesn't work.  What
> exactly are the conditions which ensure that "ambig scanning of
> registers and the C stack" will work?

The condition is: Some automatic C variable in the stack frames makes it
so that either the stack itself or a register contains the reference, in
this case a pointer to the string data (maybe interior pointer).

That's not the case, or not guaranteed, for example, if we store the
string pointer in some global variable, in our case in a struct member
of a static array of structs.

> Anyway: 'struct coding_system' is passed as argument to the functions
> involved in the en/decoding, and so is on the stack or in a register.

If it's passed by value, we are fine. If its passed as a pointer we're
not because the char * in question is not an the stack.

If it passed by value but was originally copied from a static
counterpart, we are probably not good, because that would copy the char
* from something we haven't traced.

> The string object is a member of that struct, and the 'char *' is a
> pointer into the data of that Lisp string object.  Does that answer
> your question about whether 'char *' is on the stack or not?  If not,
> maybe I don't understand what you mean by "'char *' is on the stack".
> Since it's a pointer, what should be on the stack: the pointer or the
> memory to which it points?

See above, If is passed by value we're fine - the char * is on the
stack. With the caveat I mention.

> Similarly for buffer text: the buffer object is a member of 'struct
> coding_system', which is on the stack, and the 'char *' points to the
> text of that buffer.

Same as above for strings: if we're passing the struct around by value
etc.

>> Solution is to force ambig tracing, and therefore immovability, by
>> adding an ambig root.
>
> What is an "ambig root" and how to add it?

In igc, one adds an ambig root with

static igc_root_list *
root_create_ambig (struct igc *gc, void *start, void *end)
{
  return root_create (gc, start, end, mps_rank_ambig (), scan_ambig, true);
}

GC is something like a global registry for MPS administrative objects
(roots, threads, pool, allocaton points, ...).

[START, END) is the area to be be scanned as root.

There are some higher-level functions in the igc "API" that use that
function for creating roots. Example, there is igc_xzalloc_ambig, which
is like xzalloc but makes the allocated memory also an ambig root.

The ambig root itself is represented as an mps_root_t, some pointer,
that I record in struct igc, the registry, for some checks, and to be
able to clean up at the end (currently not done).

>> > More generally, what happens with text of Emacs buffers and strings
>> > when GC runs?  I know what the old GC did, but what does the new GC
>> > do?
>>
>> For buffer text I'm pretty sure nothing happens, but I'm meanwhile a bit
>> cautious because of features I potentially don't know. For me, buffer
>> text is still either from mmap or some malloc variant. If that's still
>> the case, MPS does nothing affecting buffer text.
>
> So the relocation of buffer text that the old GC did is no longer done
> by MPS?

Compact_buffers is one of the things that I mentioned that
gargabe_collect currently does that are not directly related to GC. It's
like with the undo lists and markers and so on. We need to find a place
when/where to do that, maybe a timer function, or maybe when MPS posts
us a message.

That's currently indeed open, but it's clear what to do.

>> > A related question, no less important, is: when can MPS GC happen, and
>> > whether there's any reasonable way of being sure that it cannot
>> > possibly happen while certain code fragments run?
>>
>> By default MPS runs concurrently, so always.
>
> Then what are the rules to make sure a C pointer to a struct that
> backs a Lisp object (for example, 'struct window' which backs a window
> object) is safe to have around and use in our usual style, viz.:
>
>   struct window *w = XWINDOW (some_window);
>   ...
>   /* use the w pointer as a normal C pointer: dereference it, access
>      and set window fields via w->foo, pass w to functions, etc. */
>
> Is it enough to make sure w is on the stack?

Yes, no other rules.

> What if I do something like this:
>
>  static struct window *my_special_window = NULL;
>  void foo (Lisp_Object window)
>  {
>    my_special_window = XWINDOW (window);
>    ...
>  }
>  void bar (void)
>  {
>    if (my_special_window != NULL)
>      /* do something with my_special_window */
>  }
>
> IOW, is storing a pointer like above in a temporary global variable
> safe?

You need to staticpro.

It's basically like with the old GC. Nothing makes sure that the window
has survived when bar is called, withput staticpro. The difference to
the MPS GC is that my_special_window might have survived in the old GC
because something else keeps it alive and that is all we need (fond
memories of GCPRO, booooh ;-)).

With MPS, my_special_window is also kept alive by that other reference,
like in the old GC, but the window may move in memory while it is alive,
and when we don't trace my_special_window it will point to where the
window was when my_special_window was assigned to.

>> I think the only thing you missed is why the automatic ambig scanning of
>> registers and control stack is not sufficient in Helmut's case (the
>> struct containing the char * etc.).
>
> Why is it not safe?  I still don't think I understand that.

It is the missing reference from the stack. DOes the above help somewhat?

Re: MPS GC and its implications

[Eli Zaretskii]

> From: Gerd Möllmann <gerd.moellmann@gmail.com>
> Cc: eller.helmut@gmail.com,  emacs-devel@gnu.org
> Date: Wed, 01 May 2024 17:51:47 +0200
> 
> > First, I don't understand why the above mechanism doesn't work.  What
> > exactly are the conditions which ensure that "ambig scanning of
> > registers and the C stack" will work?
> 
> The condition is: Some automatic C variable in the stack frames makes it
> so that either the stack itself or a register contains the reference, in
> this case a pointer to the string data (maybe interior pointer).
> 
> That's not the case, or not guaranteed, for example, if we store the
> string pointer in some global variable, in our case in a struct member
> of a static array of structs.

How do you see that it's a static array of structs?

The way a coding_system is usually set when starting an
encoding/decoding operation is like this:

  Lisp_Object
  code_convert_string (Lisp_Object string, Lisp_Object coding_system,
		       Lisp_Object dst_object, bool encodep, bool nocopy,
		       bool norecord)
  {
    struct coding_system coding;
    [...]
    setup_coding_system (coding_system, &coding);
    [...]
    if (encodep)
      encode_coding_object (&coding, string, 0, 0, chars, bytes, dst_object);
    else
      decode_coding_object (&coding, string, 0, 0, chars, bytes, dst_object);

So the pointer to a 'struct coding_system' that is thereafter passed
from encode_coding_object/decode_coding_object downwards (and
eventually ends up in coding_set_source etc.), starts as a stack-based
automatic variable at the top-level caller.  Isn't that what we need
for MPS to DTRT with any pointers to Lisp objects in 'struct
coding_system'?

> > Anyway: 'struct coding_system' is passed as argument to the functions
> > involved in the en/decoding, and so is on the stack or in a register.
> 
> If it's passed by value, we are fine. If its passed as a pointer we're
> not because the char * in question is not an the stack.

See above: it _is_ on the stack, right?

> If it passed by value but was originally copied from a static
> counterpart, we are probably not good, because that would copy the char
> * from something we haven't traced.

I thought we agreed that all the global variables with Lisp objects
need to be staticpro'd, didn't we?  If we do that, we are covered for
anything that comes from there, no?

> > Similarly for buffer text: the buffer object is a member of 'struct
> > coding_system', which is on the stack, and the 'char *' points to the
> > text of that buffer.
> 
> Same as above for strings: if we're passing the struct around by value
> etc.

Is MPS tracing buffer text?  I'm looking at fix_buffer, and I don't
see where we do something with the own_text->beg there.  Is that
because the text is allocated not by MPS, and therefore isn't moved by
GC?

> > What if I do something like this:
> >
> >  static struct window *my_special_window = NULL;
> >  void foo (Lisp_Object window)
> >  {
> >    my_special_window = XWINDOW (window);
> >    ...
> >  }
> >  void bar (void)
> >  {
> >    if (my_special_window != NULL)
> >      /* do something with my_special_window */
> >  }
> >
> > IOW, is storing a pointer like above in a temporary global variable
> > safe?
> 
> You need to staticpro.
> 
> It's basically like with the old GC. Nothing makes sure that the window
> has survived when bar is called, withput staticpro. The difference to
> the MPS GC is that my_special_window might have survived in the old GC
> because something else keeps it alive and that is all we need (fond
> memories of GCPRO, booooh ;-)).

The old GC didn't move objects.  So the window could have been deleted
meanwhile, but its 'struct window' couldn't have moved to a different
address in memory.

> >> I think the only thing you missed is why the automatic ambig scanning of
> >> registers and control stack is not sufficient in Helmut's case (the
> >> struct containing the char * etc.).
> >
> > Why is it not safe?  I still don't think I understand that.
> 
> It is the missing reference from the stack. DOes the above help somewhat?

Depends on how you answer my questions and observations above.

Re: MPS GC and its implications

[Gerd Möllmann]

Eli Zaretskii <eliz@gnu.org> writes:

> How do you see that it's a static array of structs?

a326d6aed816c19f66729a7ff77d90ef6bcec07c
Author:     Helmut Eller <eller.helmut@gmail.com>
AuthorDate: Tue Apr 30 11:04:54 2024 +0200
Commit:     Gerd Möllmann <gerd@gnu.org>
CommitDate: Tue Apr 30 20:59:38 2024 +0200

Parent:     44100e5b081 Add missing #ifdef HAVE_MPS
Merged:     emacs-29
Contained:  scratch/igc
Follows:    igc-first-commit (76)

Staticpro coding_categories

* src/coding.c (syms_of_coding):

1 file changed, 10 insertions(+)
src/coding.c | 10 ++++++++++

modified   src/coding.c
@@ -11848,6 +11848,16 @@ syms_of_coding (void)
 
   Vcoding_category_table = make_nil_vector (coding_category_max);
   staticpro (&Vcoding_category_table);
+  for (size_t i; i < ARRAYELTS (coding_categories); i++)
+    {
+      struct coding_system* cs = &coding_categories[i];
+      Lisp_Object *src = &cs->src_object;
+      *src = Qnil;
+      staticpro (src);
+      Lisp_Object *dst = &cs->dst_object;
+      *dst = Qnil;
+      staticpro (dst);
+    }
   /* Followings are target of code detection.  */
   ASET (Vcoding_category_table, coding_category_iso_7,
 	intern_c_string ("coding-category-iso-7"));

> The way a coding_system is usually set when starting an
> encoding/decoding operation is like this:
>
>   Lisp_Object
>   code_convert_string (Lisp_Object string, Lisp_Object coding_system,
> 		       Lisp_Object dst_object, bool encodep, bool nocopy,
> 		       bool norecord)
>   {
>     struct coding_system coding;
>     [...]
>     setup_coding_system (coding_system, &coding);
>     [...]
>     if (encodep)
>       encode_coding_object (&coding, string, 0, 0, chars, bytes, dst_object);
>     else
>       decode_coding_object (&coding, string, 0, 0, chars, bytes, dst_object);
>
> So the pointer to a 'struct coding_system' that is thereafter passed
> from encode_coding_object/decode_coding_object downwards (and
> eventually ends up in coding_set_source etc.), starts as a stack-based
> automatic variable at the top-level caller.  Isn't that what we need
> for MPS to DTRT with any pointers to Lisp objects in 'struct
> coding_system'?

Yes!

>
>> > Anyway: 'struct coding_system' is passed as argument to the functions
>> > involved in the en/decoding, and so is on the stack or in a register.
>> 
>> If it's passed by value, we are fine. If its passed as a pointer we're
>> not because the char * in question is not an the stack.
>
> See above: it _is_ on the stack, right?

Yes!

>> If it passed by value but was originally copied from a static
>> counterpart, we are probably not good, because that would copy the char
>> * from something we haven't traced.
>
> I thought we agreed that all the global variables with Lisp objects
> need to be staticpro'd, didn't we?  If we do that, we are covered for
> anything that comes from there, no?

See the commit at the start. It seems coding_categories isn't
Lisp_Objecut and thus cannot be staticpro'd.

>> > Similarly for buffer text: the buffer object is a member of 'struct
>> > coding_system', which is on the stack, and the 'char *' points to the
>> > text of that buffer.
>> 
>> Same as above for strings: if we're passing the struct around by value
>> etc.
>
> Is MPS tracing buffer text?  I'm looking at fix_buffer, and I don't
> see where we do something with the own_text->beg there.  Is that
> because the text is allocated not by MPS, and therefore isn't moved by
> GC?

Buffer text is not managed by MPS, so it doesn't do anything with it.

>> > What if I do something like this:
>> >
>> >  static struct window *my_special_window = NULL;
>> >  void foo (Lisp_Object window)
>> >  {
>> >    my_special_window = XWINDOW (window);
>> >    ...
>> >  }
>> >  void bar (void)
>> >  {
>> >    if (my_special_window != NULL)
>> >      /* do something with my_special_window */
>> >  }
>> >
>> > IOW, is storing a pointer like above in a temporary global variable
>> > safe?
>> 
>> You need to staticpro.
>> 
>> It's basically like with the old GC. Nothing makes sure that the window
>> has survived when bar is called, withput staticpro. The difference to
>> the MPS GC is that my_special_window might have survived in the old GC
>> because something else keeps it alive and that is all we need (fond
>> memories of GCPRO, booooh ;-)).
>
> The old GC didn't move objects.  So the window could have been deleted
> meanwhile, but its 'struct window' couldn't have moved to a different
> address in memory.

Hm, I don't understand where you are aiming. I find the code is
incorrect because nothing really guarantees, in code, that
my_special_window is live when bar is called. Unless we staticpro.

Re: MPS GC and its implications

[Eli Zaretskii]

> From: Gerd Möllmann <gerd.moellmann@gmail.com>
> Cc: eller.helmut@gmail.com,  emacs-devel@gnu.org
> Date: Wed, 01 May 2024 19:17:01 +0200
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > How do you see that it's a static array of structs?
> 
> a326d6aed816c19f66729a7ff77d90ef6bcec07c
> Author:     Helmut Eller <eller.helmut@gmail.com>
> AuthorDate: Tue Apr 30 11:04:54 2024 +0200
> Commit:     Gerd Möllmann <gerd@gnu.org>
> CommitDate: Tue Apr 30 20:59:38 2024 +0200
> 
> Parent:     44100e5b081 Add missing #ifdef HAVE_MPS
> Merged:     emacs-29
> Contained:  scratch/igc
> Follows:    igc-first-commit (76)
> 
> Staticpro coding_categories
> 
> * src/coding.c (syms_of_coding):
> 
> 1 file changed, 10 insertions(+)
> src/coding.c | 10 ++++++++++
> 
> modified   src/coding.c
> @@ -11848,6 +11848,16 @@ syms_of_coding (void)
>  
>    Vcoding_category_table = make_nil_vector (coding_category_max);
>    staticpro (&Vcoding_category_table);
> +  for (size_t i; i < ARRAYELTS (coding_categories); i++)
> +    {
> +      struct coding_system* cs = &coding_categories[i];
> +      Lisp_Object *src = &cs->src_object;
> +      *src = Qnil;
> +      staticpro (src);
> +      Lisp_Object *dst = &cs->dst_object;
> +      *dst = Qnil;
> +      staticpro (dst);
> +    }
>    /* Followings are target of code detection.  */
>    ASET (Vcoding_category_table, coding_category_iso_7,
>  	intern_c_string ("coding-category-iso-7"));

I'm not sure I get your point.  The contents of coding_categories[] is
AFAIK never used to actually encode or decode something.  It is just a
handy db of known coding categories for using in all kinds of APIs
that need to examine the known categories for some reason or other.

> > I thought we agreed that all the global variables with Lisp objects
> > need to be staticpro'd, didn't we?  If we do that, we are covered for
> > anything that comes from there, no?
> 
> See the commit at the start. It seems coding_categories isn't
> Lisp_Objecut and thus cannot be staticpro'd.

I'm not sure I understand why we need that loop.  The src_object and
dst_object members of coding_categories[] is AFAICT never used
anywhere.  The values of src_object and dst_object were AFAICT never
set in the elements of coding_categories[], they are only set now by
that loop, and for what purpose? just so we could staticpro them?

In general, these are coding categories, not coding systems, AFAIK
they are just stored as coding-systems because that's convenient.

> >> It's basically like with the old GC. Nothing makes sure that the window
> >> has survived when bar is called, withput staticpro. The difference to
> >> the MPS GC is that my_special_window might have survived in the old GC
> >> because something else keeps it alive and that is all we need (fond
> >> memories of GCPRO, booooh ;-)).
> >
> > The old GC didn't move objects.  So the window could have been deleted
> > meanwhile, but its 'struct window' couldn't have moved to a different
> > address in memory.
> 
> Hm, I don't understand where you are aiming. I find the code is
> incorrect because nothing really guarantees, in code, that
> my_special_window is live when bar is called. Unless we staticpro.

staticpro doesn't guarantee that the window is live, either.  Unless
we are talking about two different meanings of "live".  What I meant
by "live" is a window that was not deleted meanwhile; that can be
tested with WINDOW_LIVE_P.

The old GC didn't delete windows; if a window was live before GC, it
would still be live after GC, and its 'struct window' will be at the
same address.  By contrast, with MPS a live window could move to a
different address, so my pointer will no longer point to it after GC.

Re: MPS GC and its implications

[Helmut Eller]

>> I'm not sure what is with the buffer case, but the string case means
>> that source can point to string data, which is in our MPS leaf_pool. I
>> think we cvould use an exact scan function for that purpose, analogous
>> to scan_specpdl. WSYT?
>
> If this is indeed needed, we are in trouble.  When we take a C pointer
> to buffer or string text and use it in the following code, it's
> because there's a hidden assumption that GC cannot happen during the
> time the C pointer is being used.  If this assumption is not true with
> MPS, nothing in Emacs will work reliably.
>
> Is this indeed a problem?  Didn't Gerd just explain to Andrea that
> every object referenced from the stack or even from a register will be
> automatically "fixed" if GC happens and decides to move the objects?
> If so, why doesn't that solve the problem here?

The stack and registers are ambiguous roots.  The objects they point to
are "pinned" i.e. not allowed to move.  So as long as we keep a pointer
in a local variable, things should work out fine.

For references in global variables, we can decide to make them exact or
ambiguous.  If we make them exact, we have to be very careful.

> More generally, what happens with text of Emacs buffers and strings
> when GC runs?  I know what the old GC did, but what does the new GC
> do?

If we only have exact references to those, then they can be moved.

> A related question, no less important, is: when can MPS GC happen, and
> whether there's any reasonable way of being sure that it cannot
> possibly happen while certain code fragments run?

Would be nice to handle one catastrophe at the time :-)

> For example, the
> old GC could only be triggered by a call to funcall or eval (and a
> small number of similar APIs); as long as the C code didn't cause any
> of these calls, we could assume GC will not happen, and so any C
> pointers to Lisp objects were safe to use.  What is the equivalent of
> that with MPS?  Since MPS is a moving GC, this becomes much more
> important; with old GC we only had 2 object types that could move
> during GC: string data and buffer text.  Now every Lisp object is in
> danger of being moved by GC, AFAIU.
>
> Am I missing something?

I think MPS is a concurrent GC, it means it runs "all the time".  The
reason why his works are the memory barriers.  But maybe Gerd knows how
things actually work.

Helmut

Re: MPS GC and its implications

[Gerd Möllmann]

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

> Would be nice to handle one catastrophe at the time :-)

:-)

>
>> For example, the
>> old GC could only be triggered by a call to funcall or eval (and a
>> small number of similar APIs); as long as the C code didn't cause any
>> of these calls, we could assume GC will not happen, and so any C
>> pointers to Lisp objects were safe to use.  What is the equivalent of
>> that with MPS?  Since MPS is a moving GC, this becomes much more
>> important; with old GC we only had 2 object types that could move
>> during GC: string data and buffer text.  Now every Lisp object is in
>> danger of being moved by GC, AFAIU.
>>
>> Am I missing something?
>
> I think MPS is a concurrent GC, it means it runs "all the time".  The
> reason why his works are the memory barriers.  But maybe Gerd knows how
> things actually work.

You're exactly right.

On a side note: I'm beginning to suspect that that char * and so on is
incorrect with the old GC as well? String data can move with the old GC
(compact_small_strings).

Re: MPS GC and its implications

[Eli Zaretskii]

> From: Gerd Möllmann <gerd.moellmann@gmail.com>
> Cc: Eli Zaretskii <eliz@gnu.org>,  emacs-devel@gnu.org
> Date: Wed, 01 May 2024 16:05:17 +0200
> 
> On a side note: I'm beginning to suspect that that char * and so on is
> incorrect with the old GC as well? String data can move with the old GC
> (compact_small_strings).

Not while the code in coding.c between coding_set_source and the end
of processing runs: GC cannot happen there.

Re: MPS GC and its implications

[Gerd Möllmann]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Gerd Möllmann <gerd.moellmann@gmail.com>
>> Cc: Eli Zaretskii <eliz@gnu.org>,  emacs-devel@gnu.org
>> Date: Wed, 01 May 2024 16:05:17 +0200
>> 
>> On a side note: I'm beginning to suspect that that char * and so on is
>> incorrect with the old GC as well? String data can move with the old GC
>> (compact_small_strings).
>
> Not while the code in coding.c between coding_set_source and the end
> of processing runs: GC cannot happen there.

That's true, but when is the char *source set? It's in a global var, and
GC hasn't always been inhibited... That's what I'm thinking about.

Re: MPS GC and its implications

[Eli Zaretskii]

> From: Gerd Möllmann <gerd.moellmann@gmail.com>
> Cc: eller.helmut@gmail.com,  emacs-devel@gnu.org
> Date: Wed, 01 May 2024 17:54:09 +0200
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> From: Gerd Möllmann <gerd.moellmann@gmail.com>
> >> Cc: Eli Zaretskii <eliz@gnu.org>,  emacs-devel@gnu.org
> >> Date: Wed, 01 May 2024 16:05:17 +0200
> >> 
> >> On a side note: I'm beginning to suspect that that char * and so on is
> >> incorrect with the old GC as well? String data can move with the old GC
> >> (compact_small_strings).
> >
> > Not while the code in coding.c between coding_set_source and the end
> > of processing runs: GC cannot happen there.
> 
> That's true, but when is the char *source set? It's in a global var, and
> GC hasn't always been inhibited... That's what I'm thinking about.

It isn't in a global variable, AFAICT.

Re: MPS GC and its implications

[Gerd Möllmann]

Eli Zaretskii <eliz@gnu.org> writes:

>> That's true, but when is the char *source set? It's in a global var, and
>> GC hasn't always been inhibited... That's what I'm thinking about.
>
> It isn't in a global variable, AFAICT.

I have to invoke Helmut, I think it is? Or is the char *source member
unused if coding_categories, if that's the right name?

Re: MPS GC and its implications

[Helmut Eller]

> Eli Zaretskii <eliz@gnu.org> writes:
>
>>> That's true, but when is the char *source set? It's in a global var, and
>>> GC hasn't always been inhibited... That's what I'm thinking about.
>>
>> It isn't in a global variable, AFAICT.
>
> I have to invoke Helmut, I think it is? Or is the char *source member
> unused if coding_categories, if that's the right name?

In coding.c, there is this global variable:
static struct coding_system coding_categories[coding_category_max];

So it's definitely in a global variable.

Re: MPS GC and its implications

[Eli Zaretskii]

> From: Helmut Eller <eller.helmut@gmail.com>
> Cc: Eli Zaretskii <eliz@gnu.org>,  emacs-devel@gnu.org
> Date: Wed, 01 May 2024 19:38:14 +0200
> 
> > Eli Zaretskii <eliz@gnu.org> writes:
> >
> >>> That's true, but when is the char *source set? It's in a global var, and
> >>> GC hasn't always been inhibited... That's what I'm thinking about.
> >>
> >> It isn't in a global variable, AFAICT.
> >
> > I have to invoke Helmut, I think it is? Or is the char *source member
> > unused if coding_categories, if that's the right name?
> 
> In coding.c, there is this global variable:
> static struct coding_system coding_categories[coding_category_max];
> 
> So it's definitely in a global variable.

Where do you see those "coding systems" in coding_categories[] used
for actually decoding or encoding anything?  I don't see that.

AFAIK, coding_categories[] is just a db of known coding categories,
and they are stored as 'struct coding_system's because we then can use
setup_coding_system to populate these categories with relevant data.
But they are never used to en/decode anything, and in fact the
src_object and dst_object fields of those "coding systems" were never
even set, until you added that loop.  Or what am I missing?

Re: MPS GC and its implications

[Helmut Eller]

>> So it's definitely in a global variable.
>
> Where do you see those "coding systems" in coding_categories[] used
> for actually decoding or encoding anything?  I don't see that.
>
> AFAIK, coding_categories[] is just a db of known coding categories,
> and they are stored as 'struct coding_system's because we then can use
> setup_coding_system to populate these categories with relevant data.
> But they are never used to en/decode anything, and in fact the
> src_object and dst_object fields of those "coding systems" were never
> even set, until you added that loop.  Or what am I missing?

I know this code hardly at all; not inside-out like you. I just looked
at the types.

Re: MPS GC and its implications

[Gerd Möllmann]

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

>>> So it's definitely in a global variable.
>>
>> Where do you see those "coding systems" in coding_categories[] used
>> for actually decoding or encoding anything?  I don't see that.
>>
>> AFAIK, coding_categories[] is just a db of known coding categories,
>> and they are stored as 'struct coding_system's because we then can use
>> setup_coding_system to populate these categories with relevant data.
>> But they are never used to en/decode anything, and in fact the
>> src_object and dst_object fields of those "coding systems" were never
>> even set, until you added that loop.  Or what am I missing?
>
> I know this code hardly at all; not inside-out like you. I just looked
> at the types.

And I fear if such depth is required, we'll make no progress.
Hopefully this has helped Eli, though.

Re: MPS GC and its implications

[Eli Zaretskii]

> From: Gerd Möllmann <gerd.moellmann@gmail.com>
> Cc: Eli Zaretskii <eliz@gnu.org>,  emacs-devel@gnu.org
> Date: Wed, 01 May 2024 20:07:53 +0200
> 
> Helmut Eller <eller.helmut@gmail.com> writes:
> 
> >>> So it's definitely in a global variable.
> >>
> >> Where do you see those "coding systems" in coding_categories[] used
> >> for actually decoding or encoding anything?  I don't see that.
> >>
> >> AFAIK, coding_categories[] is just a db of known coding categories,
> >> and they are stored as 'struct coding_system's because we then can use
> >> setup_coding_system to populate these categories with relevant data.
> >> But they are never used to en/decode anything, and in fact the
> >> src_object and dst_object fields of those "coding systems" were never
> >> even set, until you added that loop.  Or what am I missing?
> >
> > I know this code hardly at all; not inside-out like you. I just looked
> > at the types.
> 
> And I fear if such depth is required, we'll make no progress.
> Hopefully this has helped Eli, though.

It helped some, yes.

Re: MPS GC and its implications

[Helmut Eller]

>> I know this code hardly at all; not inside-out like you. I just looked
>> at the types.
>
> And I fear if such depth is required, we'll make no progress.
> Hopefully this has helped Eli, though.

We could always go back to the test-driven-debugging model.  Only write
code that fixes an actual test case.

Re: MPS GC and its implications

[Gerd Möllmann]

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

>>> I know this code hardly at all; not inside-out like you. I just looked
>>> at the types.
>>
>> And I fear if such depth is required, we'll make no progress.
>> Hopefully this has helped Eli, though.
>
> We could always go back to the test-driven-debugging model.  Only write
> code that fixes an actual test case.

Maybe.

(I would like to reach a point where one could assess interactive user
experience in a non-debug build that is somewhat usable, e.g. doesn't
crash every 10 minutes. That would at tell show if what we are doing is
worth it, you know.)

Re: MPS GC and its implications

[Eli Zaretskii]

> From: Gerd Möllmann <gerd.moellmann@gmail.com>
> Cc: Eli Zaretskii <eliz@gnu.org>,  emacs-devel@gnu.org
> Date: Wed, 01 May 2024 21:12:53 +0200
> 
> (I would like to reach a point where one could assess interactive user
> experience in a non-debug build that is somewhat usable, e.g. doesn't
> crash every 10 minutes. That would at tell show if what we are doing is
> worth it, you know.)

Agreed.

Re: MPS GC and its implications

[Eli Zaretskii]

> From: Helmut Eller <eller.helmut@gmail.com>
> Cc: gerd.moellmann@gmail.com,  emacs-devel@gnu.org
> Date: Wed, 01 May 2024 20:01:56 +0200
> 
> >> So it's definitely in a global variable.
> >
> > Where do you see those "coding systems" in coding_categories[] used
> > for actually decoding or encoding anything?  I don't see that.
> >
> > AFAIK, coding_categories[] is just a db of known coding categories,
> > and they are stored as 'struct coding_system's because we then can use
> > setup_coding_system to populate these categories with relevant data.
> > But they are never used to en/decode anything, and in fact the
> > src_object and dst_object fields of those "coding systems" were never
> > even set, until you added that loop.  Or what am I missing?
> 
> I know this code hardly at all; not inside-out like you. I just looked
> at the types.

That's okay; your addition cannot do any harm.  It just added 42 more
objects to staticpro, that's all.

Re: MPS GC and its implications

[Eli Zaretskii]

> From: Gerd Möllmann <gerd.moellmann@gmail.com>
> Cc: eller.helmut@gmail.com,  emacs-devel@gnu.org
> Date: Wed, 01 May 2024 19:19:30 +0200
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> That's true, but when is the char *source set? It's in a global var, and
> >> GC hasn't always been inhibited... That's what I'm thinking about.
> >
> > It isn't in a global variable, AFAICT.
> 
> I have to invoke Helmut, I think it is? Or is the char *source member
> unused if coding_categories, if that's the right name?

AFAICT, it is indeed unused.  Those aren't coding-systems, we just use
struct coding_system because it's convenient.

Re: MPS GC and its implications

[Gerd Möllmann]

Eli Zaretskii <eliz@gnu.org> writes:

>> I have to invoke Helmut, I think it is? Or is the char *source member
>> unused if coding_categories, if that's the right name?
>
> AFAICT, it is indeed unused.  Those aren't coding-systems, we just use
> struct coding_system because it's convenient.

Then is propose to add a comment, and forget about coding.c.
No one can guess that.

Re: MPS GC and its implications

[Eli Zaretskii]

> From: Gerd Möllmann <gerd.moellmann@gmail.com>
> Cc: eller.helmut@gmail.com,  emacs-devel@gnu.org
> Date: Wed, 01 May 2024 19:59:32 +0200
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> I have to invoke Helmut, I think it is? Or is the char *source member
> >> unused if coding_categories, if that's the right name?
> >
> > AFAICT, it is indeed unused.  Those aren't coding-systems, we just use
> > struct coding_system because it's convenient.
> 
> Then is propose to add a comment, and forget about coding.c.
> No one can guess that.

What would you like the comment to say?  The comment to the variable
already says they are categories:

  /* Table of coding-categories ordered by priority.  */
  static enum coding_category coding_priorities[coding_category_max];

Re: MPS GC and its implications

[Gerd Möllmann]

Eli Zaretskii <eliz@gnu.org> writes:

>   /* Table of coding-categories ordered by priority.  */
>   static enum coding_category coding_priorities[coding_category_max];

Good for someone knowing knowing what that means :-).

No, I meant something like "FIXME: possibnly unneccessary" or something
where we do the staticpros.

Re: MPS GC and its implications

[Eli Zaretskii]

> From: Gerd Möllmann <gerd.moellmann@gmail.com>
> Cc: eller.helmut@gmail.com,  emacs-devel@gnu.org
> Date: Wed, 01 May 2024 21:06:52 +0200
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >   /* Table of coding-categories ordered by priority.  */
> >   static enum coding_category coding_priorities[coding_category_max];
> 
> Good for someone knowing knowing what that means :-).
> 
> No, I meant something like "FIXME: possibnly unneccessary" or something
> where we do the staticpros.

Done.

Re: MPS GC and its implications

[Gerd Möllmann]

Eli Zaretskii <eliz@gnu.org> writes:

>> No, I meant something like "FIXME: possibnly unneccessary" or something
>> where we do the staticpros.
>
> Done.

Thanks

Re: MPS GC and its implications

[Andrea Corallo]

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

>>> I'm not sure what is with the buffer case, but the string case means
>>> that source can point to string data, which is in our MPS leaf_pool. I
>>> think we cvould use an exact scan function for that purpose, analogous
>>> to scan_specpdl. WSYT?
>>
>> If this is indeed needed, we are in trouble.  When we take a C pointer
>> to buffer or string text and use it in the following code, it's
>> because there's a hidden assumption that GC cannot happen during the
>> time the C pointer is being used.  If this assumption is not true with
>> MPS, nothing in Emacs will work reliably.
>>
>> Is this indeed a problem?  Didn't Gerd just explain to Andrea that
>> every object referenced from the stack or even from a register will be
>> automatically "fixed" if GC happens and decides to move the objects?
>> If so, why doesn't that solve the problem here?
>
> The stack and registers are ambiguous roots.  The objects they point to
> are "pinned" i.e. not allowed to move.  So as long as we keep a pointer
> in a local variable, things should work out fine.

[...]

>
> I think MPS is a concurrent GC, it means it runs "all the time".  The
> reason why his works are the memory barriers.  But maybe Gerd knows how
> things actually work.

It is still very unclear how it can move objects "all the time".  During
execution traced objects can be loaded on the stack and unloaded all the
time (Eli showed other examples of that) so I don't see ATM how it can
move objects without actually pausing the mutator thread.

  Andrea

Re: MPS GC and its implications

[Gerd Möllmann]

Andrea Corallo <acorallo@gnu.org> writes:

>> I think MPS is a concurrent GC, it means it runs "all the time".  The
>> reason why his works are the memory barriers.  But maybe Gerd knows how
>> things actually work.
>
> It is still very unclear how it can move objects "all the time".  During
> execution traced objects can be loaded on the stack and unloaded all the
> time (Eli showed other examples of that) so I don't see ATM how it can
> move objects without actually pausing the mutator thread.

Ja, all the time is maybe a bit too coarse grained an expression.

Maybe one could say it this way: MPS is concurrent and incremental, with
the two together meaning that it does its work in chunks in another
thread. Insofar it's not all the time but in chunks.

Don't know if that helps.

Re: MPS GC and its implications

[Eli Zaretskii]

> From: Gerd Möllmann <gerd.moellmann@gmail.com>
> Cc: Helmut Eller <eller.helmut@gmail.com>,  Eli Zaretskii <eliz@gnu.org>,
>   emacs-devel@gnu.org
> Date: Fri, 03 May 2024 07:42:16 +0200
> 
> Andrea Corallo <acorallo@gnu.org> writes:
> 
> >> I think MPS is a concurrent GC, it means it runs "all the time".  The
> >> reason why his works are the memory barriers.  But maybe Gerd knows how
> >> things actually work.
> >
> > It is still very unclear how it can move objects "all the time".  During
> > execution traced objects can be loaded on the stack and unloaded all the
> > time (Eli showed other examples of that) so I don't see ATM how it can
> > move objects without actually pausing the mutator thread.
> 
> Ja, all the time is maybe a bit too coarse grained an expression.
> 
> Maybe one could say it this way: MPS is concurrent and incremental, with
> the two together meaning that it does its work in chunks in another
> thread. Insofar it's not all the time but in chunks.
> 
> Don't know if that helps.

I think this still leaves something unclear, at least to me.

Let's say some Lisp thread in Emacs is running, and MPS decides to
perform GC.  AFAIU, the GC will run in a separate thread, right?  So
the question that bugs me is how does the GC thread and the Lisp
thread synchronize so as to prevent the situation where GC moves an
object in-between two accesses to the object by the Lisp thread?  If
the Lisp thread is stopped while GC runs, the answer is clear.  But if
the Lisp thread continues running concurrently with GC, I don't
understand how this could possibly work, especially on a modern CPU
with N > 1 execution units.  In particular, how does MPS handle the
time window between moving an object and updating all the references
to the object, so that once an object is moved, all accesses to the
object are redirected to the new location?  If nothing is done, it is
possible that a Lisp thread accesses the old address which is no
longer valid, because the object was moved between the time the Lisp
thread loaded the address into a register and the time it uses the
address to access the object, but the address in the register was not
yet updated.

What am I missing?

Re: MPS GC and its implications

[Helmut Eller]

On Fri, May 03 2024, Eli Zaretskii wrote:

> I think this still leaves something unclear, at least to me.
>
> Let's say some Lisp thread in Emacs is running, and MPS decides to
> perform GC.  AFAIU, the GC will run in a separate thread, right?  So
> the question that bugs me is how does the GC thread and the Lisp
> thread synchronize so as to prevent the situation where GC moves an
> object in-between two accesses to the object by the Lisp thread?  If
> the Lisp thread is stopped while GC runs, the answer is clear.  But if
> the Lisp thread continues running concurrently with GC, I don't
> understand how this could possibly work, especially on a modern CPU
> with N > 1 execution units.  In particular, how does MPS handle the
> time window between moving an object and updating all the references
> to the object, so that once an object is moved, all accesses to the
> object are redirected to the new location?  If nothing is done, it is
> possible that a Lisp thread accesses the old address which is no
> longer valid, because the object was moved between the time the Lisp
> thread loaded the address into a register and the time it uses the
> address to access the object, but the address in the register was not
> yet updated.
>
> What am I missing?

Memory barriers could be used for synchronization.  When the mutator
hits a barrier, it executes the signal handler.  The signal handler asks
the collector what to do.  Essentially synchronizing those two.

I don't know if MPS works quite like this, but it could.  I'm not
worried about this issue.

Re: MPS GC and its implications

[Eli Zaretskii]

> From: Helmut Eller <eller.helmut@gmail.com>
> Cc: Gerd Möllmann <gerd.moellmann@gmail.com>,
>   acorallo@gnu.org,
>   emacs-devel@gnu.org
> Date: Fri, 03 May 2024 13:32:31 +0200
> 
> On Fri, May 03 2024, Eli Zaretskii wrote:
> 
> > I think this still leaves something unclear, at least to me.
> >
> > Let's say some Lisp thread in Emacs is running, and MPS decides to
> > perform GC.  AFAIU, the GC will run in a separate thread, right?  So
> > the question that bugs me is how does the GC thread and the Lisp
> > thread synchronize so as to prevent the situation where GC moves an
> > object in-between two accesses to the object by the Lisp thread?  If
> > the Lisp thread is stopped while GC runs, the answer is clear.  But if
> > the Lisp thread continues running concurrently with GC, I don't
> > understand how this could possibly work, especially on a modern CPU
> > with N > 1 execution units.  In particular, how does MPS handle the
> > time window between moving an object and updating all the references
> > to the object, so that once an object is moved, all accesses to the
> > object are redirected to the new location?  If nothing is done, it is
> > possible that a Lisp thread accesses the old address which is no
> > longer valid, because the object was moved between the time the Lisp
> > thread loaded the address into a register and the time it uses the
> > address to access the object, but the address in the register was not
> > yet updated.
> >
> > What am I missing?
> 
> Memory barriers could be used for synchronization.  When the mutator
> hits a barrier, it executes the signal handler.  The signal handler asks
> the collector what to do.  Essentially synchronizing those two.

If this happens, I should have seen GDB intercept the exception and
kick in.  But it doesn't.  (It also would have slowed the program
quite a lot, I think.)

> I don't know if MPS works quite like this, but it could.  I'm not
> worried about this issue.

I'm worried because it sounds like we don't have a clear understanding
how this stuff works, and without that we are basically stabbing in
the dark when faced with a particular issue related to the MPS
migration.

Re: MPS GC and its implications

[Gerd Möllmann]

Eli Zaretskii <eliz@gnu.org> writes:

> I'm worried because it sounds like we don't have a clear understanding
> how this stuff works, and without that we are basically stabbing in
> the dark when faced with a particular issue related to the MPS
> migration.

Can't say anything about GDB. Here on macOS, I can, if I want, see the
Mach exceptions being handled, and I've landed there in LLDB as well.

For the rest I can only hope that some confidence will grow over time.
Have you read the old MPS paper from 2002? Maybe that helps.

Re: MPS GC and its implications

[Eli Zaretskii]

> From: Gerd Möllmann <gerd.moellmann@gmail.com>
> Cc: Helmut Eller <eller.helmut@gmail.com>,  acorallo@gnu.org,
>   emacs-devel@gnu.org
> Date: Fri, 03 May 2024 13:59:36 +0200
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > I'm worried because it sounds like we don't have a clear understanding
> > how this stuff works, and without that we are basically stabbing in
> > the dark when faced with a particular issue related to the MPS
> > migration.
> 
> Can't say anything about GDB. Here on macOS, I can, if I want, see the
> Mach exceptions being handled, and I've landed there in LLDB as well.

The same happens in GDB.  If the result of the exception is SIGSEGV,
then GDB should kick in, I think.  Didn't Helmut say something several
days ago that he needs to tell GDB to pass SIGSEGV to Emacs?

> For the rest I can only hope that some confidence will grow over time.

Let's hope.  But understanding is better than confidence.

> Have you read the old MPS paper from 2002? Maybe that helps.

Not yet.  Too busy.

Re: MPS GC and its implications

[Gerd Möllmann]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Gerd Möllmann <gerd.moellmann@gmail.com>
>> Cc: Helmut Eller <eller.helmut@gmail.com>,  acorallo@gnu.org,
>>   emacs-devel@gnu.org
>> Date: Fri, 03 May 2024 13:59:36 +0200
>> 
>> Eli Zaretskii <eliz@gnu.org> writes:
>> 
>> > I'm worried because it sounds like we don't have a clear understanding
>> > how this stuff works, and without that we are basically stabbing in
>> > the dark when faced with a particular issue related to the MPS
>> > migration.
>> 
>> Can't say anything about GDB. Here on macOS, I can, if I want, see the
>> Mach exceptions being handled, and I've landed there in LLDB as well.
>
> The same happens in GDB.  If the result of the exception is SIGSEGV,
> then GDB should kick in, I think.  Didn't Helmut say something several
> days ago that he needs to tell GDB to pass SIGSEGV to Emacs?

Yes, he said he had to ignore some signals in GDB. Or was it let them
pass through?

>> For the rest I can only hope that some confidence will grow over time.
>
> Let's hope.  But understanding is better than confidence.
>
>> Have you read the old MPS paper from 2002? Maybe that helps.
>
> Not yet.  Too busy.

I've also found a very old paper about barriers (90s) which I could send
you if you want. More modern papers are often behind some walls (ACM,
etc.). Maybe Stefan Monnier could recommend something newer.

Re: MPS GC and its implications

[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: Helmut Eller <eller.helmut@gmail.com>,  acorallo@gnu.org,
>>>   emacs-devel@gnu.org
>>> Date: Fri, 03 May 2024 13:59:36 +0200
>>> 
>>> Eli Zaretskii <eliz@gnu.org> writes:
>>> 
>>> > I'm worried because it sounds like we don't have a clear understanding
>>> > how this stuff works, and without that we are basically stabbing in
>>> > the dark when faced with a particular issue related to the MPS
>>> > migration.
>>> 
>>> Can't say anything about GDB. Here on macOS, I can, if I want, see the
>>> Mach exceptions being handled, and I've landed there in LLDB as well.
>>
>> The same happens in GDB.  If the result of the exception is SIGSEGV,
>> then GDB should kick in, I think.  Didn't Helmut say something several
>> days ago that he needs to tell GDB to pass SIGSEGV to Emacs?
>
> Yes, he said he had to ignore some signals in GDB. Or was it let them
> pass through?
>
>>> For the rest I can only hope that some confidence will grow over time.
>>
>> Let's hope.  But understanding is better than confidence.
>>
>>> Have you read the old MPS paper from 2002? Maybe that helps.
>>
>> Not yet.  Too busy.
>
> I've also found a very old paper about barriers (90s) which I could send
> you if you want.

If you could send it to me as well I'd apretiate 🙏

Thanks

  Andrea

Re: MPS GC and its implications

[Gerd Möllmann]

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

Andrea Corallo <acorallo@gnu.org> writes:

>> I've also found a very old paper about barriers (90s) which I could send
>> you if you want.
>
> If you could send it to me as well I'd apretiate 🙏


[-- Attachment #2: Barriers --]
[-- Type: application/pdf, Size: 733594 bytes --]

Re: MPS GC and its implications

[Helmut Eller]

>> Memory barriers could be used for synchronization.  When the mutator
>> hits a barrier, it executes the signal handler.  The signal handler asks
>> the collector what to do.  Essentially synchronizing those two.
>
> If this happens, I should have seen GDB intercept the exception and
> kick in.  But it doesn't.  (It also would have slowed the program
> quite a lot, I think.)

Don't call igc--collect and do something to generate enough garbage.
Then you'll see it.

Re: MPS GC and its implications

[Eli Zaretskii]

> From: Helmut Eller <eller.helmut@gmail.com>
> Cc: gerd.moellmann@gmail.com,  acorallo@gnu.org,  emacs-devel@gnu.org
> Date: Fri, 03 May 2024 14:28:13 +0200
> 
> >> Memory barriers could be used for synchronization.  When the mutator
> >> hits a barrier, it executes the signal handler.  The signal handler asks
> >> the collector what to do.  Essentially synchronizing those two.
> >
> > If this happens, I should have seen GDB intercept the exception and
> > kick in.  But it doesn't.  (It also would have slowed the program
> > quite a lot, I think.)
> 
> Don't call igc--collect and do something to generate enough garbage.
> Then you'll see it.

I don't.

Or maybe I don't know how to "do something to generate enough
garbage".  Can you suggest a couple of such "somethings"?  I tried
scrolling through xdisp.c and "M-x occur RET glyph RET" in xdisp.c
(which finds about 3500 matches).

Re: MPS GC and its implications

[Helmut Eller]

>> Don't call igc--collect and do something to generate enough garbage.
>> Then you'll see it.
>
> I don't.
>
> Or maybe I don't know how to "do something to generate enough
> garbage".  Can you suggest a couple of such "somethings"?  I tried
> scrolling through xdisp.c and "M-x occur RET glyph RET" in xdisp.c
> (which finds about 3500 matches).

 gdb --args ./src/emacs -Q -eval '(while t (point-marker))'
(gdb) r
Starting program: /scratch/emacs/emacs-igc2/src/emacs -Q -eval \(while\ t\ \(point-marker\)\)
[Thread debugging using libthread_db enabled]
[...]
[New Thread 0x7fffe1d0b6c0 (LWP 96378)]

Thread 1 "emacs" received signal SIGSEGV, Segmentation fault.
Fprogn (body=0x7fffe325bc6b) at eval.c:437
437           Lisp_Object form = XCAR (body);
(gdb) handle SIGSEGV print pass nostop
Signal        Stop      Print   Pass to program Description
SIGSEGV       No        Yes     Yes             Segmentation fault
(gdb) c
Continuing.
[Thread 0x7fffd3fff6c0 (LWP 96381) exited]

Thread 1 "emacs" received signal SIGSEGV, Segmentation fault.

Thread 1 "emacs" received signal SIGSEGV, Segmentation fault.

Thread 1 "emacs" received signal SIGSEGV, Segmentation fault.

Thread 1 "emacs" received signal SIGSEGV, Segmentation fault.

Thread 1 "emacs" received signal SIGSEGV, Segmentation fault.
^C

Re: MPS GC and its implications

[Eli Zaretskii]

> From: Helmut Eller <eller.helmut@gmail.com>
> Cc: gerd.moellmann@gmail.com,  acorallo@gnu.org,  emacs-devel@gnu.org
> Date: Fri, 03 May 2024 15:57:01 +0200
> 
>  gdb --args ./src/emacs -Q -eval '(while t (point-marker))'
> (gdb) r
> Starting program: /scratch/emacs/emacs-igc2/src/emacs -Q -eval \(while\ t\ \(point-marker\)\)
> [Thread debugging using libthread_db enabled]
> [...]
> [New Thread 0x7fffe1d0b6c0 (LWP 96378)]
> 
> Thread 1 "emacs" received signal SIGSEGV, Segmentation fault.
> Fprogn (body=0x7fffe325bc6b) at eval.c:437
> 437           Lisp_Object form = XCAR (body);

Thanks, I see them now.

Re: MPS GC and its implications

[Eli Zaretskii]

> Date: Fri, 03 May 2024 18:56:19 +0300
> From: Eli Zaretskii <eliz@gnu.org>
> Cc: gerd.moellmann@gmail.com, acorallo@gnu.org, emacs-devel@gnu.org
> 
> > From: Helmut Eller <eller.helmut@gmail.com>
> > Cc: gerd.moellmann@gmail.com,  acorallo@gnu.org,  emacs-devel@gnu.org
> > Date: Fri, 03 May 2024 15:57:01 +0200
> > 
> >  gdb --args ./src/emacs -Q -eval '(while t (point-marker))'
> > (gdb) r
> > Starting program: /scratch/emacs/emacs-igc2/src/emacs -Q -eval \(while\ t\ \(point-marker\)\)
> > [Thread debugging using libthread_db enabled]
> > [...]
> > [New Thread 0x7fffe1d0b6c0 (LWP 96378)]
> > 
> > Thread 1 "emacs" received signal SIGSEGV, Segmentation fault.
> > Fprogn (body=0x7fffe325bc6b) at eval.c:437
> > 437           Lisp_Object form = XCAR (body);
> 
> Thanks, I see them now.

Btw, how does one distinguish between these segfaults and "real"
segfaults?  When debugging Emacs crashes, being able to do that
quickly could mean the difference between an efficient debug session
and wasting quite a lot of time on some wild goose chase.

Re: MPS GC and its implications

[Helmut Eller]

On Fri, May 03 2024, Eli Zaretskii wrote:

> Btw, how does one distinguish between these segfaults and "real"
> segfaults?  When debugging Emacs crashes, being able to do that
> quickly could mean the difference between an efficient debug session
> and wasting quite a lot of time on some wild goose chase.

The .gdbinit sets up a breakpoint in terminate_due_to_signal.  When you
end up there, then it's "real".  The others are filtered out by MPS.

Re: MPS GC and its implications

[Eli Zaretskii]

> From: Helmut Eller <eller.helmut@gmail.com>
> Cc: gerd.moellmann@gmail.com,  acorallo@gnu.org,  emacs-devel@gnu.org
> Date: Sat, 04 May 2024 06:22:15 +0200
> 
> On Fri, May 03 2024, Eli Zaretskii wrote:
> 
> > Btw, how does one distinguish between these segfaults and "real"
> > segfaults?  When debugging Emacs crashes, being able to do that
> > quickly could mean the difference between an efficient debug session
> > and wasting quite a lot of time on some wild goose chase.
> 
> The .gdbinit sets up a breakpoint in terminate_due_to_signal.  When you
> end up there, then it's "real".  The others are filtered out by MPS.

But that could be too late.  By the time a fatal exception is caught
by a top-level runtime handler and converted to a signal, the context
is lost, and you might not always even know which code caused the
exception, let alone be capable of looking around in the variables
involved in the problem.  At least on MS-Windows this is what happens.
Otherwise, why would we tell GDB to catch SIGSEGV in addition to the
breakpoint in terminate_due_to_signal?

So if the above is the only way, we are going to lose a valuable
debugging aid, I think.

Re: MPS GC and its implications

[Gerd Möllmann]

Eli Zaretskii <eliz@gnu.org> writes:

> So if the above is the only way, we are going to lose a valuable
> debugging aid, I think.

Does this help?

  https://memory-pool-system.readthedocs.io/en/latest/guide/debug.html

Re: MPS GC and its implications

[Eli Zaretskii]

> From: Gerd Möllmann <gerd.moellmann@gmail.com>
> Cc: Helmut Eller <eller.helmut@gmail.com>,  acorallo@gnu.org,
>   emacs-devel@gnu.org
> Date: Sat, 04 May 2024 09:57:10 +0200
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > So if the above is the only way, we are going to lose a valuable
> > debugging aid, I think.
> 
> Does this help?
> 
>   https://memory-pool-system.readthedocs.io/en/latest/guide/debug.html

Thanks.  (For those who, like myself, have the documentation in Info
format, this is the node "Debugging with the Memory Pool System").

Unfortunately, it doesn't help in this case.  First, it says

     On macOS and Windows, barrier hits do not use signals and so do not
     enter the debugger.

which is incorrect; I guess they mean MSVC build and MSVC debugger,
not MinGW and GDB.  And second, all they have to say about this on
Posix systems is this:

     If you are using GDB on FreeBSD or Linux, you may want to avoid
     stopping on barrier (1) hits, because the MPS uses barriers to
     protect parts of memory, and barrier hits are common and
     expected.  To avoid stopping on a barrier hit, run:

          handle SIGSEGV pass nostop noprint

which doesn't answer my question: how to distinguish between SIGSEGV
due to MPS memory protection and SIGSEGV due to an unrelated bug in
Emacs.

Re: MPS GC and its implications

[Helmut Eller]

> So if the above is the only way, we are going to lose a valuable
> debugging aid, I think.

I agree.  But we have to give MPS something to work with.  Also, with
some effort we could regain some of the lost context; especially if we
are willing to modify MPS itself.

Re: MPS GC and its implications

[Mattias Engdegård]

4 maj 2024 kl. 11.39 skrev Helmut Eller <eller.helmut@gmail.com>:
> 
>> So if the above is the only way, we are going to lose a valuable
>> debugging aid, I think.
> 
> I agree.  But we have to give MPS something to work with.  Also, with
> some effort we could regain some of the lost context; especially if we
> are willing to modify MPS itself.

Would it be possible to arrange for another signal to be raised immediately if the MPS discriminator rejects the SEGV as not being part of its barrier?

Re: MPS GC and its implications

[Helmut Eller]

>> I agree.  But we have to give MPS something to work with.  Also, with
>> some effort we could regain some of the lost context; especially if we
>> are willing to modify MPS itself.
>
> Would it be possible to arrange for another signal to be raised
> immediately if the MPS discriminator rejects the SEGV as not being
> part of its barrier?

Probably not if we restrict ourselves to POSIX APIs as MPS does.  If we
can use our own API, then it shouldn't be hard.  The MPS people are
obviously aware of the issue, as can be seen in the comments in
sigHandle:

https://github.com/Ravenbrook/mps/blob/master/code/protsgix.c

Re: MPS GC and its implications

[Mattias Engdegård]

4 maj 2024 kl. 12.07 skrev Helmut Eller <eller.helmut@gmail.com>:

> Probably not if we restrict ourselves to POSIX APIs as MPS does.  If we
> can use our own API, then it shouldn't be hard.  The MPS people are
> obviously aware of the issue, as can be seen in the comments in
> sigHandle:
> 
> https://github.com/Ravenbrook/mps/blob/master/code/protsgix.c

Thank you. It seems that MPS will attempt to re-raise the SEGV with the old handler (that was registered before MPS initialisation) reinstalled, which means that we might try installing a SIGV handler that just signals USR2 or whatever we prefer before setting up MPS.

Not sure if that will give us an unbroken and useful stack trace chain that can be walked up to the original fault but it's worth a try.

Re: MPS GC and its implications

[Eli Zaretskii]

> From: Helmut Eller <eller.helmut@gmail.com>
> Cc: gerd.moellmann@gmail.com,  acorallo@gnu.org,  emacs-devel@gnu.org
> Date: Sat, 04 May 2024 11:39:51 +0200
> 
> > So if the above is the only way, we are going to lose a valuable
> > debugging aid, I think.
> 
> I agree.  But we have to give MPS something to work with.  Also, with
> some effort we could regain some of the lost context; especially if we
> are willing to modify MPS itself.

Before we consider modification of MPS code, I think we should explore
what GDB already gives us.  There is the $_siginfo convenience
variable, which provides the address of the fault and other
attributes.  Can we perhaps use this to distinguish between the two
cases?

Re: MPS GC and its implications

[Helmut Eller]

On Sat, May 04 2024, Eli Zaretskii wrote:

> Before we consider modification of MPS code, I think we should explore
> what GDB already gives us.  There is the $_siginfo convenience
> variable, which provides the address of the fault and other
> attributes.  Can we perhaps use this to distinguish between the two
> cases?

That's also a possibility.  There is a function mps_arena_has_addr that
could be used to recongize addresses managed by MPS.  The comment in
igc.c say that mps_arena_has_addr is expensive and that is_mps is faster
but could return false positives.

Yet another possibility would be to set a breakpoint in sigHandle.  One
would have to look at the disassembled code first to find the
appropriate address.

Re: MPS GC and its implications

[Gerd Möllmann]

On 2024-05-04 18:08, Helmut Eller wrote:
> On Sat, May 04 2024, Eli Zaretskii wrote:
> 
> That's also a possibility.  There is a function mps_arena_has_addr that
> could be used to recongize addresses managed by MPS.  The comment in
> igc.c say that mps_arena_has_addr is expensive and that is_mps is faster
> but could return false positives.

For use in GDB the function is probably plenty fast, BTW.

It went something like this: there was no is_mps, then MPS_FIX12 
returned true and MPS_FIX2 did not like the address at all and asserted.

Then used mps_has_addr and that worked but made the build 10x slower or 
something.

Then what we have.

Re: MPS GC and its implications

[Gerd Möllmann]

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

>> What am I missing?
>
> Memory barriers could be used for synchronization.  When the mutator
> hits a barrier, it executes the signal handler.  The signal handler asks
> the collector what to do.  Essentially synchronizing those two.

Exactly.

> I don't know if MPS works quite like this, but it could.  I'm not
> worried about this issue.

Me neither. That's the most important point (to me) of barriers. How
barriers are implemented differs (it's not always VM stuff), but the
purpose is always the same.

Re: MPS GC and its implications

[Andrea Corallo]

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

> Andrea Corallo <acorallo@gnu.org> writes:
>
>>> I think MPS is a concurrent GC, it means it runs "all the time".  The
>>> reason why his works are the memory barriers.  But maybe Gerd knows how
>>> things actually work.
>>
>> It is still very unclear how it can move objects "all the time".  During
>> execution traced objects can be loaded on the stack and unloaded all the
>> time (Eli showed other examples of that) so I don't see ATM how it can
>> move objects without actually pausing the mutator thread.
>
> Ja, all the time is maybe a bit too coarse grained an expression.
>
> Maybe one could say it this way: MPS is concurrent and incremental, with
> the two together meaning that it does its work in chunks in another
> thread. Insofar it's not all the time but in chunks.
>
> Don't know if that helps.

I guess you are saying that moving does not happen in a parallel
fashion, and so we'll still stop the mutator?

Thanks

  Andrea

Re: MPS GC and its implications

[Gerd Möllmann]

Andrea Corallo <acorallo@gnu.org> writes:

> I guess you are saying that moving does not happen in a parallel
> fashion, and so we'll still stop the mutator?

Right. In a resonse to Eli I came with this explanation attempt:

  Eli Zaretskii <eliz@gnu.org> writes:

  >> I think you're overlooking that registers and stack are ambiguous roots.
  >> The presence of u.s.data in a register or on the stack _prevents_ it
  >> from moving.
  >
  > That doesn't resolve the difficulty, it just turns the table.  Now we
  > could have a race condition between the Lisp thread loading a pointer
  > into a register or pushing it onto the C stack, and the MPS thread
  > taking a notice that the pointer is in a register or on the stack.  If
  > the pointer was neither in a register nor on the stack when MPS
  > started GC, it could decide to move the object, while the Lisp thread
  > concurrently loads the pointer into a register.  So we might think the
  > object pointed to is unmovable while it isn't.
  >
  > What am I missing now?

  Here barriers come into play. Let's say, for simplicity, that we are
  talking about an object on a VM page P.

  In its thread, MPS decides that it wants to do an increment of work on
  P, say it wants to move objects, without the chance that MPS and client
  threads interfere.

  So, MPS puts a read barrier on P (say with mprotect), so that other
  threads are interrupted by a signal when they read from P. MPS puts a
  write barrier on P so that the same happens when a thread tries to
  modifiy P.

  Does that help?

Barriers are the reason why things are done in an orderly way.

Re: MPS GC and its implications

[Andrea Corallo]

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

> Andrea Corallo <acorallo@gnu.org> writes:
>
>> I guess you are saying that moving does not happen in a parallel
>> fashion, and so we'll still stop the mutator?
>
> Right. In a resonse to Eli I came with this explanation attempt:
>
>   Eli Zaretskii <eliz@gnu.org> writes:
>
>   >> I think you're overlooking that registers and stack are ambiguous roots.
>   >> The presence of u.s.data in a register or on the stack _prevents_ it
>   >> from moving.
>   >
>   > That doesn't resolve the difficulty, it just turns the table.  Now we
>   > could have a race condition between the Lisp thread loading a pointer
>   > into a register or pushing it onto the C stack, and the MPS thread
>   > taking a notice that the pointer is in a register or on the stack.  If
>   > the pointer was neither in a register nor on the stack when MPS
>   > started GC, it could decide to move the object, while the Lisp thread
>   > concurrently loads the pointer into a register.  So we might think the
>   > object pointed to is unmovable while it isn't.
>   >
>   > What am I missing now?
>
>   Here barriers come into play. Let's say, for simplicity, that we are
>   talking about an object on a VM page P.
>
>   In its thread, MPS decides that it wants to do an increment of work on
>   P, say it wants to move objects, without the chance that MPS and client
>   threads interfere.
>
>   So, MPS puts a read barrier on P (say with mprotect), so that other
>   threads are interrupted by a signal when they read from P. MPS puts a
>   write barrier on P so that the same happens when a thread tries to
>   modifiy P.
>
>   Does that help?
>
> Barriers are the reason why things are done in an orderly way.

Thanks.  Will be interesting to see how often the mutator thread then is
paused due to hitting a barrier :)

  Andrea

Re: MPS GC and its implications

[Eli Zaretskii]

> From: Andrea Corallo <acorallo@gnu.org>
> Cc: Helmut Eller <eller.helmut@gmail.com>,  Eli Zaretskii <eliz@gnu.org>,
>   emacs-devel@gnu.org
> Date: Sat, 04 May 2024 02:25:28 -0400
> 
> Gerd Möllmann <gerd.moellmann@gmail.com> writes:
> 
> > Andrea Corallo <acorallo@gnu.org> writes:
> >
> >> I guess you are saying that moving does not happen in a parallel
> >> fashion, and so we'll still stop the mutator?
> >
> > Right. In a resonse to Eli I came with this explanation attempt:
> >
> >   Eli Zaretskii <eliz@gnu.org> writes:
> >
> >   Here barriers come into play. Let's say, for simplicity, that we are
> >   talking about an object on a VM page P.
> >
> >   In its thread, MPS decides that it wants to do an increment of work on
> >   P, say it wants to move objects, without the chance that MPS and client
> >   threads interfere.
> >
> >   So, MPS puts a read barrier on P (say with mprotect), so that other
> >   threads are interrupted by a signal when they read from P. MPS puts a
> >   write barrier on P so that the same happens when a thread tries to
> >   modifiy P.
> >
> >   Does that help?
> >
> > Barriers are the reason why things are done in an orderly way.
> 
> Thanks.  Will be interesting to see how often the mutator thread then is
> paused due to hitting a barrier :)

IME till now, extremely rarely.  I succeeded to trigger this only
after Helmut kindly gave me a recipe (which is a Lisp program unlikely
to happen in any real-life Lisp code), and even with that recipe, I
could hit the VM protection exception only half a dozen times in a 5
min session.

Maybe a more complex recipe, which attempts to read or write from many
more objects, could cause that more frequently, I don't know.

Re: MPS: face-cache

[Eli Zaretskii]

> From: Gerd Möllmann <gerd.moellmann@gmail.com>
> Cc: Eli Zaretskii <eliz@gnu.org>,  emacs-devel@gnu.org
> Date: Tue, 30 Apr 2024 08:04:17 +0200
> 
> I think it would suffice if we only trace the windows mactrices + check
> that the glyph pool of the frame is not made a root, because it need not
> be a root.
> 
> Reason is that the window matrices on tty frames are sub-allocated from
> the pool in such a way that all matrices taken together form the frame
> matrix. So, when we trace window matrices, all windows taken together
> trace the frame matrices.
> 
> WDYT Eli?

You are basically correct, but there are two exceptions to the "window
matrices sub-allocate from the frame matrix" paradigm: the menu bar
and the tab bar.  These two don't have any corresponding windows in
the TTY frame case, each one of them is a single glyph row that is
updated directly in the frame glyph matrix.  So we need to do
something special about these two, at least.

As another "interesting" anomaly, the implementation of TTY menus
writes directly into the frame glyph matrix, see
xdisp.c:display_tty_menu_item.  I wonder whether there's something we
need to do about that as well, since the menu text comes from Lisp
strings.

Re: MPS: face-cache

[Gerd Möllmann]

Eli Zaretskii <eliz@gnu.org> writes:

> As another "interesting" anomaly, the implementation of TTY menus
> writes directly into the frame glyph matrix, see
> xdisp.c:display_tty_menu_item.  I wonder whether there's something we
> need to do about that as well, since the menu text comes from Lisp
> strings.

I see, the pane_string in tty_menu_show, for example.

The char * in the tty_menu would keep the string_data alive and
immovable if they were on the stack somewhere. Alas, I don't understand
how the menus are handled. Is there some form of "event loop" running
when the menu is up and interacted with? Or is it "normal" keyboard.c
input handling?

Re: MPS: face-cache

[Eli Zaretskii]

> From: Gerd Möllmann <gerd.moellmann@gmail.com>
> Cc: eller.helmut@gmail.com,  emacs-devel@gnu.org
> Date: Tue, 30 Apr 2024 14:57:01 +0200
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > As another "interesting" anomaly, the implementation of TTY menus
> > writes directly into the frame glyph matrix, see
> > xdisp.c:display_tty_menu_item.  I wonder whether there's something we
> > need to do about that as well, since the menu text comes from Lisp
> > strings.
> 
> I see, the pane_string in tty_menu_show, for example.
> 
> The char * in the tty_menu would keep the string_data alive and
> immovable if they were on the stack somewhere. Alas, I don't understand
> how the menus are handled. Is there some form of "event loop" running
> when the menu is up and interacted with? Or is it "normal" keyboard.c
> input handling?

Some of both, actually.  We re-enter the normal input processing, see
tty_menu_activate, but we inhibit redisplay (because the display
engine has no idea someone wrote int its matrices behind its back) and
manage screen updates ourselves.  We also inhibit GC (in
tty_menu_show).

Re: MPS: face-cache

[Gerd Möllmann]

Eli Zaretskii <eliz@gnu.org> writes:

> Some of both, actually.  We re-enter the normal input processing, see
> tty_menu_activate, but we inhibit redisplay (because the display
> engine has no idea someone wrote int its matrices behind its back) and
> manage screen updates ourselves.  We also inhibit GC (in
> tty_menu_show).

Maybe the cheapest solution would be to use

specpdl_ref
igc_park_arena (void)
{
  specpdl_ref count = SPECPDL_INDEX ();
  record_unwind_protect_void (release_arena);
  mps_arena_park (global_igc->arena);
  return count;
}

This "parking" is like inhibiting GC. I've used it while pdumping so
that pdumper can write its dump without objs moving.

Re: MPS: face-cache

[Eli Zaretskii]

> From: Gerd Möllmann <gerd.moellmann@gmail.com>
> Cc: Eli Zaretskii <eliz@gnu.org>,  emacs-devel@gnu.org
> Date: Mon, 29 Apr 2024 21:36:01 +0200
> 
> Helmut Eller <eller.helmut@gmail.com> writes:
> 
> > There also are glyph matrices in struct frame that aren't traced yet,
> > but adding code to fix_frame didn't help.  Even struct tty_menu_state
> > has glyph matrices; though probably not used in GUI mode.  Either way, a
> > useful test case.
> 
> Maybe the frame matrix is actually responsible for the crashes.
> 
> Windows on tty frames, don't manage their own memory but use parts of a
> frame matrix which the frame malloc's.

There are two caveats here, see my other message.

> On startup, I believe we have initially a tty frame (invisble). 

That frame is deleted in the GUI session, but AFAIR is kept in the -nw
session and in daemon sessions.  So we should probably do something
about it as well.

Re: MPS: face-cache

[Gerd Möllmann]

Eli Zaretskii <eliz@gnu.org> writes:

>> On startup, I believe we have initially a tty frame (invisble). 
>
> That frame is deleted in the GUI session, but AFAIR is kept in the -nw
> session and in daemon sessions.  So we should probably do something
> about it as well.

I would imagine that initial frame is in Vframe_list (how would it not
be?), in which case we would see it through staticvec. We would have to
check though if it is in the loaded pdump (I believe it is), and ensure
that we don't ignore it in scan_staticvec becasue it is
pdumper_object_p. (Don't know if that could happen.)

Re: MPS: face-cache

[Eli Zaretskii]

> From: Gerd Möllmann <gerd.moellmann@gmail.com>
> Cc: eller.helmut@gmail.com,  emacs-devel@gnu.org
> Date: Tue, 30 Apr 2024 14:41:17 +0200
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> On startup, I believe we have initially a tty frame (invisble). 
> >
> > That frame is deleted in the GUI session, but AFAIR is kept in the -nw
> > session and in daemon sessions.  So we should probably do something
> > about it as well.
> 
> I would imagine that initial frame is in Vframe_list (how would it not
> be?), in which case we would see it through staticvec. We would have to
> check though if it is in the loaded pdump (I believe it is), and ensure
> that we don't ignore it in scan_staticvec becasue it is
> pdumper_object_p. (Don't know if that could happen.)

The initial frame is also in Vterminal_frame:

  old_selected_frame = Vterminal_frame = selected_frame;

  DEFVAR_LISP ("terminal-frame", Vterminal_frame,
               doc: /* The initial frame-object, which represents Emacs's stdout.  */);

Re: MPS: face-cache

[Gerd Möllmann]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Gerd Möllmann <gerd.moellmann@gmail.com>
>> Cc: eller.helmut@gmail.com,  emacs-devel@gnu.org
>> Date: Tue, 30 Apr 2024 14:41:17 +0200
>> 
>> Eli Zaretskii <eliz@gnu.org> writes:
>> 
>> >> On startup, I believe we have initially a tty frame (invisble). 
>> >
>> > That frame is deleted in the GUI session, but AFAIR is kept in the -nw
>> > session and in daemon sessions.  So we should probably do something
>> > about it as well.
>> 
>> I would imagine that initial frame is in Vframe_list (how would it not
>> be?), in which case we would see it through staticvec. We would have to
>> check though if it is in the loaded pdump (I believe it is), and ensure
>> that we don't ignore it in scan_staticvec becasue it is
>> pdumper_object_p. (Don't know if that could happen.)
>
> The initial frame is also in Vterminal_frame:
>
>   old_selected_frame = Vterminal_frame = selected_frame;
>
>   DEFVAR_LISP ("terminal-frame", Vterminal_frame,
>                doc: /* The initial frame-object, which represents Emacs's stdout.  */);

Thanks.

@Helmut: Did we already talk about what the problem with the frame in
the loaded pdump could be? Sorry that I don't remember.

Re: MPS: face-cache

[Helmut Eller]

> @Helmut: Did we already talk about what the problem with the frame in
> the loaded pdump could be? Sorry that I don't remember.

I never heard of that before.

Re: MPS: pdump

[Gerd Möllmann]

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

>> @Helmut: Did we already talk about what the problem with the frame in
>> the loaded pdump could be? Sorry that I don't remember.
>
> I never heard of that before.

As the famous philosopher Manuel Manousakis says: Katastrophe!
:-)

Okay, I think one can understand this best when I try to describe what
the pdumper does. Let's start with generating a pdump. I'll try to leave
out as much details as a can.

When we create a pdump, we start by allocating 3 big memory blocks which
I'll call H (hot), C (cold), and R (relocs).

We then traverse the graph of live objects like the old GC, starting
from known roots. Each newly encountered object is copyied to H or C in
binary form. C is used for leaf objects like strings and floats, H for
the rest.

The copying of objects is done by invoking type-specific functions,
example dump_float, dump_vector, etc.

We cannot use memcpy for the copying because we need more information
when the pdump is loaded, namely relocation information, which goes to
R. Relocation is necessary because both Emacs' DATA segment as well as
H/C may end up at different addresses in a new process.

Relocation info is recorded in S, and tells us where in the copied
objects Lisp_Objects or pointers are that need patching when loaded.

At the end, we write H, C, S to one big file.

Good. Now let's load that file. We mmap the whole file and now have H',
C', R' in the new process. C' and R'are good to go (In C are leaf
objects). H' is patches according to the reloc info that is in S'.

At the end of the relocation H' is ready to use. Some additional setup
and initalizations, and we are good to go. I won't describe these.

Thing is that H' now contains real Lisp objects of basically all types.
Lisp objects contain references, so I make H' an ambig root.

So far so good, but some Lisp objects contain not only references to
other Lisp objects but also pointers to malloc'd memory. Not initially,
in the dump, but during their lifetime.

And finally we have reached face_cache.

If initial_frame is an object in H', fix_frame won't be called for it.
It cannot because the dump is not part of the MPS memory, and is instead
traced ambigously as part of the big blob H'.

Does that make any sense?

Re: MPS: pdump

[Eli Zaretskii]

> From: Gerd Möllmann <gerd.moellmann@gmail.com>
> Cc: Eli Zaretskii <eliz@gnu.org>,  emacs-devel@gnu.org
> Date: Thu, 02 May 2024 20:28:42 +0200
> 
> And finally we have reached face_cache.
> 
> If initial_frame is an object in H', fix_frame won't be called for it.
> It cannot because the dump is not part of the MPS memory, and is instead
> traced ambigously as part of the big blob H'.

If the face cache of the initial frame is the problem, we can simply
forget about it, and create a completely new face cache for it.  The
face cache that existed during dumping doesn't matter (it probably
contains only the basic faces anyway).

So how about just calling init_frame_faces for the initial frame, in
some init_SOMETHING function called at startup?  Like in init_xfaces,
for example?

Re: MPS: pdump

[Gerd Möllmann]

Eli Zaretskii <eliz@gnu.org> writes:

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

>> If initial_frame is an object in H', fix_frame won't be called for it.
>> It cannot because the dump is not part of the MPS memory, and is instead
>> traced ambigously as part of the big blob H'.
>
> If the face cache of the initial frame is the problem, we can simply
> forget about it, and create a completely new face cache for it.  The
> face cache that existed during dumping doesn't matter (it probably
> contains only the basic faces anyway).
>
> So how about just calling init_frame_faces for the initial frame, in
> some init_SOMETHING function called at startup?  Like in init_xfaces,
> for example?

That would unfortunately not help:

If something in the face cache contains references, we must scan it. We
won't because we don't trace initial_frame, we only scan it ambigupusly
as an anonymous part of the big blob H', and the pointer to the
face_cache is just some random uninteresting pointer. In the end, we'll
never scan the face_cache.

In some other cases (itree_node, itree_tree, ...) I've made the
corresponing structs MPS-managed. With this, when we scan H', we'll find
a pointer to an MPS-managed object and MPS can tell if a pointer is to
one of its objs. We'd need a new IGC_OBJ_FACE_CACHE for that, or
something like it if we already have too many.

Re: MPS: pdump

[Eli Zaretskii]

> From: Gerd Möllmann <gerd.moellmann@gmail.com>
> Cc: eller.helmut@gmail.com,  emacs-devel@gnu.org
> Date: Thu, 02 May 2024 21:45:55 +0200
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> From: Gerd Möllmann <gerd.moellmann@gmail.com>
> 
> >> If initial_frame is an object in H', fix_frame won't be called for it.
> >> It cannot because the dump is not part of the MPS memory, and is instead
> >> traced ambigously as part of the big blob H'.
> >
> > If the face cache of the initial frame is the problem, we can simply
> > forget about it, and create a completely new face cache for it.  The
> > face cache that existed during dumping doesn't matter (it probably
> > contains only the basic faces anyway).
> >
> > So how about just calling init_frame_faces for the initial frame, in
> > some init_SOMETHING function called at startup?  Like in init_xfaces,
> > for example?
> 
> That would unfortunately not help:
> 
> If something in the face cache contains references, we must scan it.

Yes, but what are those "somethings" in the particular case of the
face cache of the initial frame?  Here's what I see:

  struct face_cache
  {
    /* Hash table of cached realized faces.  */
    struct face **buckets;

    /* Back-pointer to the frame this cache belongs to.  */
    struct frame *f;

    /* A vector of faces so that faces can be referenced by an ID.  */
    struct face **faces_by_id;

    /* The allocated size, and number of used slots of faces_by_id.  */
    ptrdiff_t size;
    int used;

    /* Flag indicating that attributes of the `menu' face have been
       changed.  */
    bool_bf menu_face_changed_p : 1;
  };

The only members of importance to us are the back-pointer to the
frame, the buckets array, and the faces_by_id array.  Do you agree?

The frame pointer should not be a problem, since Vterminal_frame
should be traced (if it isn't already) -- it is a global Lisp object,
and we agreed those should be staticpro'd.

The two 'struct face' arrays should not be a problem, because calling
init_frame_faces, as I suggested, will recreate the basic faces anew
(we can call free_all_realized_faces if the recreation will not happen
otherwise).

So what "somethings" could be a problem here, and why?

> We
> won't because we don't trace initial_frame, we only scan it ambigupusly
> as an anonymous part of the big blob H', and the pointer to the
> face_cache is just some random uninteresting pointer. In the end, we'll
> never scan the face_cache.

How is the case of the initial frame different from any other frame we
create after loading the dump file?  I feel there's some
misunderstanding here, most probably mine.

> In some other cases (itree_node, itree_tree, ...) I've made the
> corresponing structs MPS-managed. With this, when we scan H', we'll find
> a pointer to an MPS-managed object and MPS can tell if a pointer is to
> one of its objs. We'd need a new IGC_OBJ_FACE_CACHE for that, or
> something like it if we already have too many.

I don't think I understand this, probably because I need the above
question answered first.

Re: MPS: pdump

[Gerd Möllmann]

Eli Zaretskii <eliz@gnu.org> writes:

> I don't think I understand this, probably because I need the above
> question answered first.

I have to confess something:

I apparently solved this whole thing already, in my fork, in

  b61873c8db04a800d466d955a60bb00d62325f9c
  Author:     Gerd Möllmann <gerd@gnu.org>
  AuthorDate: Sat Apr 13 16:44:22 2024 +0200

face_cache became an MPS object, new function fix_face_cache...
The whole sh*t I tried to explain :-/.

And I didn't remember a single bit of this until now saw to my surprise
that there is an IGC_OBJ_FACE_CACHE already.

So the whole pdump thingy and so on - everything obsolete. It's all
true, I think, but I did what's needed already.

I'm sorry for that. To my excuse I can only say that this change
probably took only 10 minutes, and it was 3 weeks ago, and yadayadah....

Re: MPS: pdump

[Eli Zaretskii]

> From: Gerd Möllmann <gerd.moellmann@gmail.com>
> Cc: eller.helmut@gmail.com,  emacs-devel@gnu.org
> Date: Fri, 03 May 2024 09:15:34 +0200
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > I don't think I understand this, probably because I need the above
> > question answered first.
> 
> I have to confess something:
> 
> I apparently solved this whole thing already, in my fork, in
> 
>   b61873c8db04a800d466d955a60bb00d62325f9c
>   Author:     Gerd Möllmann <gerd@gnu.org>
>   AuthorDate: Sat Apr 13 16:44:22 2024 +0200
> 
> face_cache became an MPS object, new function fix_face_cache...
> The whole sh*t I tried to explain :-/.

I don't have the above commit in my clone.  Was that commit pushed
upstream?

Re: MPS: pdump

[Gerd Möllmann]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Gerd Möllmann <gerd.moellmann@gmail.com>
>> Cc: eller.helmut@gmail.com,  emacs-devel@gnu.org
>> Date: Fri, 03 May 2024 09:15:34 +0200
>> 
>> Eli Zaretskii <eliz@gnu.org> writes:
>> 
>> > I don't think I understand this, probably because I need the above
>> > question answered first.
>> 
>> I have to confess something:
>> 
>> I apparently solved this whole thing already, in my fork, in
                                                 ^^^^^^^^^^
>> 
>>   b61873c8db04a800d466d955a60bb00d62325f9c
>>   Author:     Gerd Möllmann <gerd@gnu.org>
>>   AuthorDate: Sat Apr 13 16:44:22 2024 +0200
>> 
>> face_cache became an MPS object, new function fix_face_cache...
>> The whole sh*t I tried to explain :-/.
>
> I don't have the above commit in my clone.  Was that commit pushed
> upstream?

It's in my fork, before scratch/igc existed.

Re: MPS: pdump

[Eli Zaretskii]

> From: Gerd Möllmann <gerd.moellmann@gmail.com>
> Cc: eller.helmut@gmail.com,  emacs-devel@gnu.org
> Date: Fri, 03 May 2024 13:55:44 +0200
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >>   b61873c8db04a800d466d955a60bb00d62325f9c
> >>   Author:     Gerd Möllmann <gerd@gnu.org>
> >>   AuthorDate: Sat Apr 13 16:44:22 2024 +0200
> >> 
> >> face_cache became an MPS object, new function fix_face_cache...
> >> The whole sh*t I tried to explain :-/.
> >
> > I don't have the above commit in my clone.  Was that commit pushed
> > upstream?
> 
> It's in my fork, before scratch/igc existed.

So I guess we need to merge that to the branch?

Re: MPS: pdump

[Gerd Möllmann]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Gerd Möllmann <gerd.moellmann@gmail.com>
>> Cc: eller.helmut@gmail.com,  emacs-devel@gnu.org
>> Date: Fri, 03 May 2024 13:55:44 +0200
>> 
>> Eli Zaretskii <eliz@gnu.org> writes:
>> 
>> >>   b61873c8db04a800d466d955a60bb00d62325f9c
>> >>   Author:     Gerd Möllmann <gerd@gnu.org>
>> >>   AuthorDate: Sat Apr 13 16:44:22 2024 +0200
>> >> 
>> >> face_cache became an MPS object, new function fix_face_cache...
>> >> The whole sh*t I tried to explain :-/.
>> >
>> > I don't have the above commit in my clone.  Was that commit pushed
>> > upstream?
>> 
>> It's in my fork, before scratch/igc existed.
>
> So I guess we need to merge that to the branch?

Not even that. I made scratch/igc by transfering a large diff from my
fork (the one with CL packages) to master. The change was part of the
diff, so it's already there.

Re: MPS: pdump

[Helmut Eller]

On Thu, May 02 2024, Gerd Möllmann wrote:

> Helmut Eller <eller.helmut@gmail.com> writes:
>
>>> @Helmut: Did we already talk about what the problem with the frame in
>>> the loaded pdump could be? Sorry that I don't remember.
>>
>> I never heard of that before.
>
> As the famous philosopher Manuel Manousakis says: Katastrophe!
> :-)

Never heard of that one either. :-)

[...]
> Good. Now let's load that file. We mmap the whole file and now have H',
> C', R' in the new process. C' and R'are good to go (In C are leaf
> objects). H' is patches according to the reloc info that is in S'.

In not sure what the difference between R and S is, but I guess it's a
minor detail.

> At the end of the relocation H' is ready to use. Some additional setup
> and initalizations, and we are good to go. I won't describe these.

(I think copying H' to MPS memory would not cost much more.  Due to
copy-on-write, the patching must already have some cost.  Something we
could consider in the future.)

> Thing is that H' now contains real Lisp objects of basically all types.
> Lisp objects contain references, so I make H' an ambig root.
>
> So far so good, but some Lisp objects contain not only references to
> other Lisp objects but also pointers to malloc'd memory. Not initially,
> in the dump, but during their lifetime.
>
> And finally we have reached face_cache.
>
> If initial_frame is an object in H', fix_frame won't be called for it.
> It cannot because the dump is not part of the MPS memory, and is instead
> traced ambigously as part of the big blob H'.
>
> Does that make any sense?

Yes.  I'm surprised that frames are pdumped.  Windows too?  Is that
necessary or more of an accident?

Re: MPS: pdump

[Gerd Möllmann]

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

> On Thu, May 02 2024, Gerd Möllmann wrote:
>
>> Helmut Eller <eller.helmut@gmail.com> writes:
>>
>>>> @Helmut: Did we already talk about what the problem with the frame in
>>>> the loaded pdump could be? Sorry that I don't remember.
>>>
>>> I never heard of that before.
>>
>> As the famous philosopher Manuel Manousakis says: Katastrophe!
>> :-)
>
> Never heard of that one either. :-)

(German TV series "Steel Buddies"...)

>
> [...]
>> Good. Now let's load that file. We mmap the whole file and now have H',
>> C', R' in the new process. C' and R'are good to go (In C are leaf
>> objects). H' is patches according to the reloc info that is in S'.
>
> In not sure what the difference between R and S is, but I guess it's a
> minor detail.

Sorry, the S' was fat-fingered, please replace with R'.

>> At the end of the relocation H' is ready to use. Some additional setup
>> and initalizations, and we are good to go. I won't describe these.
>
> (I think copying H' to MPS memory would not cost much more.  Due to
> copy-on-write, the patching must already have some cost.  Something we
> could consider in the future.)

Yep, see admin/igc.org :-).

Another idea we could try is to write the objects to the dump including
the igc_header. We could then walk over all objects in H' with
dflt_scan, makeing it an exact root. If we then add barriers to H'
(MPS_RM_PROT), it's almost a normal old generation, only different.

Just another idea...

>> Thing is that H' now contains real Lisp objects of basically all types.
>> Lisp objects contain references, so I make H' an ambig root.
>>
>> So far so good, but some Lisp objects contain not only references to
>> other Lisp objects but also pointers to malloc'd memory. Not initially,
>> in the dump, but during their lifetime.
>>
>> And finally we have reached face_cache.
>>
>> If initial_frame is an object in H', fix_frame won't be called for it.
>> It cannot because the dump is not part of the MPS memory, and is instead
>> traced ambigously as part of the big blob H'.
>>
>> Does that make any sense?
>
> Yes.  I'm surprised that frames are pdumped.  Windows too?  Is that
> necessary or more of an accident?

I think I've see that initial_frame is in the dumnp, but I wouldn't
trust me.

dump_vectorlike has these cases, among others

    case PVEC_FRAME:
    case PVEC_WINDOW:
    case PVEC_PROCESS:
    case PVEC_TERMINAL:
      return dump_nilled_pseudovec (ctx, &v->header);

So, windows could be there, too.

Re: MPS: pdump

[Eli Zaretskii]

> From: Helmut Eller <eller.helmut@gmail.com>
> Cc: Eli Zaretskii <eliz@gnu.org>,  emacs-devel@gnu.org
> Date: Fri, 03 May 2024 07:54:35 +0200
> 
> > If initial_frame is an object in H', fix_frame won't be called for it.
> > It cannot because the dump is not part of the MPS memory, and is instead
> > traced ambigously as part of the big blob H'.
> >
> > Does that make any sense?
> 
> Yes.  I'm surprised that frames are pdumped.  Windows too?

If the initial frame is dumped, then its windows (at least 2) are also
dumped.

> Is that necessary or more of an accident?

I'm quite sure it's a kind-of "accident".  temacs needs a frame to do
some of the loadup stuff, so the frame ends up to be a live Lisp
object when we dump.  Since it's already there, when emacs starts, it
makes use of the frame.  And the pdumper change never tried to go
through all the dumped Lisp objects and eliminate those which could be
easily created anew, because doing so would be more work: the startup
code would need to recreate those objects.  So we only did that where
absolutely necessary, and left the rest as it was with unexec, when
the entire data segment was written to the emacs executable.

Re: MPS: face-cache

[Helmut Eller]

BTW, with: src/emacs -Q -f kill-emacs
you can not only kill but also crash Emacs :-)

Re: MPS: face-cache

[Gerd Möllmann]

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

> BTW, with: src/emacs -Q -f kill-emacs
> you can not only kill but also crash Emacs :-)

We could rename kill-emacs to crash-emacs :-)

Re: MPS: face-cache

[Eli Zaretskii]

> From: Helmut Eller <eller.helmut@gmail.com>
> Cc: Eli Zaretskii <eliz@gnu.org>,  emacs-devel@gnu.org
> Date: Mon, 29 Apr 2024 21:37:04 +0200
> 
> BTW, with: src/emacs -Q -f kill-emacs
> you can not only kill but also crash Emacs :-)

This doesn't crash for me (on MS-Windows).  Can you show a backtrace
from such a crash?

Re: MPS: face-cache

[Gerd Möllmann]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Helmut Eller <eller.helmut@gmail.com>
>> Cc: Eli Zaretskii <eliz@gnu.org>,  emacs-devel@gnu.org
>> Date: Mon, 29 Apr 2024 21:37:04 +0200
>> 
>> BTW, with: src/emacs -Q -f kill-emacs
>> you can not only kill but also crash Emacs :-)
>
> This doesn't crash for me (on MS-Windows).  Can you show a backtrace
> from such a crash?

Helmut said it was a cockpit error. (Not replacing igc_free with xfree.)

Re: MPS: face-cache

[Eli Zaretskii]

> From: Gerd Möllmann <gerd.moellmann@gmail.com>
> Cc: eller.helmut@gmail.com,  emacs-devel@gnu.org
> Date: Sun, 28 Apr 2024 14:26:16 +0200
> 
> Gerd Möllmann <gerd.moellmann@gmail.com> writes:
> 
> > Eli Zaretskii <eliz@gnu.org> writes:
> >
> >>   hash_index = hash_lookup_get_hash (hash_table, key, &hash_code);
> >
> > And hash_table above is composition_hash_table which is staticpro'd. So
> > I think that works OOTB, Nice!
> 
> I take that back. The reference to key in struct composition is of
> course a separate reference from what's in the hash table, so it should
> either be traced, and be in an ambiguous root so that it can't move.

Can't we make the key an unmovable object?

Re: MPS: face-cache

[Gerd Möllmann]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Gerd Möllmann <gerd.moellmann@gmail.com>
>> Cc: eller.helmut@gmail.com,  emacs-devel@gnu.org
>> Date: Sun, 28 Apr 2024 14:26:16 +0200
>> 
>> Gerd Möllmann <gerd.moellmann@gmail.com> writes:
>> 
>> > Eli Zaretskii <eliz@gnu.org> writes:
>> >
>> >>   hash_index = hash_lookup_get_hash (hash_table, key, &hash_code);
>> >
>> > And hash_table above is composition_hash_table which is staticpro'd. So
>> > I think that works OOTB, Nice!
>> 
>> I take that back. The reference to key in struct composition is of
>> course a separate reference from what's in the hash table, so it should
>> either be traced, and be in an ambiguous root so that it can't move.
>
> Can't we make the key an unmovable object?

I've pushed Helmut's solution now, which does that. 

Re: MPS: face-cache

[Eli Zaretskii]

> From: Helmut Eller <eller.helmut@gmail.com>
> Cc: Eli Zaretskii <eliz@gnu.org>,  emacs-devel missing value
>  <emacs-devel@gnu.org>
> Date: Sun, 28 Apr 2024 08:58:11 +0200
> 
> On Sun, Apr 28 2024, Gerd Möllmann wrote:
> 
> > Wow! I'm an idiot. All three thumbs up! Did you already try it?
> 
> Yes. My test case is this
> 
> set args -Q -eval '(progn (view-hello-file) (while t (redisplay) (igc--collect) (forward-line)))'
> 
> It goes now a few lines further before it hits the next issue.

After this fix, it no longer crashes for me on MS-Windows.