bug#23917: Please consider making Bug #23917 a blocker for 25.1 (was Re: org-capture: Capture template ‘g’: Match data clobbered by buffer modification hooks)

bug#23917: Please consider making Bug #23917 a blocker for 25.1 (was Re: org-capture: Capture template ‘g’: Match data clobbered by buffer modification hooks)

[Robert Pluim]

(I'm moving this discussion to the bug, let me know if that's not OK)

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Alex Bennée <alex.bennee@linaro.org>
>> Cc: Eli Zaretskii <eliz@gnu.org>, "N. Jackson" <nljlistbox2@gmail.com>, emacs-devel@gnu.org
>> Date: Sun, 17 Jul 2016 18:28:36 +0100
>> 
>> I've just uninstalled the ELPA installed org and run into the same
>> problem with the bundled version. It certainly doesn't happen with all
>> my capture templates but this particular entry with %a and %c does
>> trigger the error. From my *Messages:
>> 
>>     Clipboard pasted as level 2 subtree
>>     org-capture: Capture template ‘g’: Match data clobbered by buffer modification hooks
>>     "/home/alex/src/emacs/install/share/emacs/25.0.95/lisp/org/org.elc"
>
> Can you come up with a reproducible recipe, starting with "emacs -Q",
> and then loading everything required for the reproduction?  Otherwise,
> I don't see how this problem could be resolved, given the sadly small
> number of people on board who are capable of debugging such problems.
>

Make sure that you have org-20160704 from elpa.

# emacs -Q

;evaluate the following 
(custom-set-variables
 '(package-selected-packages
   (quote
    (org-20160704))))
(package-initialize)

; Now do:

C-x C-f ~/.notes
M-x org-mode
M-x org-capture
t

; This should result in:
Capture template ‘t’: Match data clobbered by buffer modification
hooks

I've tried to follow who clobbers it via GDB, but I keep getting
lost. For me it's always search_regs.end[sub] that has the unexpected
value.

Regards

Robert

bug#23917: Please consider making Bug #23917 a blocker for 25.1 (was Re: org-capture: Capture template ‘g’: Match data clobbered by buffer modification hooks)

[Eli Zaretskii]

> From: Robert Pluim <rpluim@gmail.com>
> CC: 23917@debbugs.gnu.org, Alex Bennée
>  <alex.bennee@linaro.org>, jwiegley@gmail.com, nljlistbox2@gmail.com
> Date: Mon, 18 Jul 2016 14:24:59 +0200
> 
> (I'm moving this discussion to the bug, let me know if that's not OK)

Thanks.

> Make sure that you have org-20160704 from elpa.
> 
> # emacs -Q
> 
> ;evaluate the following 
> (custom-set-variables
>  '(package-selected-packages
>    (quote
>     (org-20160704))))
> (package-initialize)
> 
> ; Now do:
> 
> C-x C-f ~/.notes
> M-x org-mode
> M-x org-capture
> t
> 
> ; This should result in:
> Capture template ‘t’: Match data clobbered by buffer modification
> hooks

Thanks again.

It turns out the validation added in 3a9d6296, to solve bug#23869,
exposed a very serious bug we seem to have always had with
save-match-data called from buffer-modification hooks.

The basic problem is that set-marker restricts the buffer position
passed as its argument to valid limits, between 1 and EOB.  So if you
call set-marker with a value beyond EOB, the marker's position will
silently set to EOB.

Now, when buffer-modification hooks run due to changes made by
replace-match, the replacement has already been done.  If that
replacement shrinks the buffer, then when save-match-data is called,
and calls match-data, the latter will see the new (smaller) value of
EOB.  By default, match-data records the data in markers it creates
for beginning and end of each matched sub-expression.  So the result
is that beginning and/or end of any sub-expression that was beyond the
new EOB will be recorded as EOB.  Then restoring this saved match data
will clobber the data of those sub-expressions.

In the case in point, a single character at EOB (= 62) was deleted,
which made EOB be 61, one less than its previous value.  When
save-match-data was called from within a hook set up by Org, it tried
to record the end of the sub-expression as 62, but set-marker silently
changed that to 61.  That "corrected" value was subsequently restored
when save-match-data was exited, whereas replace-match expected to see
the original value of 62, and therefore barfed.

My suggestion to fix this is below.  I ask for opinions on (1) whether
this looks like TRT, (2) whether it is safe enough for emacs-25, and
(3) whether someone has better ideas.  If someone thinks I've
misunderstood the issue, don't hesitate to explain why, because
frankly it feels very strange to find bugs that seem to have existed
since 1990.

diff --git a/lisp/subr.el b/lisp/subr.el
index e9e19d3..1bb1cb3 100644
--- a/lisp/subr.el
+++ b/lisp/subr.el
@@ -3466,7 +3466,7 @@ save-match-data
   ;; if you need to recompile all the Lisp files using interpreted code.
   (declare (indent 0) (debug t))
   (list 'let
-	'((save-match-data-internal (match-data)))
+	'((save-match-data-internal (match-data 'integers)))
 	(list 'unwind-protect
 	      (cons 'progn body)
 	      ;; It is safe to free (evaporate) markers immediately here,

bug#23917: Please consider making Bug #23917 a blocker for 25.1 (was Re: org-capture: Capture template ‘g’: Match data clobbered by buffer modification hooks)

[John Wiegley]

[-- Attachment #1: Type: text/plain, Size: 485 bytes --]

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

> My suggestion to fix this is below. I ask for opinions on (1) whether this
> looks like TRT, (2) whether it is safe enough for emacs-25, and (3) whether
> someone has better ideas.

I didn't even know match-data took arguments, so I defer to your judgment on
this issue, Eli.

-- 
John Wiegley                  GPG fingerprint = 4710 CF98 AF9B 327B B80F
http://newartisans.com                          60E1 46C4 BD1A 7AC1 4BA2

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 629 bytes --]

bug#23917: Please consider making Bug #23917 a blocker for 25.1 (was Re: org-capture: Capture template ‘g’: Match data clobbered by buffer modification hooks)

[Eli Zaretskii]

> From: John Wiegley <jwiegley@gmail.com>
> Cc: Robert Pluim <rpluim@gmail.com>,  Stefan Monnier <monnier@iro.umontreal.ca>,  23917@debbugs.gnu.org,  alex.bennee@linaro.org,  nljlistbox2@gmail.com
> Date: Mon, 18 Jul 2016 12:04:11 -0700
> 
> >>>>> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > My suggestion to fix this is below. I ask for opinions on (1) whether this
> > looks like TRT, (2) whether it is safe enough for emacs-25, and (3) whether
> > someone has better ideas.
> 
> I didn't even know match-data took arguments, so I defer to your judgment on
> this issue, Eli.

Neither did I, but I've read the code through which I stepped ;-)

bug#23917: Please consider making Bug #23917 a blocker for 25.1 (was Re: org-capture: Capture template ‘g’: Match data clobbered by buffer modification hooks)

[Stefan Monnier]

> In the case in point, a single character at EOB (= 62) was deleted,
> which made EOB be 61, one less than its previous value.  When
> save-match-data was called from within a hook set up by Org, it tried
> to record the end of the sub-expression as 62, but set-marker silently
> changed that to 61.  That "corrected" value was subsequently restored
> when save-match-data was exited, whereas replace-match expected to see
> the original value of 62, and therefore barfed.

I think this change performed by save-match-data is harmless: the old
value (62) was not valid any more anyway.

So I think a safe fix is to try and relax the check we added to
replace-match so it doesn't get all worked up when something ≥ EOB gets
changed to something else that's also ≥ EOB.

Or maybe instead of signaling an error, we could simply skip the "Adjust
search data for this change".  I like the idea of signaling an error, as
a debugging help, but maybe for emacs-25 we should go for something less
intrusive after all?

This said, I don't fully understand what's going on: bug#23869 reported
a crash, but AFAICT the match-data here is only used to adjust
search_regs which seems like it wouldn't cause a crash, even if the new
values are bogus.  So maybe signaling an error is important because the
crash happens further down.

> -	'((save-match-data-internal (match-data)))
> +	'((save-match-data-internal (match-data 'integers)))

That looks risky.


        Stefan

bug#23917: Please consider making Bug #23917 a blocker for 25.1 (was Re: org-capture: Capture template ‘g’: Match data clobbered by buffer modification hooks)

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: Robert Pluim <rpluim@gmail.com>,  23917@debbugs.gnu.org,  alex.bennee@linaro.org,  jwiegley@gmail.com,  nljlistbox2@gmail.com
> Date: Mon, 18 Jul 2016 20:58:35 -0400
> 
> > In the case in point, a single character at EOB (= 62) was deleted,
> > which made EOB be 61, one less than its previous value.  When
> > save-match-data was called from within a hook set up by Org, it tried
> > to record the end of the sub-expression as 62, but set-marker silently
> > changed that to 61.  That "corrected" value was subsequently restored
> > when save-match-data was exited, whereas replace-match expected to see
> > the original value of 62, and therefore barfed.
> 
> I think this change performed by save-match-data is harmless: the old
> value (62) was not valid any more anyway.

In this particular case, yes.  But only in this case, because (a)
there's actually only one sub-expression, and (b) it ends exactly at
EOB.

The more general problem is when there's at least one more
sub-expression, whose start and/or end are after the new EOB.  Those
sub-expression's data will be completely bogus after the adjustment,
should the buffer-modification hooks use save-match-data.

> So I think a safe fix is to try and relax the check we added to
> replace-match so it doesn't get all worked up when something ≥ EOB gets
> changed to something else that's also ≥ EOB.

And lose the other sub-expressions in a more general case?  Really?

> Or maybe instead of signaling an error, we could simply skip the "Adjust
> search data for this change".

That would still sweep the problem under the carpet, leaving the match
data bogus, so I don't like doing that.

> This said, I don't fully understand what's going on: bug#23869 reported
> a crash, but AFAICT the match-data here is only used to adjust
> search_regs which seems like it wouldn't cause a crash, even if the new
> values are bogus.

The crash in bug#23869 was due to this:

  newpoint = search_regs.start[sub] + SCHARS (newtext);
  [...]
  /* Now move point "officially" to the start of the inserted replacement.  */
  move_if_not_intangible (newpoint);  <<<<<<<<<<<<<<<<<<<<<<<

because due to clobbering, newpoint became -1.

> > -	'((save-match-data-internal (match-data)))
> > +	'((save-match-data-internal (match-data 'integers)))
> 
> That looks risky.

Then how about manually doing the equivalent of save-match-data around
the call to replace_range, calling match-data with non-nil argument?

bug#23917: Please consider making Bug #23917 a blocker for 25.1 (was Re: org-capture: Capture template ‘g’: Match data clobbered by buffer modification hooks)

[Stefan Monnier]

> The more general problem is when there's at least one more
> sub-expression, whose start and/or end are after the new EOB.
> Those sub-expression's data will be completely bogus after the
> adjustment,

If they were after the EOB, they were already bogus to start with.
So there's really not much harm moving them around.  And in any case,
that's what has been happening for ever and has proved safe enough.

>> So I think a safe fix is to try and relax the check we added to
>> replace-match so it doesn't get all worked up when something ≥ EOB gets
>> changed to something else that's also ≥ EOB.
> And lose the other sub-expressions in a more general case?  Really?

I'm not sure what you mean by "losing sub-expressions".  But yes,
I think the behavior of save-match-data you describe is not
a real problem.  Arguably if save-match-data moves positions from
outside BEGV...ZV to inside it it's a problem.  But if it moves them
from outside BEG...Z to inside it, I think it's perfectly fine.

>> Or maybe instead of signaling an error, we could simply skip the "Adjust
>> search data for this change".
> That would still sweep the problem under the carpet, leaving the match
> data bogus, so I don't like doing that.

Maybe I'm not 100% satisfied with the behavior either, but I don't think
it's a significant problem and I don't think it'd cause the crash we saw
in bug#23869.

> The crash in bug#23869 was due to this:
>
>   newpoint = search_regs.start[sub] + SCHARS (newtext);
>   [...]
>   /* Now move point "officially" to the start of the inserted replacement.  */
>   move_if_not_intangible (newpoint);  <<<<<<<<<<<<<<<<<<<<<<<
>
> because due to clobbering, newpoint became -1.

Ah, I see.

Then maybe another fix is to compute newpoint before we call
replace_range, so it uses search_regs.start[sub] before the
*-change-functions can mess it up.  IOW:

    @@ -2726,9 +2726,9 @@ since only regular expressions have distinguished subexpressions.  */)
       unsigned  num_regs = search_regs.num_regs;
     
       /* Replace the old text with the new in the cleanest possible way.  */
    +  newpoint = search_regs.start[sub] + SCHARS (newtext);
       replace_range (search_regs.start[sub], search_regs.end[sub],
     		 newtext, 1, 0, 1);
    -  newpoint = search_regs.start[sub] + SCHARS (newtext);
     
       if (case_action == all_caps)
         Fupcase_region (make_number (search_regs.start[sub]),

Would that be sufficient to avoid the crash?  Why not?


        Stefan

bug#23917: Please consider making Bug #23917 a blocker for 25.1 (was Re: org-capture: Capture template ‘g’: Match data clobbered by buffer modification hooks)

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: rpluim@gmail.com,  23917@debbugs.gnu.org,  alex.bennee@linaro.org,  jwiegley@gmail.com,  nljlistbox2@gmail.com
> Date: Tue, 19 Jul 2016 00:48:19 -0400
> 
> > The more general problem is when there's at least one more
> > sub-expression, whose start and/or end are after the new EOB.
> > Those sub-expression's data will be completely bogus after the
> > adjustment,
> 
> If they were after the EOB, they were already bogus to start with.

I think we are mis-communicating.  I mean the following scenario:

Before call to replace_range in replace-match:

   |---------------------------|---|------|----|
   s1                         e1   s2    e2   EOB

(s1, e1, etc. are the start and end of the corresponding
sub-expressions.)

After the call to replace_range in replace-match:

   |---------|---|------|----|
   s1       e1   s2    e2   EOB

IOW, the 1st sub-expression got replaced with a much shorter text,
which made EOB be smaller than the original beginning and end of the
2nd sub-expression.  There's nothing bogus with this, is there?  The
user will expect to get match-data adjusted as shown in the second
diagram, and that's what she will really get -- unless there are
buffer-modification hooks that use save-match-data.  In the latter
case, what the user will get instead is this:

   |---------|---|------|----|
   s1                       EOB
                            e1
			    s2
			    e2

and that is even before the adjustment code kicks in and makes
"adjustments" with an incorrect adjustment value, which is computed as

  newpoint = search_regs.start[sub] + SCHARS (newtext);
  [...]
  ptrdiff_t change = newpoint - search_regs.end[sub];

and so will use the new EOB as search_regs.end[sub], instead of the
correct original value of e1 from the first diagram above.

IOW, the call to save-match-data in a buffer-modification hook
_disrupts_ the normal operation of replace-match in this case, by
indirectly sabotaging the adjustment of match data after the
replacement.

Am I missing something?

> And in any case, that's what has been happening for ever and has
> proved safe enough.

So you are saying that if a bug has been happening "for ever", it
doesn't have to be fixed?  (I disagree about "safe enough": the amount
of bug reports in our data base that are not reproducible and about
whose reasons we have no clear idea is non-negligible, so we don't
really know whether this particular issue caused trouble in the past
or not.)

> >> So I think a safe fix is to try and relax the check we added to
> >> replace-match so it doesn't get all worked up when something ≥ EOB gets
> >> changed to something else that's also ≥ EOB.
> > And lose the other sub-expressions in a more general case?  Really?
> 
> I'm not sure what you mean by "losing sub-expressions".

See above: the match data for any sub-expressions beyond the one that
shrunk too much is now bogus.  Thus "losing".

> >> Or maybe instead of signaling an error, we could simply skip the "Adjust
> >> search data for this change".
> > That would still sweep the problem under the carpet, leaving the match
> > data bogus, so I don't like doing that.
> 
> Maybe I'm not 100% satisfied with the behavior either, but I don't think
> it's a significant problem and I don't think it'd cause the crash we saw
> in bug#23869.

We don't only solve bugs that cause crashes, do we?  When I debugged
the crash in bug#23869, I found and tried to solve the root cause of
it, not just the symptom that caused the assertion violation.  I still
think we should strive to solve the root cause.

As for the significance of the problem, I hope you will reconsider
this after reading the above description of the scenario I have in
mind.  (Or tell me where I am wrong.)

