bug#25362: 26.0.50; comment-region goes into an infinite loop

bug#25362: 26.0.50; comment-region goes into an infinite loop

[Dima Kogan]

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

Hi. I'm using a very recent emacs built from master:

  eb3416016b4

I'm observing (comment-region) go into an infinite loop when presented
with particular data. I'm attaching a small source file that
demonstrates the issue. To see it break, run this:

    emacs --eval '(add-hook (quote c-mode-common-hook) (lambda () (setq comment-start "//" comment-end "")))'  \
          --eval '(global-set-key (kbd "<f5>") (lambda () (interactive) (comment-region 16 45)))'              \
          -Q tst6.c

This loads the demo file with a clean configuration, and runs two bits
of lisp:

1. Sets up a c++ commenting style. This is required for the issue to
   present itself

2. Binds f5 to run the problematic (comment-region)

On my machine, invoking emacs that way, and then hitting f5 shows the problem.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: tst6.c --]
[-- Type: text/x-csrc, Size: 47 bytes --]

void f(void)
{

    g( // output
      s );

}

bug#25362: 26.0.50; comment-region goes into an infinite loop

[Alan Mackenzie]

Hello, Dima.

On Wed, Jan 04, 2017 at 03:37:25PM -0800, Dima Kogan wrote:
> Hi. I'm using a very recent emacs built from master:

>   eb3416016b4

> I'm observing (comment-region) go into an infinite loop when presented
> with particular data. I'm attaching a small source file that
> demonstrates the issue. To see it break, run this:

>     emacs --eval '(add-hook (quote c-mode-common-hook) (lambda () (setq comment-start "//" comment-end "")))'  \
>           --eval '(global-set-key (kbd "<f5>") (lambda () (interactive) (comment-region 16 45)))'              \
>           -Q tst6.c

> This loads the demo file with a clean configuration, and runs two bits
> of lisp:

> 1. Sets up a c++ commenting style. This is required for the issue to
>    present itself

> 2. Binds f5 to run the problematic (comment-region)

> On my machine, invoking emacs that way, and then hitting f5 shows the problem.

Just as an aside, this bug happens even in Emacs 25.1 without any of the
settings you call it with.  Just visit the file, mark that region and do
C-c C-c.

The hang happens in comment-region-internal on attempting to insert "//
" to the first non-empty line in the range.  Something funny is
happening in a before/after-change-function or a font-lock function in
CC Mode.  I'll be looking into it.

Thanks.

> void f(void)
> {
>
>     g( // output
>       s );
>
> }

-- 
Alan Mackenzie (Nuremberg, Germany).

bug#25362: 26.0.50; comment-region goes into an infinite loop

[Eli Zaretskii]

> Date: Thu, 5 Jan 2017 20:54:52 +0000
> From: Alan Mackenzie <acm@muc.de>
> Cc: 25362@debbugs.gnu.org
> 
> Just as an aside, this bug happens even in Emacs 25.1 without any of the
> settings you call it with.  Just visit the file, mark that region and do
> C-c C-c.

Is this bug new in Emacs 25.1?

bug#25362: 26.0.50; comment-region goes into an infinite loop

[Alan Mackenzie]

Good morning, Eli.

On Fri, Jan 06, 2017 at 09:41:26AM +0200, Eli Zaretskii wrote:
> > Date: Thu, 5 Jan 2017 20:54:52 +0000
> > From: Alan Mackenzie <acm@muc.de>
> > Cc: 25362@debbugs.gnu.org

> > Just as an aside, this bug happens even in Emacs 25.1 without any of the
> > settings you call it with.  Just visit the file, mark that region and do
> > C-c C-c.

> Is this bug new in Emacs 25.1?

Yes, it is.  I've just tried the scenario out on Emacs 24.5, and it
works OK there.

-- 
Alan Mackenzie (Nuremberg, Germany).

bug#25362: 26.0.50; comment-region goes into an infinite loop

[Eli Zaretskii]

> Date: Fri, 6 Jan 2017 09:32:37 +0000
> Cc: dima@secretsauce.net, 25362@debbugs.gnu.org
> From: Alan Mackenzie <acm@muc.de>
> 
> > Is this bug new in Emacs 25.1?
> 
> Yes, it is.  I've just tried the scenario out on Emacs 24.5, and it
> works OK there.

