Calling Lisp from undo.c's record_* functions

Calling Lisp from undo.c's record_* functions

[Eli Zaretskii]

Bootstrapping Emacs crashed on me today while building python-wy.el.
Emacs segfaulted while trying to access memory outside its address
space.

Debugging uncovered the following sequence of calls:

  . some Lisp calls 'insert' whose argument is a 12K string
  . this eventually calls insert_from_string_1, which enlarges the
    buffer gap to accommodate for the inserted text
  . in the midst of manipulating the gap, insert_from_string_1 calls
    record_insert
  . record_insert calls record_point, which calls run_undoable_change,
    which calls Lisp
  . the Lisp interpreter decides it's a good time to GC and calls
    garbage_collect
  . garbage_collect calls compact_buffer, which decides the buffer in
    which the insertion happened can be compacted (since the gap
    manipulation is not yet done, and it looks like the buffer has a
    lot of slack space), so it shrinks the gap
  . bottom line: the gap was shrunk behind the back of
    insert_from_string_1, which totally doesn't expect that, and
    proceeds doing silly things, like setting the gap size to a large
    negative value, and from there we are on a certain and very short
    path to a crash

This was caused by a recent change that added a call to
run_undoable_change to various functions in undo.c that record
changes; run_undoable_change calls a Lisp function.

My dilemma is: how to fix this cleanly and correctly?

The record_* functions that are affected by this are called from quite
a few places, most of them in insdel.c, but some in other places.  I
didn't audit all of them, but those I did generally manipulate the gap
and have C pointers to buffer text lying around, because they don't
expect any Lisp to be run or GC to happen.  All of those places are
now living dangerously.

Question #1: do we really need to call Lisp from so deep inside the
bowels of buffer manipulation routines?  Is that safe?  Perhaps we
should reimplement undo-auto--undoable-change inC?

Question #2: one solution is inhibit GC in run_undoable_change.  But
since that could run arbitrary Lisp, is that a good idea? what if we
run out of memory?

Question #3: another possible solution is to set the current buffer's
inhibit_shrinking flag around the call to Lisp in run_undoable_change
-- is this better?  Note that this won't prevent GC in general, so the
follow-up question is can insdel.c functions afford a GC while they
run?

Comments?  Suggestions?

TIA

Re: Calling Lisp from undo.c's record_* functions

[Phillip Lord]

Eli Zaretskii <eliz@gnu.org> writes:

> Question #1: do we really need to call Lisp from so deep inside the
> bowels of buffer manipulation routines?  Is that safe?  Perhaps we
> should reimplement undo-auto--undoable-change inC?
>
> Question #2: one solution is inhibit GC in run_undoable_change.  But
> since that could run arbitrary Lisp, is that a good idea? what if we
> run out of memory?
>
> Question #3: another possible solution is to set the current buffer's
> inhibit_shrinking flag around the call to Lisp in run_undoable_change
> -- is this better?  Note that this won't prevent GC in general, so the
> follow-up question is can insdel.c functions afford a GC while they
> run?
>
> Comments?  Suggestions?


Bah. I spent ages getting that working, and then you go and break it!

My immediate response would be a variation on #1. All
undo-auto--undoable-change really does is a "add-to-list" call. Easy to
reimplement this into C, I think.

The second thing it does is ensure that a timer is being run, which
seems a bit harder. Using an idle timer (the timer only does anything
when there are no commands happening) would solve the problem, although
(obviously) the timer would run when Emacs is entirely idle.

I'd be happy for Stefan to comment, as well.

Phil

Re: Calling Lisp from undo.c's record_* functions

[Stefan Monnier]

> Debugging uncovered the following sequence of calls:

>   . some Lisp calls 'insert' whose argument is a 12K string
>   . this eventually calls insert_from_string_1, which enlarges the
>     buffer gap to accommodate for the inserted text
>   . in the midst of manipulating the gap, insert_from_string_1 calls
>     record_insert
>   . record_insert calls record_point, which calls run_undoable_change,
>     which calls Lisp
>   . the Lisp interpreter decides it's a good time to GC and calls
>     garbage_collect
>   . garbage_collect calls compact_buffer, which decides the buffer in
>     which the insertion happened can be compacted (since the gap
>     manipulation is not yet done, and it looks like the buffer has a
>     lot of slack space), so it shrinks the gap
>   . bottom line: the gap was shrunk behind the back of
>     insert_from_string_1, which totally doesn't expect that, and
>     proceeds doing silly things, like setting the gap size to a large
>     negative value, and from there we are on a certain and very short
>     path to a crash

Duh!

> My dilemma is: how to fix this cleanly and correctly?

Not sure what's the best solution, but the precise moment when
run_undoable_change is executed is not terribly important, so if we
could just move it to either before or after the "critical section",
then that would be a good solution.

> Question #1: do we really need to call Lisp from so deep inside the
> bowels of buffer manipulation routines?  Is that safe?  Perhaps we
> should reimplement undo-auto--undoable-change inC?

That should work, yes.

> Question #2: one solution is inhibit GC in run_undoable_change.  But
> since that could run arbitrary Lisp, is that a good idea? what if we
> run out of memory?

Nah, that'd be too ugly and brittle.

> Question #3: another possible solution is to set the current buffer's
> inhibit_shrinking flag around the call to Lisp in run_undoable_change
> -- is this better?  Note that this won't prevent GC in general, so the
> follow-up question is can insdel.c functions afford a GC while they
> run?

That also sounds risky.


        Stefan

Re: Calling Lisp from undo.c's record_* functions

[Phillip Lord]

Stefan Monnier <monnier@IRO.UMontreal.CA> writes:

>> Debugging uncovered the following sequence of calls:
>
>>   . bottom line: the gap was shrunk behind the back of
>>     insert_from_string_1, which totally doesn't expect that, and
>>     proceeds doing silly things, like setting the gap size to a large
>>     negative value, and from there we are on a certain and very short
>>     path to a crash
>
> Duh!
>
>> My dilemma is: how to fix this cleanly and correctly?
>
> Not sure what's the best solution, but the precise moment when
> run_undoable_change is executed is not terribly important, so if we
> could just move it to either before or after the "critical section",
> then that would be a good solution.