> > The crash in bug#23869 was due to this:
> >
> >   newpoint = search_regs.start[sub] + SCHARS (newtext);
> >   [...]
> >   /* Now move point "officially" to the start of the inserted replacement.  */
> >   move_if_not_intangible (newpoint);  <<<<<<<<<<<<<<<<<<<<<<<
> >
> > because due to clobbering, newpoint became -1.
> 
> Ah, I see.
> 
> Then maybe another fix is to compute newpoint before we call
> replace_range, so it uses search_regs.start[sub] before the
> *-change-functions can mess it up.  IOW:
> 
>     @@ -2726,9 +2726,9 @@ since only regular expressions have distinguished 
> subexpressions.  */)
>        unsigned  num_regs = search_regs.num_regs;
>     
>        /* Replace the old text with the new in the cleanest possible way.  */
>     +  newpoint = search_regs.start[sub] + SCHARS (newtext);
>        replace_range (search_regs.start[sub], search_regs.end[sub],
>                  newtext, 1, 0, 1);
>     -  newpoint = search_regs.start[sub] + SCHARS (newtext);
>      
>        if (case_action == all_caps)
>          Fupcase_region (make_number (search_regs.start[sub]),
> 
> Would that be sufficient to avoid the crash?  Why not?

To avoid the crash in that particular use case, yes (but it can be
avoided even easier, by validating the value of newpoint before the
call to move_if_not_intangible).  But what about the root cause?  It
is a clear bug, and could even cause crashes, if one of the match-data
end-point positions becomes negative as result of the bogus
adjustment, and someone then uses those positions for something.
What's worse, these bugs happen in programs that are completely valid
on the Lisp level.

bug#23917: Please consider making Bug #23917 a blocker for 25.1 (was Re: org-capture: Capture template ‘g’: Match data clobbered by buffer modification hooks)

[Stefan Monnier]

> Before call to replace_range in replace-match:
>
>    |---------------------------|---|------|----|
>    s1                         e1   s2    e2   EOB
>
> (s1, e1, etc. are the start and end of the corresponding
> sub-expressions.)
>
> After the call to replace_range in replace-match:
>
>    |---------|---|------|----|
>    s1       e1   s2    e2   EOB

Ah, right, now I see my confusion, thank you.

So, the data is within bounds before replace_range but after bounds
afterwards and the subsequent adjustments should fix it, but an
intervening save-match-data will mess it up.

Hmm... indeed, the adjustment isn't working correctly in this case.

I don't think we can safely change the way save-match-data works, so
I guess the next best thing is:
- copy search_regs.start and search_regs.end before calling replace_range.
- use that copy when adjusting the match data.
Or equivalently, use save-match-data.  IOW go back to your original patch.
Duh!


        Stefan

bug#23917: Please consider making Bug #23917 a blocker for 25.1 (was Re: org-capture: Capture template ‘g’: Match data clobbered by buffer modification hooks)

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: rpluim@gmail.com,  23917@debbugs.gnu.org,  alex.bennee@linaro.org,  jwiegley@gmail.com,  nljlistbox2@gmail.com
> Date: Tue, 19 Jul 2016 12:03:51 -0400
> 
> I guess the next best thing is:
> - copy search_regs.start and search_regs.end before calling replace_range.
> - use that copy when adjusting the match data.
> Or equivalently, use save-match-data.  IOW go back to your original patch.
> Duh!

Do we care that using save-match-data in every call to replace-match
might mean a performance hit?  If it will, then this will again punish
most of the users for the benefit of those few who (1) have
buffer-modification hooks, and (2) those hooks call save-match-data.

bug#23917: [O] bug#23917: Please consider making Bug #23917 a blocker for 25.1 (was Re: org-capture: Capture template ‘g’: Match data clobbered by buffer modification hooks)

[Alex Bennée]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Stefan Monnier <monnier@iro.umontreal.ca>
>> Cc: rpluim@gmail.com,  23917@debbugs.gnu.org,  alex.bennee@linaro.org,  jwiegley@gmail.com,  nljlistbox2@gmail.com
>> Date: Tue, 19 Jul 2016 12:03:51 -0400
>>
>> I guess the next best thing is:
>> - copy search_regs.start and search_regs.end before calling replace_range.
>> - use that copy when adjusting the match data.
>> Or equivalently, use save-match-data.  IOW go back to your original patch.
>> Duh!
>
> Do we care that using save-match-data in every call to replace-match
> might mean a performance hit?  If it will, then this will again punish
> most of the users for the benefit of those few who (1) have
> buffer-modification hooks, and (2) those hooks call save-match-data.

I care unless there is an easy way to identify which buffer modification
hooks are responsible so I can take steps as a user to mitigate the
problems.

--
Alex Bennée

bug#23917: [O] bug#23917: Please consider making Bug #23917 a blocker for 25.1 (was Re: org-capture: Capture template ‘g’: Match data clobbered by buffer modification hooks)

[Eli Zaretskii]

> From: Alex Bennée <alex.bennee@linaro.org>
> Cc: Stefan Monnier <monnier@iro.umontreal.ca>, 23917@debbugs.gnu.org, rpluim@gmail.com, jwiegley@gmail.com, nljlistbox2@gmail.com
> Date: Tue, 19 Jul 2016 18:05:37 +0100
> 
> > Do we care that using save-match-data in every call to replace-match
> > might mean a performance hit?  If it will, then this will again punish
> > most of the users for the benefit of those few who (1) have
> > buffer-modification hooks, and (2) those hooks call save-match-data.
> 
> I care unless there is an easy way to identify which buffer modification
> hooks are responsible so I can take steps as a user to mitigate the
> problems.

Any hook in before-change-functions or after-change-functions that
calls save-match-data.

If we care about the performance hit, we need to come up with a
different solution for this problem (or measure the performance hit
and convince ourselves it is not a big deal).

bug#23917: [O] bug#23917: Please consider making Bug #23917 a blocker for 25.1 (was Re: org-capture: Capture template ‘g’: Match data clobbered by buffer modification hooks)

[Alex Bennée]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Alex Bennée <alex.bennee@linaro.org>
>> Cc: Stefan Monnier <monnier@iro.umontreal.ca>, 23917@debbugs.gnu.org, rpluim@gmail.com, jwiegley@gmail.com, nljlistbox2@gmail.com
>> Date: Tue, 19 Jul 2016 18:05:37 +0100
>>
>> > Do we care that using save-match-data in every call to replace-match
>> > might mean a performance hit?  If it will, then this will again punish
>> > most of the users for the benefit of those few who (1) have
>> > buffer-modification hooks, and (2) those hooks call save-match-data.
>>
>> I care unless there is an easy way to identify which buffer modification
>> hooks are responsible so I can take steps as a user to mitigate the
>> problems.
>
> Any hook in before-change-functions or after-change-functions that
> calls save-match-data.
>
> If we care about the performance hit, we need to come up with a
> different solution for this problem (or measure the performance hit
> and convince ourselves it is not a big deal).

Thanks for the hint. So in my case it was flycheck-handle-change which
was triggering the problem:

(defun flycheck-handle-change (beg end _len)
  "Handle a buffer change between BEG and END.

BEG and END mark the beginning and end of the change text.  _LEN
is ignored.

Start a syntax check if a new line has been inserted into the
buffer."
  ;; Save and restore the match data, as recommended in (elisp)Change Hooks
  (save-match-data
    (when flycheck-mode
      ;; The buffer was changed, thus clear the idle timer
      (flycheck-clear-idle-change-timer)
      (if (string-match-p (rx "\n") (buffer-substring beg end))
          (flycheck-buffer-automatically 'new-line 'force-deferred)
        (setq flycheck-idle-change-timer
              (run-at-time flycheck-idle-change-delay nil
                           #'flycheck-handle-idle-change))))))

However it doesn't look as though it tweaks the buffer until idle timer
has run. Weird....

--
Alex Bennée

bug#23917: [O] bug#23917: Please consider making Bug #23917 a blocker for 25.1 (was Re: org-capture: Capture template ‘g’: Match data clobbered by buffer modification hooks)

[Sebastian Wiesner]

Please do not CC me on Emacs bug threads.  If there's an issue in Flycheck please take it to our issue tracker at https://github.com/flycheck/flycheck/issues where we can keep track of it.

bug#23917: [O] bug#23917: Please consider making Bug #23917 a blocker for 25.1 (was Re: org-capture: Capture template ‘g’: Match data clobbered by buffer modification hooks)

[Eli Zaretskii]

> From: Alex Bennée <alex.bennee@linaro.org>
> Cc: monnier@iro.umontreal.ca, 23917@debbugs.gnu.org, rpluim@gmail.com, jwiegley@gmail.com, nljlistbox2@gmail.com, me@lunaryorn.com
> Date: Tue, 19 Jul 2016 18:45:44 +0100
> 
>   ;; Save and restore the match data, as recommended in (elisp)Change Hooks
>   (save-match-data
>     (when flycheck-mode
>       ;; The buffer was changed, thus clear the idle timer
>       (flycheck-clear-idle-change-timer)
>       (if (string-match-p (rx "\n") (buffer-substring beg end))
>           (flycheck-buffer-automatically 'new-line 'force-deferred)
>         (setq flycheck-idle-change-timer
>               (run-at-time flycheck-idle-change-delay nil
>                            #'flycheck-handle-idle-change))))))
> 
> However it doesn't look as though it tweaks the buffer until idle timer
> has run. Weird....

Tweaking the buffer is not what causes the problem.  It's the call to
save-match-data itself.  It doesn't matter at all what the code inside
save-match-data does.

bug#23917: [O] bug#23917: bug#23917: Please consider making Bug #23917 a blocker for 25.1 (was Re: org-capture: Capture template ‘g’: Match data clobbered by buffer modification hooks)

[Alex Bennée]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Alex Bennée <alex.bennee@linaro.org>
>> Cc: monnier@iro.umontreal.ca, 23917@debbugs.gnu.org, rpluim@gmail.com, jwiegley@gmail.com, nljlistbox2@gmail.com, me@lunaryorn.com
>> Date: Tue, 19 Jul 2016 18:45:44 +0100
>>
>>   ;; Save and restore the match data, as recommended in (elisp)Change Hooks
>>   (save-match-data
>>     (when flycheck-mode
>>       ;; The buffer was changed, thus clear the idle timer
>>       (flycheck-clear-idle-change-timer)
>>       (if (string-match-p (rx "\n") (buffer-substring beg end))
>>           (flycheck-buffer-automatically 'new-line 'force-deferred)
>>         (setq flycheck-idle-change-timer
>>               (run-at-time flycheck-idle-change-delay nil
>>                            #'flycheck-handle-idle-change))))))
>>
>> However it doesn't look as though it tweaks the buffer until idle timer
>> has run. Weird....
>
> Tweaking the buffer is not what causes the problem.  It's the call to
> save-match-data itself.  It doesn't matter at all what the code inside
> save-match-data does.

Ahh I misunderstood the description of the problem. I thought it was a
changing of the buffer underneath that meant the match data wasn't
updated and hence got out of sync. So is the match data already out of
sync by the time the save-match-data call is made?

--
Alex Bennée

bug#23917: [O] bug#23917: bug#23917: Please consider making Bug #23917 a blocker for 25.1 (was Re: org-capture: Capture template ‘g’: Match data clobbered by buffer modification hooks)

[Eli Zaretskii]

> From: Alex Bennée <alex.bennee@linaro.org>
> Cc: 23917@debbugs.gnu.org, rpluim@gmail.com, jwiegley@gmail.com, monnier@iro.umontreal.ca, nljlistbox2@gmail.com
> Date: Wed, 20 Jul 2016 10:48:45 +0100
> 
> So is the match data already out of sync by the time the
> save-match-data call is made?

Yes.

bug#23917: Please consider making Bug #23917 a blocker for 25.1 (was Re: org-capture: Capture template ‘g’: Match data clobbered by buffer modification hooks)

[Stefan Monnier]

> Do we care that using save-match-data in every call to replace-match
> might mean a performance hit?

I do but:
- to be honest, it's probably lost in the noise.
- if we copy search_regs.start and search_regs.end with something like
  alloca+memcpy (instead of calling Fmatch_data), the cost should be even more
  lost in the noise.  Especially if you consider that the current code
  already loops through the match-data to adjust it.
- it's the best fix we've found so far.

> If it will, then this will again punish
> most of the users for the benefit of those few who (1) have
> buffer-modification hooks, and (2) those hooks call save-match-data.

I think the combination of 1 and 2 is actually pretty frequent.


        Stefan


PS: I can think of one (theoretical) other/better way to fix this
    problem: move the match-data adjustment so it's done within
    replace_range before running the after-change-functions.  I think
    this would be very satisfactory, since it would mean that the Elisp
    code would always see the valid match-data (whereas currently the
    after-change-functions get passed not-yet-adjusted match-data), so
    save-match-data wouldn't mess it up.  But I fear this would require
    much larger changes (and might involve a heavier performance cost as
    well).

bug#23917: Please consider making Bug #23917 a blocker for 25.1 (was Re: org-capture: Capture template ‘g’: Match data clobbered by buffer modification hooks)

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: rpluim@gmail.com,  23917@debbugs.gnu.org,  alex.bennee@linaro.org,  jwiegley@gmail.com,  nljlistbox2@gmail.com
> Date: Tue, 19 Jul 2016 21:50:07 -0400
> 
> > Do we care that using save-match-data in every call to replace-match
> > might mean a performance hit?
> 
> I do but:
> - to be honest, it's probably lost in the noise.
> - if we copy search_regs.start and search_regs.end with something like
>   alloca+memcpy (instead of calling Fmatch_data), the cost should be even more
>   lost in the noise.  Especially if you consider that the current code
>   already loops through the match-data to adjust it.
> - it's the best fix we've found so far.

What about Noam's suggestion:

> Is it not possible to adjust the match data *before* calling buffer
> modification hooks?  Seems to me the root of the problem is that buffer
> modification hooks get to see this invalid intermediate state where the
> match data is out of sync with the buffer.

Is it OK to adjust the match data before actually making the
replacement?  If so, I think it's a simpler solution.

> PS: I can think of one (theoretical) other/better way to fix this
>     problem: move the match-data adjustment so it's done within
>     replace_range before running the after-change-functions.

Isn't that almost the same as what Noam suggested?

bug#23917: Please consider making Bug #23917 a blocker for 25.1 (was Re: org-capture: Capture template ‘g’: Match data clobbered by buffer modification hooks)

[Stefan Monnier]

> Is it OK to adjust the match data before actually making the
> replacement?  If so, I think it's a simpler solution.
>> PS: I can think of one (theoretical) other/better way to fix this
>> problem: move the match-data adjustment so it's done within
>> replace_range before running the after-change-functions.
> Isn't that almost the same as what Noam suggested?

Yes, it's the same.  And yes, I like the idea, but I just don't know
what it would look like as a patch.  I have the impression that it could
prove either expensive in CPU time and backward incompatible
(e.g. adjust markers for every buffer modification), or require
extensive code surgery and/or breaking some abstractions.

This is just an impression, tho.  I think it'd definitely be the better
solution, so it's worth investigating anyway, if only for "master" rather
than for "emacs-25".


        Stefan

bug#23917: Please consider making Bug #23917 a blocker for 25.1 (was Re: org-capture: Capture template ‘g’: Match data clobbered by buffer modification hooks)

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: rpluim@gmail.com,  23917@debbugs.gnu.org,  alex.bennee@linaro.org,  jwiegley@gmail.com,  nljlistbox2@gmail.com,  npostavs@users.sourceforge.net
> Date: Wed, 20 Jul 2016 14:19:59 -0400
> 
> > Is it OK to adjust the match data before actually making the
> > replacement?  If so, I think it's a simpler solution.
> >> PS: I can think of one (theoretical) other/better way to fix this
> >> problem: move the match-data adjustment so it's done within
> >> replace_range before running the after-change-functions.
> > Isn't that almost the same as what Noam suggested?
> 
> Yes, it's the same.  And yes, I like the idea, but I just don't know
> what it would look like as a patch.  I have the impression that it could
> prove either expensive in CPU time and backward incompatible
> (e.g. adjust markers for every buffer modification), or require
> extensive code surgery and/or breaking some abstractions.
> 
> This is just an impression, tho.  I think it'd definitely be the better
> solution, so it's worth investigating anyway, if only for "master" rather
> than for "emacs-25".

Maybe there's a misunderstanding.  What Noam suggested was just to
move the code which adjusts search_regs.start[i] and .end[i] to before
the call to replace_range.  The above values are not markers, and no
other markers are involved, so I'm not sure which markers did you
allude to.  Or why it would be CPU intensive.

What did I miss?

bug#23917: Please consider making Bug #23917 a blocker for 25.1 (was Re: org-capture: Capture template ‘g’: Match data clobbered by buffer modification hooks)

[Stefan Monnier]

> Maybe there's a misunderstanding.  What Noam suggested was just to
> move the code which adjusts search_regs.start[i] and .end[i] to before
> the call to replace_range.

Oh, right, that's also an option.  It might suffer from another problem,
which is that the match-data will be broken while the
before-change-functions are run, so if there's a save-match-data there
we're back to square one.

Admittedly, before-change-functions is used less often, so it might be
good enough.


        Stefan

bug#23917: Please consider making Bug #23917 a blocker for 25.1 (was Re: org-capture: Capture template ‘g’: Match data clobbered by buffer modification hooks)

[npostavs]

[-- Attachment #1: Type: text/plain, Size: 792 bytes --]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>> Maybe there's a misunderstanding.  What Noam suggested was just to
>> move the code which adjusts search_regs.start[i] and .end[i] to before
>> the call to replace_range.
>
> Oh, right, that's also an option.  It might suffer from another problem,
> which is that the match-data will be broken while the
> before-change-functions are run, so if there's a save-match-data there
> we're back to square one.

Solution: adjust in between the before and after change functions.
Patch below.  I think there shouldn't be performance problems, although
it does add yet another flag to replace_range (by the way, I noticed
that the MARKERS flags is never 0, so it's redundant).  I tested with
the attached bug-23917-match-data-buffer-modhook.el.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: patch --]
[-- Type: text/x-diff, Size: 7445 bytes --]

From a8098080dff5f83f7cbcbec2bc263f9db3b45ad9 Mon Sep 17 00:00:00 2001
From: Noam Postavsky <npostavs@gmail.com>
Date: Wed, 20 Jul 2016 20:15:14 -0400
Subject: [PATCH v1] Adjust match data before calling after-change-funs

* src/insdel.c (replace_range): Add new parameter ADJUST_MATCH_DATA, if
true.  Update all callers except Freplace_match to pass 0 for the new
parameter.
* src/search.c (update_search_regs): New function, extracted from
Freplace_match.
(Freplace_match): Remove match data adjustment code, pass 1 for
ADJUST_MATCH_DATA to replace_range instead.
---
 src/cmds.c    |  2 +-
 src/editfns.c |  6 +++---
 src/insdel.c  | 10 ++++++++--
 src/lisp.h    |  4 +++-
 src/search.c  | 50 +++++++++++++++++++++++++++++---------------------
 5 files changed, 44 insertions(+), 28 deletions(-)

diff --git a/src/cmds.c b/src/cmds.c
index 1e44ddd..4003d8b 100644
--- a/src/cmds.c
+++ b/src/cmds.c
@@ -447,7 +447,7 @@ internal_self_insert (int c, EMACS_INT n)
 	  string = concat2 (string, tem);
 	}
 
-      replace_range (PT, PT + chars_to_delete, string, 1, 1, 1);
+      replace_range (PT, PT + chars_to_delete, string, 1, 1, 1, 0);
       Fforward_char (make_number (n));
     }
   else if (n > 1)
diff --git a/src/editfns.c b/src/editfns.c
index 412745d..32c8bec 100644
--- a/src/editfns.c
+++ b/src/editfns.c
@@ -3192,7 +3192,7 @@ DEFUN ("subst-char-in-region", Fsubst_char_in_region,
 	      /* replace_range is less efficient, because it moves the gap,
 		 but it handles combining correctly.  */
 	      replace_range (pos, pos + 1, string,
-			     0, 0, 1);
+			     0, 0, 1, 0);
 	      pos_byte_next = CHAR_TO_BYTE (pos);
 	      if (pos_byte_next > pos_byte)
 		/* Before combining happened.  We should not increment
@@ -3405,7 +3405,7 @@ DEFUN ("translate-region-internal", Ftranslate_region_internal,
 		  /* This is less efficient, because it moves the gap,
 		     but it should handle multibyte characters correctly.  */
 		  string = make_multibyte_string ((char *) str, 1, str_len);
-		  replace_range (pos, pos + 1, string, 1, 0, 1);
+		  replace_range (pos, pos + 1, string, 1, 0, 1, 0);
 		  len = str_len;
 		}
 	      else
@@ -3446,7 +3446,7 @@ DEFUN ("translate-region-internal", Ftranslate_region_internal,
 		{
 		  string = Fmake_string (make_number (1), val);
 		}
-	      replace_range (pos, pos + len, string, 1, 0, 1);
+	      replace_range (pos, pos + len, string, 1, 0, 1, 0);
 	      pos_byte += SBYTES (string);
 	      pos += SCHARS (string);
 	      cnt += SCHARS (string);
diff --git a/src/insdel.c b/src/insdel.c
index 4ad1074..fc3f19f 100644
--- a/src/insdel.c
+++ b/src/insdel.c
@@ -1268,7 +1268,9 @@ adjust_after_insert (ptrdiff_t from, ptrdiff_t from_byte,
 /* Replace the text from character positions FROM to TO with NEW,
    If PREPARE, call prepare_to_modify_buffer.
    If INHERIT, the newly inserted text should inherit text properties
-   from the surrounding non-deleted text.  */
+   from the surrounding non-deleted text.
+   If ADJUST_MATCH_DATA, then adjust the match data before calling
+   signal_after_change.  */
 
 /* Note that this does not yet handle markers quite right.
    Also it needs to record a single undo-entry that does a replacement
@@ -1279,7 +1281,8 @@ adjust_after_insert (ptrdiff_t from, ptrdiff_t from_byte,
 
 void
 replace_range (ptrdiff_t from, ptrdiff_t to, Lisp_Object new,
-	       bool prepare, bool inherit, bool markers)
+               bool prepare, bool inherit, bool markers,
+               bool adjust_match_data)
 {
   ptrdiff_t inschars = SCHARS (new);
   ptrdiff_t insbytes = SBYTES (new);
@@ -1426,6 +1429,9 @@ replace_range (ptrdiff_t from, ptrdiff_t to, Lisp_Object new,
   MODIFF++;
   CHARS_MODIFF = MODIFF;
 
+  if (adjust_match_data)
+    update_search_regs (from, to, from + SCHARS (new));
+
   signal_after_change (from, nchars_del, GPT - from);
   update_compositions (from, GPT, CHECK_BORDER);
 }
diff --git a/src/lisp.h b/src/lisp.h
index 6a98adb..25f811e 100644
--- a/src/lisp.h
+++ b/src/lisp.h
@@ -3516,7 +3516,7 @@ extern void adjust_after_insert (ptrdiff_t, ptrdiff_t, ptrdiff_t,
 				 ptrdiff_t, ptrdiff_t);
 extern void adjust_markers_for_delete (ptrdiff_t, ptrdiff_t,
 				       ptrdiff_t, ptrdiff_t);
-extern void replace_range (ptrdiff_t, ptrdiff_t, Lisp_Object, bool, bool, bool);
+extern void replace_range (ptrdiff_t, ptrdiff_t, Lisp_Object, bool, bool, bool, bool);
 extern void replace_range_2 (ptrdiff_t, ptrdiff_t, ptrdiff_t, ptrdiff_t,
 			     const char *, ptrdiff_t, ptrdiff_t, bool);
 extern void syms_of_insdel (void);
@@ -3994,6 +3994,8 @@ extern Lisp_Object make_temp_name (Lisp_Object, bool);
 /* Defined in search.c.  */
 extern void shrink_regexp_cache (void);
 extern void restore_search_regs (void);
+extern void update_search_regs (ptrdiff_t oldstart,
+                                ptrdiff_t oldend, ptrdiff_t newend);
 extern void record_unwind_save_match_data (void);
 struct re_registers;
 extern struct re_pattern_buffer *compile_pattern (Lisp_Object,
diff --git a/src/search.c b/src/search.c
index 5c949ad..1b82c94 100644
--- a/src/search.c
+++ b/src/search.c
@@ -2727,8 +2727,15 @@ DEFUN ("replace-match", Freplace_match, Sreplace_match, 1, 5, 0,
 
   /* Replace the old text with the new in the cleanest possible way.  */
   replace_range (search_regs.start[sub], search_regs.end[sub],
-		 newtext, 1, 0, 1);
+                 newtext, 1, 0, 1, 1);
   newpoint = search_regs.start[sub] + SCHARS (newtext);
+  /* Update saved data to match adjustment made by replace_range.  */
+  {
+    ptrdiff_t change = newpoint - sub_end;
+    if (sub_start >= sub_end)
+      sub_start += change;
+    sub_end += change;
+  }
 
   if (case_action == all_caps)
     Fupcase_region (make_number (search_regs.start[sub]),
@@ -2742,26 +2749,6 @@ DEFUN ("replace-match", Freplace_match, Sreplace_match, 1, 5, 0,
       || search_regs.num_regs != num_regs)
     error ("Match data clobbered by buffer modification hooks");
 
-  /* Adjust search data for this change.  */
-  {
-    ptrdiff_t oldend = search_regs.end[sub];
-    ptrdiff_t oldstart = search_regs.start[sub];
-    ptrdiff_t change = newpoint - search_regs.end[sub];
-    ptrdiff_t i;
-
-    for (i = 0; i < search_regs.num_regs; i++)
-      {
-	if (search_regs.start[i] >= oldend)
-	  search_regs.start[i] += change;
-	else if (search_regs.start[i] > oldstart)
-	  search_regs.start[i] = oldstart;
-	if (search_regs.end[i] >= oldend)
-	  search_regs.end[i] += change;
-	else if (search_regs.end[i] > oldstart)
-	  search_regs.end[i] = oldstart;
-      }
-  }
-
   /* Put point back where it was in the text.  */
   if (opoint <= 0)
     TEMP_SET_PT (opoint + ZV);
@@ -3102,6 +3089,27 @@ restore_search_regs (void)
     }
 }
 
+/* Called from replace-match via replace_range.  */
+void
+update_search_regs (ptrdiff_t oldstart, ptrdiff_t oldend, ptrdiff_t newend)
+{
+  /* Adjust search data for this change.  */
+  ptrdiff_t change = newend - oldend;
+  ptrdiff_t i;
+
+  for (i = 0; i < search_regs.num_regs; i++)
+    {
+      if (search_regs.start[i] >= oldend)
+        search_regs.start[i] += change;
+      else if (search_regs.start[i] > oldstart)
+        search_regs.start[i] = oldstart;
+      if (search_regs.end[i] >= oldend)
+        search_regs.end[i] += change;
+      else if (search_regs.end[i] > oldstart)
+        search_regs.end[i] = oldstart;
+    }
+}
+
 static void
 unwind_set_match_data (Lisp_Object list)
 {
-- 
2.8.0


[-- Attachment #3: test elisp --]
[-- Type: application/emacs-lisp, Size: 963 bytes --]

bug#23917: Please consider making Bug #23917 a blocker for 25.1 (was Re: org-capture: Capture template ‘g’: Match data clobbered by buffer modification hooks)

[Stefan Monnier]

> Solution: adjust in between the before and after change functions.
> Patch below.  I think there shouldn't be performance problems, although
> it does add yet another flag to replace_range (by the way, I noticed
> that the MARKERS flags is never 0, so it's redundant).  I tested with
> the attached bug-23917-match-data-buffer-modhook.el.

Looks pretty good, indeed.  More specifically, looks like the better fix.
2 comments:
- I'd take advantage of this change to replace the 0/1 booleans with
  false/true [but that's just my preference of bikeshed color).
- maybe we can even adjust_match_data in every call to replace_range
  rather than just in the one from Freplace_match.
Thanks,


        Stefan


> From a8098080dff5f83f7cbcbec2bc263f9db3b45ad9 Mon Sep 17 00:00:00 2001
> From: Noam Postavsky <npostavs@gmail.com>
> Date: Wed, 20 Jul 2016 20:15:14 -0400
> Subject: [PATCH v1] Adjust match data before calling after-change-funs

> * src/insdel.c (replace_range): Add new parameter ADJUST_MATCH_DATA, if
> true.  Update all callers except Freplace_match to pass 0 for the new
> parameter.
> * src/search.c (update_search_regs): New function, extracted from
> Freplace_match.
> (Freplace_match): Remove match data adjustment code, pass 1 for
> ADJUST_MATCH_DATA to replace_range instead.
> ---
>  src/cmds.c    |  2 +-
>  src/editfns.c |  6 +++---
>  src/insdel.c  | 10 ++++++++--
>  src/lisp.h    |  4 +++-
>  src/search.c  | 50 +++++++++++++++++++++++++++++---------------------
>  5 files changed, 44 insertions(+), 28 deletions(-)

> diff --git a/src/cmds.c b/src/cmds.c
> index 1e44ddd..4003d8b 100644
> --- a/src/cmds.c
> +++ b/src/cmds.c
> @@ -447,7 +447,7 @@ internal_self_insert (int c, EMACS_INT n)
>  	  string = concat2 (string, tem);
>  	}
 
> -      replace_range (PT, PT + chars_to_delete, string, 1, 1, 1);
> +      replace_range (PT, PT + chars_to_delete, string, 1, 1, 1, 0);
>        Fforward_char (make_number (n));
>      }
>    else if (n > 1)
> diff --git a/src/editfns.c b/src/editfns.c
> index 412745d..32c8bec 100644
> --- a/src/editfns.c
> +++ b/src/editfns.c
> @@ -3192,7 +3192,7 @@ DEFUN ("subst-char-in-region", Fsubst_char_in_region,
>  	      /* replace_range is less efficient, because it moves the gap,
>  		 but it handles combining correctly.  */
>  	      replace_range (pos, pos + 1, string,
> -			     0, 0, 1);
> +			     0, 0, 1, 0);
>  	      pos_byte_next = CHAR_TO_BYTE (pos);
>  	      if (pos_byte_next > pos_byte)
>  		/* Before combining happened.  We should not increment
> @@ -3405,7 +3405,7 @@ DEFUN ("translate-region-internal", Ftranslate_region_internal,
>  		  /* This is less efficient, because it moves the gap,
>  		     but it should handle multibyte characters correctly.  */
>  		  string = make_multibyte_string ((char *) str, 1, str_len);
> -		  replace_range (pos, pos + 1, string, 1, 0, 1);
> +		  replace_range (pos, pos + 1, string, 1, 0, 1, 0);
>  		  len = str_len;
>  		}
>  	      else
> @@ -3446,7 +3446,7 @@ DEFUN ("translate-region-internal", Ftranslate_region_internal,
>  		{
>  		  string = Fmake_string (make_number (1), val);
>  		}
> -	      replace_range (pos, pos + len, string, 1, 0, 1);
> +	      replace_range (pos, pos + len, string, 1, 0, 1, 0);
>  	      pos_byte += SBYTES (string);
>  	      pos += SCHARS (string);
>  	      cnt += SCHARS (string);
> diff --git a/src/insdel.c b/src/insdel.c
> index 4ad1074..fc3f19f 100644
> --- a/src/insdel.c
> +++ b/src/insdel.c
> @@ -1268,7 +1268,9 @@ adjust_after_insert (ptrdiff_t from, ptrdiff_t from_byte,
>  /* Replace the text from character positions FROM to TO with NEW,
>     If PREPARE, call prepare_to_modify_buffer.
>     If INHERIT, the newly inserted text should inherit text properties
> -   from the surrounding non-deleted text.  */
> +   from the surrounding non-deleted text.
> +   If ADJUST_MATCH_DATA, then adjust the match data before calling
> +   signal_after_change.  */
 
>  /* Note that this does not yet handle markers quite right.
>     Also it needs to record a single undo-entry that does a replacement
> @@ -1279,7 +1281,8 @@ adjust_after_insert (ptrdiff_t from, ptrdiff_t from_byte,
 
>  void
>  replace_range (ptrdiff_t from, ptrdiff_t to, Lisp_Object new,
> -	       bool prepare, bool inherit, bool markers)
> +               bool prepare, bool inherit, bool markers,
> +               bool adjust_match_data)
>  {
>    ptrdiff_t inschars = SCHARS (new);
>    ptrdiff_t insbytes = SBYTES (new);
> @@ -1426,6 +1429,9 @@ replace_range (ptrdiff_t from, ptrdiff_t to, Lisp_Object new,
>    MODIFF++;
>    CHARS_MODIFF = MODIFF;
 
> +  if (adjust_match_data)
> +    update_search_regs (from, to, from + SCHARS (new));
> +
>    signal_after_change (from, nchars_del, GPT - from);
>    update_compositions (from, GPT, CHECK_BORDER);
>  }
> diff --git a/src/lisp.h b/src/lisp.h
> index 6a98adb..25f811e 100644
> --- a/src/lisp.h
> +++ b/src/lisp.h
> @@ -3516,7 +3516,7 @@ extern void adjust_after_insert (ptrdiff_t, ptrdiff_t, ptrdiff_t,
>  				 ptrdiff_t, ptrdiff_t);
>  extern void adjust_markers_for_delete (ptrdiff_t, ptrdiff_t,
>  				       ptrdiff_t, ptrdiff_t);
> -extern void replace_range (ptrdiff_t, ptrdiff_t, Lisp_Object, bool, bool, bool);
> +extern void replace_range (ptrdiff_t, ptrdiff_t, Lisp_Object, bool, bool, bool, bool);
>  extern void replace_range_2 (ptrdiff_t, ptrdiff_t, ptrdiff_t, ptrdiff_t,
>  			     const char *, ptrdiff_t, ptrdiff_t, bool);
>  extern void syms_of_insdel (void);
> @@ -3994,6 +3994,8 @@ extern Lisp_Object make_temp_name (Lisp_Object, bool);
>  /* Defined in search.c.  */
>  extern void shrink_regexp_cache (void);
>  extern void restore_search_regs (void);
> +extern void update_search_regs (ptrdiff_t oldstart,
> +                                ptrdiff_t oldend, ptrdiff_t newend);
>  extern void record_unwind_save_match_data (void);
>  struct re_registers;
>  extern struct re_pattern_buffer *compile_pattern (Lisp_Object,
> diff --git a/src/search.c b/src/search.c
> index 5c949ad..1b82c94 100644
> --- a/src/search.c
> +++ b/src/search.c
> @@ -2727,8 +2727,15 @@ DEFUN ("replace-match", Freplace_match, Sreplace_match, 1, 5, 0,
 
>    /* Replace the old text with the new in the cleanest possible way.  */
>    replace_range (search_regs.start[sub], search_regs.end[sub],
> -		 newtext, 1, 0, 1);
> +                 newtext, 1, 0, 1, 1);
>    newpoint = search_regs.start[sub] + SCHARS (newtext);
> +  /* Update saved data to match adjustment made by replace_range.  */
> +  {
> +    ptrdiff_t change = newpoint - sub_end;
> +    if (sub_start >= sub_end)
> +      sub_start += change;
> +    sub_end += change;
> +  }
 
>    if (case_action == all_caps)
>      Fupcase_region (make_number (search_regs.start[sub]),
> @@ -2742,26 +2749,6 @@ DEFUN ("replace-match", Freplace_match, Sreplace_match, 1, 5, 0,
>        || search_regs.num_regs != num_regs)
>      error ("Match data clobbered by buffer modification hooks");
 
> -  /* Adjust search data for this change.  */
> -  {
> -    ptrdiff_t oldend = search_regs.end[sub];
> -    ptrdiff_t oldstart = search_regs.start[sub];
> -    ptrdiff_t change = newpoint - search_regs.end[sub];
> -    ptrdiff_t i;
> -
> -    for (i = 0; i < search_regs.num_regs; i++)
> -      {
> -	if (search_regs.start[i] >= oldend)
> -	  search_regs.start[i] += change;
> -	else if (search_regs.start[i] > oldstart)
> -	  search_regs.start[i] = oldstart;
> -	if (search_regs.end[i] >= oldend)
> -	  search_regs.end[i] += change;
> -	else if (search_regs.end[i] > oldstart)
> -	  search_regs.end[i] = oldstart;
> -      }
> -  }
> -
>    /* Put point back where it was in the text.  */
>    if (opoint <= 0)
>      TEMP_SET_PT (opoint + ZV);
> @@ -3102,6 +3089,27 @@ restore_search_regs (void)
>      }
>  }
 
> +/* Called from replace-match via replace_range.  */
> +void
> +update_search_regs (ptrdiff_t oldstart, ptrdiff_t oldend, ptrdiff_t newend)
> +{
> +  /* Adjust search data for this change.  */
> +  ptrdiff_t change = newend - oldend;
> +  ptrdiff_t i;
> +
> +  for (i = 0; i < search_regs.num_regs; i++)
> +    {
> +      if (search_regs.start[i] >= oldend)
> +        search_regs.start[i] += change;
> +      else if (search_regs.start[i] > oldstart)
> +        search_regs.start[i] = oldstart;
> +      if (search_regs.end[i] >= oldend)
> +        search_regs.end[i] += change;
> +      else if (search_regs.end[i] > oldstart)
> +        search_regs.end[i] = oldstart;
> +    }
> +}
> +
>  static void
>  unwind_set_match_data (Lisp_Object list)
>  {
> -- 
> 2.8.0

bug#23917: Please consider making Bug #23917 a blocker for 25.1 (was Re: org-capture: Capture template ‘g’: Match data clobbered by buffer modification hooks)

[Noam Postavsky]

On Wed, Jul 20, 2016 at 9:47 PM, Stefan Monnier
<monnier@iro.umontreal.ca> wrote:

> - maybe we can even adjust_match_data in every call to replace_range
>   rather than just in the one from Freplace_match.

That would be simpler, though I wasn't sure if it would be correct.

bug#23917: Please consider making Bug #23917 a blocker for 25.1 (was Re: org-capture: Capture template ‘g’: Match data clobbered by buffer modification hooks)

[Stefan Monnier]

>> - maybe we can even adjust_match_data in every call to replace_range
>> rather than just in the one from Freplace_match.
> That would be simpler, though I wasn't sure if it would be correct.

I think it's definitely not an option for emacs-25.  But maybe we can
try it on master.


        Stefan

bug#23917: Please consider making Bug #23917 a blocker for 25.1 (was Re: org-capture: Capture template ‘g’: Match data clobbered by buffer modification hooks)

[Eli Zaretskii]

> From: npostavs@users.sourceforge.net
> Cc: Eli Zaretskii <eliz@gnu.org>,  23917@debbugs.gnu.org,  nljlistbox2@gmail.com,  jwiegley@gmail.com,  rpluim@gmail.com,  alex.bennee@linaro.org
> Date: Wed, 20 Jul 2016 20:56:28 -0400
> 
> >> Maybe there's a misunderstanding.  What Noam suggested was just to
> >> move the code which adjusts search_regs.start[i] and .end[i] to before
> >> the call to replace_range.
> >
> > Oh, right, that's also an option.  It might suffer from another problem,
> > which is that the match-data will be broken while the
> > before-change-functions are run, so if there's a save-match-data there
> > we're back to square one.
> 
> Solution: adjust in between the before and after change functions.
> Patch below.  I think there shouldn't be performance problems, although
> it does add yet another flag to replace_range (by the way, I noticed
> that the MARKERS flags is never 0, so it's redundant).  I tested with
> the attached bug-23917-match-data-buffer-modhook.el.

Thanks.

Please also make sure bug#23869 is still fixed after this.

bug#23917: Please consider making Bug #23917 a blocker for 25.1 (was Re: org-capture: Capture template ‘g’: Match data clobbered by buffer modification hooks)

[npostavs]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: npostavs@users.sourceforge.net
>> Cc: Eli Zaretskii <eliz@gnu.org>,  23917@debbugs.gnu.org,  nljlistbox2@gmail.com,  jwiegley@gmail.com,  rpluim@gmail.com,  alex.bennee@linaro.org
>> Date: Wed, 20 Jul 2016 20:56:28 -0400
>> 
>> >> Maybe there's a misunderstanding.  What Noam suggested was just to
>> >> move the code which adjusts search_regs.start[i] and .end[i] to before
>> >> the call to replace_range.
>> >
>> > Oh, right, that's also an option.  It might suffer from another problem,
>> > which is that the match-data will be broken while the
>> > before-change-functions are run, so if there's a save-match-data there
>> > we're back to square one.
>> 
>> Solution: adjust in between the before and after change functions.
>> Patch below.  I think there shouldn't be performance problems, although
>> it does add yet another flag to replace_range (by the way, I noticed
>> that the MARKERS flags is never 0, so it's redundant).  I tested with
>> the attached bug-23917-match-data-buffer-modhook.el.
>
> Thanks.
>
> Please also make sure bug#23869 is still fixed after this.

Following the recipe in
http://debbugs.gnu.org/cgi/bugreport.cgi?bug=23869#11 gives me 'Lisp
error: (error "Match data clobbered by buffer modification hooks")',
that indicates it's still fixed, right?

bug#23917: Please consider making Bug #23917 a blocker for 25.1 (was Re: org-capture: Capture template ‘g’: Match data clobbered by buffer modification hooks)

[Eli Zaretskii]

> From: npostavs@users.sourceforge.net
> Cc: 23917@debbugs.gnu.org,  nljlistbox2@gmail.com,  jwiegley@gmail.com,  rpluim@gmail.com,  monnier@iro.umontreal.ca,  alex.bennee@linaro.org
> Date: Wed, 20 Jul 2016 23:00:59 -0400
> 
> > Please also make sure bug#23869 is still fixed after this.
> 
> Following the recipe in
> http://debbugs.gnu.org/cgi/bugreport.cgi?bug=23869#11 gives me 'Lisp
> error: (error "Match data clobbered by buffer modification hooks")',
> that indicates it's still fixed, right?

Yes, but I thought we want to remove the error-out code.  Since we now
protect ourselves from clobbered data, we don't need that extra
protection, and I think leaving it in place will cause false positives
(as a few people already reported).  That's because the adjustment of
the search registers in the new function you introduce will itself
trigger the error message, won't it?

Perhaps we should error out only if the number of registers changed,
because that can never be valid, I think.

bug#23917: Please consider making Bug #23917 a blocker for 25.1 (was Re: org-capture: Capture template ‘g’: Match data clobbered by buffer modification hooks)

[npostavs]

[-- Attachment #1: Type: text/plain, Size: 1456 bytes --]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: npostavs@users.sourceforge.net
>> Cc: 23917@debbugs.gnu.org,  nljlistbox2@gmail.com,  jwiegley@gmail.com,  rpluim@gmail.com,  monnier@iro.umontreal.ca,  alex.bennee@linaro.org
>> Date: Wed, 20 Jul 2016 23:00:59 -0400
>> 
>> > Please also make sure bug#23869 is still fixed after this.
>> 
>> Following the recipe in
>> http://debbugs.gnu.org/cgi/bugreport.cgi?bug=23869#11 gives me 'Lisp
>> error: (error "Match data clobbered by buffer modification hooks")',
>> that indicates it's still fixed, right?
>
> Yes, but I thought we want to remove the error-out code.  Since we now
> protect ourselves from clobbered data, we don't need that extra
> protection, and I think leaving it in place will cause false positives
> (as a few people already reported).  That's because the adjustment of
> the search registers in the new function you introduce will itself
> trigger the error message, won't it?

I made the same adjustments to the saved sub_start and sub_end
variables, but I had a mistake in that adjustment which caused the false
positives.  Fixed in the attached v2 patch.  We could just drop the
check, though I've already found it useful to catch bugs
(https://github.com/joaotavora/yasnippet/issues/720).

If I drop the checks (see attached v3 patch), then after following the
bug#23869 recipe, I get:

    ## -*- Octave -*-
    -module(bug).
    -export([identity/1, is_even/1, size/1, reverse/1]).


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: patch v2 --]
[-- Type: text/x-diff, Size: 7671 bytes --]

From 3e8f9b9f89108bcebf04e06e41a5ce2c719c6ad2 Mon Sep 17 00:00:00 2001
From: Noam Postavsky <npostavs@gmail.com>
Date: Wed, 20 Jul 2016 20:15:14 -0400
Subject: [PATCH v2] Adjust match data before calling after-change-funs

* src/insdel.c (replace_range): Add new parameter ADJUST_MATCH_DATA, if
true call update_search_regs.  Update all callers (except
Freplace_match) to pass 0 for the new parameter.
* src/search.c (update_search_regs): New function, extracted from
Freplace_match.
(Freplace_match): Remove match data adjustment code, pass 1 for
ADJUST_MATCH_DATA to replace_range instead.
---
 src/cmds.c    |  2 +-
 src/editfns.c |  6 +++---
 src/insdel.c  | 10 ++++++++--
 src/lisp.h    |  4 +++-
 src/search.c  | 52 ++++++++++++++++++++++++++++++----------------------
 5 files changed, 45 insertions(+), 29 deletions(-)

diff --git a/src/cmds.c b/src/cmds.c
index 1e44ddd..4003d8b 100644
--- a/src/cmds.c
+++ b/src/cmds.c
@@ -447,7 +447,7 @@ internal_self_insert (int c, EMACS_INT n)
 	  string = concat2 (string, tem);
 	}
 
-      replace_range (PT, PT + chars_to_delete, string, 1, 1, 1);
+      replace_range (PT, PT + chars_to_delete, string, 1, 1, 1, 0);
       Fforward_char (make_number (n));
     }
   else if (n > 1)
diff --git a/src/editfns.c b/src/editfns.c
index 412745d..32c8bec 100644
--- a/src/editfns.c
+++ b/src/editfns.c
@@ -3192,7 +3192,7 @@ DEFUN ("subst-char-in-region", Fsubst_char_in_region,
 	      /* replace_range is less efficient, because it moves the gap,
 		 but it handles combining correctly.  */
 	      replace_range (pos, pos + 1, string,
-			     0, 0, 1);
+			     0, 0, 1, 0);
 	      pos_byte_next = CHAR_TO_BYTE (pos);
 	      if (pos_byte_next > pos_byte)
 		/* Before combining happened.  We should not increment
@@ -3405,7 +3405,7 @@ DEFUN ("translate-region-internal", Ftranslate_region_internal,
 		  /* This is less efficient, because it moves the gap,
 		     but it should handle multibyte characters correctly.  */
 		  string = make_multibyte_string ((char *) str, 1, str_len);
-		  replace_range (pos, pos + 1, string, 1, 0, 1);
+		  replace_range (pos, pos + 1, string, 1, 0, 1, 0);
 		  len = str_len;
 		}
 	      else
@@ -3446,7 +3446,7 @@ DEFUN ("translate-region-internal", Ftranslate_region_internal,
 		{
 		  string = Fmake_string (make_number (1), val);
 		}
-	      replace_range (pos, pos + len, string, 1, 0, 1);
+	      replace_range (pos, pos + len, string, 1, 0, 1, 0);
 	      pos_byte += SBYTES (string);
 	      pos += SCHARS (string);
 	      cnt += SCHARS (string);
diff --git a/src/insdel.c b/src/insdel.c
index 4ad1074..fc3f19f 100644
--- a/src/insdel.c
+++ b/src/insdel.c
@@ -1268,7 +1268,9 @@ adjust_after_insert (ptrdiff_t from, ptrdiff_t from_byte,
 /* Replace the text from character positions FROM to TO with NEW,
    If PREPARE, call prepare_to_modify_buffer.
    If INHERIT, the newly inserted text should inherit text properties
-   from the surrounding non-deleted text.  */
+   from the surrounding non-deleted text.
+   If ADJUST_MATCH_DATA, then adjust the match data before calling
+   signal_after_change.  */
 
 /* Note that this does not yet handle markers quite right.
    Also it needs to record a single undo-entry that does a replacement
@@ -1279,7 +1281,8 @@ adjust_after_insert (ptrdiff_t from, ptrdiff_t from_byte,
 
 void
 replace_range (ptrdiff_t from, ptrdiff_t to, Lisp_Object new,
-	       bool prepare, bool inherit, bool markers)
+               bool prepare, bool inherit, bool markers,
+               bool adjust_match_data)
 {
   ptrdiff_t inschars = SCHARS (new);
   ptrdiff_t insbytes = SBYTES (new);
@@ -1426,6 +1429,9 @@ replace_range (ptrdiff_t from, ptrdiff_t to, Lisp_Object new,
   MODIFF++;
   CHARS_MODIFF = MODIFF;
 
+  if (adjust_match_data)
+    update_search_regs (from, to, from + SCHARS (new));
+
   signal_after_change (from, nchars_del, GPT - from);
   update_compositions (from, GPT, CHECK_BORDER);
 }
diff --git a/src/lisp.h b/src/lisp.h
index 6a98adb..25f811e 100644
--- a/src/lisp.h
+++ b/src/lisp.h
@@ -3516,7 +3516,7 @@ extern void adjust_after_insert (ptrdiff_t, ptrdiff_t, ptrdiff_t,
 				 ptrdiff_t, ptrdiff_t);
 extern void adjust_markers_for_delete (ptrdiff_t, ptrdiff_t,
 				       ptrdiff_t, ptrdiff_t);
-extern void replace_range (ptrdiff_t, ptrdiff_t, Lisp_Object, bool, bool, bool);
+extern void replace_range (ptrdiff_t, ptrdiff_t, Lisp_Object, bool, bool, bool, bool);
 extern void replace_range_2 (ptrdiff_t, ptrdiff_t, ptrdiff_t, ptrdiff_t,
 			     const char *, ptrdiff_t, ptrdiff_t, bool);
 extern void syms_of_insdel (void);
@@ -3994,6 +3994,8 @@ extern Lisp_Object make_temp_name (Lisp_Object, bool);
 /* Defined in search.c.  */
 extern void shrink_regexp_cache (void);
 extern void restore_search_regs (void);
+extern void update_search_regs (ptrdiff_t oldstart,
+                                ptrdiff_t oldend, ptrdiff_t newend);
 extern void record_unwind_save_match_data (void);
 struct re_registers;
 extern struct re_pattern_buffer *compile_pattern (Lisp_Object,
diff --git a/src/search.c b/src/search.c
index 5c949ad..4bd6a46 100644
--- a/src/search.c
+++ b/src/search.c
@@ -2724,11 +2724,18 @@ DEFUN ("replace-match", Freplace_match, Sreplace_match, 1, 5, 0,
   ptrdiff_t sub_start = search_regs.start[sub];
   ptrdiff_t sub_end = search_regs.end[sub];
   unsigned  num_regs = search_regs.num_regs;
+  newpoint = search_regs.start[sub] + SCHARS (newtext);
 
   /* Replace the old text with the new in the cleanest possible way.  */
   replace_range (search_regs.start[sub], search_regs.end[sub],
-		 newtext, 1, 0, 1);
-  newpoint = search_regs.start[sub] + SCHARS (newtext);
+                 newtext, 1, 0, 1, 1);
+  /* Update saved data to match adjustment made by replace_range.  */
+  {
+    ptrdiff_t change = newpoint - sub_end;
+    if (sub_start >= sub_end)
+      sub_start += change;
+    sub_end += change;
+  }
 
   if (case_action == all_caps)
     Fupcase_region (make_number (search_regs.start[sub]),
@@ -2742,26 +2749,6 @@ DEFUN ("replace-match", Freplace_match, Sreplace_match, 1, 5, 0,
       || search_regs.num_regs != num_regs)
     error ("Match data clobbered by buffer modification hooks");
 
-  /* Adjust search data for this change.  */
-  {
-    ptrdiff_t oldend = search_regs.end[sub];
-    ptrdiff_t oldstart = search_regs.start[sub];
-    ptrdiff_t change = newpoint - search_regs.end[sub];
-    ptrdiff_t i;
-
-    for (i = 0; i < search_regs.num_regs; i++)
-      {
-	if (search_regs.start[i] >= oldend)
-	  search_regs.start[i] += change;
-	else if (search_regs.start[i] > oldstart)
-	  search_regs.start[i] = oldstart;
-	if (search_regs.end[i] >= oldend)
-	  search_regs.end[i] += change;
-	else if (search_regs.end[i] > oldstart)
-	  search_regs.end[i] = oldstart;
-      }
-  }
-
   /* Put point back where it was in the text.  */
   if (opoint <= 0)
     TEMP_SET_PT (opoint + ZV);
@@ -3102,6 +3089,27 @@ restore_search_regs (void)
     }
 }
 
+/* Called from replace-match via replace_range.  */
+void
+update_search_regs (ptrdiff_t oldstart, ptrdiff_t oldend, ptrdiff_t newend)
+{
+  /* Adjust search data for this change.  */
+  ptrdiff_t change = newend - oldend;
+  ptrdiff_t i;
+
+  for (i = 0; i < search_regs.num_regs; i++)
+    {
+      if (search_regs.start[i] >= oldend)
+        search_regs.start[i] += change;
+      else if (search_regs.start[i] > oldstart)
+        search_regs.start[i] = oldstart;
+      if (search_regs.end[i] >= oldend)
+        search_regs.end[i] += change;
+      else if (search_regs.end[i] > oldstart)
+        search_regs.end[i] = oldstart;
+    }
+}
+
 static void
 unwind_set_match_data (Lisp_Object list)
 {
-- 
2.8.0


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: patch v3 --]
[-- Type: text/x-diff, Size: 8010 bytes --]

From bb533ffd333a28d2ea8bdb6c81b79eb4eda89fcc Mon Sep 17 00:00:00 2001
From: Noam Postavsky <npostavs@gmail.com>
Date: Wed, 20 Jul 2016 20:15:14 -0400
Subject: [PATCH v3] Adjust match data before calling after-change-funs

* src/insdel.c (replace_range): Add new parameter ADJUST_MATCH_DATA, if
true call update_search_regs.  Update all callers (except
Freplace_match) to pass 0 for the new parameter.
* src/search.c (update_search_regs): New function, extracted from
Freplace_match.
(Freplace_match): Remove match data adjustment code, pass 1 for
ADJUST_MATCH_DATA to replace_range instead.  Remove checks for match
data modification.
---
 src/cmds.c    |  2 +-
 src/editfns.c |  6 +++---
 src/insdel.c  | 10 ++++++++--
 src/lisp.h    |  4 +++-
 src/search.c  | 57 +++++++++++++++++++++++----------------------------------
 5 files changed, 38 insertions(+), 41 deletions(-)

diff --git a/src/cmds.c b/src/cmds.c
index 1e44ddd..4003d8b 100644
--- a/src/cmds.c
+++ b/src/cmds.c
@@ -447,7 +447,7 @@ internal_self_insert (int c, EMACS_INT n)
 	  string = concat2 (string, tem);
 	}
 
-      replace_range (PT, PT + chars_to_delete, string, 1, 1, 1);
+      replace_range (PT, PT + chars_to_delete, string, 1, 1, 1, 0);
       Fforward_char (make_number (n));
     }
   else if (n > 1)
diff --git a/src/editfns.c b/src/editfns.c
index 412745d..32c8bec 100644
--- a/src/editfns.c
+++ b/src/editfns.c
@@ -3192,7 +3192,7 @@ DEFUN ("subst-char-in-region", Fsubst_char_in_region,
 	      /* replace_range is less efficient, because it moves the gap,
 		 but it handles combining correctly.  */
 	      replace_range (pos, pos + 1, string,
-			     0, 0, 1);
+			     0, 0, 1, 0);
 	      pos_byte_next = CHAR_TO_BYTE (pos);
 	      if (pos_byte_next > pos_byte)
 		/* Before combining happened.  We should not increment
@@ -3405,7 +3405,7 @@ DEFUN ("translate-region-internal", Ftranslate_region_internal,
 		  /* This is less efficient, because it moves the gap,
 		     but it should handle multibyte characters correctly.  */
 		  string = make_multibyte_string ((char *) str, 1, str_len);
-		  replace_range (pos, pos + 1, string, 1, 0, 1);
+		  replace_range (pos, pos + 1, string, 1, 0, 1, 0);
 		  len = str_len;
 		}
 	      else
@@ -3446,7 +3446,7 @@ DEFUN ("translate-region-internal", Ftranslate_region_internal,
 		{
 		  string = Fmake_string (make_number (1), val);
 		}
-	      replace_range (pos, pos + len, string, 1, 0, 1);
+	      replace_range (pos, pos + len, string, 1, 0, 1, 0);
 	      pos_byte += SBYTES (string);
 	      pos += SCHARS (string);
 	      cnt += SCHARS (string);
diff --git a/src/insdel.c b/src/insdel.c
index 4ad1074..fc3f19f 100644
--- a/src/insdel.c
+++ b/src/insdel.c
@@ -1268,7 +1268,9 @@ adjust_after_insert (ptrdiff_t from, ptrdiff_t from_byte,
 /* Replace the text from character positions FROM to TO with NEW,
    If PREPARE, call prepare_to_modify_buffer.
    If INHERIT, the newly inserted text should inherit text properties
-   from the surrounding non-deleted text.  */
+   from the surrounding non-deleted text.
+   If ADJUST_MATCH_DATA, then adjust the match data before calling
+   signal_after_change.  */
 
 /* Note that this does not yet handle markers quite right.
    Also it needs to record a single undo-entry that does a replacement
@@ -1279,7 +1281,8 @@ adjust_after_insert (ptrdiff_t from, ptrdiff_t from_byte,
 
 void
 replace_range (ptrdiff_t from, ptrdiff_t to, Lisp_Object new,
-	       bool prepare, bool inherit, bool markers)
+               bool prepare, bool inherit, bool markers,
+               bool adjust_match_data)
 {
   ptrdiff_t inschars = SCHARS (new);
   ptrdiff_t insbytes = SBYTES (new);
@@ -1426,6 +1429,9 @@ replace_range (ptrdiff_t from, ptrdiff_t to, Lisp_Object new,
   MODIFF++;
   CHARS_MODIFF = MODIFF;
 
+  if (adjust_match_data)
+    update_search_regs (from, to, from + SCHARS (new));
+
   signal_after_change (from, nchars_del, GPT - from);
   update_compositions (from, GPT, CHECK_BORDER);
 }
diff --git a/src/lisp.h b/src/lisp.h
index 6a98adb..25f811e 100644
--- a/src/lisp.h
+++ b/src/lisp.h
@@ -3516,7 +3516,7 @@ extern void adjust_after_insert (ptrdiff_t, ptrdiff_t, ptrdiff_t,
 				 ptrdiff_t, ptrdiff_t);
 extern void adjust_markers_for_delete (ptrdiff_t, ptrdiff_t,
 				       ptrdiff_t, ptrdiff_t);
-extern void replace_range (ptrdiff_t, ptrdiff_t, Lisp_Object, bool, bool, bool);
+extern void replace_range (ptrdiff_t, ptrdiff_t, Lisp_Object, bool, bool, bool, bool);
 extern void replace_range_2 (ptrdiff_t, ptrdiff_t, ptrdiff_t, ptrdiff_t,
 			     const char *, ptrdiff_t, ptrdiff_t, bool);
 extern void syms_of_insdel (void);
@@ -3994,6 +3994,8 @@ extern Lisp_Object make_temp_name (Lisp_Object, bool);
 /* Defined in search.c.  */
 extern void shrink_regexp_cache (void);
 extern void restore_search_regs (void);
+extern void update_search_regs (ptrdiff_t oldstart,
+                                ptrdiff_t oldend, ptrdiff_t newend);
 extern void record_unwind_save_match_data (void);
 struct re_registers;
 extern struct re_pattern_buffer *compile_pattern (Lisp_Object,
diff --git a/src/search.c b/src/search.c
index 5c949ad..7da1d86 100644
--- a/src/search.c
+++ b/src/search.c
@@ -2717,18 +2717,11 @@ DEFUN ("replace-match", Freplace_match, Sreplace_match, 1, 5, 0,
       xfree (substed);
     }
 
-  /* The functions below modify the buffer, so they could trigger
-     various modification hooks (see signal_before_change and
-     signal_after_change), which might clobber the match data we need
-     to adjust after the replacement.  If that happens, we error out.  */
-  ptrdiff_t sub_start = search_regs.start[sub];
-  ptrdiff_t sub_end = search_regs.end[sub];
-  unsigned  num_regs = search_regs.num_regs;
+  newpoint = search_regs.start[sub] + SCHARS (newtext);
 
   /* Replace the old text with the new in the cleanest possible way.  */
   replace_range (search_regs.start[sub], search_regs.end[sub],
-		 newtext, 1, 0, 1);
-  newpoint = search_regs.start[sub] + SCHARS (newtext);
+                 newtext, 1, 0, 1, 1);
 
   if (case_action == all_caps)
     Fupcase_region (make_number (search_regs.start[sub]),
@@ -2737,31 +2730,6 @@ DEFUN ("replace-match", Freplace_match, Sreplace_match, 1, 5, 0,
     Fupcase_initials_region (make_number (search_regs.start[sub]),
 			     make_number (newpoint));
 
-  if (search_regs.start[sub] != sub_start
-      || search_regs.end[sub] != sub_end
-      || search_regs.num_regs != num_regs)
-    error ("Match data clobbered by buffer modification hooks");
-
-  /* Adjust search data for this change.  */
-  {
-    ptrdiff_t oldend = search_regs.end[sub];
-    ptrdiff_t oldstart = search_regs.start[sub];
-    ptrdiff_t change = newpoint - search_regs.end[sub];
-    ptrdiff_t i;
-
-    for (i = 0; i < search_regs.num_regs; i++)
-      {
-	if (search_regs.start[i] >= oldend)
-	  search_regs.start[i] += change;
-	else if (search_regs.start[i] > oldstart)
-	  search_regs.start[i] = oldstart;
-	if (search_regs.end[i] >= oldend)
-	  search_regs.end[i] += change;
-	else if (search_regs.end[i] > oldstart)
-	  search_regs.end[i] = oldstart;
-      }
-  }
-
   /* Put point back where it was in the text.  */
   if (opoint <= 0)
     TEMP_SET_PT (opoint + ZV);
@@ -3102,6 +3070,27 @@ restore_search_regs (void)
     }
 }
 
+/* Called from replace-match via replace_range.  */
+void
+update_search_regs (ptrdiff_t oldstart, ptrdiff_t oldend, ptrdiff_t newend)
+{
+  /* Adjust search data for this change.  */
+  ptrdiff_t change = newend - oldend;
+  ptrdiff_t i;
+
+  for (i = 0; i < search_regs.num_regs; i++)
+    {
+      if (search_regs.start[i] >= oldend)
+        search_regs.start[i] += change;
+      else if (search_regs.start[i] > oldstart)
+        search_regs.start[i] = oldstart;
+      if (search_regs.end[i] >= oldend)
+        search_regs.end[i] += change;
+      else if (search_regs.end[i] > oldstart)
+        search_regs.end[i] = oldstart;
+    }
+}
+
 static void
 unwind_set_match_data (Lisp_Object list)
 {
-- 
2.8.0

bug#23917: Please consider making Bug #23917 a blocker for 25.1 (was Re: org-capture: Capture template ‘g’: Match data clobbered by buffer modification hooks)

[Eli Zaretskii]

> From: npostavs@users.sourceforge.net
> Cc: 23917@debbugs.gnu.org,  nljlistbox2@gmail.com,  jwiegley@gmail.com,  rpluim@gmail.com,  monnier@iro.umontreal.ca,  alex.bennee@linaro.org
> Date: Thu, 21 Jul 2016 21:08:43 -0400
> 
> I made the same adjustments to the saved sub_start and sub_end
> variables, but I had a mistake in that adjustment which caused the false
> positives.  Fixed in the attached v2 patch.  We could just drop the
> check, though I've already found it useful to catch bugs
> (https://github.com/joaotavora/yasnippet/issues/720).
> 
> If I drop the checks (see attached v3 patch), then after following the
> bug#23869 recipe, I get:
> 
>     ## -*- Octave -*-
>     -module(bug).
>     -export([identity/1, is_even/1, size/1, reverse/1]).

OK, let's wait for a few days to give time to the people who were
affected by the issue to test the patch, and if no new issues come up,
please push the version with the error code to emacs-25.

Thanks.

bug#23917: Please consider making Bug #23917 a blocker for 25.1 (was Re: org-capture: Capture template ‘g’: Match data clobbered by buffer modification hooks)

[Robert Pluim]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: npostavs@users.sourceforge.net
>> Cc: 23917@debbugs.gnu.org,  nljlistbox2@gmail.com,  jwiegley@gmail.com,  rpluim@gmail.com,  monnier@iro.umontreal.ca,  alex.bennee@linaro.org
>> Date: Thu, 21 Jul 2016 21:08:43 -0400
>> 
>> I made the same adjustments to the saved sub_start and sub_end
>> variables, but I had a mistake in that adjustment which caused the false
>> positives.  Fixed in the attached v2 patch.  We could just drop the
>> check, though I've already found it useful to catch bugs
>> (https://github.com/joaotavora/yasnippet/issues/720).
>> 
>> If I drop the checks (see attached v3 patch), then after following the
>> bug#23869 recipe, I get:
>> 
>>     ## -*- Octave -*-
>>     -module(bug).
>>     -export([identity/1, is_even/1, size/1, reverse/1]).
>
> OK, let's wait for a few days to give time to the people who were
> affected by the issue to test the patch, and if no new issues come up,
> please push the version with the error code to emacs-25.
>

Patch v2 fixes 'emacs -Q' and my normal capture templates, and I'm
using the patched emacs for this email. I'll keep running with it for
the next few days.

Regards

Robert

bug#23917: Please consider making Bug #23917 a blocker for 25.1 (was Re: org-capture: Capture template ‘g’: Match data clobbered by buffer modification hooks)

[Nicolas Petton]

[-- Attachment #1: Type: text/plain, Size: 354 bytes --]

Eli Zaretskii <eliz@gnu.org> writes:

> OK, let's wait for a few days to give time to the people who were
> affected by the issue to test the patch, and if no new issues come up,
> please push the version with the error code to emacs-25.

I'll wait until Monday for the first release candidate to give time to
this patch to be applied to emacs-25.

Nico

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 512 bytes --]

bug#23917: Please consider making Bug #23917 a blocker for 25.1 (was Re: org-capture: Capture template ‘g’: Match data clobbered by buffer modification hooks)

[npostavs]

tags 23917 fixed
close 23917 25.1
quit

Eli Zaretskii <eliz@gnu.org> writes:

>> From: npostavs@users.sourceforge.net
>> Cc: 23917@debbugs.gnu.org,  nljlistbox2@gmail.com,  jwiegley@gmail.com,  rpluim@gmail.com,  monnier@iro.umontreal.ca,  alex.bennee@linaro.org
>> Date: Thu, 21 Jul 2016 21:08:43 -0400
>> 
>> I made the same adjustments to the saved sub_start and sub_end
>> variables, but I had a mistake in that adjustment which caused the false
>> positives.  Fixed in the attached v2 patch.  We could just drop the
>> check, though I've already found it useful to catch bugs
>> (https://github.com/joaotavora/yasnippet/issues/720).
>> 
>> If I drop the checks (see attached v3 patch), then after following the
>> bug#23869 recipe, I get:
>> 
>>     ## -*- Octave -*-
>>     -module(bug).
>>     -export([identity/1, is_even/1, size/1, reverse/1]).
>
> OK, let's wait for a few days to give time to the people who were
> affected by the issue to test the patch, and if no new issues come up,
> please push the version with the error code to emacs-25.

Since they've now reported that the new patch is working well, I've
pushed it as e1def617.

bug#23917: Re: bug#23917: 25.0.95; commit 3a9d6296b35e5317c497674d5725eb52699bd3b8 causing org-capture to error out

[N. Jackson]

At 21:08 -0400 on Thursday 2016-07-21, npostavs@users.sourceforge.net wrote:

> I made the same adjustments to the saved sub_start and sub_end
> variables, but I had a mistake in that adjustment which caused the false
> positives.  Fixed in the attached v2 patch.  We could just drop the
> check, though I've already found it useful to catch bugs
> (https://github.com/joaotavora/yasnippet/issues/720).
>
> If I drop the checks (see attached v3 patch), then after following the
> bug#23869 recipe, I get:
>
>     ## -*- Octave -*-
>     -module(bug).
>     -export([identity/1, is_even/1, size/1, reverse/1]).

Thanks Noam,

Both the v2 and the v3 patch work for me with all my Org captures under
my full config, and with the simple recipe under `emacs -Q".

I'm not familiar with the details of the bug or the patches, but turning
off checks doesn't sound right, so I've stayed with the v2 patch.

I am now running the v2 patch as my every day Emacs.

FWIW, my versions of Emacs and Org are:

    GNU Emacs 25.0.95.15 (x86_64-unknown-linux-gnu, GTK+ Version 3.18.9)
     of 2016-07-22 built on moondust
    Repository revision: 4157159a37b43712440da91a45a6d5f71eb96e8a

and 

    Org-mode version 8.3.5 (8.3.5-elpa @ /home/nlj/.emacs.d/elpa/org-20160719/)

.

bug#23917: 25.0.95; commit 3a9d6296b35e5317c497674d5725eb52699bd3b8 causing org-capture to error out

[Eli Zaretskii]

> From: nljlistbox2@gmail.com (N. Jackson)
> Cc: 23917@debbugs.gnu.org,  Eli Zaretskii <eliz@gnu.org>,  jwiegley@gmail.com,  rpluim@gmail.com,  monnier@iro.umontreal.ca,  alex.bennee@linaro.org
> Date: Fri, 22 Jul 2016 21:42:08 -0300
> 
> Both the v2 and the v3 patch work for me with all my Org captures under
> my full config, and with the simple recipe under `emacs -Q".
> 
> I'm not familiar with the details of the bug or the patches, but turning
> off checks doesn't sound right, so I've stayed with the v2 patch.
> 
> I am now running the v2 patch as my every day Emacs.

Thanks.

Noam, I think this means your v2 patch can be pushed to emacs-25,
let's say by end of today.

bug#23917: 25.0.95; commit 3a9d6296b35e5317c497674d5725eb52699bd3b8 causing org-capture to error out

[N. Jackson]

At 20:56 -0400 on Wednesday 2016-07-20, npostavs@users.sourceforge.net wrote:
>
> From a8098080dff5f83f7cbcbec2bc263f9db3b45ad9 Mon Sep 17 00:00:00 2001
> From: Noam Postavsky <npostavs@gmail.com>
> Date: Wed, 20 Jul 2016 20:15:14 -0400
> Subject: [PATCH v1] Adjust match data before calling after-change-funs
>
> * src/insdel.c (replace_range): Add new parameter ADJUST_MATCH_DATA, if
> true.  Update all callers except Freplace_match to pass 0 for the new
> parameter.
> * src/search.c (update_search_regs): New function, extracted from
> Freplace_match.
> (Freplace_match): Remove match data adjustment code, pass 1 for
> ADJUST_MATCH_DATA to replace_range instead.
> ---
>  src/cmds.c    |  2 +-
>  src/editfns.c |  6 +++---
>  src/insdel.c  | 10 ++++++++--
>  src/lisp.h    |  4 +++-
>  src/search.c  | 50 +++++++++++++++++++++++++++++---------------------
>  5 files changed, 44 insertions(+), 28 deletions(-)
>
> diff --git a/src/cmds.c b/src/cmds.c
> index 1e44ddd..4003d8b 100644
> --- a/src/cmds.c
> +++ b/src/cmds.c
> @@ -447,7 +447,7 @@ internal_self_insert (int c, EMACS_INT n)
>  	  string = concat2 (string, tem);
>  	}
>  
> -      replace_range (PT, PT + chars_to_delete, string, 1, 1, 1);
> +      replace_range (PT, PT + chars_to_delete, string, 1, 1, 1, 0);
>        Fforward_char (make_number (n));
>      }
>    else if (n > 1)
> diff --git a/src/editfns.c b/src/editfns.c
> index 412745d..32c8bec 100644
> --- a/src/editfns.c
> +++ b/src/editfns.c
> @@ -3192,7 +3192,7 @@ DEFUN ("subst-char-in-region", Fsubst_char_in_region,
>  	      /* replace_range is less efficient, because it moves the gap,
>  		 but it handles combining correctly.  */
>  	      replace_range (pos, pos + 1, string,
> -			     0, 0, 1);
> +			     0, 0, 1, 0);
>  	      pos_byte_next = CHAR_TO_BYTE (pos);
>  	      if (pos_byte_next > pos_byte)
>  		/* Before combining happened.  We should not increment
> @@ -3405,7 +3405,7 @@ DEFUN ("translate-region-internal", Ftranslate_region_internal,
>  		  /* This is less efficient, because it moves the gap,
>  		     but it should handle multibyte characters correctly.  */
>  		  string = make_multibyte_string ((char *) str, 1, str_len);
> -		  replace_range (pos, pos + 1, string, 1, 0, 1);
> +		  replace_range (pos, pos + 1, string, 1, 0, 1, 0);
>  		  len = str_len;
>  		}
>  	      else
> @@ -3446,7 +3446,7 @@ DEFUN ("translate-region-internal", Ftranslate_region_internal,
>  		{
>  		  string = Fmake_string (make_number (1), val);
>  		}
> -	      replace_range (pos, pos + len, string, 1, 0, 1);
> +	      replace_range (pos, pos + len, string, 1, 0, 1, 0);
>  	      pos_byte += SBYTES (string);
>  	      pos += SCHARS (string);
>  	      cnt += SCHARS (string);
> diff --git a/src/insdel.c b/src/insdel.c
> index 4ad1074..fc3f19f 100644
> --- a/src/insdel.c
> +++ b/src/insdel.c
> @@ -1268,7 +1268,9 @@ adjust_after_insert (ptrdiff_t from, ptrdiff_t from_byte,
>  /* Replace the text from character positions FROM to TO with NEW,
>     If PREPARE, call prepare_to_modify_buffer.
>     If INHERIT, the newly inserted text should inherit text properties
> -   from the surrounding non-deleted text.  */
> +   from the surrounding non-deleted text.
> +   If ADJUST_MATCH_DATA, then adjust the match data before calling
> +   signal_after_change.  */
>  
>  /* Note that this does not yet handle markers quite right.
>     Also it needs to record a single undo-entry that does a replacement
> @@ -1279,7 +1281,8 @@ adjust_after_insert (ptrdiff_t from, ptrdiff_t from_byte,
>  
>  void
>  replace_range (ptrdiff_t from, ptrdiff_t to, Lisp_Object new,
> -	       bool prepare, bool inherit, bool markers)
> +               bool prepare, bool inherit, bool markers,
> +               bool adjust_match_data)
>  {
>    ptrdiff_t inschars = SCHARS (new);
>    ptrdiff_t insbytes = SBYTES (new);
> @@ -1426,6 +1429,9 @@ replace_range (ptrdiff_t from, ptrdiff_t to, Lisp_Object new,
>    MODIFF++;
>    CHARS_MODIFF = MODIFF;
>  
> +  if (adjust_match_data)
> +    update_search_regs (from, to, from + SCHARS (new));
> +
>    signal_after_change (from, nchars_del, GPT - from);
>    update_compositions (from, GPT, CHECK_BORDER);
>  }
> diff --git a/src/lisp.h b/src/lisp.h
> index 6a98adb..25f811e 100644
> --- a/src/lisp.h
> +++ b/src/lisp.h
> @@ -3516,7 +3516,7 @@ extern void adjust_after_insert (ptrdiff_t, ptrdiff_t, ptrdiff_t,
>  				 ptrdiff_t, ptrdiff_t);
>  extern void adjust_markers_for_delete (ptrdiff_t, ptrdiff_t,
>  				       ptrdiff_t, ptrdiff_t);
> -extern void replace_range (ptrdiff_t, ptrdiff_t, Lisp_Object, bool, bool, bool);
> +extern void replace_range (ptrdiff_t, ptrdiff_t, Lisp_Object, bool, bool, bool, bool);
>  extern void replace_range_2 (ptrdiff_t, ptrdiff_t, ptrdiff_t, ptrdiff_t,
>  			     const char *, ptrdiff_t, ptrdiff_t, bool);
>  extern void syms_of_insdel (void);
> @@ -3994,6 +3994,8 @@ extern Lisp_Object make_temp_name (Lisp_Object, bool);
>  /* Defined in search.c.  */
>  extern void shrink_regexp_cache (void);
>  extern void restore_search_regs (void);
> +extern void update_search_regs (ptrdiff_t oldstart,
> +                                ptrdiff_t oldend, ptrdiff_t newend);
>  extern void record_unwind_save_match_data (void);
>  struct re_registers;
>  extern struct re_pattern_buffer *compile_pattern (Lisp_Object,
> diff --git a/src/search.c b/src/search.c
> index 5c949ad..1b82c94 100644
> --- a/src/search.c
> +++ b/src/search.c
> @@ -2727,8 +2727,15 @@ DEFUN ("replace-match", Freplace_match, Sreplace_match, 1, 5, 0,
>  
>    /* Replace the old text with the new in the cleanest possible way.  */
>    replace_range (search_regs.start[sub], search_regs.end[sub],
> -		 newtext, 1, 0, 1);
> +                 newtext, 1, 0, 1, 1);
>    newpoint = search_regs.start[sub] + SCHARS (newtext);
> +  /* Update saved data to match adjustment made by replace_range.  */
> +  {
> +    ptrdiff_t change = newpoint - sub_end;
> +    if (sub_start >= sub_end)
> +      sub_start += change;
> +    sub_end += change;
> +  }
>  
>    if (case_action == all_caps)
>      Fupcase_region (make_number (search_regs.start[sub]),
> @@ -2742,26 +2749,6 @@ DEFUN ("replace-match", Freplace_match, Sreplace_match, 1, 5, 0,
>        || search_regs.num_regs != num_regs)
>      error ("Match data clobbered by buffer modification hooks");
>  
> -  /* Adjust search data for this change.  */
> -  {
> -    ptrdiff_t oldend = search_regs.end[sub];
> -    ptrdiff_t oldstart = search_regs.start[sub];
> -    ptrdiff_t change = newpoint - search_regs.end[sub];
> -    ptrdiff_t i;
> -
> -    for (i = 0; i < search_regs.num_regs; i++)
> -      {
> -	if (search_regs.start[i] >= oldend)
> -	  search_regs.start[i] += change;
> -	else if (search_regs.start[i] > oldstart)
> -	  search_regs.start[i] = oldstart;
> -	if (search_regs.end[i] >= oldend)
> -	  search_regs.end[i] += change;
> -	else if (search_regs.end[i] > oldstart)
> -	  search_regs.end[i] = oldstart;
> -      }
> -  }
> -
>    /* Put point back where it was in the text.  */
>    if (opoint <= 0)
>      TEMP_SET_PT (opoint + ZV);
> @@ -3102,6 +3089,27 @@ restore_search_regs (void)
>      }
>  }
>  
> +/* Called from replace-match via replace_range.  */
> +void
> +update_search_regs (ptrdiff_t oldstart, ptrdiff_t oldend, ptrdiff_t newend)
> +{
> +  /* Adjust search data for this change.  */
> +  ptrdiff_t change = newend - oldend;
> +  ptrdiff_t i;
> +
> +  for (i = 0; i < search_regs.num_regs; i++)
> +    {
> +      if (search_regs.start[i] >= oldend)
> +        search_regs.start[i] += change;
> +      else if (search_regs.start[i] > oldstart)
> +        search_regs.start[i] = oldstart;
> +      if (search_regs.end[i] >= oldend)
> +        search_regs.end[i] += change;
> +      else if (search_regs.end[i] > oldstart)
> +        search_regs.end[i] = oldstart;
> +    }
> +}
> +
>  static void
>  unwind_set_match_data (Lisp_Object list)
>  {

FWIW on my system applying this patch only partially resolves the
org-capture issue. I'm testing with org-20160718 from GNU Elpa and
latest Emacs 25 branch from the git (Repository revision:
4157159a37b43712440da91a45a6d5f71eb96e8a).

The patch successfully eliminates the match-data-clobbered error/abort
during org-capture with all my capture templates when I have my entire
config loaded, but with a minimal recipe from emacs -Q the org-capture
match-data-clobbered error still occurs.

The minimal recipe I'm testing with is similar to that posted by Robert
Pluim on 2016-07-18, specifically

  src/emacs -Q

  M-: (custom-set-variables '(package-selected-packages (quote (org-20160718)))) RET
  M-x package-initialize RET

  C-x C-f		; find file.
  C-S-backspace		; kill-whole-line.
  ~/.notes RET		; Open the file expected by default capture template.
  M-x org-mode RET	; put the buffer into Org Mode.
  M-x org-capture RET t ; Run the default "Task" capture template bound to the t key.

With your patch I still get the error: 

  org-capture: Capture template ‘t’: Match data clobbered by buffer modification hooks

.

It puzzles me that your patch doesn't work for the emacs -Q recipe but
does work for my normal configuration, so much so that I suspected that
I had made a mistake, but I have reset and reapplied the patch three
times and I continue to see the same results.

I hope this helps.

bug#23917: 25.0.95; commit 3a9d6296b35e5317c497674d5725eb52699bd3b8 causing org-capture to error out

[Robert Pluim]

(this is with org-20160704 from elpa. The org version ("8.2.10") in
emacs-25 does not exhibit this error)

This is from emacs -Q, with a subsequent changing of load-paths to
point at the elpa org. Reverting the commit mentioned above makes the
error go away.

In gdb:

Breakpoint 3, Freplace_match (newtext=8699012, fixedcase=<optimised out>, literal=0, string=<optimised out>, subexp=<optimised out>)
    at search.c:2710
2710	    error ("Match data clobbered by buffer modification hooks");
(gdb) xbacktrace 
"replace-match" (0xffffc680)
"org-capture-empty-lines-after" (0xffffc850)
"org-capture-place-entry" (0xffffca30)
"org-capture-place-template" (0xffffcc10)
"org-capture" (0xffffcf20)
"funcall-interactively" (0xffffcf18)
"call-interactively" (0xffffd180)
"command-execute" (0xffffd328)
"execute-extended-command" (0xffffd580)
"funcall-interactively" (0xffffd578)
"call-interactively" (0xffffd820)
"command-execute" (0xffffd998)

Based on the discussion in Bug#23869, there's a hook somewhere that
needs to do save-match-data, but I have no idea how to figure out
which one.

bug#23917: 25.0.95; commit 3a9d6296b35e5317c497674d5725eb52699bd3b8 causing org-capture to error out

[Eli Zaretskii]

> From: Robert Pluim <rpluim@gmail.com>
> Date: Fri, 08 Jul 2016 14:42:02 +0200
> 
> (this is with org-20160704 from elpa. The org version ("8.2.10") in
> emacs-25 does not exhibit this error)
> 
> This is from emacs -Q, with a subsequent changing of load-paths to
> point at the elpa org. Reverting the commit mentioned above makes the
> error go away.
> 
> In gdb:
> 
> Breakpoint 3, Freplace_match (newtext=8699012, fixedcase=<optimised out>, literal=0, string=<optimised out>, subexp=<optimised out>)
>     at search.c:2710
> 2710	    error ("Match data clobbered by buffer modification hooks");
> (gdb) xbacktrace 
> "replace-match" (0xffffc680)
> "org-capture-empty-lines-after" (0xffffc850)
> "org-capture-place-entry" (0xffffca30)
> "org-capture-place-template" (0xffffcc10)
> "org-capture" (0xffffcf20)
> "funcall-interactively" (0xffffcf18)
> "call-interactively" (0xffffd180)
> "command-execute" (0xffffd328)
> "execute-extended-command" (0xffffd580)
> "funcall-interactively" (0xffffd578)
> "call-interactively" (0xffffd820)
> "command-execute" (0xffffd998)
> 
> Based on the discussion in Bug#23869, there's a hook somewhere that
> needs to do save-match-data

Right.

> but I have no idea how to figure out which one.

Run Emacs under a debugger, and set a breakpoint before the call to
replace_range in Freplace_match.  When the breakpoint breaks, make
sure it's indeed being called from org-capture-empty-lines-after, and
if so, do this:

  (gdb) watch -l search_regs.start[sub]
  (gdb) watch -l search_regs.end[sub]
  (gdb) watch -l search_regs.num_regs
  (gdb) continue

Then GDB will kick in as soon as one of these 3 is clobbered, and the
backtrace will show you whodunit.

bug#23917: 25.0.95; commit 3a9d6296b35e5317c497674d5725eb52699bd3b8 causing org-capture to error out

[Robert Pluim]

Eli Zaretskii <eliz@gnu.org> writes:

> Run Emacs under a debugger, and set a breakpoint before the call to
> replace_range in Freplace_match.  When the breakpoint breaks, make
> sure it's indeed being called from org-capture-empty-lines-after, and
> if so, do this:
>
>   (gdb) watch -l search_regs.start[sub]
>   (gdb) watch -l search_regs.end[sub]
>   (gdb) watch -l search_regs.num_regs
>   (gdb) continue
>
> Then GDB will kick in as soon as one of these 3 is clobbered, and the
> backtrace will show you whodunit.

So that gives me (after recompiling with -O0 -g3 :-) )

Hardware watchpoint 6: -location search_regs.end[sub]

Old value = 77
New value = 76
Fset_match_data (list=33497827, reseat=19440) at search.c:3010
3010		    if (!NILP (reseat) && MARKERP (m))

Lisp Backtrace:
"set-match-data" (0xffff9ad0)
0x1f6e8b0 PVEC_COMPILED
"org-element--cache-after-change" (0xffffa6c8)
"replace-match" (0xffffaa20)
"org-capture-empty-lines-after" (0xffffaf60)
"org-capture-place-entry" (0xffffb4b0)
"org-capture-place-template" (0xffffba00)
"org-capture" (0xffffc0b0)
"funcall-interactively" (0xffffc0a8)
"call-interactively" (0xffffc3e0)
"command-execute" (0xffffc968)
"execute-extended-command" (0xffffcfd0)
"funcall-interactively" (0xffffcfc8)
"call-interactively" (0xffffd360)
"command-execute" (0xffffd8c8)
(gdb) c
Continuing.

Breakpoint 4, Freplace_match (newtext=9563044, fixedcase=0, literal=0, string=0, subexp=0) at search.c:2710
2710	    error ("Match data clobbered by buffer modification hooks");

org-element--cache-after-change is:

(defun org-element--cache-after-change (beg end pre)
  "Update buffer modifications for current buffer.
BEG and END are the beginning and end of the range of changed
text, and the length in bytes of the pre-change text replaced by
that range.  See `after-change-functions' for more information."
  (when (org-element--cache-active-p)
    (org-with-wide-buffer
     (goto-char beg)
     (beginning-of-line)
     (save-match-data
       (let ((top (point))
	     (bottom (save-excursion (goto-char end) (line-end-position))))
	 ;; Determine if modified area needs to be extended, according
	 ;; to both previous and current state.  We make a special
	 ;; case for headline editing: if a headline is modified but
	 ;; not removed, do not extend.
	 (when (case org-element--cache-change-warning
		 ((t) t)
		 (headline
		  (not (and (org-with-limited-levels (org-at-heading-p))
			    (= (line-end-position) bottom))))
		 (otherwise
		  (let ((case-fold-search t))
		    (re-search-forward
		     org-element--cache-sensitive-re bottom t))))
	   ;; Effectively extend modified area.
	   (org-with-limited-levels
	    (setq top (progn (goto-char top)
			     (when (outline-previous-heading) (forward-line))
			     (point)))
	    (setq bottom (progn (goto-char bottom)
				(if (outline-next-heading) (1- (point))
				  (point))))))
	 ;; Store synchronization request.
	 (let ((offset (- end beg pre)))
	   (org-element--cache-submit-request top (- bottom offset) offset)))))
    ;; Activate a timer to process the request during idle time.
    (org-element--cache-set-timer (current-buffer))))

which already does save-match-data. If I globally disable the org
element cache by (setq org-element-use-cache nil) the issue
disappears, so now I'm confused as to what's going on.

bug#23917: 25.0.95; commit 3a9d6296b35e5317c497674d5725eb52699bd3b8 causing org-capture to error out

[Eli Zaretskii]

> From: Robert Pluim <rpluim@gmail.com>
> Cc: 23917@debbugs.gnu.org
> Date: Fri, 08 Jul 2016 17:40:42 +0200
> 
> org-element--cache-after-change is:
> 
> (defun org-element--cache-after-change (beg end pre)
>   "Update buffer modifications for current buffer.
> BEG and END are the beginning and end of the range of changed
> text, and the length in bytes of the pre-change text replaced by
> that range.  See `after-change-functions' for more information."
>   (when (org-element--cache-active-p)
>     (org-with-wide-buffer
>      (goto-char beg)
>      (beginning-of-line)
>      (save-match-data
>        (let ((top (point))
> 	     (bottom (save-excursion (goto-char end) (line-end-position))))
> 	 ;; Determine if modified area needs to be extended, according
> 	 ;; to both previous and current state.  We make a special
> 	 ;; case for headline editing: if a headline is modified but
> 	 ;; not removed, do not extend.
> 	 (when (case org-element--cache-change-warning
> 		 ((t) t)
> 		 (headline
> 		  (not (and (org-with-limited-levels (org-at-heading-p))
> 			    (= (line-end-position) bottom))))
> 		 (otherwise
> 		  (let ((case-fold-search t))
> 		    (re-search-forward
> 		     org-element--cache-sensitive-re bottom t))))
> 	   ;; Effectively extend modified area.
> 	   (org-with-limited-levels
> 	    (setq top (progn (goto-char top)
> 			     (when (outline-previous-heading) (forward-line))
> 			     (point)))
> 	    (setq bottom (progn (goto-char bottom)
> 				(if (outline-next-heading) (1- (point))
> 				  (point))))))
> 	 ;; Store synchronization request.
> 	 (let ((offset (- end beg pre)))
> 	   (org-element--cache-submit-request top (- bottom offset) offset)))))
>     ;; Activate a timer to process the request during idle time.
>     (org-element--cache-set-timer (current-buffer))))
> 
> which already does save-match-data. If I globally disable the org
> element cache by (setq org-element-use-cache nil) the issue
> disappears, so now I'm confused as to what's going on.

Load the file where this function lives as a .el file, and when the
watchpoint triggers, show the results of "xbacktrace".

bug#23917: 25.0.95; commit 3a9d6296b35e5317c497674d5725eb52699bd3b8 causing org-capture to error out

[Eli Zaretskii]

> Date: Fri, 08 Jul 2016 20:03:35 +0300
> From: Eli Zaretskii <eliz@gnu.org>
> Cc: 23917@debbugs.gnu.org
> 
> > which already does save-match-data. If I globally disable the org
> > element cache by (setq org-element-use-cache nil) the issue
> > disappears, so now I'm confused as to what's going on.
> 
> Load the file where this function lives as a .el file, and when the
> watchpoint triggers, show the results of "xbacktrace".

Actually, we might be looking in the wrong direction: since some of
these functions do use save-match-data, the search registers might be
legitimately changed and restored several times.  So I think you need
to type "continue" and see what other parts of code change the match
data, all the way until the call to replace_range returns.  Once we've
seen all those changes, with backtraces for each of them, we will
probably see the culprit.

It is still advisable to load the involved files as *.el files, as the
results of "xbacktrace" will then be more detailed.

Thanks.

bug#23917: 25.0.95; commit 3a9d6296b35e5317c497674d5725eb52699bd3b8 causing org-capture to error out

[N. Jackson]

At 10:19 +0200 on Thursday 2016-07-21, Robert Pluim wrote:
>
> nljlistbox2@gmail.com (N. Jackson) writes:
>
>> At 20:56 -0400 on Wednesday 2016-07-20, npostavs@users.sourceforge.net wrote:
>>>
>>> From: Noam Postavsky <npostavs@gmail.com>
>>> Subject: [PATCH v1] Adjust match data before calling after-change-funs
>>
>> It puzzles me that your patch doesn't work for the emacs -Q recipe but
>> does work for my normal configuration, so much so that I suspected that
>> I had made a mistake, but I have reset and reapplied the patch three
>> times and I continue to see the same results.
>
> You're not alone: this patch doesn't fix the issue for me either with
> emacs -Q or with my normal capture templates.

FWIW, taking into account that I needed to rebuild Org Mode using the
patched Emacs, I redid my tests of Noam's patch and got the same
puzzling results as before. That is the patch *does* fix the problem for
me with my normal caputure templates, but it does not fix the problem
with the simple recipe in emacs -Q.

N.

bug#23917: 25.0.95; commit 3a9d6296b35e5317c497674d5725eb52699bd3b8 causing org-capture to error out

[Robert Pluim]

nljlistbox2@gmail.com (N. Jackson) writes:

> At 20:56 -0400 on Wednesday 2016-07-20, npostavs@users.sourceforge.net wrote:
>>
>> From a8098080dff5f83f7cbcbec2bc263f9db3b45ad9 Mon Sep 17 00:00:00 2001
>> From: Noam Postavsky <npostavs@gmail.com>
>> Date: Wed, 20 Jul 2016 20:15:14 -0400
>> Subject: [PATCH v1] Adjust match data before calling after-change-funs
>>
>> * src/insdel.c (replace_range): Add new parameter ADJUST_MATCH_DATA, if
>> true.  Update all callers except Freplace_match to pass 0 for the new
>> parameter.
>> * src/search.c (update_search_regs): New function, extracted from
>> Freplace_match.
>> (Freplace_match): Remove match data adjustment code, pass 1 for
>> ADJUST_MATCH_DATA to replace_range instead.
> FWIW on my system applying this patch only partially resolves the
> org-capture issue. I'm testing with org-20160718 from GNU Elpa and
> latest Emacs 25 branch from the git (Repository revision:
> 4157159a37b43712440da91a45a6d5f71eb96e8a).
>
> The patch successfully eliminates the match-data-clobbered error/abort
> during org-capture with all my capture templates when I have my entire
> config loaded, but with a minimal recipe from emacs -Q the org-capture
> match-data-clobbered error still occurs.
>
> The minimal recipe I'm testing with is similar to that posted by Robert
> Pluim on 2016-07-18, specifically
>
>   src/emacs -Q
>
>   M-: (custom-set-variables '(package-selected-packages (quote (org-20160718)))) RET
>   M-x package-initialize RET
>
>   C-x C-f		; find file.
>   C-S-backspace		; kill-whole-line.
>   ~/.notes RET		; Open the file expected by default capture template.
>   M-x org-mode RET	; put the buffer into Org Mode.
>   M-x org-capture RET t ; Run the default "Task" capture template bound to the t key.
>
> With your patch I still get the error: 
>
>   org-capture: Capture template ‘t’: Match data clobbered by buffer modification hooks
>
> .
>
> It puzzles me that your patch doesn't work for the emacs -Q recipe but
> does work for my normal configuration, so much so that I suspected that
> I had made a mistake, but I have reset and reapplied the patch three
> times and I continue to see the same results.

You're not alone: this patch doesn't fix the issue for me either with
emacs -Q or with my normal capture templates.

Regards

Robert

bug#23917: Please consider making Bug #23917 a blocker for 25.1 (was Re: org-capture: Capture template ‘g’: Match data clobbered by buffer modification hooks)

[npostavs]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Stefan Monnier <monnier@iro.umontreal.ca>
>> Cc: rpluim@gmail.com,  23917@debbugs.gnu.org,  alex.bennee@linaro.org,  jwiegley@gmail.com,  nljlistbox2@gmail.com
>> Date: Tue, 19 Jul 2016 00:48:19 -0400
>> 
>> > The more general problem is when there's at least one more
>> > sub-expression, whose start and/or end are after the new EOB.
>> > Those sub-expression's data will be completely bogus after the
>> > adjustment,
>> 
>> If they were after the EOB, they were already bogus to start with.
>
> I think we are mis-communicating.  I mean the following scenario:
>
> Before call to replace_range in replace-match:
>
>    |---------------------------|---|------|----|
>    s1                         e1   s2    e2   EOB
>
> (s1, e1, etc. are the start and end of the corresponding
> sub-expressions.)
>
> After the call to replace_range in replace-match:
>
>    |---------|---|------|----|
>    s1       e1   s2    e2   EOB
>
> IOW, the 1st sub-expression got replaced with a much shorter text,
> which made EOB be smaller than the original beginning and end of the
> 2nd sub-expression.  There's nothing bogus with this, is there?  The
> user will expect to get match-data adjusted as shown in the second
> diagram, and that's what she will really get -- unless there are
> buffer-modification hooks that use save-match-data.  In the latter
> case, what the user will get instead is this:
>
>    |---------|---|------|----|
>    s1                       EOB
>                             e1
> 			    s2
> 			    e2
>
> and that is even before the adjustment code kicks in and makes
> "adjustments" with an incorrect adjustment value, which is computed as
>
>   newpoint = search_regs.start[sub] + SCHARS (newtext);
>   [...]
>   ptrdiff_t change = newpoint - search_regs.end[sub];
>
> and so will use the new EOB as search_regs.end[sub], instead of the
> correct original value of e1 from the first diagram above.
>
> IOW, the call to save-match-data in a buffer-modification hook
> _disrupts_ the normal operation of replace-match in this case, by
> indirectly sabotaging the adjustment of match data after the
> replacement.

Is it not possible to adjust the match data *before* calling buffer
modification hooks?  Seems to me the root of the problem is that buffer
modification hooks get to see this invalid intermediate state where the
match data is out of sync with the buffer.

bug#23917: Please consider making Bug #23917 a blocker for 25.1 (was Re: org-capture: Capture template ‘g’: Match data clobbered by buffer modification hooks)

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: Robert Pluim <rpluim@gmail.com>,  23917@debbugs.gnu.org,  alex.bennee@linaro.org,  jwiegley@gmail.com,  nljlistbox2@gmail.com
> Date: Mon, 18 Jul 2016 20:58:35 -0400
> 
> > I think this change performed by save-match-data is harmless: the old
> > value (62) was not valid any more anyway.
> 
> In this particular case, yes.  But only in this case, because (a)
> there's actually only one sub-expression, and (b) it ends exactly at
> EOB.

Moreover, if the value of 62 was left alone by save-match-data, the
adjustment code in replace-match would have fixed it.  That's what
that code is all about: it fixes the match data due to changes to the
buffer made by replacing the old text by the new.  Any attempts to
"fix" those values under that code's feet are not helpful; quite the
contrary.

> > So I think a safe fix is to try and relax the check we added to
> > replace-match so it doesn't get all worked up when something ≥ EOB gets
> > changed to something else that's also ≥ EOB.
> 
> And lose the other sub-expressions in a more general case?  Really?

What really bothers me is that by just loosening the conditions under
which we signal an error we defeat _valid_ code, which did use
save-match-data, and yet the match data still ends up being
clobbered.  I don't mind making the test more loose so that invalid
programs have a longer rope to hang themselves, but valid programs
should not be failed, IMO.

> > Or maybe instead of signaling an error, we could simply skip the "Adjust
> > search data for this change".

> That would still sweep the problem under the carpet, leaving the match
> data bogus, so I don't like doing that.

> > This said, I don't fully understand what's going on: bug#23869 reported
> > a crash, but AFAICT the match-data here is only used to adjust
> > search_regs which seems like it wouldn't cause a crash, even if the new
> > values are bogus.

> The crash in bug#23869 was due to this:

>   newpoint = search_regs.start[sub] + SCHARS (newtext);
>   [...]
>   /* Now move point "officially" to the start of the inserted replacement.  */
>   move_if_not_intangible (newpoint);  <<<<<<<<<<<<<<<<<<<<<<<

> because due to clobbering, newpoint became -1.


> > > -   '((save-match-data-internal (match-data)))
> > > +   '((save-match-data-internal (match-data 'integers)))
> > 
> > That looks risky.
> 
> Then how about manually doing the equivalent of save-match-data around
> the call to replace_range, calling match-data with non-nil argument?

Here are some more alternatives for dealing with this issue:

 (1) Make match-data use integers instead of markers by default when a
     call to replace-match is in progress, i.e. when match-data is
     called from some buffer-modification hook triggered by
     replace-match

 (2) Don't signal an error, even if match data seems clobbered, if the
     new value of point is valid and either (a) there's only one
     search register, or (b) the adjustment value is zero (i.e. the
     registers will be left unchanged).

I like (1) better than (2) because (1) will let valid programs avoid
clobbering match data, but maybe it's too risky for emacs-25.  Also,
is it plausible that some buffer-modification hook will edit the
buffer in the save-match-data forms, and expect the match data to be
adjusted, or is this something that a buffer-modification hook should
never do?  If valid programs can do this, then (1) is probably not a
good idea.

bug#23917: 25.0.95; commit 3a9d6296b35e5317c497674d5725eb52699bd3b8 causing org-capture to error out

[N. Jackson]

At 21:09 +0300 on Monday 2016-07-18, Eli Zaretskii wrote:

> My suggestion to fix this is below.  I ask for opinions on (1) whether
> this looks like TRT, (2) whether it is safe enough for emacs-25, and
> (3) whether someone has better ideas.  If someone thinks I've
> misunderstood the issue, don't hesitate to explain why, because
> frankly it feels very strange to find bugs that seem to have existed
> since 1990.
>
> diff --git a/lisp/subr.el b/lisp/subr.el
> index e9e19d3..1bb1cb3 100644
> --- a/lisp/subr.el
> +++ b/lisp/subr.el
> @@ -3466,7 +3466,7 @@ save-match-data
>    ;; if you need to recompile all the Lisp files using interpreted code.
>    (declare (indent 0) (debug t))
>    (list 'let
> -	'((save-match-data-internal (match-data)))
> +	'((save-match-data-internal (match-data 'integers)))
>  	(list 'unwind-protect
>  	      (cons 'progn body)
>  	      ;; It is safe to free (evaporate) markers immediately here,

FWIW on my system applying this patch does not resolve the org-capture
issue. I'm testing with org-20160718 from GNU Elpa and latest Emacs 25
branch from the git (Repository revision: 4157159a37b43712440da91a45a6d5f71eb96e8a).

With these versions of Org and Emacs and your patch applied, with a
recipe similar to that posted by Robert Pluim on 2016-07-18,
specifically

  src/emacs -Q

  M-: (custom-set-variables '(package-selected-packages (quote (org-20160718)))) RET
  M-x package-initialize RET

  C-x C-f		; find file.
  C-S-backspace		; kill-whole-line.
  ~/.notes RET		; Open the file expected by default capture template.
  M-x org-mode RET	; put the buffer into Org Mode.
  M-x org-capture RET t ; Run the default "Task" capture template bound to the t key.

I get the error: 

  org-capture: Capture template ‘t’: Match data clobbered by buffer modification hooks

. I also get a similar error with your patch and my full configuration
loaded and using my own capture templates:

  org-capture: Capture abort: (error Match data clobbered by buffer modification hooks)

.

The results above are same as I get if I do not apply your patch.

[On the other hand, with the same version of Org as above and Emacs from the
25.0.95 tarball, I do not see these error.]

bug#23917: 25.0.95; commit 3a9d6296b35e5317c497674d5725eb52699bd3b8 causing org-capture to error out

[Robert Pluim]

nljlistbox2@gmail.com (N. Jackson) writes:

> At 21:09 +0300 on Monday 2016-07-18, Eli Zaretskii wrote:
>
>> diff --git a/lisp/subr.el b/lisp/subr.el
>> index e9e19d3..1bb1cb3 100644
>> --- a/lisp/subr.el
>> +++ b/lisp/subr.el
>> @@ -3466,7 +3466,7 @@ save-match-data
>>    ;; if you need to recompile all the Lisp files using interpreted code.
>>    (declare (indent 0) (debug t))
>>    (list 'let
>> -	'((save-match-data-internal (match-data)))
>> +	'((save-match-data-internal (match-data 'integers)))
>>  	(list 'unwind-protect
>>  	      (cons 'progn body)
>>  	      ;; It is safe to free (evaporate) markers immediately here,
>
> FWIW on my system applying this patch does not resolve the org-capture
> issue. I'm testing with org-20160718 from GNU Elpa and latest Emacs 25
> branch from the git (Repository revision: 4157159a37b43712440da91a45a6d5f71eb96e8a).
>
> With these versions of Org and Emacs and your patch applied, with a
> recipe similar to that posted by Robert Pluim on 2016-07-18,
> specifically
>
>   src/emacs -Q
>
>   M-: (custom-set-variables '(package-selected-packages (quote (org-20160718)))) RET
>   M-x package-initialize RET
>
>   C-x C-f		; find file.
>   C-S-backspace		; kill-whole-line.
>   ~/.notes RET		; Open the file expected by default capture template.
>   M-x org-mode RET	; put the buffer into Org Mode.
>   M-x org-capture RET t ; Run the default "Task" capture template bound to the t key.
>
> I get the error: 
>
>   org-capture: Capture template ‘t’: Match data clobbered by buffer modification hooks

save-match-data is a macro. Did you recompile org with the modified
emacs?

That patch works for me when using that version of org uncompiled.

Regards

Robert

bug#23917: 25.0.95; commit 3a9d6296b35e5317c497674d5725eb52699bd3b8 causing org-capture to error out

[N. Jackson]

At 10:08 +0200 on Thursday 2016-07-21, Robert Pluim wrote:
>>
> nljlistbox2@gmail.com (N. Jackson) writes:
>
>> At 21:09 +0300 on Monday 2016-07-18, Eli Zaretskii wrote:
>>
>>> diff --git a/lisp/subr.el b/lisp/subr.el
>>> index e9e19d3..1bb1cb3 100644
>>> --- a/lisp/subr.el
>>> +++ b/lisp/subr.el
>>> @@ -3466,7 +3466,7 @@ save-match-data
>>>    ;; if you need to recompile all the Lisp files using interpreted code.
>>>    (declare (indent 0) (debug t))
>>>    (list 'let
>>> -	'((save-match-data-internal (match-data)))
>>> +	'((save-match-data-internal (match-data 'integers)))
>>>  	(list 'unwind-protect
>>>  	      (cons 'progn body)
>>>  	      ;; It is safe to free (evaporate) markers immediately here,
>>
>> FWIW on my system applying this patch does not resolve the org-capture
>> issue. I'm testing with org-20160718 from GNU Elpa and latest Emacs 25
>> branch from the git (Repository revision: 4157159a37b43712440da91a45a6d5f71eb96e8a).
>
> save-match-data is a macro. Did you recompile org with the modified
> emacs?

Thanks Robert. I failed to take that into account.

After re-applying Eli's patch above and then rebuilding Org Mode from
GNU Elpa (now org-20160719), the org-capture match-data-clobbered
error/abort no longer occurs, neither with the simple recipe in emacs -Q
nor with my own capture templates with my full config loaded.

Sorry for the noise.

[For completeness, I should say that I then removed the patch and
rebuilt org-20160719 again, and confirmed that it does trigger the bug
in both emacs -Q and with my configuration.]

N.

bug#23917: Please consider making Bug #23917 a blocker for 25.1 (was Re: org-capture: Capture template ‘g’: Match data clobbered by buffer modification hooks)

[Kaushal Modi]

[-- Attachment #1: Type: text/plain, Size: 473 bytes --]

I use org-capture heavily, but I do not use a template that causes this
issue.

This issue was posted once again today on the org mailing list and I would
vote for this to be made a blocking bug too.

One useful update we get from Eric Fraga (
http://thread.gmane.org/gmane.emacs.orgmode/108289 ) is that this bug is
seen only after 25.0.94 pretest.

@Eric Would it be possible for you to provide a recipe to recreate this
issue from an emacs -Q session?
-- 

Kaushal Modi

[-- Attachment #2: Type: text/html, Size: 742 bytes --]

bug#23917: Please consider making Bug #23917 a blocker for 25.1 (was Re: org-capture: Capture template ‘g’: Match data clobbered by buffer modification hooks)

[Eric S Fraga]

On Monday, 18 Jul 2016 at 14:50, Kaushal Modi wrote:
> @Eric Would it be possible for you to provide a recipe to recreate
> this issue from an emacs -Q session?

With emacs-snapshot (aka 25.0.95.1), and the following emacs-minimal.el
file:

--8<---------------cut here---------------start------------->8---
(add-to-list 'load-path "~/git/org-mode/lisp")
(require 'org)
(setq org-capture-templates '(("t" "todo" entry (file+datetree "/tmp/tasks.org") "* TODO %^{Task} %^G\nSCHEDULED: %t\n%i%?")))
--8<---------------cut here---------------end--------------->8---

the following sequence:

--8<---------------cut here---------------start------------->8---
emacs -Q -l emacs-minimal.el
M-x org-capture RET t this is a test RET testing RET
--8<---------------cut here---------------end--------------->8---

gives

--8<---------------cut here---------------start------------->8---
Debugger entered--Lisp error: (error "Capture template ‘t’: Match data clobbered by buffer modification hooks")
  signal(error ("Capture template ‘t’: Match data clobbered by buffer modification hooks"))
  error("Capture template `%s': %s" "t" "Match data clobbered by buffer modification hooks")
  (condition-case error (org-capture-place-template (equal (car (org-capture-get :target)) (quote function))) ((error quit) (if (and (buffer-base-buffer (current-buffer)) (string-match "\\`CAPTURE-" (buffer-name))) (kill-buffer (current-buffer))) (set-window-configuration (org-capture-get :return-to-wconf)) (error "Capture template `%s': %s" (org-capture-get :key) (nth 1 error))))
  (if (equal goto 0) (org-capture-insert-template-here) (condition-case error (org-capture-place-template (equal (car (org-capture-get :target)) (quote function))) ((error quit) (if (and (buffer-base-buffer (current-buffer)) (string-match "\\`CAPTURE-" (buffer-name))) (kill-buffer (current-buffer))) (set-window-configuration (org-capture-get :return-to-wconf)) (error "Capture template `%s': %s" (org-capture-get :key) (nth 1 error)))) (if (and (derived-mode-p (quote org-mode)) (org-capture-get :clock-in)) (condition-case nil (progn (if (org-clock-is-active) (org-capture-put :interrupted-clock (copy-marker org-clock-marker))) (org-clock-in) (set (make-local-variable (quote org-capture-clock-was-started)) t)) (error "Could not start the clock in this capture buffer"))) (if (org-capture-get :immediate-finish) (org-capture-finalize)))
  (cond ((equal entry "C") (customize-variable (quote org-capture-templates))) ((equal entry "q") (user-error "Abort")) (t (org-capture-set-plist entry) (org-capture-get-template) (org-capture-put :original-buffer orig-buf :original-file (or (buffer-file-name orig-buf) (and (featurep (quote dired)) (car (rassq orig-buf dired-buffers)))) :original-file-nondirectory (and (buffer-file-name orig-buf) (file-name-nondirectory (buffer-file-name orig-buf))) :annotation annotation :initial initial :return-to-wconf (current-window-configuration) :default-time (or org-overriding-default-time (org-current-time))) (org-capture-set-target-location) (condition-case error (org-capture-put :template (org-capture-fill-template)) ((error quit) (if (get-buffer "*Capture*") (kill-buffer "*Capture*")) (error "Capture abort: %s" error))) (setq org-capture-clock-keep (org-capture-get :clock-keep)) (if (equal goto 0) (org-capture-insert-template-here) (condition-case error (org-capture-place-template (equal (car (org-capture-get :target)) (quote function))) ((error quit) (if (and (buffer-base-buffer ...) (string-match "\\`CAPTURE-" ...)) (kill-buffer (current-buffer))) (set-window-configuration (org-capture-get :return-to-wconf)) (error "Capture template `%s': %s" (org-capture-get :key) (nth 1 error)))) (if (and (derived-mode-p (quote org-mode)) (org-capture-get :clock-in)) (condition-case nil (progn (if (org-clock-is-active) (org-capture-put :interrupted-clock ...)) (org-clock-in) (set (make-local-variable ...) t)) (error "Could not start the clock in this capture buffer"))) (if (org-capture-get :immediate-finish) (org-capture-finalize)))))
  (let* ((orig-buf (current-buffer)) (annotation (if (and (boundp (quote org-capture-link-is-already-stored)) org-capture-link-is-already-stored) (plist-get org-store-link-plist :annotation) (condition-case nil (progn (org-store-link nil)) (error nil)))) (entry (or org-capture-entry (org-capture-select-template keys))) initial) (setq initial (or org-capture-initial (and (org-region-active-p) (buffer-substring (point) (mark))))) (if (stringp initial) (progn (remove-text-properties 0 (length initial) (quote (read-only t)) initial))) (if (stringp annotation) (progn (remove-text-properties 0 (length annotation) (quote (read-only t)) annotation))) (cond ((equal entry "C") (customize-variable (quote org-capture-templates))) ((equal entry "q") (user-error "Abort")) (t (org-capture-set-plist entry) (org-capture-get-template) (org-capture-put :original-buffer orig-buf :original-file (or (buffer-file-name orig-buf) (and (featurep (quote dired)) (car (rassq orig-buf dired-buffers)))) :original-file-nondirectory (and (buffer-file-name orig-buf) (file-name-nondirectory (buffer-file-name orig-buf))) :annotation annotation :initial initial :return-to-wconf (current-window-configuration) :default-time (or org-overriding-default-time (org-current-time))) (org-capture-set-target-location) (condition-case error (org-capture-put :template (org-capture-fill-template)) ((error quit) (if (get-buffer "*Capture*") (kill-buffer "*Capture*")) (error "Capture abort: %s" error))) (setq org-capture-clock-keep (org-capture-get :clock-keep)) (if (equal goto 0) (org-capture-insert-template-here) (condition-case error (org-capture-place-template (equal (car ...) (quote function))) ((error quit) (if (and ... ...) (kill-buffer ...)) (set-window-configuration (org-capture-get :return-to-wconf)) (error "Capture template `%s': %s" (org-capture-get :key) (nth 1 error)))) (if (and (derived-mode-p (quote org-mode)) (org-capture-get :clock-in)) (condition-case nil (progn (if ... ...) (org-clock-in) (set ... t)) (error "Could not start the clock in this capture buffer"))) (if (org-capture-get :immediate-finish) (org-capture-finalize))))))
  (cond ((equal goto (quote (4))) (org-capture-goto-target)) ((equal goto (quote (16))) (org-capture-goto-last-stored)) (t (let* ((orig-buf (current-buffer)) (annotation (if (and (boundp ...) org-capture-link-is-already-stored) (plist-get org-store-link-plist :annotation) (condition-case nil (progn ...) (error nil)))) (entry (or org-capture-entry (org-capture-select-template keys))) initial) (setq initial (or org-capture-initial (and (org-region-active-p) (buffer-substring (point) (mark))))) (if (stringp initial) (progn (remove-text-properties 0 (length initial) (quote (read-only t)) initial))) (if (stringp annotation) (progn (remove-text-properties 0 (length annotation) (quote (read-only t)) annotation))) (cond ((equal entry "C") (customize-variable (quote org-capture-templates))) ((equal entry "q") (user-error "Abort")) (t (org-capture-set-plist entry) (org-capture-get-template) (org-capture-put :original-buffer orig-buf :original-file (or (buffer-file-name orig-buf) (and ... ...)) :original-file-nondirectory (and (buffer-file-name orig-buf) (file-name-nondirectory ...)) :annotation annotation :initial initial :return-to-wconf (current-window-configuration) :default-time (or org-overriding-default-time (org-current-time))) (org-capture-set-target-location) (condition-case error (org-capture-put :template (org-capture-fill-template)) ((error quit) (if ... ...) (error "Capture abort: %s" error))) (setq org-capture-clock-keep (org-capture-get :clock-keep)) (if (equal goto 0) (org-capture-insert-template-here) (condition-case error (org-capture-place-template ...) (... ... ... ...)) (if (and ... ...) (condition-case nil ... ...)) (if (org-capture-get :immediate-finish) (org-capture-finalize))))))))
  org-capture(nil)
  funcall-interactively(org-capture nil)
  call-interactively(org-capture record nil)
  command-execute(org-capture record)
  execute-extended-command(nil "org-capture" "org-capture")
  funcall-interactively(execute-extended-command nil "org-capture" "org-capture")
  call-interactively(execute-extended-command nil nil)
  command-execute(execute-extended-command)
--8<---------------cut here---------------end--------------->8---

and the /tmp/tasks.org file looks like this:


--8<---------------cut here---------------start------------->8---
* 2016
** 2016-07 July
*** 2016-07-18 Monday
**** TODO this is a test					    :testing:
     SCHEDULED: <2016-07-18 Mon>
   %?
--8<---------------cut here---------------end--------------->8---

The file did not exist before org-capture was invoked so we can see that
the majority of the template was invoked.

HTH,
eric

-- 
: Eric S Fraga (0xFFFCF67D), Emacs 25.0.94.1, Org release_8.3.4-1049-g481709