Subject: Re: bug#75754: styled_format stack usage/GC protection

From: Pip Cet <[email-masked]>
Date: Fri, 24 Jan 2025 15:02:20 +0000

"Eli Zaretskii" <[email-masked]> writes:

>> Date: Fri, 24 Jan 2025 12:51:22 +0000
>> From: Pip Cet <[email-masked]>
>> Cc: [email-masked], [email-masked], Michael Albinus <[email-masked]>
>>
>> Michael Albinus, could you please C-s ert-how in this message and
>> comment briefly if appropriate? I think adding a new defstruct may help
>> improve ert in at least three ways, including this one.  Please indicate
>> whether this would be *potentially* acceptable; if it is, I'll prepare a
>> separate bug.  If it isn't, I might have to think of something else.
>
> I think it will take time for Michael to respond, due to his personal
> circumstances.  So if you expect the answers soon, that might not
> happen.

Will file a new bug, with an initial message hopefully clear enough to
save Michael some time for when he gets around to it.

>> "Eli Zaretskii" <[email-masked]> writes:
>>
>> >> Date: Thu, 23 Jan 2025 23:58:41 +0000
>> >> From: Pip Cet <[email-masked]>
>> >> Cc: [email-masked], [email-masked]
>> >>
>> >> 1. Reload format_start and format (and end, and format0) after every
>> >> call which might have GC'd.  If you think we should do this, please
>> >> tell me whether lisp_string_width can GC.
>> >
>> > It can, if called with the last argument 'true' (because
>> > find_automatic_composition calls into Lisp).  Currently, we call it
>> > with 'false', so it cannot.
>>
>> Is this documented anywhere?
>
> If you mean under what circumstances some internal function calls Lisp
> (or can otherwise GC), then no, we don't document that anywhere.  I
> just looked at the source code to answer your question.

(This is an honest question: do you believe Emacs would be worse off if
we added an ASSUME_NO_GC macro like

  ASSUME_NO_GC (true);
  p = SDATA (str);

  532 lines of code here

  use p for the last time

  ASSUME_NO_GC (false);

This seems to me to be readable (better names/API welcome, of course),
even if it's a nop.  Faster than writing down these assumptions in
English, and it does offer the ability to eventually find such cases
automatically.

That would have found this bug if Fprin1 contained

  /* Due to print-unreadable-function, all calls to prin1 may call Lisp,
     which may GC.  Ensure we're not accidentally called from a critical
     section.  */

#ifdef ENABLE_CHECK_NO_GC
  if (assume_no_gc >= 0)
    emacs_abort ();
#endif

It'd make life easier for relative Emacs newcomers like myself who do
not know by heart which functions call GC and which don't.)

>> Thank you very much for suggesting this: if I hadn't been wrong about
>> the first test case being useful (it was), I'd probably have refused.
>> While writing this test didn't uncover new bugs, it was an educational
>> experience.
>>
>> New test:
>>
>> (ert-deftest editfns-tests-styled-print ()
>>    (let* ((gc-me (copy-sequence "foo"))
>>           (print-unreadable-function
>> 	   (lambda (&rest args)
>>              (make-string 10 10 t)
>>              (garbage-collect)
>>              (make-string 10 10 t)
>>              (prog1 (make-string 100 ?Ā t)
>>                (setq gc-me nil)
>>                (make-string 10 10 t))))
>>           (str "\"[1] ĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀ ĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀ ĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀĀ\""))
>>      (should (string=
>>               (format "%S" (format (copy-sequence "%S %S %S %S") [1]
>>                                    (symbol-function '+)
>>                                    (symbol-function '*)
>>                                    (symbol-function '-)))
>>               str))))
>>
>> The "problem" writing this test is that string compaction is very often
>> idempotent: the only way a string is moved again in a second GC is when
>> another string which is *older* than the string moved in the first GC is
>> freed.  That's 'gc-me', which must be live during the first call to
>> garbage-collect but unreachable for the second call.
>
> I suggest to have at least part of the above in comments to the test,
> since these details are so important for the test to be effective.

Yes, definitely, if it is to be useful.  Without comments this is so
much line noise.

> What are the general ideas for that?

This doesn't belong here; I'll file a new bug even if I can't provide
such details now (unless there already is one), as a wishlist item.

>> >> feature/igc avoids all of these problems by keeping SDATA pointers
>> >> valid, even when the string is resized.
>> >
>> > Unless the string is GC'ed, right?
>>
>> Even if it is GC'd.
>>
>> The SDATA will stay around while there are live pointers to it.  The
>> SDATA doesn't keep the string alive, but there's no way to get a
>> reference to the now-dead Lisp_String structure from the SDATA, so this
>> won't cause bugs.
>
> Even if the code modifies the string after GC?

If the GC free'd the string, but not the SDATA structure, we cannot
regenerate the string to call resize_string_data.  So, no, this cannot
happen.

Note that resizing a live string while you have a live SDATA pointer to
it might result in the SDATA pointer referring to the pre-modification
SDATA, not the current one.

Thanks for continuing to ask such questions!  It's important we don't
forget about one such question and cause crashes, particularly on
feature/igc where there's a shortage of testers (which is not anyone's
fault but mine; I'm working on making it more testable)!

Pip