I'm not sure how I would identify the "critical section".

At one point, this function call was actually a hook
(after-undoable-change-hook). Would this solve the problem?

>
>> Question #1: do we really need to call Lisp from so deep inside the
>> bowels of buffer manipulation routines?  Is that safe?  Perhaps we
>> should reimplement undo-auto--undoable-change inC?
>
> That should work, yes.

I've pushed an unfinished fix to fix/segfault-from-run-undoable-change

I'm pretty sure my implementation in C could be simpler. I wasn't sure
how to get from the current-buffer variable in C, to the Lisp_Object, so
in the end, I just called Fcurrent_buffer.

The second part of the plan is to change simple.el to use a idle timer,
as I suggested yesterday. I'll do that later today.

At the moment, I can't replicate the problem, though.

Phil

Re: Calling Lisp from undo.c's record_* functions

[Stefan Monnier]

>>> Debugging uncovered the following sequence of calls:
>>> . bottom line: the gap was shrunk behind the back of
>>> insert_from_string_1, which totally doesn't expect that, and
>>> proceeds doing silly things, like setting the gap size to a large
>>> negative value, and from there we are on a certain and very short
>>> path to a crash
>> Duh!
>>> My dilemma is: how to fix this cleanly and correctly?
>> Not sure what's the best solution, but the precise moment when
>> run_undoable_change is executed is not terribly important, so if we
>> could just move it to either before or after the "critical section",
>> then that would be a good solution.
> I'm not sure how I would identify the "critical section".

The "critical section" is the span of code during which the buffer is
"being worked on" by the insertion function, so during this time, the
buffer shouldn't be modified by anyone else in any way: no Elisp code
should be run, no GC should take place.

> At one point, this function call was actually a hook
> (after-undoable-change-hook). Would this solve the problem?

Nope, makes no difference.


        Stefan

Re: Calling Lisp from undo.c's record_* functions

[Phillip Lord]

Stefan Monnier <monnier@iro.umontreal.ca> writes:
>>>> My dilemma is: how to fix this cleanly and correctly?
>>> Not sure what's the best solution, but the precise moment when
>>> run_undoable_change is executed is not terribly important, so if we
>>> could just move it to either before or after the "critical section",
>>> then that would be a good solution.
>> I'm not sure how I would identify the "critical section".
>
> The "critical section" is the span of code during which the buffer is
> "being worked on" by the insertion function, so during this time, the
> buffer shouldn't be modified by anyone else in any way: no Elisp code
> should be run, no GC should take place.

Assuming the documentation of undo.c is rigourously followed, then this
should only be a problem for insert. All the others say "at the
beginning of the command" or "about to happen". Only insertion says
"before or after". Although, record_insert calls record_point which
rather confuses me -- surely in record_insert can be after, so can
record_point.

In theory, therefore, we should already be safe? Or can the critical
section be longer than a single function call to undo.c? Could the
unsafe period cover several calls?

There are only 7 calls to record_insert so changing the so record_insert
is *always* before would be possible. Big change to make at this point
in the release cycle.


>> At one point, this function call was actually a hook
>> (after-undoable-change-hook). Would this solve the problem?
>
> Nope, makes no difference.


Ok.

I'll finish my patch off this evening, hopefully.

Phil

Re: Calling Lisp from undo.c's record_* functions

[Stefan Monnier]

> In theory, therefore, we should already be safe? Or can the critical
> section be longer than a single function call to undo.c? Could the
> unsafe period cover several calls?

The problem is not in undo.c but in the fact that insdel.c calls
record_insert at a moment where Elisp code can't be run.  So the call to
record_insert would need to be moved.

But of course, in reality it's not the whole record_insert that needs to
be moved, only the run_undoable_change within it.

So if it's difficult to move record_insert to safe spot, maybe we should
take run_undoable_change out of it.

E.g. maybe we could call run_undoable_change from
prepare_to_modify_buffer instead.


        Stefan

Re: Calling Lisp from undo.c's record_* functions

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: Eli Zaretskii <eliz@gnu.org>,  <emacs-devel@gnu.org>
> Date: Tue, 17 Nov 2015 10:40:39 -0500
> 
> > In theory, therefore, we should already be safe? Or can the critical
> > section be longer than a single function call to undo.c? Could the
> > unsafe period cover several calls?
> 
> The problem is not in undo.c but in the fact that insdel.c calls
> record_insert at a moment where Elisp code can't be run.  So the call to
> record_insert would need to be moved.

Even if run_undoable_change doesn't call Lisp, but instead conses the
list by calling Fcons directly?  That's what Phillip did on his
branch.

> But of course, in reality it's not the whole record_insert that needs to
> be moved, only the run_undoable_change within it.
> 
> So if it's difficult to move record_insert to safe spot, maybe we should
> take run_undoable_change out of it.
> 
> E.g. maybe we could call run_undoable_change from
> prepare_to_modify_buffer instead.

Beware: prepare_to_modify_buffer is not always called.

Why call this at such a low level?  Why not at the level of
general_insert_function, Fdelete_region, etc.?  (Yes, that would be
more places to change, but so what?)

Re: Calling Lisp from undo.c's record_* functions

[Eli Zaretskii]

> From: phillip.lord@russet.org.uk (Phillip Lord)
> Cc: Eli Zaretskii <eliz@gnu.org>,  <emacs-devel@gnu.org>
> Date: Tue, 17 Nov 2015 14:42:44 +0000
> 
> > The "critical section" is the span of code during which the buffer is
> > "being worked on" by the insertion function, so during this time, the
> > buffer shouldn't be modified by anyone else in any way: no Elisp code
> > should be run, no GC should take place.
> 
> Assuming the documentation of undo.c is rigourously followed, then this
> should only be a problem for insert. All the others say "at the
> beginning of the command" or "about to happen". Only insertion says
> "before or after". Although, record_insert calls record_point which
> rather confuses me -- surely in record_insert can be after, so can
> record_point.

