bug#60237: 30.0.50; tree sitter core dumps when I edebug view a node

[Stefan Monnier]

I pushed your change to `master`, with my patch on top.  Plus a few
other patches to reduce redundancy a bit and fix a FIXME.


        Stefan


Stefan Monnier [2023-03-10 13:28:20] wrote:

>>> I tried cargo-culting the cpu_gc_count stuff for the memory profiler,
>>> see the patch below.  However, something is amiss: this assertion in
>>> profiler.el sometimes triggers:
>>>
>>>     (maphash
>>>      (lambda (backtrace _count)
>>>        (let* ((max (1- (length backtrace)))
>>>               (head (aref backtrace max))
>>>               (best-parent nil)
>>>               (best-match (1+ max))
>>>               (parents (gethash head fun-map)))
>>>          (pcase-dolist (`(,i . ,parent) parents)
>>>            (when t ;; (<= (- max i) best-match) ;Else, it can't be better.
>>>              (let ((match max)
>>>                    (imatch i))
>>>                (cl-assert (>= match imatch))  <<<<<<<<<<<<<<<<<<<<<<<<<<<<
>>>                (cl-assert (function-equal (aref backtrace max)
>>>                                           (aref parent i)))
>>>
>>> I cannot reliably reproduce this, and don't understand what causes the
>>> assertion.  Any hints?
>>
>> Hmm... I just took a look but can't see neither why your change would
>> be more likely to trigger this error than the existing code for the
>> `cpu` case, nor why this assertion should always be true.
>
> I can imagine corner cases where this could trigger, but they all
> involve funny business where we change `profiler-max-stack-depth` during
> a single profiling run (I think you'd need to write ad-hoc ELisp code
> for that).  The only other explanation I can see is that we
> somehow end up with a backtrace that includes `Automatic_GC` somewhere
> not at the top (maybe this can happen with a `post-gc-hook`?).
>
> If you manage to reproduce it, I'd be interested to know the value of
> `backtrace` and `parent` when the assertion fails (and maybe just save
> the `log` hash-table so we can look at it).  It might be a symptom of
> another bug.
>
> And I still can't see how/why this would happen only for the `memory`
> profiler and not for the `cpu` profiler, so I assume it can also happen
> for the `cpu` profiler and we've just been lucky not to bump into it yet.
>
> This said, I think the patch below should fix it for the `cpu` profiler
> and a similar change should fix it for your patch (and the patch is
> arguably right in the sense that without this `nil` entry, the backtrace
> entry created for `Automatic_GC` is not really complete).
>
>
>         Stefan
>
>
> diff --git a/src/profiler.c b/src/profiler.c
> index 8247b2e90c6..295c47a2acd 100644
> --- a/src/profiler.c
> +++ b/src/profiler.c
> @@ -423,7 +423,7 @@ DEFUN ("profiler-cpu-log", Fprofiler_cpu_log, 
> Sprofiler_cpu_log,
>       more for our use afterwards since we can't rely on its special
>       pre-allocated keys anymore.  So we have to allocate a new one.  */
>    cpu_log = profiler_cpu_running ? make_log () : Qnil;
> -  Fputhash (make_vector (1, QAutomatic_GC),
> +  Fputhash (CALLN (Fvector, QAutomatic_GC, Qnil),
>           make_fixnum (cpu_gc_count),
>           result);
>    cpu_gc_count = 0;