Then it would be good to have a safe enough solution that we could
install on the release branch, or, failing that, a safe workaround.

Thanks.

bug#25362: 26.0.50; comment-region goes into an infinite loop

[Alan Mackenzie]

Hello again, Eli.

On Fri, Jan 06, 2017 at 11:37:12AM +0200, Eli Zaretskii wrote:
> > Date: Fri, 6 Jan 2017 09:32:37 +0000
> > Cc: dima@secretsauce.net, 25362@debbugs.gnu.org
> > From: Alan Mackenzie <acm@muc.de>

> > > Is this bug new in Emacs 25.1?

> > Yes, it is.  I've just tried the scenario out on Emacs 24.5, and it
> > works OK there.

> Then it would be good to have a safe enough solution that we could
> install on the release branch, or, failing that, a safe workaround.

I'm working on it at the moment.  It's a bit of a tricky one, because
the cause is triggering during both command processing and redisplay.
But I hope to be in touch again soon with a fix.

> Thanks.

-- 
Alan Mackenzie (Nuremberg, Germany).

bug#25362: 26.0.50; comment-region goes into an infinite loop

[Alan Mackenzie]

Hello, Eli.

An update.

On Fri, Jan 06, 2017 at 11:37:12AM +0200, Eli Zaretskii wrote:
> > Date: Fri, 6 Jan 2017 09:32:37 +0000
> > Cc: dima@secretsauce.net, 25362@debbugs.gnu.org
> > From: Alan Mackenzie <acm@muc.de>

> > > Is this bug new in Emacs 25.1?

> > Yes, it is.  I've just tried the scenario out on Emacs 24.5, and it
> > works OK there.

> Then it would be good to have a safe enough solution that we could
> install on the release branch, or, failing that, a safe workaround.

There are actually two distinct bugs at work, here.  One of them is
simple, the fix being merely giving a limit to an invocation of
c-forward-syntactic-ws.

The other bug is more involved.  It involves the invalidation of the
whitespace cache, which is currently done incorrectly when "//" is
inserted into a line which already has comment markers later on that
line.  It should be simple to fix, but is actually quite tricky, with a
likelihood of unwanted side effects if it's not done very carefully.  I
might be able to fix it today, but I'm not certain of that.

I've done a bisection on the repository, and the commit that
"introduced" the bug didn't really introduce it at all - it merely
uncovered it.  That commit was 4b9ac23960d2998f899287ffcf696ad33b63a69a
from 2016-07-03, "Speed up CC Mode fontification with less accurate
functions extending region".

As a summary, I'm not convinced that the fix I'm working on ought to go
into 25.2, even though the bug is quite serious.  Maybe that will
clarify itself today, sometime.

> Thanks.

-- 
Alan Mackenzie (Nuremberg, Germany).

bug#25362: 26.0.50; comment-region goes into an infinite loop

[Eli Zaretskii]

> Date: Sat, 7 Jan 2017 08:20:23 +0000
> Cc: dima@secretsauce.net, 25362@debbugs.gnu.org
> From: Alan Mackenzie <acm@muc.de>
> 
> There are actually two distinct bugs at work, here.  One of them is
> simple, the fix being merely giving a limit to an invocation of
> c-forward-syntactic-ws.

Does doing that prevent the infloop?

> The other bug is more involved.  It involves the invalidation of the
> whitespace cache, which is currently done incorrectly when "//" is
> inserted into a line which already has comment markers later on that
> line.  It should be simple to fix, but is actually quite tricky, with a
> likelihood of unwanted side effects if it's not done very carefully.  I
> might be able to fix it today, but I'm not certain of that.
> 
> I've done a bisection on the repository, and the commit that
> "introduced" the bug didn't really introduce it at all - it merely
> uncovered it.  That commit was 4b9ac23960d2998f899287ffcf696ad33b63a69a
> from 2016-07-03, "Speed up CC Mode fontification with less accurate
> functions extending region".

If no better solution emerges, would it make sense to revert 4b9ac23
on the release branch, as a temporary workaround that prevents the
infloop?

Thanks.

bug#25362: 26.0.50; comment-region goes into an infinite loop

[Alan Mackenzie]