I don't recommend relying on such promises too much.  You can never
know if someone, some day violates it, whether consciously or
inadvertently.  In fact, it looks like replace_range already does just
that.

> There are only 7 calls to record_insert so changing the so record_insert
> is *always* before would be possible.

Did you look at insert_from_gap?  AFAIU, it assumes that the gap
position stays put during the call to record_insert.  If that can call
Lisp or GC, that assumption is false.

> Big change to make at this point in the release cycle.

No, I don't think so.  We still have several months to go.

Btw, could you perhaps write a short description of this change and
its effects in the Lisp level for etc/NEWS?  I see there's not a word
about this there.  Bonus points if you also update the ELisp manual.

Thanks.

Re: Calling Lisp from undo.c's record_* functions

[Eli Zaretskii]

> From: phillip.lord@russet.org.uk (Phillip Lord)
> Cc: Eli Zaretskii <eliz@gnu.org>,  <emacs-devel@gnu.org>
> Date: Tue, 17 Nov 2015 12:14:27 +0000
> 
> I've pushed an unfinished fix to fix/segfault-from-run-undoable-change

Thanks.

> I'm pretty sure my implementation in C could be simpler. I wasn't sure
> how to get from the current-buffer variable in C, to the Lisp_Object

See XSETBUFFER.

> The second part of the plan is to change simple.el to use a idle timer,
> as I suggested yesterday. I'll do that later today.

What does that timer do?

Would it work to have a non-idle timer that is started once at
startup, and then never shut down, and have its job be put on some
list that the timer will examine?

> At the moment, I can't replicate the problem, though.

The crash happened for me when the build ran this command in
admin/grammars/:

  EMACSLOADPATH= "../../src/emacs.exe" -batch --no-site-file --no-site-lisp -l semantic/wisent/grammar -f wisent-batch-make-parser -o "../../lisp/cedet/semantic/wisent/python-wy.el" python.wy

Try it, maybe it will happen to you as well.  The crash is elusive
(naturally, since it depends on how much consing was done before
that).  It happened to me in a 32-bit build with wide ints, but not
without wide ints.  Also, the build was optimized (-O2), so maybe
that, too, is a prerequisite for reproducing.

Re: Calling Lisp from undo.c's record_* functions

[Stefan Monnier]

>> The problem is not in undo.c but in the fact that insdel.c calls
>> record_insert at a moment where Elisp code can't be run.  So the call to
>> record_insert would need to be moved.
> Even if run_undoable_change doesn't call Lisp, but instead conses the
> list by calling Fcons directly?

My answers were assuming that we want a solution that keeps using Elisp
for run_undoable_change.

> That's what Phillip did on his branch.

Yes, I saw that branch, but that's a separate thread.

>> But of course, in reality it's not the whole record_insert that needs to
>> be moved, only the run_undoable_change within it.
>> So if it's difficult to move record_insert to safe spot, maybe we should
>> take run_undoable_change out of it.
>> E.g. maybe we could call run_undoable_change from
>> prepare_to_modify_buffer instead.
> Beware: prepare_to_modify_buffer is not always called.

When would it not be called?  You mean there are cases where we'd add
stuff to the undo list but we don't run before-change-functions?
Wouldn't that be a bug?

> Why call this at such a low level?

To me, prepare_to_modify_buffer is actually higher-level than record_insert.

> Why not at the level of general_insert_function, Fdelete_region, etc.?
> (Yes, that would be more places to change, but so what?)

Yes, we could push it to an even higher level, but if
prepare_to_modify_buffer works, it's preferable, I think.

In any case, these are just suggestions, I don't have strong opinions on
these issues now ;-)


        Stefan

Re: Calling Lisp from undo.c's record_* functions

[Stefan Monnier]

> Would it work to have a non-idle timer that is started once at
> startup, and then never shut down, and have its job be put on some
> list that the timer will examine?

I ruled that out early on in the design cycle because I think it's wrong
for an application to have such constant background activity.


        Stefan

Re: Calling Lisp from undo.c's record_* functions

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: phillip.lord@russet.org.uk,  emacs-devel@gnu.org
> Date: Tue, 17 Nov 2015 11:49:04 -0500
> 
> >> But of course, in reality it's not the whole record_insert that needs to
> >> be moved, only the run_undoable_change within it.
> >> So if it's difficult to move record_insert to safe spot, maybe we should
> >> take run_undoable_change out of it.
> >> E.g. maybe we could call run_undoable_change from
> >> prepare_to_modify_buffer instead.
> > Beware: prepare_to_modify_buffer is not always called.
> 
> When would it not be called?

E.g., in insert_from_gap.  Also, in many insdel.c functions when they
are called with the PREPARE argument false.

> You mean there are cases where we'd add stuff to the undo list but
> we don't run before-change-functions?

I don't know.  I don't think we have such bugs, but thinking is one
thing and convincing yourself it's true by looking at the callers is
something entirely different...

Re: Calling Lisp from undo.c's record_* functions

[Stefan Monnier]

>> When would it not be called?
> E.g., in insert_from_gap.  Also, in many insdel.c functions when they
> are called with the PREPARE argument false.

AFAIK these are cases where prepare_to_modify_buffer has already been
called earlier.

>> You mean there are cases where we'd add stuff to the undo list but
>> we don't run before-change-functions?
> I don't know.  I don't think we have such bugs, but thinking is one
> thing and convincing yourself it's true by looking at the callers is
> something entirely different...

That's OK, then: I believe that failing to call run_undoable_change is
not more serious than failing to run before-change-functions.

So I think moving the call to run_undoable_change into
prepare_to_modify_buffer is n attractive solution to this problem, since
it preserves the use of Elisp, and it probably also simplifies the code
(since we can remove most/all other calls to run_undoable_change).


        Stefan

Re: Calling Lisp from undo.c's record_* functions

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: phillip.lord@russet.org.uk,  emacs-devel@gnu.org
> Date: Tue, 17 Nov 2015 12:34:08 -0500
> 
> >> When would it not be called?
> > E.g., in insert_from_gap.  Also, in many insdel.c functions when they
> > are called with the PREPARE argument false.
> 
> AFAIK these are cases where prepare_to_modify_buffer has already been
> called earlier.

