bug#9642: move-overlay creates an empty overlay with the evaporate property

bug#9642: move-overlay creates an empty overlay with the evaporate property

[Paul Eggert]

Package: Emacs
Version: 24.0.90
Tags: patch

In a nonempty buffer, the following:

  (let ((o (make-overlay 1 2)))
    (overlay-put o 'evaporate t)
    (move-overlay o 0 1))

returns an empty overlay that has the evaporate property.
But this is not supposed to happen: when an overlay with that
property becomes empty, it's supposed to be deleted.

Here's a patch that I'd like to install after a bit more testing.


* buffer.c (Fmove_overlay): Delete an evaporating overlay
if it becomes empty after its bounds are adjusted to fit within
its buffer.  Without this fix, in a nonempty buffer (let ((o
(make-overlay 1 2))) (overlay-put o 'evaporate t) (move-overlay o 0 1))
yields an empty overlay that has the evaporate property, which is
not supposed to happen.

=== modified file 'src/buffer.c'
--- src/buffer.c	2011-09-30 20:22:01 +0000
+++ src/buffer.c	2011-09-30 22:29:34 +0000
@@ -3673,6 +3673,7 @@
   struct buffer *b, *ob;
   Lisp_Object obuffer;
   int count = SPECPDL_INDEX ();
+  ptrdiff_t n_beg, n_end;

   CHECK_OVERLAY (overlay);
   if (NILP (buffer))
@@ -3691,15 +3692,20 @@
   CHECK_NUMBER_COERCE_MARKER (beg);
   CHECK_NUMBER_COERCE_MARKER (end);

-  if (XINT (beg) == XINT (end) && ! NILP (Foverlay_get (overlay, Qevaporate)))
+  if (XINT (beg) > XINT (end))
+    {
+      Lisp_Object temp;
+      temp = beg; beg = end; end = temp;
+    }
+
+  Fset_marker (OVERLAY_START (overlay), beg, buffer);
+  Fset_marker (OVERLAY_END (overlay), end, buffer);
+  n_beg = marker_position (OVERLAY_START (overlay));
+  n_end = marker_position (OVERLAY_END (overlay));
+
+  if (n_beg == n_end && ! NILP (Foverlay_get (overlay, Qevaporate)))
     return Fdelete_overlay (overlay);

-  if (XINT (beg) > XINT (end))
-    {
-      Lisp_Object temp;
-      temp = beg; beg = end; end = temp;
-    }
-
   specbind (Qinhibit_quit, Qt);

   obuffer = Fmarker_buffer (OVERLAY_START (overlay));
@@ -3722,7 +3728,7 @@
 	}

       /* Redisplay where the overlay is going to be.  */
-      modify_overlay (b, XINT (beg), XINT (end));
+      modify_overlay (b, n_beg, n_end);
     }
   else
     /* Redisplay the area the overlay has just left, or just enclosed.  */
@@ -3732,16 +3738,12 @@
       o_beg = OVERLAY_POSITION (OVERLAY_START (overlay));
       o_end = OVERLAY_POSITION (OVERLAY_END (overlay));

-      if (o_beg == XINT (beg))
-	modify_overlay (b, o_end, XINT (end));
-      else if (o_end == XINT (end))
-	modify_overlay (b, o_beg, XINT (beg));
+      if (o_beg == n_beg)
+	modify_overlay (b, o_end, n_end);
+      else if (o_end == n_end)
+	modify_overlay (b, o_beg, n_beg);
       else
-	{
-	  if (XINT (beg) < o_beg) o_beg = XINT (beg);
-	  if (XINT (end) > o_end) o_end = XINT (end);
-	  modify_overlay (b, o_beg, o_end);
-	}
+	modify_overlay (b, min (o_beg, n_beg), max (o_end, n_end));
     }

   if (!NILP (obuffer))
@@ -3753,12 +3755,8 @@
       eassert (XOVERLAY (overlay)->next == NULL);
     }

-  Fset_marker (OVERLAY_START (overlay), beg, buffer);
-  Fset_marker (OVERLAY_END   (overlay), end, buffer);
-
   /* Put the overlay on the wrong list.  */
