bug#56623: memory leak introduced by new nonrecursive lisp reader

[Greg Hendershott]

Hi, Lars.

Your previous email from a month ago doesn't seem to have been delivered
to me; maybe same happened for Tom?

Today I took some time to try making both changes to lread.c. That did
not seem to help -- in my testing using Tom's artifacts, memory use
still grew unbounded (whereas it levels off with e.g. Emacs 25.2.2).

Bu as a sanity check, it would be great if Tom could try this, too, to
make sure?

I've gone to some non-trivial effort to make a much more minimal recipe
-- but so far I can't. On the one hand, Tom's bisect seems to point
clearly at this reader change. On the other hand, I don't have any
mental model for how/why, and I can't elicit similar symptoms so far by
working up from simpler examples exercising the reader. :(

Greg

On Sat 27 Aug 2022 at 15:35, Lars Ingebrigtsen <larsi@gnus.org> wrote:
> Mattias Engdegård <mattiase@acm.org> writes:
>
>> A counter-argument is that these undead objects will only be kept 
>> artificially alive as long as GC only takes place in the reader. Anyway, the 
>> hunch is easily tested: try either or both of these changes:
>>
>> 1. reduce stackbufsize:
>>
>> --- a/src/lread.c
>> +++ b/src/lread.c
>> @@ -2919,7 +2919,7 @@ digit_to_number (int character, int base)
>>  /* Size of the fixed-size buffer used during reading.
>>     It should be at least big enough for `invalid_radix_integer' but
>>     can usefully be much bigger than that.  */
>> -enum { stackbufsize = 1024 };
>> +enum { stackbufsize = 64 };
>>
>>  static void
>>  invalid_radix_integer (EMACS_INT radix, char stackbuf[VLA_ELEMS 
>> (stackbufsize)],
>>
>> 2. zero the buffer on entry:
>>
>> --- a/src/lread.c
>> +++ b/src/lread.c
>> @@ -3678,6 +3678,7 @@ read_stack_push (struct read_stack_entry e)
>>  read0 (Lisp_Object readcharfun, bool locate_syms)
>>  {
>>    char stackbuf[stackbufsize];
>> +  memset (stackbuf, 0, stackbufsize);
>>    char *read_buffer = stackbuf;
>>    ptrdiff_t read_buffer_size = sizeof stackbuf;
>>    char *heapbuf = NULL;
>>
>> If either makes things better, maybe we can come up with a solution that 
>> doesn't hurt performance.
>
> This was a month ago -- Tom, did you try these changes?