That's what the comments say, yes.  How deep do we believe them?

> >> You mean there are cases where we'd add stuff to the undo list but
> >> we don't run before-change-functions?
> > I don't know.  I don't think we have such bugs, but thinking is one
> > thing and convincing yourself it's true by looking at the callers is
> > something entirely different...
> 
> That's OK, then: I believe that failing to call run_undoable_change is
> not more serious than failing to run before-change-functions.
> 
> So I think moving the call to run_undoable_change into
> prepare_to_modify_buffer is n attractive solution to this problem, since
> it preserves the use of Elisp, and it probably also simplifies the code
> (since we can remove most/all other calls to run_undoable_change).

Did you take a look at subst-char-in-region?  It calls
prepare_to_modify_buffer from within a loop which seems to assume that
(a) gap position doesn't move, and (b) that pointer into buffer text
is valid across the call to prepare_to_modify_buffer.  GC could
invalidate both assumptions, no?

transpose-regions also does something funky, but it looks safe.

zlib-decompress-region, OTOH, seems to call insert_from_gap, and
doesn't call prepare_to_modify_buffer at all, except at its very end,
when it deletes the uncompressed data.

Bottom line: I think all the functions that manipulate the gap should
be carefully audited to identify any potential problem with this
approach.

Re: Calling Lisp from undo.c's record_* functions

[Stefan Monnier]

> Did you take a look at subst-char-in-region?  It calls
> prepare_to_modify_buffer from within a loop which seems to assume that
> (a) gap position doesn't move, and (b) that pointer into buffer text
> is valid across the call to prepare_to_modify_buffer.  GC could
> invalidate both assumptions, no?
[...]
> Bottom line: I think all the functions that manipulate the gap should
> be carefully audited to identify any potential problem with this
> approach.

prepare_to_modify_buffer runs before-change-functions, so it's been
running Elisp code for many years.  Maybe there are still problems
lurking, but I'm not too worried and they'd have to be fixed regardless
of what we do with run_undoable_change.


        Stefan

Re: Calling Lisp from undo.c's record_* functions

[Eli Zaretskii]

> From: Stefan Monnier <monnier@IRO.UMontreal.CA>
> Cc: phillip.lord@russet.org.uk, emacs-devel@gnu.org
> Date: Tue, 17 Nov 2015 14:09:36 -0500
> 
> prepare_to_modify_buffer runs before-change-functions, so it's been
> running Elisp code for many years.  Maybe there are still problems
> lurking, but I'm not too worried and they'd have to be fixed regardless
> of what we do with run_undoable_change.

I _am_ worried, and I think we should look into this now, and fix
anything that needs fixing.

Re: Calling Lisp from undo.c's record_* functions

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: phillip.lord@russet.org.uk (Phillip Lord),  emacs-devel@gnu.org
> Date: Tue, 17 Nov 2015 11:51:25 -0500
> 
> > Would it work to have a non-idle timer that is started once at
> > startup, and then never shut down, and have its job be put on some
> > list that the timer will examine?
> 
> I ruled that out early on in the design cycle because I think it's wrong
> for an application to have such constant background activity.

OK, but then the same considerations should prohibit using an idle
timer, no?

How about the following idea: we don't start the timer from
run_undoable_change; instead, we set a flag there that will be checked
by the command loop when it finishes execution of a command, and the
call to start the timer will be made then?  We don't really need to
attempt to start the timer for each and every change of every buffer,
do we?

Re: Calling Lisp from undo.c's record_* functions

[Phillip Lord]

Eli Zaretskii <eliz@gnu.org> writes:
>> There are only 7 calls to record_insert so changing the so record_insert
>> is *always* before would be possible.
>
> Did you look at insert_from_gap?  AFAIU, it assumes that the gap
> position stays put during the call to record_insert.  If that can call
> Lisp or GC, that assumption is false.

I didn't look at this. I'm learning C as I go, so I wouldn't put too
much weight behind my own conclusions even if I had.


>> Big change to make at this point in the release cycle.
>
> No, I don't think so.  We still have several months to go.
>
> Btw, could you perhaps write a short description of this change and
> its effects in the Lisp level for etc/NEWS?  I see there's not a word
> about this there.  Bonus points if you also update the ELisp manual.

I will do. Do I get a prize when I get enough points?

Phil

Re: Calling Lisp from undo.c's record_* functions

[Phillip Lord]

Stefan Monnier <monnier@iro.umontreal.ca> writes:
> My answers were assuming that we want a solution that keeps using Elisp
> for run_undoable_change.
>
>> That's what Phillip did on his branch.
>
> Yes, I saw that branch, but that's a separate thread.

I just wanted to see if it worked. I don't intend the branch to
necessarily be the right fix. I would prefer not to be moving
functionality back to C if possible.


>>> But of course, in reality it's not the whole record_insert that needs to
>>> be moved, only the run_undoable_change within it.
>>> So if it's difficult to move record_insert to safe spot, maybe we should
>>> take run_undoable_change out of it.
>>> E.g. maybe we could call run_undoable_change from
>>> prepare_to_modify_buffer instead.
>> Beware: prepare_to_modify_buffer is not always called.
>
> When would it not be called?  You mean there are cases where we'd add
> stuff to the undo list but we don't run before-change-functions?
> Wouldn't that be a bug?

Actually, b-c-f is called by prepare_to_modify_buffer_1, so this
conclusion isn't valid.

As far as I can tell, changing a text property will result in changes to
the undo list, but doesn't call prepare_to_modify_buffer. Rather
modify_text_properties appears to call prepare_to_modify_buffer_1
directly. Bit surprising -- I wouldn't have expected a function called
"blah_1" to be called directly from anywhere other than "blah".


