bug#75322: SAFE_ALLOCA assumed to root Lisp_Objects/SSDATA(string)

[Pip Cet via "Bug reports for GNU Emacs, the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>]

"Eli Zaretskii" <eliz@gnu.org> writes:

>> From: Gerd Möllmann <gerd.moellmann@gmail.com>
>> Cc: pipcet@protonmail.com,  75322@debbugs.gnu.org
>> Date: Sat, 04 Jan 2025 09:47:43 +0100
>>
>> Eli Zaretskii <eliz@gnu.org> writes:
>>
>> > Let's discuss this on a case by case basis.  Not all uses of alloca
>> > are the same or have the same requirements and restrictions.
>>
>> Okay. For the SDATA case, I find Pip's commit does the right thing. It
>> uses xstrdup for the strings and makes sure these are freed later by
>> registering them in one of the special specpdl entries that exist for
>> that purpose (record_unwind_protect_ptr). Works with both GCs, is always
>> safe, I don't see disadvantages, and we don't have to check if GC runs
>> or not and compact strings (old) or moves string data around (igc).
>
> The disadvantage is to xstrdup strings when that is not needed.  It
> increases memory pressure and also costs some CPU time.  Not very
> significant disadvantage, admittedly, but still.  If this is needed,
> it's a small price to pay, but if it isn't needed, it's a waste.

It's easier to xstrdup rather than figure out which no-GC assumptions
are in the subprocess code, which of them are valid (probably not all of
them, considering Fexpand_file_name), and how to document them in a way
that doesn't turn this area of code into more of a minefield than it is
already :-)

> So let's see if we agree that it is indeed needed.  The way to do that
> is to explain how GC could happen while the code which assembles the
> environment in make_environment_block and the code in emacs_spawn
> afterwards run.  Note that we block SIGCHLD and block input while
> emacs_spawn runs.

I'm not going to explain "how GC could happen" in the MPS case: GC can
strike at any time.

For traditional GC, if we want to change the code as little as possible,
we could indeed try to establish no-GC assumptions, but we don't have
to: we can simply start with correct but slow code and maybe drop a note
to the usual suspects that there is optimization potential here.

>> For the Lisp_Object cases, I strongly suspect that these cases are an
>> oversight and were left over when SAFE_ALLOCA_LISP was introduced. The
>> reason for introducing SAFE_ALLOCA_LISP is, IMO, what Pip said (old GC):
>> to make sure that the Lisp_Objects are marked, via specpdl stack entries
>> again, when the specpdl stack(s) are marked. Not doing that looks
>> definitely wrong. Replacing the other ALLOCA macros that are currently
>> used for holding Lisp_Objects with SAFE_ALLOCA_LISP would solve things
>> for both GCs.
>
> Can we first identify those cases, so that we could discuss the
> specific codes in question?

TBH, if there is a single bug caused by an incorrect SAFE_ALLOCA which
should be a SAFE_ALLOCA_LISP, I think that's reason enough to modify
SAFE_ALLOCA to conservatively scan any xmalloc'd memory it uses.  No
performance impact as this case is nonexistent in practice.

Pip