Effect of deletions on indirect buffers (Bug#8219)

Effect of deletions on indirect buffers (Bug#8219)

[Chong Yidong]

Indirect bufffers are allowed to have their own values of point,
BUF_BEGV, and BUF_ZV (indeed, that's one of their roles).  Their other
attributes inherit from the base buffer, e.g.

#define BUF_Z(buf) ((buf)->text->z)

where `text' points to the base buffer's text object.

Now consider what happens when a deletion is performed in buffer A,
which is the base buffer for an indirect buffer B.  It appears that the
responsible functions, such as del_range_2, only update the attributes
of buffer A, making no effort to update buffer B.

Hence, in the aftermath of a deletion, buffer B's values of PT (and
BUF_BEGV and BUF_ZV) can be larger than BUF_ZV.  This is the proximate
cause of the crash in Bug#8219: there, we have

 if (prev_pt > BUF_BEGV (buf) && prev_pt < BUF_ZV (buf)
     && find_composition (prev_pt, -1, &start, &end, &prop, buffer)

and find_composition aborts because prev_pt is larger than the size of
the buffer.


I'm not sure what the best solution is.  The narrowest fix is to change
find_composition, and the functions it calls, so that it does not abort
when supplied with a position that's beyond BUF_Z.  This might be the
best approach for the emacs-23 branch.

However, I suspect that we have other places in the code that assumes
that if a point is smaller than BUF_ZV, it's necessarily smaller than
BUF_Z---which we now see if not that case.  So, a more comprehensive fix
is needed for the trunk.

Any thoughts?

Re: Effect of deletions on indirect buffers (Bug#8219)

[Eli Zaretskii]

> From: Chong Yidong <cyd@stupidchicken.com>
> Date: Fri, 11 Mar 2011 14:48:21 -0500
> Cc: 8219@debbugs.gnu.org
> 
> Now consider what happens when a deletion is performed in buffer A,
> which is the base buffer for an indirect buffer B.  It appears that the
> responsible functions, such as del_range_2, only update the attributes
> of buffer A, making no effort to update buffer B.
> 
> Hence, in the aftermath of a deletion, buffer B's values of PT (and
> BUF_BEGV and BUF_ZV) can be larger than BUF_ZV.

Isn't that a bug, right there?  Why doesn't del_range_2 update the
indirect buffer (B) as well, when deletion removes text so that its PT
becomes invalid?

The ELisp manual says:

     The text of the indirect buffer is always identical to the text of
  its base buffer; changes made by editing either one are visible
  immediately in the other.  This includes the text properties as well as
  the characters themselves.

     In all other respects, the indirect buffer and its base buffer are
  completely separate.  They have different names, independent values of
  point, independent narrowing, independent markers and overlays (though
  inserting or deleting text in either buffer relocates the markers and
  overlays for both), [...]

This makes a lot of sense, but your description seems to point out
that the implementation does not behave according to the docs: if
markers are (or should be) relocated in sync as result of insertion
and deletion, the same should happen with PT, BUF_BEGV, etc.

Am I missing something?  If not, the solution is to update the
attributes of the indirect buffer so as to preserve the invariants,
like PT <= BUF_Z etc.

> However, I suspect that we have other places in the code that assumes
> that if a point is smaller than BUF_ZV, it's necessarily smaller than
> BUF_Z

It is madness IMO to allow BUF_Z and BUF_ZV be out of sync.

Re: bug#8219: Effect of deletions on indirect buffers (Bug#8219)

[Chong Yidong]

Eli Zaretskii <eliz@gnu.org> writes:

>> Hence, in the aftermath of a deletion, buffer B's values of PT (and
>> BUF_BEGV and BUF_ZV) can be larger than BUF_ZV.
>
> Isn't that a bug, right there?  Why doesn't del_range_2 update the
> indirect buffer (B) as well, when deletion removes text so that its PT
> becomes invalid?

This is not currently easy.  We don't keep track of indirect buffers
(looping over the buffer list each time we do a deletion would be rather
inefficient), so this would need some new members of the buffer struct.

Re: Effect of deletions on indirect buffers (Bug#8219)

[Stefan Monnier]

> This makes a lot of sense, but your description seems to point out
> that the implementation does not behave according to the docs: if
> markers are (or should be) relocated in sync as result of insertion
> and deletion, the same should happen with PT, BUF_BEGV, etc.

Actually, indirect buffers keep their point and narrowing in real
markers.  So when they get "active" the BUF_PT, BUF_ZV, etc... get
updated, but until that time, BUF_PT and friends may hold invalid data.
I.e. any place where we use BUF_PT on something else than
current_buffer, we have a bug waiting to happen unless we know for sure
that that buffer is not indirect.


        Stefan

Re: bug#8219: Effect of deletions on indirect buffers (Bug#8219)

[Eli Zaretskii]

> From: Chong Yidong <cyd@stupidchicken.com>
> Cc: 8219@debbugs.gnu.org, emacs-devel@gnu.org
> Date: Fri, 11 Mar 2011 15:58:57 -0500
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> Hence, in the aftermath of a deletion, buffer B's values of PT (and
> >> BUF_BEGV and BUF_ZV) can be larger than BUF_ZV.
> >
> > Isn't that a bug, right there?  Why doesn't del_range_2 update the
> > indirect buffer (B) as well, when deletion removes text so that its PT
> > becomes invalid?
> 
> This is not currently easy.  We don't keep track of indirect buffers
> (looping over the buffer list each time we do a deletion would be rather
> inefficient), so this would need some new members of the buffer struct.

We could do that lazily, whenever something (including redisplay) is
done on a buffer that happens to be an indirect buffer.

Re: Effect of deletions on indirect buffers (Bug#8219)

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: Chong Yidong <cyd@stupidchicken.com>,  8219@debbugs.gnu.org,  emacs-devel@gnu.org
> Date: Fri, 11 Mar 2011 18:19:25 -0500
> 
> I.e. any place where we use BUF_PT on something else than
> current_buffer, we have a bug waiting to happen unless we know for sure
> that that buffer is not indirect.

It follows that BUF_PT, when used on other than the current buffer
should update PT if necessary, if that buffer is an indirect buffer,
before returning the value.  And the same with BUF_BEGV, BUF_ZV, etc.

Right?

Re: Effect of deletions on indirect buffers (Bug#8219)

[Stefan Monnier]

> It follows that BUF_PT, when used on other than the current buffer
> should update PT if necessary, if that buffer is an indirect buffer,
> before returning the value.  And the same with BUF_BEGV, BUF_ZV, etc.

> Right?

Yes.  At least that was my conclusion last time we bumped into such
a bug.  I even added a comment in buffer.h about it:

/* !!!FIXME:  all the BUF_BEGV/BUF_ZV/BUF_PT macros are flawed:
   on indirect (or base) buffers, that value is only correct if that buffer
   is the current_buffer, or if the buffer's text hasn't been modified (via
   an indirect buffer) since it was last current.  */


        Stefan

Re: bug#8219: Effect of deletions on indirect buffers (Bug#8219)

[Chong Yidong]

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

> Yes.  At least that was my conclusion last time we bumped into such
> a bug.  I even added a comment in buffer.h about it:
>
> /* !!!FIXME:  all the BUF_BEGV/BUF_ZV/BUF_PT macros are flawed:
>    on indirect (or base) buffers, that value is only correct if that buffer
>    is the current_buffer, or if the buffer's text hasn't been modified (via
>    an indirect buffer) since it was last current.  */

I think we should do away with the BUF_BEGV and related macros.  Let the
code in buffer.c manipulate the buffer->begv members directly; and the
usage in other parts of Emacs can be replaced with buf_begv() functions
that will update begv if necessary, and return the corrected result.

Re: bug#8219: Effect of deletions on indirect buffers (Bug#8219)

[Chong Yidong]

Chong Yidong <cyd@stupidchicken.com> writes:

> I think we should do away with the BUF_BEGV and related macros.  Let the
> code in buffer.c manipulate the buffer->begv members directly; and the
> usage in other parts of Emacs can be replaced with buf_begv() functions
> that will update begv if necessary, and return the corrected result.

Or rather, these functions should consult the pt_marker, begv_marker and
zv_marker markers instead of accessing begv etc directly.

Re: Effect of deletions on indirect buffers (Bug#8219)

[Chong Yidong]

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

> /* !!!FIXME:  all the BUF_BEGV/BUF_ZV/BUF_PT macros are flawed:
>    on indirect (or base) buffers, that value is only correct if that buffer
>    is the current_buffer, or if the buffer's text hasn't been modified (via
>    an indirect buffer) since it was last current.  */

OK, I've committed a fix to trunk.  Instead of doing away with the BUF_*
macros, I reworked them to handle the indirect buffer case properly.
This means they can no longer be used in `BUF_PT (foo) = bar' assignment
statements, so I've changed the callers that used them this way.

Not sure if the patch is appropriate for the emacs-23 branch, though.

Re: bug#8219: Effect of deletions on indirect buffers (Bug#8219)

[Juanma Barranquero]

> OK, I've committed a fix to trunk.  Instead of doing away with the BUF_*
> macros, I reworked them to handle the indirect buffer case properly.
> This means they can no longer be used in `BUF_PT (foo) = bar' assignment
> statements, so I've changed the callers that used them this way.

emacs -Q --eval '(clone-indirect-buffer "clone" nil)'

buffer.c:614: Emacs fatal error: assertion failed: NILP (BVAR (b, begv_marker))

Breakpoint 1, w32_abort () at w32fns.c:7208
7208      button = MessageBox (NULL,
(gdb) bt
#0  w32_abort () at w32fns.c:7208
#1  0x01047867 in die (msg=0x1590744 "assertion failed: NILP (BVAR (b,
begv_marker))", file=0x158f518 "buffer.c", line=614) at alloc.c:6101
#2  0x010caa80 in Fmake_indirect_buffer (base_buffer=52342789,
name=54741601, clone=52303922) at buffer.c:614
#3  0x0103beb5 in Ffuncall (nargs=4, args=0x88f0c0) at eval.c:2849
#4  0x01142a8c in Fbyte_code (bytestr=20838137, vector=20838245,
maxdepth=16) at bytecode.c:689
#5  0x0103cbea in funcall_lambda (fun=20838093, nargs=2,
arg_vector=0x88f260) at eval.c:3028
#6  0x0103c620 in apply_lambda (fun=20838093, args=57330886,
eval_flag=1) at eval.c:2954
#7  0x0103a2bc in Feval (form=57330894) at eval.c:2296
#8  0x0103bd75 in Ffuncall (nargs=2, args=0x88f4b0) at eval.c:2842
#9  0x01142a8c in Fbyte_code (bytestr=20645329, vector=20645821,
maxdepth=40) at bytecode.c:689
#10 0x0103cbea in funcall_lambda (fun=20645301, nargs=1,
arg_vector=0x88f704) at eval.c:3028
#11 0x0103c2fc in Ffuncall (nargs=2, args=0x88f700) at eval.c:2891
#12 0x01142a8c in Fbyte_code (bytestr=20627337, vector=20628333,
maxdepth=28) at bytecode.c:689
#13 0x0103cbea in funcall_lambda (fun=20627317, nargs=0,
arg_vector=0x88f944) at eval.c:3028
#14 0x0103c2fc in Ffuncall (nargs=1, args=0x88f940) at eval.c:2891
#15 0x01142a8c in Fbyte_code (bytestr=20623889, vector=20624101,
maxdepth=24) at bytecode.c:689
#16 0x0103cbea in funcall_lambda (fun=20623869, nargs=0,
arg_vector=0x88faf0) at eval.c:3028
#17 0x0103c620 in apply_lambda (fun=20623869, args=52303898,
eval_flag=1) at eval.c:2954
#18 0x0103a2bc in Feval (form=52791574) at eval.c:2296
#19 0x0100537b in top_level_2 () at keyboard.c:1138
#20 0x010378f4 in internal_condition_case (bfun=0x1005368
<top_level_2>, handlers=52357530, hfun=0x1004e81 <cmd_error>) at
eval.c:1408
#21 0x010053ad in top_level_1 (ignore=52303898) at keyboard.c:1146
#22 0x01037313 in internal_catch (tag=52355626, func=0x100537d
<top_level_1>, arg=52303898) at eval.c:1152
#23 0x010052f1 in command_loop () at keyboard.c:1101
#24 0x01004573 in recursive_edit_1 () at keyboard.c:731
#25 0x01004a97 in Frecursive_edit () at keyboard.c:793
#26 0x01002797 in main (argc=4, argv=0xca2da8) at emacs.c:1684

Lisp Backtrace:
"make-indirect-buffer" (0x88f0c4)
"clone-indirect-buffer" (0x88f260)
"eval" (0x88f4b4)
"command-line-1" (0x88f704)
"command-line" (0x88f944)
"normal-top-level" (0x88faf0)

Re: bug#8219: Effect of deletions on indirect buffers (Bug#8219)

[Chong Yidong]

Juanma Barranquero <lekktu@gmail.com> writes:

>> OK, I've committed a fix to trunk.  Instead of doing away with the BUF_*
>> macros, I reworked them to handle the indirect buffer case properly.
>> This means they can no longer be used in `BUF_PT (foo) = bar' assignment
>> statements, so I've changed the callers that used them this way.
>
> emacs -Q --eval '(clone-indirect-buffer "clone" nil)'
>
> buffer.c:614: Emacs fatal error: assertion failed: NILP (BVAR (b, begv_marker))

Thanks for the catch.  This should be fixed now.