>> Why call this at such a low level?
>
> To me, prepare_to_modify_buffer is actually higher-level than record_insert.
>
>> Why not at the level of general_insert_function, Fdelete_region, etc.?
>> (Yes, that would be more places to change, but so what?)
>
> Yes, we could push it to an even higher level, but if
> prepare_to_modify_buffer works, it's preferable, I think.
>
> In any case, these are just suggestions, I don't have strong opinions on
> these issues now ;-)


I've pushed a branch which moves the run_undoable_change functionality
to insdel.c instead. I've moved the entire function which seems to make
more sense to me. All other functionality remains in lisp.

Phil

Re: Calling Lisp from undo.c's record_* functions

[Phillip Lord]

Eli Zaretskii <eliz@gnu.org> writes:
>> So I think moving the call to run_undoable_change into
>> prepare_to_modify_buffer is n attractive solution to this problem, since
>> it preserves the use of Elisp, and it probably also simplifies the code
>> (since we can remove most/all other calls to run_undoable_change).
>
> Did you take a look at subst-char-in-region?  It calls
> prepare_to_modify_buffer from within a loop which seems to assume that
> (a) gap position doesn't move, and (b) that pointer into buffer text
> is valid across the call to prepare_to_modify_buffer.  GC could
> invalidate both assumptions, no?

This function also signals before-change and after-change values which
don't match, so feel free to rewrite it! It's on my list of things to
do, but it will take me a while.

Phil

Re: Calling Lisp from undo.c's record_* functions

[Phillip Lord]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: phillip.lord@russet.org.uk (Phillip Lord)
>> Cc: Eli Zaretskii <eliz@gnu.org>,  <emacs-devel@gnu.org>
>> Date: Tue, 17 Nov 2015 12:14:27 +0000
>> 
>> I've pushed an unfinished fix to fix/segfault-from-run-undoable-change
>
> Thanks.

That potential fix is finished now. It seems to work (in terms of
functionality) but I can't assess whether it fixes the segfault.

>> I'm pretty sure my implementation in C could be simpler. I wasn't sure
>> how to get from the current-buffer variable in C, to the Lisp_Object
>
> See XSETBUFFER.

Thanks!

>
>> The second part of the plan is to change simple.el to use a idle timer,
>> as I suggested yesterday. I'll do that later today.
>
> What does that timer do?

The timer is an emergency backstop for circumstances when Emacs is
making undoable changes but commands are not happening. Obvious
circumstances for this would be Emacs with a process buffer.


> Would it work to have a non-idle timer that is started once at
> startup, and then never shut down, and have its job be put on some
> list that the timer will examine?

Yes, but it's wasteful.



>> At the moment, I can't replicate the problem, though.
>
> The crash happened for me when the build ran this command in
> admin/grammars/:
>
>   EMACSLOADPATH= "../../src/emacs.exe" -batch --no-site-file --no-site-lisp -l
> semantic/wisent/grammar -f wisent-batch-make-parser -o
> "../../lisp/cedet/semantic/wisent/python-wy.el" python.wy
>
> Try it, maybe it will happen to you as well.  The crash is elusive
> (naturally, since it depends on how much consing was done before
> that).  It happened to me in a 32-bit build with wide ints, but not
> without wide ints.  Also, the build was optimized (-O2), so maybe
> that, too, is a prerequisite for reproducing.

I'm not running on w32, unfortunately. I tried:

EMACSLOADPATH= "../../src/emacs"  -batch --no-site-file --no-site-lisp -l semantic/wisent/grammar -f wisent-batch-make-parser -o "../../lisp/cedet/semantic/wisent/python-wy.el" python.wy

which works fine.

Re: Calling Lisp from undo.c's record_* functions

[Phillip Lord]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Stefan Monnier <monnier@iro.umontreal.ca>
>> Cc: phillip.lord@russet.org.uk (Phillip Lord),  emacs-devel@gnu.org
>> Date: Tue, 17 Nov 2015 11:51:25 -0500
>> 
>> > Would it work to have a non-idle timer that is started once at
>> > startup, and then never shut down, and have its job be put on some
>> > list that the timer will examine?
>> 
>> I ruled that out early on in the design cycle because I think it's wrong
>> for an application to have such constant background activity.
>
> OK, but then the same considerations should prohibit using an idle
> timer, no?

Yep, pretty much.


> How about the following idea: we don't start the timer from
> run_undoable_change; instead, we set a flag there that will be checked
> by the command loop when it finishes execution of a command, and the
> call to start the timer will be made then?

We don't need the timer when the command loops is running. The command
loop will add undo-boundaries.


> We don't really need to attempt to start the timer for each and every
> change of every buffer, do we?

At the moment, we do, yes -- note that we actually only start the timer
if one isn't already running.

There are other solutions. We could just count the number of undoable
events since the last command, and add a boundary once a threshold has
been reached.

Phil

Re: Calling Lisp from undo.c's record_* functions

[Stefan Monnier]

>> > Would it work to have a non-idle timer that is started once at
>> > startup, and then never shut down, and have its job be put on some
>> > list that the timer will examine?
>> I ruled that out early on in the design cycle because I think it's wrong
>> for an application to have such constant background activity.
> OK, but then the same considerations should prohibit using an idle
> timer, no?

No.  The idle timer won't keep firing while Emacs stays idle.

> How about the following idea: we don't start the timer from
> run_undoable_change; instead, we set a flag there that will be checked
> by the command loop when it finishes execution of a command, and the
> call to start the timer will be made then?  We don't really need to
> attempt to start the timer for each and every change of every buffer,
> do we?

That would work similarly to an idle timer, so yes, that would be OK
(tho an idle timer would be simpler).  It suffers from the same problem
as the idle timer: if there's no user interaction but there's continued
process output, we'll fail to re-run the undo-auto-timer after the
first N seconds of idleness.  Nothing too serious (it's still better
than what we have now anyway), tho.


        Stefan

Re: Calling Lisp from undo.c's record_* functions

[Stefan Monnier]

> As far as I can tell, changing a text property will result in changes to
> the undo list, but doesn't call prepare_to_modify_buffer. Rather
> modify_text_properties appears to call prepare_to_modify_buffer_1
> directly. Bit surprising -- I wouldn't have expected a function called
> "blah_1" to be called directly from anywhere other than "blah".