Hello, Eli.

On Sat, Jan 07, 2017 at 11:44:34AM +0200, Eli Zaretskii wrote:
> > Date: Sat, 7 Jan 2017 08:20:23 +0000
> > Cc: dima@secretsauce.net, 25362@debbugs.gnu.org
> > From: Alan Mackenzie <acm@muc.de>

> > There are actually two distinct bugs at work, here.  One of them is
> > simple, the fix being merely giving a limit to an invocation of
> > c-forward-syntactic-ws.

> Does doing that prevent the infloop?

It prevents one infloop, but not the other one, which happens after the
first one is fixed.  

> > The other bug is more involved.  It involves the invalidation of the
> > whitespace cache, which is currently done incorrectly when "//" is
> > inserted into a line which already has comment markers later on that
> > line.  It should be simple to fix, but is actually quite tricky, with a
> > likelihood of unwanted side effects if it's not done very carefully.  I
> > might be able to fix it today, but I'm not certain of that.

> > I've done a bisection on the repository, and the commit that
> > "introduced" the bug didn't really introduce it at all - it merely
> > uncovered it.  That commit was 4b9ac23960d2998f899287ffcf696ad33b63a69a
> > from 2016-07-03, "Speed up CC Mode fontification with less accurate
> > functions extending region".

> If no better solution emerges, would it make sense to revert 4b9ac23
> on the release branch, as a temporary workaround that prevents the
> infloop?

Apologies, that commit is only in master, not the emacs-25 branch.  I'll
need to do some more careful bisection.

I'll get back to you later.

> Thanks.

-- 
Alan Mackenzie (Nuremberg, Germany).

bug#25362: 26.0.50; comment-region goes into an infinite loop

[Eli Zaretskii]

> Date: Sat, 7 Jan 2017 16:29:08 +0000
> From: Alan Mackenzie <acm@muc.de>
> 
> Sorry, I've made a mistake with my testing, somehow, probably by
> forgetting -Q.  Dima's test runs flawlessly on Emacs 25.1, and also runs
> flawlessly on the pretest 25.1.91, both being started with -Q.  It also
> runs flawlessly on the emacs-25 branch, updated ~20 minutes ago.

Thanks, this means Emacs 25.2 is off the hook.

> I believe that 4b9ac23 was indeed the change that uncovered this bug in
> master.  I'll carry on working on it in the master branch.

Thank you.

bug#25362: 26.0.50; comment-region goes into an infinite loop

[Alan Mackenzie]

Hello again, Dima.

On Wed, Jan 04, 2017 at 03:37:25PM -0800, Dima Kogan wrote:
> Hi. I'm using a very recent emacs built from master:

>   eb3416016b4

> I'm observing (comment-region) go into an infinite loop when presented
> with particular data. I'm attaching a small source file that
> demonstrates the issue. To see it break, run this:

>     emacs --eval '(add-hook (quote c-mode-common-hook) (lambda () (setq comment-start "//" comment-end "")))'  \
>           --eval '(global-set-key (kbd "<f5>") (lambda () (interactive) (comment-region 16 45)))'              \
>           -Q tst6.c

> This loads the demo file with a clean configuration, and runs two bits
> of lisp:

> 1. Sets up a c++ commenting style. This is required for the issue to
>    present itself

> 2. Binds f5 to run the problematic (comment-region)

> On my machine, invoking emacs that way, and then hitting f5 shows the problem.


> void f(void)
> {
> 
>     g( // output
>       s );
> 
> }

As already observed, this bug happens simply in C Mode by attempting to
comment out the four lines inside the function, doing this by marking
the lines then C-c C-c.

The cause was a rather nasty situation where a "syntactic whitespace
cache", designed to facilitate moving rapidly over large areas of WS
(particularly long comments at the beginning of files) got screwed up by
the insertion of "// " onto a line already containing a "//" later on.

Here is a patch.  It is not yet in its final form (the comments need
fixing).  Would you please try it out and let me know how well/how badly
it works:



diff -r a3e0a2a33629 cc-engine.el
--- a/cc-engine.el	Sat Dec 31 16:59:41 2016 +0000
+++ b/cc-engine.el	Mon Jan 09 21:03:52 2017 +0000
@@ -1712,46 +1712,122 @@
 	 `((c-debug-remove-face beg end 'c-debug-is-sws-face)
 	   (c-debug-remove-face beg end 'c-debug-in-sws-face)))))
 
-(defsubst c-invalidate-sws-region-after (beg end)
+;; The type of literal position `end' is in in a `before-change-functions'
+;; function - one of `c', `c++', `string', `pound', or nil.
+(defvar c-sws-lit-type nil)
+;; A cons (BEG . END) of the literal/CPP construct enclosing END, if any, else
+;; nil.
+(defvar c-sws-lit-limits nil)
+
+(defun c-invalidate-sws-region-before (end)
+  ;; Called from c-before-change.  END is the end of the change region, the
+  ;; standard parameter given to all before-change-functions.
+  ;;
+  ;; Note whether END is inside a comment of CPP construct, and if so note its
+  ;; bounds.
+  (save-excursion
+    (goto-char end)
+    (let* ((limits (c-literal-limits))
+	   (lit-type (c-literal-type limits)))
+      (cond
+       ((memq lit-type '(c c++))
+	(setq c-sws-lit-type lit-type
+	      c-sws-lit-limits limits))
+       ((c-beginning-of-macro)
+	(setq c-sws-lit-type 'pound
+	      c-sws-lit-limits (cons (point)
+				     (progn (c-end-of-macro) (point)))))
+       (t (setq c-sws-lit-type nil
+		c-sws-lit-limits nil))))))
+
+(defun c-invalidate-sws-region-after-del (beg end old-len)
+  ;; Text has been deleted, OLD-LEN characters of it starting from position
+  ;; BEG.  END is typically eq to BEG.  Should there have been a comment or
+  ;; CPP construct open at END before the deletion, check whether this
+  ;; deletion deleted or "damaged" its opening delimiter.  If so, return the
+  ;; current position of where the construct ended, otherwise return nil.
+  (when c-sws-lit-limits
+    (setcdr c-sws-lit-limits (- (cdr c-sws-lit-limits) old-len))
+    (if (and (< beg (+ (car c-sws-lit-limits) 2))
+	     (or (get-text-property end 'c-in-sws)
+		 (next-single-property-change end 'c-in-sws nil
+					      (cdr c-sws-lit-limits))
+		 (get-text-property end 'c-is-sws)
+		 (next-single-property-change end 'c-is-sws nil
+					      (cdr c-sws-lit-limits))))
+	(cdr c-sws-lit-limits))))
+
+(defun c-invalidate-sws-region-after-ins (end)
+  ;; Text has been inserted, ending at buffer position END.  Should there be a
+  ;; literal or CPP construct open at END, check whether there are `c-in-sws'
+  ;; or `c-is-sws' text properties inside this literal.  If there are, return
+  ;; the buffer position of the end of the literal, else return nil.
+  (save-excursion
+    (let* ((limits (c-literal-limits))
+	   (lit-type (c-literal-type limits)))
+      (goto-char end)
+      (when (and (not (memq lit-type '(c c++)))
+		 (c-beginning-of-macro))
+	(setq lit-type 'pound
+	      limits (cons (point)
+			   (progn (c-end-of-macro) (point)))))
+      (when (memq lit-type '(c c++ pound))
+	(let ((next-in (next-single-property-change (car limits) 'c-in-sws
+						    nil (cdr limits)))
+	      (next-is (next-single-property-change (car limits) 'c-is-sws
+						    nil (cdr limits))))
+	  (and (or next-in next-is)
+	       (cdr limits)))))))
+
+(defun c-invalidate-sws-region-after (beg end old-len)
   ;; Called from `after-change-functions'.  Note that if
   ;; `c-forward-sws' or `c-backward-sws' are used outside
   ;; `c-save-buffer-state' or similar then this will remove the cache
   ;; properties right after they're added.
   ;;
   ;; This function does hidden buffer changes.
-
-  (save-excursion
-    ;; Adjust the end to remove the properties in any following simple
-    ;; ws up to and including the next line break, if there is any
-    ;; after the changed region. This is necessary e.g. when a rung
-    ;; marked empty line is converted to a line comment by inserting
-    ;; "//" before the line break. In that case the line break would
-    ;; keep the rung mark which could make a later `c-backward-sws'
-    ;; move into the line comment instead of over it.
-    (goto-char end)
-    (skip-chars-forward " \t\f\v")
-    (when (and (eolp) (not (eobp)))
-      (setq end (1+ (point)))))
-
-  (when (and (= beg end)
-	     (get-text-property beg 'c-in-sws)
-	     (> beg (point-min))
-	     (get-text-property (1- beg) 'c-in-sws))
-    ;; Ensure that an `c-in-sws' range gets broken.  Note that it isn't
-    ;; safe to keep a range that was continuous before the change.  E.g:
-    ;;
-    ;;    #define foo
-    ;;         \
-    ;;    bar
-    ;;
-    ;; There can be a "ladder" between "#" and "b".  Now, if the newline
-    ;; after "foo" is removed then "bar" will become part of the cpp
-    ;; directive instead of a syntactically relevant token.  In that
-    ;; case there's no longer syntactic ws from "#" to "b".
-    (setq beg (1- beg)))
-
-  (c-debug-sws-msg "c-invalidate-sws-region-after [%s..%s]" beg end)
-  (c-remove-is-and-in-sws beg end))
+  (let ((del-end
+	 (and (> old-len 0)
+	      (c-invalidate-sws-region-after-del beg end old-len)))
+	(ins-end
+	 (and (> end beg)
+	      (c-invalidate-sws-region-after-ins end))))
+    (save-excursion
+      ;; Adjust the end to remove the properties in any following simple
+      ;; ws up to and including the next line break, if there is any
+      ;; after the changed region. This is necessary e.g. when a rung
+      ;; marked empty line is converted to a line comment by inserting
+      ;; "//" before the line break. In that case the line break would
+      ;; keep the rung mark which could make a later `c-backward-sws'
+      ;; move into the line comment instead of over it.
+      (goto-char end)
+      (skip-chars-forward " \t\f\v")
+      (when (and (eolp) (not (eobp)))
+	(setq end (1+ (point)))))
+
+    (when (and (= beg end)
+	       (get-text-property beg 'c-in-sws)
+	       (> beg (point-min))
+	       (get-text-property (1- beg) 'c-in-sws))
+      ;; Ensure that an `c-in-sws' range gets broken.  Note that it isn't
+      ;; safe to keep a range that was continuous before the change.  E.g:
+      ;;
+      ;;    #define foo
+      ;;         \
+      ;;    bar
+      ;;
+      ;; There can be a "ladder" between "#" and "b".  Now, if the newline
+      ;; after "foo" is removed then "bar" will become part of the cpp
+      ;; directive instead of a syntactically relevant token.  In that
+      ;; case there's no longer syntactic ws from "#" to "b".
+      (setq beg (1- beg)))
+
+    (setq end (max (or del-end end)
+		   (or ins-end end)
+		   end))
+
+    (c-debug-sws-msg "c-invalidate-sws-region-after [%s..%s]" beg end)
+    (c-remove-is-and-in-sws beg end)))
 
 (defun c-forward-sws ()
   ;; Used by `c-forward-syntactic-ws' to implement the unbounded search.
diff -r a3e0a2a33629 cc-langs.el
--- a/cc-langs.el	Sat Dec 31 16:59:41 2016 +0000
+++ b/cc-langs.el	Mon Jan 09 21:03:52 2017 +0000
@@ -1428,6 +1428,15 @@
   t    "*/"
   awk  nil)
 
+(c-lang-defconst c-block-comment-ender-regexp
+  ;; Regexp which matches the end of a block comment (if such exists in the
+  ;; language)
+  t (if (c-lang-const c-block-comment-ender)
+	(regexp-quote (c-lang-const c-block-comment-ender))
+      "\\<\\>"))
+(c-lang-defvar c-block-comment-ender-regexp
+	       (c-lang-const c-block-comment-ender-regexp))
+
 (c-lang-defconst c-comment-start-regexp
   ;; Regexp to match the start of any type of comment.
   t (let ((re (c-make-keywords-re nil
diff -r a3e0a2a33629 cc-mode.el
--- a/cc-mode.el	Sat Dec 31 16:59:41 2016 +0000
+++ b/cc-mode.el	Mon Jan 09 21:03:52 2017 +0000
@@ -1189,6 +1189,7 @@
 	  ;; Are we coalescing two tokens together, e.g. "fo o" -> "foo"?
 	  (when (< beg end)
 	    (c-unfind-coalesced-tokens beg end))
+	  (c-invalidate-sws-region-before end)
 	  ;; Are we (potentially) disrupting the syntactic context which
 	  ;; makes a type a type?  E.g. by inserting stuff after "foo" in
 	  ;; "foo bar;", or before "foo" in "typedef foo *bar;"?
@@ -1314,7 +1315,7 @@
 	      (c-clear-char-property-with-value beg end 'syntax-table nil)))
 
 	  (c-trim-found-types beg end old-len) ; maybe we don't need all of these.
-	  (c-invalidate-sws-region-after beg end)
+	  (c-invalidate-sws-region-after beg end old-len)
 	  ;; (c-invalidate-state-cache beg) ; moved to `c-before-change'.
 	  (c-invalidate-find-decl-cache beg)
 


Thanks for this rather obscure bug report.  :-)

-- 
Alan Mackenzie (Nuremberg, Germany).

bug#25362: 26.0.50; comment-region goes into an infinite loop

[Dima Kogan]

Alan Mackenzie <acm@muc.de> writes:

> Hello again, Dima.
>
> As already observed, this bug happens simply in C Mode by attempting to
> comment out the four lines inside the function, doing this by marking
> the lines then C-c C-c.
>
> The cause was a rather nasty situation where a "syntactic whitespace
> cache", designed to facilitate moving rapidly over large areas of WS
> (particularly long comments at the beginning of files) got screwed up by
> the insertion of "// " onto a line already containing a "//" later on.
>
> Here is a patch.  It is not yet in its final form (the comments need
> fixing).  Would you please try it out and let me know how well/how badly
> it works:

Hi. I'm now running with this patch, and some cursory testing says that
it works. I briefly ran into an issue where I

1. highlighted a problematic region
2. hit M-; (this worked correctly)
3. Hit M-/ to undo

This undo restored the text correctly, but the whole region remained
highlighted as a comment, even after the undo removed the comment
characters. I hit this once, but cannot reproduce it anymore. If this
description means something to you, feel free to act on it :)


> Thanks for this rather obscure bug report.  :-)

Heh. I just report issues that pop up during the course of the day. We
owe you hugely for responding to these bugs and maintaining cc-mode.
Thanks!

bug#25362: 26.0.50; comment-region goes into an infinite loop

[Alan Mackenzie]

Hello, Dima

On Mon, Jan 09, 2017 at 01:46:28PM -0800, Dima Kogan wrote:
> Alan Mackenzie <acm@muc.de> writes:

> > As already observed, this bug happens simply in C Mode by attempting to
> > comment out the four lines inside the function, doing this by marking
> > the lines then C-c C-c.

> > The cause was a rather nasty situation where a "syntactic whitespace
> > cache", designed to facilitate moving rapidly over large areas of WS
> > (particularly long comments at the beginning of files) got screwed up by
> > the insertion of "// " onto a line already containing a "//" later on.

> > Here is a patch.  It is not yet in its final form (the comments need
> > fixing).  Would you please try it out and let me know how well/how badly
> > it works:

> Hi. I'm now running with this patch, and some cursory testing says that
> it works. I briefly ran into an issue where I

> 1. highlighted a problematic region
> 2. hit M-; (this worked correctly)
> 3. Hit M-/ to undo

> This undo restored the text correctly, but the whole region remained
> highlighted as a comment, even after the undo removed the comment
> characters. I hit this once, but cannot reproduce it anymore. If this
> description means something to you, feel free to act on it :)

I'll have a think about this some time (it's late at night in Europe,
now).

> > Thanks for this rather obscure bug report.  :-)

> Heh. I just report issues that pop up during the course of the day. We
> owe you hugely for responding to these bugs and maintaining cc-mode.
> Thanks!

That's appreciated!

-- 
Alan Mackenzie (Nuremberg, Germany).

bug#25362: 26.0.50; comment-region goes into an infinite loop

[Alan Mackenzie]

Bug fixed.  Closing.

-- 
Alan Mackenzie (Nuremberg, Germany).