-  end = OVERLAY_END (overlay);
-  if (OVERLAY_POSITION (end) < b->overlay_center)
+  if (n_end < b->overlay_center)
     {
       XOVERLAY (overlay)->next = b->overlays_after;
       b->overlays_after = XOVERLAY (overlay);

bug#9642: move-overlay creates an empty overlay with the evaporate property

[Stefan Monnier]

> In a nonempty buffer, the following:

>   (let ((o (make-overlay 1 2)))
>     (overlay-put o 'evaporate t)
>     (move-overlay o 0 1))

> returns an empty overlay that has the evaporate property.
> But this is not supposed to happen: when an overlay with that
> property becomes empty, it's supposed to be deleted.

OTOH if the user goes through the trouble to do the above, he probably
doesn't want this overlay to disappear right away.  So I'm not sure we
want to second guess the user here.


        Stefan

bug#9642: move-overlay creates an empty overlay with the evaporate property

[Stefan Monnier]

>> returns an empty overlay that has the evaporate property.
>> But this is not supposed to happen: when an overlay with that
>> property becomes empty, it's supposed to be deleted.

> OTOH if the user goes through the trouble to do the above, he probably
> doesn't want this overlay to disappear right away.  So I'm not sure we
> want to second guess the user here.

We can discuss it now, but in any case I don't think we should change
this behavior for 24.1.


        Stefan

bug#9642: move-overlay creates an empty overlay with the evaporate property

[Paul Eggert]

On 09/30/11 21:01, Stefan Monnier wrote:

> if the user goes through the trouble to do the above, he probably
> doesn't want this overlay to disappear right away

I don't see why not.  Evaporating overlays are supposed
disappear when they become empty -- that's their whole point.
Why would one want an evaporating overflow that's empty?

Let's look at it a different way.  Suppose we were to try
to document the current behavior.  We'd add something to
the manual like "overlays with the evaporate property are deleted when
they become empty, except when X".  What would X be,
and why would it be a good idea to have X as an exception?

> I don't think we should change
> this behavior for 24.1.

OK, I'll leave it alone then.

bug#9642: move-overlay creates an empty overlay with the evaporate property

[Eli Zaretskii]

> Date: Sat, 01 Oct 2011 00:33:49 -0700
> From: Paul Eggert <eggert@cs.ucla.edu>
> Cc: 9642@debbugs.gnu.org
> 
> On 09/30/11 21:01, Stefan Monnier wrote:
> 
> > if the user goes through the trouble to do the above, he probably
> > doesn't want this overlay to disappear right away
> 
> I don't see why not.  Evaporating overlays are supposed
> disappear when they become empty -- that's their whole point.
> Why would one want an evaporating overflow that's empty?

But what is the meaning of moving an overlay to begin at buffer
position zero?  Such a position doesn't exist in any buffer.  So doing
this invokes undefined behavior, whose result can be anything,
including the end of the world.

> Let's look at it a different way.  Suppose we were to try
> to document the current behavior.  We'd add something to
> the manual like "overlays with the evaporate property are deleted when
> they become empty, except when X".  What would X be,

X == "empty means the beginning and the end positions of the overlay
are identical".

bug#9642: move-overlay creates an empty overlay with the evaporate property

[Paul Eggert]

On 10/01/11 01:22, Eli Zaretskii wrote:
> doing this invokes undefined behavior

No, everywhere else the overlay code consistently defines
the behavior: out-of-range values are treated as if they were
the nearest in-range values.  For example,

(make-overlay 0 (1+ (point-max)))

is treated exactly as if it were

(make-overlay 1 (point-max))

This is almost always true for move-overlay, too: for example,
in a nonempty buffer:

(move-overlay foo 0 (1+ (point-max)))

is exactly equivalent to

(move-overlay foo 1 (point-max))

This behavior is a longstanding Emacs tradition, in many functions,
e.g., (goto-char 0) is exactly equivalent to (goto-char 1).

I suppose it might make sense for the overlay code to consistently treat
out-of-range values as errors, and throw exceptions instead.
But what doesn't make sense is for the overlay behavior to be inconsistent,
where all the primitives follow goto-char's lead
except that in some (but not all) cases move-overlay screws up
and silently returns a bogus zero-length evaporating overlay.

bug#9642: move-overlay creates an empty overlay with the evaporate property

[Eli Zaretskii]

> Date: Sat, 01 Oct 2011 22:38:24 -0700
> From: Paul Eggert <eggert@cs.ucla.edu>
> CC: monnier@iro.umontreal.ca, 9642@debbugs.gnu.org
> 
> On 10/01/11 01:22, Eli Zaretskii wrote:
> > doing this invokes undefined behavior
> 
> No, everywhere else the overlay code consistently defines
> the behavior: out-of-range values are treated as if they were
> the nearest in-range values.

That is your interpretation of what Emacs does.  But this seemingly
"consistent" behavior is nothing more than one possible undefined
behavior.  That it looks to you "consistent" is just an accident.

> This behavior is a longstanding Emacs tradition

Sorry, but I submit that we should start treating these references to
"tradition" as bogus.  Upholding such "traditional" misbehavior causes
all kinds of unexpected quirks in the code that are very hard to
maintain and downright impossible to take into account when designing
significant changes.

IOW, an attempt to treat such undefined behavior consistently bears
grave maintenance costs and in fact stands in the way of any
significant progress, because it makes it impossible to produce
elegant and simple design of new features that is based on assumptions
such as "buffer positions are always between (point-min) and
(point-max)."  One thinks this must be the case and writes the code
based on that, and then the code crashes because some Lisp expected
zero to be "traditionally" treated as one.

That way lies madness.

I say we should fix any code that "traditionally" relies on any such
behavior, and stop heeding to requests of fixing undefined behavior.
There should be no need for any code to put an overlay starting at
position zero nor go to such a position, because these positions
simply do not exist.

> I suppose it might make sense for the overlay code to consistently treat
> out-of-range values as errors, and throw exceptions instead.

I didn't say we should throw errors.  Undefined behavior is just
that--you can expect anything.  Including treating 0 as 1 in some
cases, but not others.

> But what doesn't make sense is for the overlay behavior to be inconsistent

It is consistently undefined, from my POV.  And that is the only sense
I can possibly find in code which does this.

bug#9642: move-overlay creates an empty overlay with the evaporate property

[Paul Eggert]

On 10/01/11 23:56, Eli Zaretskii wrote:
> I say we should fix any code that "traditionally" relies on any such
> behavior, and stop heeding to requests of fixing undefined behavior.

This sounds like a more-general issue, that affects many
aspects of Emacs other than just this bug report, so I'll
raise this on emacs-devel.

But regardless of the more-general issue,
the behavior of move-overlay is not undefined in this case.
The Elisp documentation states that for overlays with the
evaporate property, "If this property is non-nil, the overlay
is deleted automatically if it becomes empty (i.e., if its
length becomes zero)."  This is a general statement about
overlays, and it constrains the behavior of all Emacs built-ins.

The documented constraint does not require move-overlay to raise
an exception for out-of-range values, or to silently substitute
an in-range value.  All it says is that whenever move-overlay
causes an overlay to become entry, it must delete the overlay.
Since make-overlay does not do that, there is a bug in Emacs:
either in the code, or in the documentation, or both.

bug#9642: move-overlay creates an empty overlay with the evaporate property

[Eli Zaretskii]

> Date: Sun, 02 Oct 2011 18:09:43 -0700
> From: Paul Eggert <eggert@cs.ucla.edu>
> CC: monnier@iro.umontreal.ca, 9642@debbugs.gnu.org
> 
> But regardless of the more-general issue,
> the behavior of move-overlay is not undefined in this case.
> The Elisp documentation states that for overlays with the
> evaporate property, "If this property is non-nil, the overlay
> is deleted automatically if it becomes empty (i.e., if its
> length becomes zero)."  This is a general statement about
> overlays, and it constrains the behavior of all Emacs built-ins.

The interval 0..1 is not empty.

bug#9642: move-overlay creates an empty overlay with the evaporate property

[Paul Eggert]

On 10/02/11 20:56, Eli Zaretskii wrote:
> The interval 0..1 is not empty.

True, but (move-interval FOO 0 1) creates
an empty interval, with bounds 1..1, and this
empty interval is what's causing the problem.

If (move-interval FOO 0 1) created a nonempty interval,
the problem would not be happening
(though we might have other problems ...).

bug#9642: move-overlay creates an empty overlay with the evaporate property

[Stefan Monnier]

>> In a nonempty buffer, the following:
>> (let ((o (make-overlay 1 2)))
>> (overlay-put o 'evaporate t)
>> (move-overlay o 0 1))

>> returns an empty overlay that has the evaporate property.
>> But this is not supposed to happen: when an overlay with that
>> property becomes empty, it's supposed to be deleted.

> OTOH if the user goes through the trouble to do the above, he probably
> doesn't want this overlay to disappear right away.  So I'm not sure we
> want to second guess the user here.

I see I was confused: I didn't realize that your example's use of
out-of-bounds position was important, and I thought your change also
affected (move-overlay OL A A).  So, I now think your patch is
perfectly fine, thank you.


        Stefan

bug#9642: move-overlay creates an empty overlay with the evaporate property

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Date: Sun, 02 Oct 2011 23:15:56 -0400
> Cc: 9642@debbugs.gnu.org
> 
> >> In a nonempty buffer, the following:
> >> (let ((o (make-overlay 1 2)))
> >> (overlay-put o 'evaporate t)
> >> (move-overlay o 0 1))
> 
> >> returns an empty overlay that has the evaporate property.
> >> But this is not supposed to happen: when an overlay with that
> >> property becomes empty, it's supposed to be deleted.
> 
> > OTOH if the user goes through the trouble to do the above, he probably
> > doesn't want this overlay to disappear right away.  So I'm not sure we
> > want to second guess the user here.
> 
> I see I was confused: I didn't realize that your example's use of
> out-of-bounds position was important, and I thought your change also
> affected (move-overlay OL A A).  So, I now think your patch is
> perfectly fine, thank you.

If we are going to support this misbehavior (the fact that BEG can be
zero or negative), please add comments in the code to that effect, and
maybe also document it in the doc string.  Without comments, the
intent of the "tricksy" code with Fset_marker is entirely unclear.

FWIW, I think clipping of positions inside set-marker is for the case
of narrowed buffer, not for being able to handle positions outside the
valid range of values.  But I guess I will be ignored.

bug#9642: move-overlay creates an empty overlay with the evaporate property

[Stefan Monnier]

> If we are going to support this misbehavior (the fact that BEG can be
> zero or negative), please add comments in the code to that effect, and
> maybe also document it in the doc string.

Comment in the code would be good, yes.  But documenting this behavior
is not a good idea, I think.

> FWIW, I think clipping of positions inside set-marker is for the case
> of narrowed buffer, not for being able to handle positions outside the
> valid range of values.

I think so too.  But the issue is only about `evaporate', which should
ensure the overlay never stays empty without evaporating.  I don't think
this matters much either way, so let's not bikeshed too much.


        Stefan

bug#9642: move-overlay creates an empty overlay with the evaporate property

[Paul Eggert]

I added a comment along the lines suggested
installed this into the trunk as bzr 108012,
and am marking this bug as done.

bug#9642: closed (Re: bug#9642: move-overlay creates an empty overlay with the evaporate property)

[Tassilo Horn]

help-debbugs@gnu.org (GNU bug Tracking System) writes:

Hi Paul,

> Your bug report
>
> #9642: move-overlay creates an empty overlay with the evaporate property
>
> which was filed against the emacs package, has been closed.

I think your fix (revno 108012) has some bad side effects.  At least
since I updated my emacs checkout today from a 3-4 days old version, I
frequently get errors like this one:

--8<---------------cut here---------------start------------->8---
Debugger entered--Lisp error: (error "Marker does not point anywhere")
  overlays-in(1 54815)
  preview-clearout(1 54815 (20377 23663))
  preview-kill-buffer-cleanup()
  kill-buffer("expose.tex")
  call-interactively(kill-buffer nil nil)
--8<---------------cut here---------------end--------------->8---

That one happened when trying to kill a LaTeX-mode (AUCTeX) buffer.
I've tried moving point or setting mark, but that didn't change
anything.  I couldn't kill the buffer.  Then I've hit C-< which resulted
in an emacs freeze that couldn't be recovered from with C-g.  I had to
kill it.

Before that, I already had two freezes where I had to kill emacs where
that error was triggered from some function in `post-command-hook'.

Sadly, I have no recipe how to reproduce that issue.  But I'll start
running emacs from the debugger to get a backtrace the next time it
happens.

Should I append to this already closed bug report, or should I file a
new one?

Bye,
Tassilo

bug#9642: closed (Re: bug#9642: move-overlay creates an empty overlay with the evaporate property)

[Tassilo Horn]

Tassilo Horn <tassilo@member.fsf.org> writes:

> Sadly, I have no recipe how to reproduce that issue.  But I'll start
> running emacs from the debugger to get a backtrace the next time it
> happens.

It happened again, and this time I was able to get a backtrace.  But it
doesn't look overly helpful...

Bye,
Tassilo

--8<---------------cut here---------------start------------->8---
Program received signal SIGTSTP, Stopped (user).
0x00007ffff2ffe233 in select () from /lib64/libc.so.6
(gdb) bt full
#0  0x00007ffff2ffe233 in select () from /lib64/libc.so.6
No symbol table info available.
#1  0x00000000004b4ca0 in XTflash (f=0xfcd670) at xterm.c:3165
        current = {
          tv_sec = 0, 
          tv_usec = 129804
        }
        timeout = {
          tv_sec = 0, 
          tv_usec = 4833
        }
        wakeup = {
          tv_sec = 1335536248, 
          tv_usec = 621151
        }
        flash_height = 15
        flash_left = 1
        flash_right = <optimized out>
        height = 917
        width = 776
        gc = 0x2e88970
#2  XTring_bell (f=0xfcd670) at xterm.c:3236
No locals.
#3  0x00000000004f4c28 in cmd_error_internal (data=102278934, 
    context=0x7fffffffd5b0 "") at keyboard.c:1103
        sf = <optimized out>
#4  0x00000000004f4d79 in cmd_error (data=102278934) at keyboard.c:1036
        old_level = 40
        old_length = 40
        macroerror = "\000>\266\000\000\000\000\000\342\214\306\000\000\000\000\000\302>\266\000\000\000\000\000\002\000\000\000\000\000\000\000\302>\266\000\000\000\000\000\360\333\377\377\377\177\000\000\001"
#5  0x00000000005642b1 in internal_condition_case (
    bfun=0x4ff810 <command_loop_1>, handlers=11995810, 
    hfun=0x4f4ca0 <cmd_error>) at eval.c:1438
        val = <optimized out>
        c = {
          tag = 11943618, 
          val = 102278934, 
          next = 0x7fffffffd7b0, 
          gcpro = 0x0, 
          jmp = {{
              __jmpbuf = {14332576, 4024398992634898477, 11943618, 
                140737488346096, 1, 0, -4024397528753412051, 
                4024397920526100525}, 
              __mask_was_saved = 0, 
              __saved_mask = {
                __val = {140737353884880, 140737488344808, 4294967295, 
                  140737354129832, 1, 8544784, 0, 1, 0, 0, 140737351944161, 1, 
                  0, 0, 140737269361080, 0}
              }
            }}, 
          backlist = 0x0, 
          handlerlist = 0x0, 
          lisp_eval_depth = 0, 
          pdlcount = 2, 
          poll_suppress_count = 1, 
          interrupt_input_blocked = 0, 
          byte_stack = 0x0
        }
        h = {
          handler = 11995810, 
          var = 11943618, 
          chosen_clause = 11943666, 
          tag = 0x7fffffffd640, 
          next = 0x0
        }
#6  0x00000000004f390e in command_loop_2 (ignore=<optimized out>)
    at keyboard.c:1160
        val = -514
#7  0x0000000000564168 in internal_catch (
    tag=<error reading variable: Cannot access memory at address 0xffffffffffffffe0>, func=0x4f38f0 <command_loop_2>, arg=11943618) at eval.c:1205
        c = {
          tag = 11991554, 
          val = 11943618, 
          next = 0x0, 
          gcpro = 0x0, 
          jmp = {{
              __jmpbuf = {14332576, 4024398992634898477, 11943618, 
                140737488346096, 1, 0, -4024397528803743699, 
                4024397920421373997}, 
              __mask_was_saved = 0, 
              __saved_mask = {
                __val = {0, 344, 94489280512, 352, 5, 384, 0, 0, 0, 0, 
                  227633266710, 472446402651, 532575944823, 12171106, 5666934, 
                  6236710}
              }
            }}, 
          backlist = 0x0, 
          handlerlist = 0x0, 
          lisp_eval_depth = 0, 
          pdlcount = 2, 
          poll_suppress_count = 1, 
          interrupt_input_blocked = 0, 
          byte_stack = 0x0
        }
#8  0x00000000004f475a in command_loop () at keyboard.c:1139
No locals.
#9  recursive_edit_1 () at keyboard.c:759
        val = 11943618
#10 0x00000000004f4a9b in Frecursive_edit () at keyboard.c:823
        buffer = 11943618
#11 0x00000000004ef76b in main (argc=<optimized out>, argv=0x7fffffffdd38)
    at emacs.c:1711
        dummy = 140737488346440
        stack_bottom_variable = 0 '\000'
        do_initial_setlocale = <optimized out>
        skip_args = 0
        rlim = {
          rlim_cur = 8720000, 
          rlim_max = 18446744073709551615
        }
        no_loadup = 0
        junk = 0x0
        dname_arg = 0x0
        ch_to_dir = 0x0
(gbd)
(gdb) xbacktrace 
(gdb)
--8<---------------cut here---------------end--------------->8---

bug#9642: closed (Re: bug#9642: move-overlay creates an empty overlay with the evaporate property)

[Tassilo Horn]

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

Tassilo Horn <thorn@fastmail.fm> writes:

> It happened again, and this time I was able to get a backtrace.  But
> it doesn't look overly helpful...

Attached is another one, which looks more helpful.


[-- Attachment #2: backtrace2.txt.gz --]
[-- Type: application/octet-stream, Size: 8911 bytes --]

[-- Attachment #3: Type: text/plain, Size: 14 bytes --]


Bye,
Tassilo

bug#9642: closed (Re: bug#9642: move-overlay creates an empty overlay with the evaporate property)

[Eli Zaretskii]

> From: Tassilo Horn <tassilo@member.fsf.org>
> Cc: Paul Eggert <eggert@cs.ucla.edu>,  Eli Zaretskii <eliz@gnu.org>,  Stefan Monnier <monnier@iro.umontreal.ca>
> Date: Fri, 27 Apr 2012 20:38:33 +0200
> 
> Tassilo Horn <thorn@fastmail.fm> writes:
> 
> > It happened again, and this time I was able to get a backtrace.  But
> > it doesn't look overly helpful...
> 
> Attached is another one, which looks more helpful.

Please post backtraces inline, it makes browsing them easier.

Anyway, if we have bogus markers pointing nowhere, the display engine
will certainly barf at some point.  Stefan, is it worth my while to
find a way to make redisplay resistant to this kind of problems?  It
will sweep such problems under the carpet.

Paul, could you please look at fixing this ASAP?  If a simple enough
fix is not in sight, maybe we should just revert the change which
triggered these problems.

bug#9642: closed (Re: bug#9642: move-overlay creates an empty overlay with the evaporate property)

[Stefan Monnier]

> will certainly barf at some point.  Stefan, is it worth my while to
> find a way to make redisplay resistant to this kind of problems?

No, these are bugs elsewhere, and should be fixed elsewhere.


        Stefan

bug#9642: closed (Re: bug#9642: move-overlay creates an empty overlay with the evaporate property)

[Paul Eggert]

On 04/27/2012 11:38 AM, Tassilo Horn wrote:
> Attached is another one, which looks more helpful.

Thanks, but that backtrace looks rather optimized, which makes it
hard to debug.  Perhaps you can build a version of
Emacs without debugging, and reproduce the problem?

In the meantime I've reverted the change in the trunk.
I'll see if I can at some point come up with a fix
for the original bug that is less of a troublemaker.

bug#9642: closed (Re: bug#9642: move-overlay creates an empty overlay with the evaporate property)

[Tassilo Horn]

Paul Eggert <eggert@cs.ucla.edu> writes:

>> Attached is another one, which looks more helpful.
>
> Thanks, but that backtrace looks rather optimized, which makes it
> hard to debug.  Perhaps you can build a version of
> Emacs without debugging, and reproduce the problem?

I've set CFLAGS for emacs to

  CFLAGS="-mtune=native -O0 -pipe -g -ggdb"

but it seems autoconf mangles those and eventually I end up with

  -mtune=native -pipe -g -ggdb -O2

Bye,
Tassilo

bug#9642: closed (Re: bug#9642: move-overlay creates an empty overlay with the evaporate property)

[Paul Eggert]

On 04/29/2012 12:57 AM, Tassilo Horn wrote:
> I've set CFLAGS for emacs to
>
>    CFLAGS="-mtune=native -O0 -pipe -g -ggdb"
>
> but it seems autoconf mangles those and eventually I end up with
>
>    -mtune=native -pipe -g -ggdb -O2

Autoconf shouldn't do that.  But regardless of
what Autoconf does, you can override it by building with
"make CFLAGS='-g'".

bug#9642: closed (Re: bug#9642: move-overlay creates an empty overlay with the evaporate property)

[Troels Nielsen]

Hi all,

I ran into this and I think I got my head around the problem with
revision 108012.

In 108012 this test case reproduces the problem reliably for me:

(let ((b (get-buffer-create "*test-11351*"))
      (o (make-overlay (point-min) (point-max))))
  (kill-buffer b)
  (move-overlay o 1 1 b))

Executing that code will freeze up my emacs solid if using a version
between 108012 and 108059.

The problem appears to have been this: Fset_marker would set the
markers buffer to null if the new buffer was dead, and then
marker_position would throw an error, but the original buffer would
then still have a reference to the now corrupt overlay in
overlays_before or overlays_after etc..

I propose the following patch, which rearranges some lines from
#108012, adds some comments and also puts in an explicit test for
moving an overlay to a dead buffer. (though because of the
rearrangements that check is probably not strictly necessary now).
The explicit test changes the behavior slightly as earlier (before
#108012) the overlay would be removed from its buffer when trying to
move it to a dead one, while now the overlay will not be touched.

I can of course not be certain that this was everything there was to
#11351, but with the patch I have as of yet to experience any freeze
or error of the kind: "Marker does not point anywhere", and the test
mentioned in the start works.

Regards
Troels

=== modified file 'src/ChangeLog'
--- src/ChangeLog	2012-04-28 22:17:27 +0000
+++ src/ChangeLog	2012-04-29 14:51:48 +0000
@@ -1,3 +1,10 @@
+2012-04-29  Troels Nielsen <bn.troels@gmail.com>
+
+	* buffer.c (Fmove_overlay): Reinstate the earlier fix for
+	(Bug#9642), but explicitly check that the buffer the overlay would
+	be moved to is live and rearrange lines to make sure that errors
+	will not put the overlay in an inconsistent state.
+
 2012-04-28  Paul Eggert  <eggert@cs.ucla.edu>

 	Do not avoid creating empty evaporating overlays (Bug#9642).

=== modified file 'src/buffer.c'
--- src/buffer.c	2012-04-28 22:17:27 +0000
+++ src/buffer.c	2012-04-29 14:42:48 +0000
@@ -3679,8 +3679,9 @@
 buffer.  */)
   (Lisp_Object overlay, Lisp_Object beg, Lisp_Object end, Lisp_Object buffer)
 {
-  struct buffer *b, *ob;
+  struct buffer *b = 0, *ob = 0;
   Lisp_Object obuffer;
+  EMACS_INT n_beg, n_end, o_beg, o_end;
   int count = SPECPDL_INDEX ();

   CHECK_OVERLAY (overlay);
@@ -3690,6 +3691,9 @@
     XSETBUFFER (buffer, current_buffer);
   CHECK_BUFFER (buffer);

+  if (NILP (Fbuffer_live_p (buffer)))
+    error ("Attempt to move overlay to a dead buffer");
+
   if (MARKERP (beg)
       && ! EQ (Fmarker_buffer (beg), buffer))
     error ("Marker points into wrong buffer");
@@ -3700,9 +3704,6 @@
   CHECK_NUMBER_COERCE_MARKER (beg);
   CHECK_NUMBER_COERCE_MARKER (end);

-  if (XINT (beg) == XINT (end) && ! NILP (Foverlay_get (overlay, Qevaporate)))
-    return Fdelete_overlay (overlay);
-
   if (XINT (beg) > XINT (end))
     {
       Lisp_Object temp;
@@ -3711,50 +3712,16 @@

   specbind (Qinhibit_quit, Qt);

+  b = XBUFFER (buffer);
+
   obuffer = Fmarker_buffer (OVERLAY_START (overlay));
-  b = XBUFFER (buffer);
-  ob = BUFFERP (obuffer) ? XBUFFER (obuffer) : (struct buffer *) 0;
-
-  /* If the overlay has changed buffers, do a thorough redisplay.  */
-  if (!EQ (buffer, obuffer))
-    {
-      /* Redisplay where the overlay was.  */
-      if (!NILP (obuffer))
-	{
-	  EMACS_INT o_beg;
-	  EMACS_INT o_end;
-
-	  o_beg = OVERLAY_POSITION (OVERLAY_START (overlay));
-	  o_end = OVERLAY_POSITION (OVERLAY_END (overlay));
-
-	  modify_overlay (ob, o_beg, o_end);
-	}
-
-      /* Redisplay where the overlay is going to be.  */
-      modify_overlay (b, XINT (beg), XINT (end));
-    }
-  else
-    /* Redisplay the area the overlay has just left, or just enclosed.  */
-    {
-      EMACS_INT o_beg, o_end;
-
+  if (!NILP (obuffer))
+    {
+      /* Make notice of original overlay values */
+      ob = XBUFFER (obuffer);
       o_beg = OVERLAY_POSITION (OVERLAY_START (overlay));
       o_end = OVERLAY_POSITION (OVERLAY_END (overlay));

-      if (o_beg == XINT (beg))
-	modify_overlay (b, o_end, XINT (end));
-      else if (o_end == XINT (end))
-	modify_overlay (b, o_beg, XINT (beg));
-      else
-	{
-	  if (XINT (beg) < o_beg) o_beg = XINT (beg);
-	  if (XINT (end) > o_end) o_end = XINT (end);
-	  modify_overlay (b, o_beg, o_end);
-	}
-    }
-
-  if (!NILP (obuffer))
-    {
       ob->overlays_before
 	= unchain_overlay (ob->overlays_before, XOVERLAY (overlay));
       ob->overlays_after
@@ -3762,12 +3729,42 @@
       eassert (XOVERLAY (overlay)->next == NULL);
     }

+  /* Set the overlay boundaries, which may clip them.  */
   Fset_marker (OVERLAY_START (overlay), beg, buffer);
   Fset_marker (OVERLAY_END   (overlay), end, buffer);
-
-  /* Put the overlay on the wrong list.  */
-  end = OVERLAY_END (overlay);
-  if (OVERLAY_POSITION (end) < b->overlay_center)
+  n_beg = marker_position (OVERLAY_START (overlay));
+  n_end = marker_position (OVERLAY_END (overlay));
+
+  /* Find out how much we should redisplay */
+  if (!EQ (buffer, obuffer))
+    /* If the overlay has changed buffers, do a thorough redisplay.  */
+    {
+      /* Redisplay where the overlay was. */
+      if (!NILP (obuffer))
+        modify_overlay (ob, o_beg, o_end);
+
+      /* Redisplay where the overlay is going to be.  */
+      modify_overlay (b, n_beg, n_end);
+    }
+  else
+    /* Redisplay the area the overlay has just left, or just enclosed.  */
+    {
+      if (o_beg == n_beg)
+	modify_overlay (b, o_end, n_end);
+      else if (o_end == n_end)
+	modify_overlay (b, o_beg, n_beg);
+      else
+        modify_overlay (b, min(n_beg, o_beg), max(o_end, n_end));
+    }
+
+  /* Delete the overlay if it is empty after clipping and has the
+     evaporate property.  */
+  if (n_beg == n_end && ! NILP (Foverlay_get (overlay, Qevaporate)))
+    return unbind_to(count, Fdelete_overlay (overlay));
+
+  /* Put the overlay into the new buffer's overlay lists, first on the
+     wrong list.  */
+  if (n_end < b->overlay_center)
     {
       XOVERLAY (overlay)->next = b->overlays_after;
       b->overlays_after = XOVERLAY (overlay);

bug#9642: closed (Re: bug#9642: move-overlay creates an empty overlay with the evaporate property)

[Stefan Monnier]

> I ran into this and I think I got my head around the problem with
> revision 108012.

Thanks, your analysis sounds right and your patch looks OK.
Could you please send an updated patch with the following nitpicks
addressed:
- Add a space before the opening paren of function calls.
- Use two spaces to end sentences in comments.
- Don't forget to punctuate your comments.

> +      /* Make notice of original overlay values */

E.g. here, add ". " after "values".

> +    return unbind_to(count, Fdelete_overlay (overlay));

and here add a space after "unbind_to".

[ Of course, I could do it myself, but we can't install your patch right
now anyway because that's going beyond what we consider as "tiny patch"
so we need your copyright paperwork first.  ]


        Stefan

bug#9642: closed (Re: bug#9642: move-overlay creates an empty overlay with the evaporate property)

[Troels Nielsen]

On Mon, May 14, 2012 at 7:46 AM, Stefan Monnier
<monnier@iro.umontreal.ca> wrote:
>> I ran into this and I think I got my head around the problem with
>> revision 108012.
> - Add a space before the opening paren of function calls.
> - Use two spaces to end sentences in comments.
> - Don't forget to punctuate your comments.
>
> [ Of course, I could do it myself, but we can't install your patch right
> now anyway because that's going beyond what we consider as "tiny patch"
> so we need your copyright paperwork first.  ]
>

Thanks, the small stuff should be fixed now, and paperwork completed,
and I have changed the patch to go against current trunk.

It furthermore fixes a newly introduced issue where overlays could be
made to end at a buffer position before they began.

Regards
Troels

=== modified file 'src/ChangeLog'
--- src/ChangeLog	2012-05-27 07:51:09 +0000
+++ src/ChangeLog	2012-05-27 21:43:24 +0000
@@ -1,3 +1,11 @@
+2012-05-27  Troels Nielsen <bn.troels@gmail.com>
+
+	* buffer.c (Fmove_overlay): Reinstate the earlier fix for
+	(Bug#9642), but explicitly check that the buffer the overlay would
+	be moved to is live and rearrange lines to make sure that errors
+	will not put the overlay in an inconsistent state.
+	(Fdelete_overlay): Cosmetics.
+
 2012-05-27  Paul Eggert  <eggert@cs.ucla.edu>

 	* lisp.h [REL_ALLOC]: Omit duplicate prototypes.

=== modified file 'src/buffer.c'
--- src/buffer.c	2012-05-27 01:06:44 +0000
+++ src/buffer.c	2012-05-27 22:00:22 +0000
@@ -3676,10 +3676,10 @@
 buffer.  */)
   (Lisp_Object overlay, Lisp_Object beg, Lisp_Object end, Lisp_Object buffer)
 {
-  struct buffer *b, *ob;
+  struct buffer *b, *ob = 0;
   Lisp_Object obuffer;
   ptrdiff_t count = SPECPDL_INDEX ();
-  ptrdiff_t n_beg, n_end;
+  ptrdiff_t n_beg, n_end, o_beg, o_end;

   CHECK_OVERLAY (overlay);
   if (NILP (buffer))
@@ -3688,6 +3688,9 @@
     XSETBUFFER (buffer, current_buffer);
   CHECK_BUFFER (buffer);

+  if (NILP (Fbuffer_live_p (buffer)))
+    error ("Attempt to move overlay to a dead buffer");
+
   if (MARKERP (beg)
       && ! EQ (Fmarker_buffer (beg), buffer))
     error ("Marker points into wrong buffer");
@@ -3697,73 +3700,69 @@

   CHECK_NUMBER_COERCE_MARKER (beg);
   CHECK_NUMBER_COERCE_MARKER (end);
-  n_beg = clip_to_bounds (PTRDIFF_MIN, XINT (beg), PTRDIFF_MAX);
-  n_end = clip_to_bounds (PTRDIFF_MIN, XINT (end), PTRDIFF_MAX);
-
-  if (n_beg == n_end && ! NILP (Foverlay_get (overlay, Qevaporate)))
-    return Fdelete_overlay (overlay);
-
-  if (n_beg > n_end)
+
+  if (XINT (beg) > XINT (end))
     {
-      ptrdiff_t temp;
-      temp = n_beg; n_beg = n_end; n_end = temp;
+      Lisp_Object temp;
+      temp = beg; beg = end; end = temp;
     }

   specbind (Qinhibit_quit, Qt);

+  b = XBUFFER (buffer);
   obuffer = Fmarker_buffer (OVERLAY_START (overlay));
-  b = XBUFFER (buffer);
-  ob = BUFFERP (obuffer) ? XBUFFER (obuffer) : (struct buffer *) 0;
-
-  /* If the overlay has changed buffers, do a thorough redisplay.  */
-  if (!EQ (buffer, obuffer))
-    {
-      /* Redisplay where the overlay was.  */
-      if (!NILP (obuffer))
-	{
-	  ptrdiff_t o_beg;
-	  ptrdiff_t o_end;
-
-	  o_beg = OVERLAY_POSITION (OVERLAY_START (overlay));
-	  o_end = OVERLAY_POSITION (OVERLAY_END (overlay));
-
-	  modify_overlay (ob, o_beg, o_end);
-	}
-
-      /* Redisplay where the overlay is going to be.  */
-      modify_overlay (b, n_beg, n_end);
-    }
-  else
-    /* Redisplay the area the overlay has just left, or just enclosed.  */
-    {
-      ptrdiff_t o_beg, o_end;
+
+  if (!NILP (obuffer))
+    {
+      ob = XBUFFER (obuffer);

       o_beg = OVERLAY_POSITION (OVERLAY_START (overlay));
       o_end = OVERLAY_POSITION (OVERLAY_END (overlay));

+      ob->overlays_before =
+        unchain_overlay (ob->overlays_before, XOVERLAY (overlay));
+      ob->overlays_after =
+        unchain_overlay (ob->overlays_after, XOVERLAY (overlay));
+      eassert (XOVERLAY (overlay)->next == NULL);
+    }
+
+  /* Set the overlay boundaries, which may clip them.  */
+  Fset_marker (OVERLAY_START (overlay), beg, buffer);
+  Fset_marker (OVERLAY_END (overlay), end, buffer);
+
+  n_beg = marker_position (OVERLAY_START (overlay));
+  n_end = marker_position (OVERLAY_END (overlay));
+
+  /* Find out how much we should redisplay.  */
+  if (EQ (buffer, obuffer))
+    /* Redisplay the area the overlay has just left, or just enclosed.  */
+    {
       if (o_beg == n_beg)
-	modify_overlay (b, o_end, n_end);
+        modify_overlay (b, o_end, n_end);
       else if (o_end == n_end)
-	modify_overlay (b, o_beg, n_beg);
+        modify_overlay (b, o_beg, n_beg);
       else
-	modify_overlay (b, min (o_beg, n_beg), max (o_end, n_end));
+        modify_overlay (b, min(n_beg, o_beg), max(o_end, n_end));
     }
-
-  if (!NILP (obuffer))
+  else
+    /* If the overlay has changed buffers, do a thorough redisplay.  */
     {
-      ob->overlays_before
-	= unchain_overlay (ob->overlays_before, XOVERLAY (overlay));
-      ob->overlays_after
-	= unchain_overlay (ob->overlays_after, XOVERLAY (overlay));
-      eassert (XOVERLAY (overlay)->next == NULL);
+      /* Redisplay where the overlay was.  */
+      if (ob)
+        modify_overlay (ob, o_beg, o_end);
+
+      /* Redisplay where the overlay is going to be.  */
+      modify_overlay (b, n_beg, n_end);
     }

-  Fset_marker (OVERLAY_START (overlay), beg, buffer);
-  Fset_marker (OVERLAY_END   (overlay), end, buffer);
+  /* Delete the overlay if it is empty after clipping and has the
+     evaporate property.  */
+  if (n_beg == n_end && !NILP (Foverlay_get (overlay, Qevaporate)))
+    return unbind_to (count, Fdelete_overlay (overlay));

-  /* Put the overlay on the wrong list.  */
-  end = OVERLAY_END (overlay);
-  if (OVERLAY_POSITION (end) < b->overlay_center)
+  /* Put the overlay into the new buffer's overlay lists, first on the
+     wrong list.  */
+  if (n_end < b->overlay_center)
     {
       XOVERLAY (overlay)->next = b->overlays_after;
       b->overlays_after = XOVERLAY (overlay);
@@ -3797,9 +3796,12 @@
   b = XBUFFER (buffer);
   specbind (Qinhibit_quit, Qt);

-  b->overlays_before = unchain_overlay (b->overlays_before,XOVERLAY (overlay));
-  b->overlays_after  = unchain_overlay (b->overlays_after, XOVERLAY (overlay));
+  b->overlays_before =
+    unchain_overlay (b->overlays_before, XOVERLAY (overlay));
+  b->overlays_after =
+    unchain_overlay (b->overlays_after, XOVERLAY (overlay));
   eassert (XOVERLAY (overlay)->next == NULL);
+
   modify_overlay (b,
 		  marker_position (OVERLAY_START (overlay)),
 		  marker_position (OVERLAY_END   (overlay)));

bug#9642: closed (Re: bug#9642: move-overlay creates an empty overlay with the evaporate property)

[Paul Eggert]

Thanks, I made some minor changes to that fix and
installed it into the trunk in your name, as bzr 108423.
The minor changes were to shrink the patch by restoring
the original indenting and then-else choices, and to
add a couple of IF_LINTs to pacify gcc -Wall.