Ah, so maybe the call to run_undoable_change should be in
prepare_to_modify_buffer_1 rather than in prepare_to_modify_buffer.
I haven't actually looked at the code.


        Stefan

Re: Calling Lisp from undo.c's record_* functions

[Eli Zaretskii]

> From: phillip.lord@russet.org.uk (Phillip Lord)
> Cc: <monnier@iro.umontreal.ca>,  <emacs-devel@gnu.org>
> Date: Tue, 17 Nov 2015 20:52:24 +0000
> 
> > Btw, could you perhaps write a short description of this change and
> > its effects in the Lisp level for etc/NEWS?  I see there's not a word
> > about this there.  Bonus points if you also update the ELisp manual.
> 
> I will do.

Thanks.

> Do I get a prize when I get enough points?

Of course.

Re: Calling Lisp from undo.c's record_* functions

[Eli Zaretskii]

> From: Stefan Monnier <monnier@IRO.UMontreal.CA>
> Cc: phillip.lord@russet.org.uk, emacs-devel@gnu.org
> Date: Tue, 17 Nov 2015 21:52:49 -0500
> 
> > How about the following idea: we don't start the timer from
> > run_undoable_change; instead, we set a flag there that will be checked
> > by the command loop when it finishes execution of a command, and the
> > call to start the timer will be made then?  We don't really need to
> > attempt to start the timer for each and every change of every buffer,
> > do we?
> 
> That would work similarly to an idle timer, so yes, that would be OK
> (tho an idle timer would be simpler).  It suffers from the same problem
> as the idle timer: if there's no user interaction but there's continued
> process output, we'll fail to re-run the undo-auto-timer after the
> first N seconds of idleness.  Nothing too serious (it's still better
> than what we have now anyway), tho.

So we could start the timer not in the command loop, but somewhere in
the loop that waits for process output, perhaps.

But if an idle timer is fine, I think Phillip already has everyuthing
sorted out on a branch.

Re: Calling Lisp from undo.c's record_* functions

[Phillip Lord]

Eli Zaretskii <eliz@gnu.org> writes:
>> I will do.
>
> Thanks.
>
>> Do I get a prize when I get enough points?
>
> Of course.


A big happy smily face from Eli?

Phil

Re: Calling Lisp from undo.c's record_* functions

[David Kastrup]

phillip.lord@russet.org.uk (Phillip Lord) writes:

> Eli Zaretskii <eliz@gnu.org> writes:
>>> I will do.
>>
>> Thanks.
>>
>>> Do I get a prize when I get enough points?
>>
>> Of course.
>
> A big happy smily face from Eli?

You probably also expect to win a yacht in a tombola.

-- 
David Kastrup

Re: Calling Lisp from undo.c's record_* functions

[Phillip Lord]

Stefan Monnier <monnier@IRO.UMontreal.CA> writes:

>> As far as I can tell, changing a text property will result in changes to
>> the undo list, but doesn't call prepare_to_modify_buffer. Rather
>> modify_text_properties appears to call prepare_to_modify_buffer_1
>> directly. Bit surprising -- I wouldn't have expected a function called
>> "blah_1" to be called directly from anywhere other than "blah".
>
> Ah, so maybe the call to run_undoable_change should be in
> prepare_to_modify_buffer_1 rather than in prepare_to_modify_buffer.
> I haven't actually looked at the code.


Yeah, that's on the other branch

fix/segfault-undoable-change-prepare-for-buffer

I think I have managed to replicate this error now -- not unit testable,
but forcing a GC in undo-auto--undoable-change like so:

