On Wed, Sep 2, 2015 at 3:08 PM, Eli Zaretskii <eliz@gnu.org> wrote:

> > Date: Tue, 1 Sep 2015 20:48:18 +0000
> > From: Pip Cet <pipcet@gmail.com>
> > Cc: 21380@debbugs.gnu.org
> >
> >     Alternatively, we could turn off atimers (by calling turn_on_atimers)
> >     while Fcopy_sequence runs.
> >
> >
> > I think that would be a better solution. I've done a quick grep for the
> current
> > atimers and at first glance they appear to be okay, but obviously that's
> no
> > guarantee for the future. It might be worth thinking about
> > block_input_and_atimers ().
> >
> > I think it's safe to assume that Lisp timers are only checked if atimers
> are
> > enabled.
>
> Those are two completely separate and independent features, so no,
> it's not safe to make that assumption.  Not sure why you need to
> assume that, though.
>

So we can call turn_on_atimers (true) without potentially enabling atimers
in a critical section.

My assumption was that the reason we have both Lisp timers and atimers is
that atimers run strictly more often than Lisp timers.


> > If it isn't, I think the best way forward is to write
> > block_input_and_atimers () and lock atimers with a counter just like
> input is.
>
> Not sure I follow you.  Are you saying that just calling block_input
> followed by turn_on_atimers is somehow not enough to prevent some Lisp
> from changing Vtimer_list under our feet?
>

I'm not saying that, no, but if another function disables atimers, then
runs Lisp timers, then does something critical that needs atimers to be
disabled, it might break.

> --- a/src/fns.c
> > +++ b/src/fns.c
> > @@ -744,6 +744,9 @@ concat (ptrdiff_t nargs, Lisp_Object *args,
> >           /* Store this element into the result.  */
> >           if (toindex < 0)
> >             {
> > +                if (NILP (tail))
> > +                  break;
> > +
>
> Is this part still needed?


As far as I know, the other two fixes are sufficient. It's needed in case
someone calls copy_sequence on a list that's messed with by code run from a
hook from QUIT, and merely succeeds in avoiding a segfault and producing
incorrect results instead, so I'm not at all sure it should go in.