From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.org!not-for-mail From: Eli Zaretskii Newsgroups: gmane.emacs.bugs Subject: bug#11417: 24.0.96; infinite looping in xdisp.c Date: Sat, 12 May 2012 13:45:05 +0300 Message-ID: <83k40hy8y6.fsf@gnu.org> References: <83havt4716.fsf@gnu.org> <83havr3nv2.fsf@gnu.org> <837gwn3ihi.fsf@gnu.org> <87havrcpi1.fsf@gnu.org> <83bolwyls1.fsf@gnu.org> Reply-To: Eli Zaretskii NNTP-Posting-Host: plane.gmane.org X-Trace: dough.gmane.org 1336819442 26027 80.91.229.3 (12 May 2012 10:44:02 GMT) X-Complaints-To: usenet@dough.gmane.org NNTP-Posting-Date: Sat, 12 May 2012 10:44:02 +0000 (UTC) Cc: 11417@debbugs.gnu.org To: sdl.web@gmail.com, cyd@gnu.org Original-X-From: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane.org@gnu.org Sat May 12 12:44:01 2012 Return-path: Envelope-to: geb-bug-gnu-emacs@m.gmane.org Original-Received: from lists.gnu.org ([208.118.235.17]) by plane.gmane.org with esmtp (Exim 4.69) (envelope-from ) id 1ST9ns-0002nK-OH for geb-bug-gnu-emacs@m.gmane.org; Sat, 12 May 2012 12:44:00 +0200 Original-Received: from localhost ([::1]:39619 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ST9ns-0001oB-3n for geb-bug-gnu-emacs@m.gmane.org; Sat, 12 May 2012 06:44:00 -0400 Original-Received: from eggs.gnu.org ([208.118.235.92]:40296) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ST9no-0001nv-5a for bug-gnu-emacs@gnu.org; Sat, 12 May 2012 06:43:58 -0400 Original-Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ST9nl-0003nL-MN for bug-gnu-emacs@gnu.org; Sat, 12 May 2012 06:43:55 -0400 Original-Received: from debbugs.gnu.org ([140.186.70.43]:55749) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ST9nl-0003n9-Ij for bug-gnu-emacs@gnu.org; Sat, 12 May 2012 06:43:53 -0400 Original-Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.72) (envelope-from ) id 1ST9nu-0003WK-Hi for bug-gnu-emacs@gnu.org; Sat, 12 May 2012 06:44:02 -0400 X-Loop: help-debbugs@gnu.org Resent-From: Eli Zaretskii Original-Sender: debbugs-submit-bounces@debbugs.gnu.org Resent-CC: bug-gnu-emacs@gnu.org Resent-Date: Sat, 12 May 2012 10:44:02 +0000 Resent-Message-ID: Resent-Sender: help-debbugs@gnu.org X-GNU-PR-Message: followup 11417 X-GNU-PR-Package: emacs X-GNU-PR-Keywords: Original-Received: via spool by 11417-submit@debbugs.gnu.org id=B11417.133681942613501 (code B ref 11417); Sat, 12 May 2012 10:44:02 +0000 Original-Received: (at 11417) by debbugs.gnu.org; 12 May 2012 10:43:46 +0000 Original-Received: from localhost ([127.0.0.1]:49820 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.72) (envelope-from ) id 1ST9ne-0003Vi-3s for submit@debbugs.gnu.org; Sat, 12 May 2012 06:43:46 -0400 Original-Received: from mtaout21.012.net.il ([80.179.55.169]:54722) by debbugs.gnu.org with esmtp (Exim 4.72) (envelope-from ) id 1ST9nI-0003VA-D4 for 11417@debbugs.gnu.org; Sat, 12 May 2012 06:43:44 -0400 Original-Received: from conversion-daemon.a-mtaout21.012.net.il by a-mtaout21.012.net.il (HyperSendmail v2007.08) id <0M3W00F00NPLBA00@a-mtaout21.012.net.il> for 11417@debbugs.gnu.org; Sat, 12 May 2012 13:43:01 +0300 (IDT) Original-Received: from HOME-C4E4A596F7 ([87.69.210.75]) by a-mtaout21.012.net.il (HyperSendmail v2007.08) with ESMTPA id <0M3W00FZ9OFOEV40@a-mtaout21.012.net.il>; Sat, 12 May 2012 13:43:01 +0300 (IDT) In-reply-to: <83bolwyls1.fsf@gnu.org> X-012-Sender: halo1@inter.net.il X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.13 Precedence: list X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.6 (newer, 2) X-Received-From: 140.186.70.43 X-BeenThere: bug-gnu-emacs@gnu.org List-Id: "Bug reports for GNU Emacs, the Swiss army knife of text editors" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane.org@gnu.org Original-Sender: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane.org@gnu.org Xref: news.gmane.org gmane.emacs.bugs:59955 Archived-At: > Date: Thu, 10 May 2012 20:43:26 +0300 > From: Eli Zaretskii > Cc: cyd@gnu.org, 11417@debbugs.gnu.org > > emacs -Q -nw > C-x C-f xdisp.c RET > M-: (let ((ov (make-overlay 4928 4933 nil t t)) (fringe (propertize "!" 'display (list 'left-fringe 'question-mark)))) (overlay-put ov 'before-string fringe)) RET > > (The last one is a single long line to type into the minibuffer.) > > Redisplay only shows part of the screen after that, but Emacs doesn't > yet infloop. Move a cursor a bit, and it will. > > Of course, the choice of the file (xdisp.c) and the position where to > put the overlay are arbitrary. > > This only happens in 'emacs -nw", a GUI session (which can actually > display the fringe bitmap) doesn't have any problems with this recipe. > > I will work on fixing this. The immediate cause of this bug is that the display iterator would return zero at the end of the overlay string, and the display engine would take that as a sign that it is done with redisplay, instead of popping the stack and continuing to display the rest. Unfortunately, fixing this and a couple of other blunders exposed by the fix requires changes in places that are heavily used by the display engine, and are not limited to left-fringe/right-fringe display specs on a TTY alone. See the diffs below. They seem quite simple, almost no-brainers, to me, but I've been wrong before when the innermost bowels of the display engine are concerned. So I'm not sure whether to apply this to the emacs-24 branch or the trunk. Here are the pros and cons for applying to the branch: . Pros: . The changes are by themselves quite simple. . They fix a number of problems that could bite us in other use cases: . the fact that left-fringe/right-fringe display property was incorrectly reported to the rest of the code as non-replacing, whereas in fact it was (the underlying text is not displayed); . incorrect condition in iterate_out_of_display_property for determining whether we are iterating a string or a buffer; . improper termination of display when there's more stuff on the iterator stack; . call to get_overlay_strings_1 without a check that we already have overlay strings loaded. . Cons: . This problem existed since v22.1, where left-fringe/right-fringe was first introduced. . Features that display bitmaps on the fringe generally need to treat the TTY case specially anyway. This bug was exposed because Leo's code didn't. . Some of the changes touch very sensitive parts of the display engine on its lowest level: the iteration itself. . Prudence would suggest another pretest, should these changes be applied to the branch. So I will await decision by Stefan and Chong before committing this. In the meantime, Leo, please see that this fixes your problem, and doesn't introduce any new ones. TIA. Here are the changes: === modified file 'src/xdisp.c' --- src/xdisp.c 2012-05-11 14:05:06 +0000 +++ src/xdisp.c 2012-05-12 10:03:08 +0000 @@ -839,6 +839,7 @@ static int try_cursor_movement (Lisp_Obj static int trailing_whitespace_p (EMACS_INT); static intmax_t message_log_check_duplicate (EMACS_INT, EMACS_INT); static void push_it (struct it *, struct text_pos *); +static void iterate_out_of_display_property (struct it *); static void pop_it (struct it *); static void sync_frame_with_window_matrix_rows (struct window *); static void select_frame_for_redisplay (Lisp_Object); @@ -3125,7 +3126,15 @@ handle_stop (struct it *it) overlays even if the actual buffer text is replaced. */ if (!handle_overlay_change_p || it->sp > 1 - || !get_overlay_strings_1 (it, 0, 0)) + /* Don't call get_overlay_strings_1 if we already + have overlay strings loaded, because doing so + will load them again and push the iterator state + onto the stack one more time, which is not + expected by the rest of the code that processes + overlay strings. */ + || (it->n_overlay_strings <= 0 + ? !get_overlay_strings_1 (it, 0, 0) + : 0)) { if (it->ellipsis_p) setup_for_ellipsis (it, 0); @@ -4681,7 +4690,19 @@ handle_single_display_spec (struct it *i if (!FRAME_WINDOW_P (it->f)) /* If we return here, POSITION has been advanced across the text with this property. */ - return 1; + { + /* Synchronize the bidi iterator with POSITION. This is + needed because we are not going to push the iterator + on behalf of this display property, so there will be + no pop_it call to do this synchronization for us. */ + if (it->bidi_p) + { + it->position = *position; + iterate_out_of_display_property (it); + *position = it->position; + } + return 1; + } } else if (!frame_window_p) return 1; @@ -4692,7 +4713,15 @@ handle_single_display_spec (struct it *i || !(fringe_bitmap = lookup_fringe_bitmap (value))) /* If we return here, POSITION has been advanced across the text with this property. */ - return 1; + { + if (it && it->bidi_p) + { + it->position = *position; + iterate_out_of_display_property (it); + *position = it->position; + } + return 1; + } if (it) { @@ -5611,7 +5640,7 @@ push_it (struct it *it, struct text_pos static void iterate_out_of_display_property (struct it *it) { - int buffer_p = BUFFERP (it->object); + int buffer_p = !STRINGP (it->string); EMACS_INT eob = (buffer_p ? ZV : it->end_charpos); EMACS_INT bob = (buffer_p ? BEGV : 0); @@ -6780,6 +6809,16 @@ get_next_display_element (struct it *it) && FACE_FROM_ID (it->f, face_id)->box == FACE_NO_BOX); } } + /* If we reached the end of the object we've been iterating (e.g., a + display string or an overlay string), and there's something on + IT->stack, proceed with what's on the stack. It doesn't make + sense to return zero if there's unprocessed stuff on the stack, + because otherwise that stuff will never be displayed. */ + if (!success_p && it->sp > 0) + { + set_iterator_to_next (it, 0); + success_p = get_next_display_element (it); + } /* Value is 0 if end of buffer or string reached. */ return success_p; @@ -6961,7 +7000,7 @@ set_iterator_to_next (struct it *it, int display vector entry (these entries may contain faces). */ it->face_id = it->saved_face_id; - if (it->dpvec + it->current.dpvec_index == it->dpend) + if (it->dpvec + it->current.dpvec_index >= it->dpend) { int recheck_faces = it->ellipsis_p; @@ -6999,6 +7038,26 @@ set_iterator_to_next (struct it *it, int case GET_FROM_STRING: /* Current display element is a character from a Lisp string. */ xassert (it->s == NULL && STRINGP (it->string)); + /* Don't advance past string end. These conditions are true + when set_iterator_to_next is called at the end of + get_next_display_element, in which case the Lisp string is + already exhausted, and all we want is pop the iterator + stack. */ + if (it->current.overlay_string_index >= 0) + { + /* This is an overlay string, so there's no padding with + spaces, and the number of characters in the string is + where the string ends. */ + if (IT_STRING_CHARPOS (*it) >= SCHARS (it->string)) + goto consider_string_end; + } + else + { + /* Not an overlay string. There could be padding, so test + against it->end_charpos . */ + if (IT_STRING_CHARPOS (*it) >= it->end_charpos) + goto consider_string_end; + } if (it->cmp_it.id >= 0) { int i;