(defun undo-auto--undoable-change ()
  "Called after every undoable buffer change."
  (garbage-collect)
  (add-to-list 'undo-auto--undoably-changed-buffers (current-buffer))
  (undo-auto--boundary-ensure-timer))

then running make check seems to segfault pretty reliably, usually
during bootstrap. fix/segfault-undoable-change-prepare-for-buffer seems
to behave rather better (even with the forced gc).

This seems like the better solution to me.

Phil

Re: Calling Lisp from undo.c's record_* functions

[Phillip Lord]

Eli Zaretskii <eliz@gnu.org> writes:
>> That would work similarly to an idle timer, so yes, that would be OK
>> (tho an idle timer would be simpler).  It suffers from the same problem
>> as the idle timer: if there's no user interaction but there's continued
>> process output, we'll fail to re-run the undo-auto-timer after the
>> first N seconds of idleness.  Nothing too serious (it's still better
>> than what we have now anyway), tho.
>
> So we could start the timer not in the command loop, but somewhere in
> the loop that waits for process output, perhaps.
>
> But if an idle timer is fine, I think Phillip already has everyuthing
> sorted out on a branch.

Not entirely. Either the idle timer runs all the time (which Stefan
doesn't like), or we only run it once (which missed the case when a
process runs). This is (in my experience) an edge case, but then the
timer is there for an edge case anyway. In most circumstances, the
undo-boundaries are inserted by the command-loop, or explicitly.

I don't have a strong edge case for the timer at the moment. All the
process handling modes that I have found add undo-boundaries correctly
anyway.

Phil

Re: Calling Lisp from undo.c's record_* functions

[Eli Zaretskii]

> From: phillip.lord@russet.org.uk (Phillip Lord)
> Cc: <monnier@iro.umontreal.ca>,  <emacs-devel@gnu.org>
> Date: Wed, 18 Nov 2015 09:56:25 +0000
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> >> I will do.
> >
> > Thanks.
> >
> >> Do I get a prize when I get enough points?
> >
> > Of course.
> 
> 
> A big happy smily face from Eli?

Didn't you see it?

Re: Calling Lisp from undo.c's record_* functions

[Eli Zaretskii]

> From: phillip.lord@russet.org.uk (Phillip Lord)
> Cc: Stefan Monnier <monnier@IRO.UMontreal.CA>,  <emacs-devel@gnu.org>
> Date: Wed, 18 Nov 2015 12:31:33 +0000
> 
> > But if an idle timer is fine, I think Phillip already has everyuthing
> > sorted out on a branch.
> 
> Not entirely. Either the idle timer runs all the time (which Stefan
> doesn't like), or we only run it once (which missed the case when a
> process runs).

Really?  IOW, an idle timer doesn't run when we have read something
from a subprocess, and return to the waiting loop?  I would expect the
idle timers to run in that situation.

Re: Calling Lisp from undo.c's record_* functions

[Stefan Monnier]

> Really?  IOW, an idle timer doesn't run when we have read something
> from a subprocess, and return to the waiting loop?

Indeed, no, they don't, AFAIK.


        Stefan

Re: Calling Lisp from undo.c's record_* functions

[Phillip Lord]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: phillip.lord@russet.org.uk (Phillip Lord)
>> Cc: Stefan Monnier <monnier@IRO.UMontreal.CA>,  <emacs-devel@gnu.org>
>> Date: Wed, 18 Nov 2015 12:31:33 +0000
>> 
>> > But if an idle timer is fine, I think Phillip already has everyuthing
>> > sorted out on a branch.
>> 
>> Not entirely. Either the idle timer runs all the time (which Stefan
>> doesn't like), or we only run it once (which missed the case when a
>> process runs).
>
> Really?  IOW, an idle timer doesn't run when we have read something
> from a subprocess, and return to the waiting loop?  I would expect the
> idle timers to run in that situation.

You've misunderstood. Emacs idle timers run when the user is idle, not
emacs!

Can you test this branch for me, and see if you can reproduce the error.

fix/segfault-undoable-change-prepare-for-buffer

This leaves the implementation as was, but moves the run_undoable_change
call to prepare_for_change_1.

Phil

Re: Calling Lisp from undo.c's record_* functions

[Eli Zaretskii]

> From: phillip.lord@russet.org.uk (Phillip Lord)
> Cc: <monnier@IRO.UMontreal.CA>,  <emacs-devel@gnu.org>
> Date: Thu, 19 Nov 2015 10:16:43 +0000
> 
> > Really?  IOW, an idle timer doesn't run when we have read something
> > from a subprocess, and return to the waiting loop?  I would expect the
> > idle timers to run in that situation.
> 
> You've misunderstood. Emacs idle timers run when the user is idle, not
> emacs!

The reason I expected that is that AFAIK the same loop in which Emacs
waits for input and where it determines whether it's idle is the same
loop where Emacs also checks for input from processes (and any other
kind of input that comes from file descriptors, like network or serial
traffic).  That is why I expected that input from processes and
keyboard input will be handled similarly.

> Can you test this branch for me, and see if you can reproduce the error.
> 
> fix/segfault-undoable-change-prepare-for-buffer
> 
> This leaves the implementation as was, but moves the run_undoable_change
> call to prepare_for_change_1.

I will try, but I don't see the need to delay installing these
changes, if the reason is the need to wait for me to run a bootstrap,
since I believe you've found a way to reproduce the original problem
on your system.  I do, however, want to hear one more opinion about
calling Lisp from such a low-level code.

Richard, can we please have your opinion on this?  To recap, the
original problem was that record_point, called by insert_from_string,
changed the buffer gap while insert_from_string was itself
manipulating the gap.  This happened because record_point called Lisp
which caused GC, which decided to compact the buffer.  More details
are here:

  http://lists.gnu.org/archive/html/emacs-devel/2015-11/msg01532.html

The proposed solution moves the call to Lisp to a subroutine of
prepare_to_modify_buffer.  This seems to avoid calling Lisp while the
gap is being manipulated, but my question is: are there any pitfalls
to calling Lisp code on the level of insdel.c functions?  I'd like to
hear your opinion before we decide whether to install the proposed
solution, or look for a safer one.

Thanks.

Re: Calling Lisp from undo.c's record_* functions

[Stefan Monnier]

> prepare_to_modify_buffer.  This seems to avoid calling Lisp while the
> gap is being manipulated, but my question is: are there any pitfalls
> to calling Lisp code on the level of insdel.c functions?  I'd like to
> hear your opinion before we decide whether to install the proposed
> solution, or look for a safer one.

As mentioned earlier, this function has been running Elisp code forever
already via the before-change-functions hook.
So we don't really have a choice here.


        Stefan

Re: Calling Lisp from undo.c's record_* functions

[Eli Zaretskii]

> From: Stefan Monnier <monnier@IRO.UMontreal.CA>
> Cc: phillip.lord@russet.org.uk (Phillip Lord), Richard Stallman <rms@gnu.org>,
>         emacs-devel@gnu.org
> Date: Thu, 19 Nov 2015 12:49:57 -0500
> 
> > prepare_to_modify_buffer.  This seems to avoid calling Lisp while the
> > gap is being manipulated, but my question is: are there any pitfalls
> > to calling Lisp code on the level of insdel.c functions?  I'd like to
> > hear your opinion before we decide whether to install the proposed
> > solution, or look for a safer one.
> 
> As mentioned earlier, this function has been running Elisp code forever
> already via the before-change-functions hook.

That is true, but unlike before-change-functions, which is not always
non-nil, the undo-related call will always be run.  So if calling Lisp
there could cause some problems, those problems will become much more
frequent now.

> So we don't really have a choice here.

There's always a choice.

Re: Calling Lisp from undo.c's record_* functions

[Stefan Monnier]

>> > prepare_to_modify_buffer.  This seems to avoid calling Lisp while the
>> > gap is being manipulated, but my question is: are there any pitfalls
>> > to calling Lisp code on the level of insdel.c functions?  I'd like to
>> > hear your opinion before we decide whether to install the proposed
>> > solution, or look for a safer one.
>> As mentioned earlier, this function has been running Elisp code forever
>> already via the before-change-functions hook.
> That is true, but unlike before-change-functions, which is not always
> non-nil, the undo-related call will always be run.  So if calling Lisp
> there could cause some problems, those problems will become much more
> frequent now.

That's true.  So we may uncover bugs.

Since those bugs would presumably also exist when
before-change-functions is non-nil, we'd have to find a solution for
before-change-functions and that solution would probably also be
applicable to run_undoable_changes.


        Stefan

Re: Calling Lisp from undo.c's record_* functions

[Phillip Lord]

Eli Zaretskii <eliz@gnu.org> writes:

>> Can you test this branch for me, and see if you can reproduce the error.
>> 
>> fix/segfault-undoable-change-prepare-for-buffer
>> 
>> This leaves the implementation as was, but moves the run_undoable_change
>> call to prepare_for_change_1.
>
> I will try, but I don't see the need to delay installing these
> changes, if the reason is the need to wait for me to run a bootstrap,
> since I believe you've found a way to reproduce the original problem
> on your system.

I'll merge this in tomorrow hopefully.

Just to check I have this correct, now that we have feature freeze, I
should merge (or rebase) to the emacs-25 branch? And, then I have to
cherry pick this across to master? Or the other way around? Or does it
not matter?

Phil

Re: Calling Lisp from undo.c's record_* functions

[John Wiegley]

>>>>> Phillip Lord <phillip.lord@russet.org.uk> writes:

> Just to check I have this correct, now that we have feature freeze, I should
> merge (or rebase) to the emacs-25 branch? And, then I have to cherry pick
> this across to master? Or the other way around? Or does it not matter?

Optionally rebase against emacs-25, then merge it there. Your merge commit
will get carried back to master in due course.

John

Re: Calling Lisp from undo.c's record_* functions

[Eli Zaretskii]

> From: phillip.lord@russet.org.uk (Phillip Lord)
> Cc: <monnier@IRO.UMontreal.CA>,  <emacs-devel@gnu.org>
> Date: Sun, 22 Nov 2015 21:44:30 +0000
> 
> > I will try, but I don't see the need to delay installing these
> > changes, if the reason is the need to wait for me to run a bootstrap,
> > since I believe you've found a way to reproduce the original problem
> > on your system.
> 
> I'll merge this in tomorrow hopefully.

Please wait a bit, I'm still waiting for Richard's opinion.

Thanks.

Re: Calling Lisp from undo.c's record_* functions

[Phillip Lord]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: phillip.lord@russet.org.uk (Phillip Lord)
>> Cc: <monnier@IRO.UMontreal.CA>,  <emacs-devel@gnu.org>
>> Date: Sun, 22 Nov 2015 21:44:30 +0000
>> 
>> > I will try, but I don't see the need to delay installing these
>> > changes, if the reason is the need to wait for me to run a bootstrap,
>> > since I believe you've found a way to reproduce the original problem
>> > on your system.
>> 
>> I'll merge this in tomorrow hopefully.
>
> Please wait a bit, I'm still waiting for Richard's opinion.


No worries. I assumed "I don't see the need to delay installing these
changes" meant you wanted me to add them.

Phil

Re: Calling Lisp from undo.c's record_* functions

[Phillip Lord]

John Wiegley <jwiegley@gmail.com> writes:

>>>>>> Phillip Lord <phillip.lord@russet.org.uk> writes:
>
>> Just to check I have this correct, now that we have feature freeze, I should
>> merge (or rebase) to the emacs-25 branch? And, then I have to cherry pick
>> this across to master? Or the other way around? Or does it not matter?
>
> Optionally rebase against emacs-25, then merge it there. Your merge commit
> will get carried back to master in due course.


Ah, merging to master is someone elses problem. That sounds good to me.

Phil

Re: Calling Lisp from undo.c's record_* functions

[Eli Zaretskii]

> From: phillip.lord@russet.org.uk (Phillip Lord)
> Cc: <monnier@IRO.UMontreal.CA>,  <emacs-devel@gnu.org>
> Date: Mon, 23 Nov 2015 17:28:23 +0000
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> From: phillip.lord@russet.org.uk (Phillip Lord)
> >> Cc: <monnier@IRO.UMontreal.CA>,  <emacs-devel@gnu.org>
> >> Date: Sun, 22 Nov 2015 21:44:30 +0000
> >> 
> >> > I will try, but I don't see the need to delay installing these
> >> > changes, if the reason is the need to wait for me to run a bootstrap,
> >> > since I believe you've found a way to reproduce the original problem
> >> > on your system.
> >> 
> >> I'll merge this in tomorrow hopefully.
> >
> > Please wait a bit, I'm still waiting for Richard's opinion.
> 
> 
> No worries. I assumed "I don't see the need to delay installing these
> changes" meant you wanted me to add them.

Thanks for your patience.

Today Richard responded (off list) saying that it is okay to call Lisp
from prepare_to_modify_buffer.  So please go ahead with merging your
branch.

Re: Calling Lisp from undo.c's record_* functions

[Richard Stallman]

[[[ To any NSA and FBI agents reading my email: please consider    ]]]
[[[ whether defending the US Constitution against all enemies,     ]]]
[[[ foreign or domestic, requires you to follow Snowden's example. ]]]

What I precisely said was

> I think it has been true for decades that
> prepare_to_modify_buffer calls Lisp code.
> Thus, it should be ok for insert_from_string to call Lisp code.

I think prepare_to_modify_buffer has called Lisp code for decades
because it runs the before-change properties, and that code was
added in the early 90s.

But it can't hurt to verify this.

-- 
Dr Richard Stallman
President, Free Software Foundation (gnu.org, fsf.org)
Internet Hall-of-Famer (internethalloffame.org)
Skype: No way! See stallman.org/skype.html.

Re: Calling Lisp from undo.c's record_* functions

[Phillip Lord]

Eli Zaretskii <eliz@gnu.org> writes:
>> >> I'll merge this in tomorrow hopefully.
>> >
>> > Please wait a bit, I'm still waiting for Richard's opinion.
>> 
>> 
>> No worries. I assumed "I don't see the need to delay installing these
>> changes" meant you wanted me to add them.
>
> Thanks for your patience.
>
> Today Richard responded (off list) saying that it is okay to call Lisp
> from prepare_to_modify_buffer.  So please go ahead with merging your
> branch.

I've merged these changes in.

Thanks for checking this out for me.

Phil