bug#14281: 24.3; replace-match leaves point at wrong place

[Barry OReilly <gundaetiapo@gmail.com>]

[-- Attachment #1.1: Type: text/plain, Size: 2114 bytes --]

> Can you show us the offenders?

Offenders and status:
   semantic-change-function      Fixed in CEDET mainline
   evil-ex-search-update-pattern Fixed in Evil mainline
   evil-track-last-insertion     See below
   Undo tree                     Haven't investigated

I'm not so sure search_regs are saved and restored through save-match-data.
I have this advice:

(defadvice evil-track-last-insertion (around
my-advice-evil-track-last-insertion activate)
  (my-msg "DEBUG: 01 evil-track-last-insertion match-data=%s
match-beginning=%s" (match-data) (match-beginning 0))
  (save-match-data ad-do-it)
  (my-msg "DEBUG: 02 evil-track-last-insertion match-data=%s
match-beginning=%s" (match-data) (match-beginning 0))
  )

Note: my-msg is like message but prefixes the time.

When I inserted a new line in a .cc file, I get:

2013-05-14T12:51:39.010590 DEBUG: 01 evil-track-last-insertion
match-data=(#<marker at 1 in FileNameRedacted.cc> #<marker at 5 in
FileNameRedacted.cc> #<marker at 4 in FileNameRedacted.cc> #<marker at 5 in
FileNameRedacted.cc>) match-beginning=0
2013-05-14T12:51:39.020153 DEBUG: 02 evil-track-last-insertion
match-data=(#<marker at 1 in FileNameRedacted.cc> #<marker at 5 in
FileNameRedacted.cc> #<marker at 4 in FileNameRedacted.cc> #<marker at 5 in
FileNameRedacted.cc>) match-beginning=1
ad-handle-definition: `semantic-change-function' got redefined
2013-05-14T12:51:39.023610 DEBUG: 01 semantic-change-function
match-beginning=1
2013-05-14T12:51:39.025341 DEBUG: 02 semantic-change-function
match-beginning=1
2013-05-14T12:51:39.025525 DEBUG: 01 c-after-change match-beginning=1
2013-05-14T12:51:39.026920 DEBUG: 02 c-after-change match-beginning=1
2013-05-14T12:51:39.027110 DEBUG: 01 jit-lock-after-change match-beginning=1
2013-05-14T12:51:39.027319 DEBUG: 02 jit-lock-after-change match-beginning=1
newline: Match data corrupted in after-change-functions. Consider using
save-match-data

In this case, the change of match-beginning from 0 to 1 in
evil-track-last-insertion causes the error.

For reference, I am currently using the attached patch, which incorporates
your feedback.

[-- Attachment #1.2: Type: text/html, Size: 2323 bytes --]

[-- Attachment #2: check-match-beginning.diff --]
[-- Type: application/octet-stream, Size: 5325 bytes --]

diff --git a/src/insdel.c b/src/insdel.c
index 8029291..db2147f 100644
--- a/src/insdel.c
+++ b/src/insdel.c
@@ -28,6 +28,7 @@ along with GNU Emacs.  If not, see <http://www.gnu.org/licenses/>.  */
 #include "buffer.h"
 #include "window.h"
 #include "blockinput.h"
+#include "regex.h"
 #include "region-cache.h"
 
 static void insert_from_string_1 (Lisp_Object, ptrdiff_t, ptrdiff_t, ptrdiff_t,
@@ -2034,6 +2035,14 @@ signal_after_change (ptrdiff_t charpos, ptrdiff_t lendel, ptrdiff_t lenins)
 
   specbind (Qinhibit_modification_hooks, Qt);
 
+  /* Change hook functions are expected to use save-match-data if using regexps.
+     This checks match-beginning to detect cases of hooks corrupting match-data.
+     TODO: Implement in signal_before_change too
+   */
+  Lisp_Object match_beginning = 0 < search_regs.num_regs ?
+                                make_number (search_regs.start[0]) :
+                                Qnil;
+
   if (!NILP (Vafter_change_functions))
     {
       Lisp_Object args[4];
@@ -2053,6 +2062,12 @@ signal_after_change (ptrdiff_t charpos, ptrdiff_t lendel, ptrdiff_t lenins)
       XSETCDR (rvoe_arg, Qt);
     }
 
+  if ( ! EQ (match_beginning, 0 < search_regs.num_regs ?
+                              make_number (search_regs.start[0]) :
+                              Qnil))
+    error ("Match data corrupted in after-change-functions. "
+           "Consider using save-match-data");
+
   if (buffer_has_overlays ())
     report_overlay_modification (make_number (charpos),
 				 make_number (charpos + lenins),
@@ -2061,12 +2076,24 @@ signal_after_change (ptrdiff_t charpos, ptrdiff_t lendel, ptrdiff_t lenins)
 				 make_number (charpos + lenins),
 				 make_number (lendel));
 
+  if ( ! EQ (match_beginning, 0 < search_regs.num_regs ?
+                              make_number (search_regs.start[0]) :
+                              Qnil))
+    error ("Match data corrupted in overlay modification hooks. "
+           "Consider using save-match-data");
+
   /* After an insertion, call the text properties
      insert-behind-hooks or insert-in-front-hooks.  */
   if (lendel == 0)
     report_interval_modification (make_number (charpos),
 				  make_number (charpos + lenins));
 
+  if ( ! EQ (match_beginning, 0 < search_regs.num_regs ?
+                              make_number (search_regs.start[0]) :
+                              Qnil))
+    error ("Match data corrupted in text property insertion hooks. "
+           "Consider using save-match-data");
+
   unbind_to (count, Qnil);
 }
 
diff --git a/src/regex.h b/src/regex.h
index 8fe7ba1..b4833dc 100644
--- a/src/regex.h
+++ b/src/regex.h
@@ -429,6 +429,24 @@ struct re_registers
   regoff_t *end;
 };
 
+/* Every call to re_match, etc., must pass &search_regs as the regs
+   argument unless you can show it is unnecessary (i.e., if re_match
+   is certainly going to be called again before region-around-match
+   can be called).
+
+   Since the registers are now dynamically allocated, we need to make
+   sure not to refer to the Nth register before checking that it has
+   been allocated by checking search_regs.num_regs.
+
+   The regex code keeps track of whether it has allocated the search
+   buffer using bits in the re_pattern_buffer.  This means that whenever
+   you compile a new pattern, it completely forgets whether it has
+   allocated any registers, and will allocate new registers the next
+   time you call a searching or matching function.  Therefore, we need
+   to call re_set_registers after compiling a new pattern or after
+   setting the match registers, so that the regex functions will be
+   able to free or re-allocate it properly.  */
+extern struct re_registers search_regs;
 
 /* If `regs_allocated' is REGS_UNALLOCATED in the pattern buffer,
    `re_match_2' returns information about at least this many registers
diff --git a/src/search.c b/src/search.c
index ea36133..ffbea28 100644
--- a/src/search.c
+++ b/src/search.c
@@ -59,25 +59,7 @@ static struct regexp_cache searchbufs[REGEXP_CACHE_SIZE];
 /* The head of the linked list; points to the most recently used buffer.  */
 static struct regexp_cache *searchbuf_head;
 
-
-/* Every call to re_match, etc., must pass &search_regs as the regs
-   argument unless you can show it is unnecessary (i.e., if re_match
-   is certainly going to be called again before region-around-match
-   can be called).
-
-   Since the registers are now dynamically allocated, we need to make
-   sure not to refer to the Nth register before checking that it has
-   been allocated by checking search_regs.num_regs.
-
-   The regex code keeps track of whether it has allocated the search
-   buffer using bits in the re_pattern_buffer.  This means that whenever
-   you compile a new pattern, it completely forgets whether it has
-   allocated any registers, and will allocate new registers the next
-   time you call a searching or matching function.  Therefore, we need
-   to call re_set_registers after compiling a new pattern or after
-   setting the match registers, so that the regex functions will be
-   able to free or re-allocate it properly.  */
-static struct re_registers search_regs;
+struct re_registers search_regs;
 
 /* The buffer in which the last search was performed, or
    Qt if the last search was done in a string;