bug#20734: 25.0.50; "Args out of range" with help-window-select t

bug#20734: 25.0.50; "Args out of range" with help-window-select t

[Nicolas Richard]

From
./emacs -Q

Eval:
(progn
  (setq help-window-select t
        debug-on-error t)
  (split-window)
  (switch-to-buffer "*Messages*")
  (message (make-string 9999 ?x)))

Hit: C-h f if RET

I get:
Debugger entered--Lisp error: (args-out-of-range 20077)

(no backtrace is shown).

We can however get a gdb backtrace (I put a breakpoint in Fsignal, but
it breaks more often than needed, so I say "continue" a few times in
order to get the following) :

#2  0x081ec7fe in xsignal1 (error_symbol=4392, arg=40270) at eval.c:1596
No locals.
#3  0x0824bd80 in get_char_property_and_overlay (position=40270, prop=8640, object=140654805, overlay=0xbfffeec0) at textprop.c:636
        noverlays = 22
        overlay_vec = 0x20
        obuf = 0x853b698
        sa_avail = 22
        sa_count = -1073746264
        sa_must_free = 8
        w = 0x8696cd0
#4  0x08163623 in adjust_point_for_property (last_pt=22, modified=true) at keyboard.c:1704
        beg = 139541880
        end = -1073746216
        val = 135666171
        overlay = 0
        tmp = -1073746216
        check_composition = false
        check_display = true
        check_invisible = true
        orig_pt = 10067
#5  0x081633b4 in command_loop_1 () at keyboard.c:1636
        cmd = 1695712
        keybuf = {34, 410, 0, 139555992, 0, 1, -1073745932, 143526584, 139555992, 0, 0, 0, -1073746040, 136237958, 2, 141273859, 3, 59, -1220163913, 
          141710811, 0, 2, 135640074, 141710811, -1073746008, 135668881, 141710811, 0, 0, -1073746008}
        i = 2
        prev_modiff = 10542
        prev_buffer = 0x853b698
        already_adjusted = false
#6  0x081ebfe8 in internal_condition_case (bfun=0x8162727 <command_loop_1>, handlers=9288, hfun=0x8161f3d <cmd_error>) at eval.c:1348
        val = -1073745960
        c = 0x88f54f0
#7  0x08162449 in command_loop_2 (ignore=0) at keyboard.c:1139
        val = 0
#8  0x081eb8f2 in internal_catch (tag=23016, func=0x816241e <command_loop_2>, arg=0) at eval.c:1108
        val = 23016
        c = 0x88e0ac0
#9  0x081623e3 in command_loop () at keyboard.c:1118
No locals.
#10 0x08161b9c in recursive_edit_1 () at keyboard.c:728
        count = 1
        val = 136247918
#11 0x08161d0e in Frecursive_edit () at keyboard.c:799
        count = 0
        buffer = 0
#12 0x0815fcfa in main (argc=10, argv=0xbffff1f4) at emacs.c:1626
        dummy = -1073745664
        stack_bottom_variable = 8 '\b'
        do_initial_setlocale = true
        dumping = false
        skip_args = 0
        rlim = {rlim_cur = 8388608, rlim_max = 18446744073709551615}
        no_loadup = false
        junk = 0x0
        dname_arg = 0x0
        ch_to_dir = 0x0
        original_pwd = 0x0

Looking at recent history shows that adjust_point_for_property was
touched recently :

Commit 18338b1dff981b17426048dfa20a4c6e6e12c5aa
Author:     Eli Zaretskii <eliz@gnu.org>
AuthorDate: Tue May 19 18:39:25 2015 +0300
Commit:     Eli Zaretskii <eliz@gnu.org>
CommitDate: Tue May 19 18:39:25 2015 +0300

    Fix display of overlapping window-specific overlays
    
    * src/keyboard.c (adjust_point_for_property): When adjusting point
    due to display strings, ignore overlays that are specific to
    windows other than the currently selected one.
    
    * src/xdisp.c (handle_single_display_spec): If the display
    property comes from an overlay, arrange for buffer iteration to
    resume only after the end of that overlay.  (Bug#20607)

I understand that
  get_char_property_and_overlay
      (make_number (PT), Qdisplay, selected_window,
       &overlay))
should not refer to PT but to "PT in selected_window", but I don't know
how to do that.

-- 
Nico.

bug#20734: 25.0.50; "Args out of range" with help-window-select t

[Stefan Monnier]

> We can however get a gdb backtrace (I put a breakpoint in Fsignal, but
> it breaks more often than needed, so I say "continue" a few times in
> order to get the following) :

You can put a break point in maybe_call_debugger, which tends to work
better in this sense.


        Stefan

bug#20734: 25.0.50; "Args out of range" with help-window-select t

[Nicolas Richard]

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

>> We can however get a gdb backtrace (I put a breakpoint in Fsignal, but
>> it breaks more often than needed, so I say "continue" a few times in
>> order to get the following) :
>
> You can put a break point in maybe_call_debugger, which tends to work
> better in this sense.

Thanks. Would the following make sense in etc/DEBUG ?

-`Fsignal' is a very useful place to put a breakpoint in.
-All Lisp errors go through there.
+`Fsignal' is a very useful place to put a breakpoint in.  All Lisp
+errors go through there.  If you are only interested in errors that
+would fire the debugger, breaking at `maybe_call_debugger' is useful.

-- 
Nico.

bug#20734: 25.0.50; "Args out of range" with help-window-select t

[Eli Zaretskii]

> From: Nicolas Richard <youngfrog@members.fsf.org>
> Date: Fri, 05 Jun 2015 11:33:46 +0200
> Cc: 20734@debbugs.gnu.org
> 
> Stefan Monnier <monnier@iro.umontreal.ca> writes:
> 
> >> We can however get a gdb backtrace (I put a breakpoint in Fsignal, but
> >> it breaks more often than needed, so I say "continue" a few times in
> >> order to get the following) :
> >
> > You can put a break point in maybe_call_debugger, which tends to work
> > better in this sense.
> 
> Thanks. Would the following make sense in etc/DEBUG ?
> 
> -`Fsignal' is a very useful place to put a breakpoint in.
> -All Lisp errors go through there.
> +`Fsignal' is a very useful place to put a breakpoint in.  All Lisp
> +errors go through there.  If you are only interested in errors that
> +would fire the debugger, breaking at `maybe_call_debugger' is useful.

This is good, thanks.

bug#20734: 25.0.50; "Args out of range" with help-window-select t

[Eli Zaretskii]

> From: Nicolas Richard <youngfrog@members.fsf.org>
> Date: Thu, 04 Jun 2015 21:47:08 +0200
> 
>     Fix display of overlapping window-specific overlays
>     
>     * src/keyboard.c (adjust_point_for_property): When adjusting point
>     due to display strings, ignore overlays that are specific to
>     windows other than the currently selected one.
>     
>     * src/xdisp.c (handle_single_display_spec): If the display
>     property comes from an overlay, arrange for buffer iteration to
>     resume only after the end of that overlay.  (Bug#20607)
> 
> I understand that
>   get_char_property_and_overlay
>       (make_number (PT), Qdisplay, selected_window,
>        &overlay))
> should not refer to PT but to "PT in selected_window", but I don't know
> how to do that.

Are you saying that adjust_point_for_property is called with the
current buffer different from what's recorded in selected_window's
buffer?  If so, please show the evidence: what is in current_buffer
and what is recorded as the selected window's buffer at that point.

And please note that the conditions in the 'if' clause that determine
whether adjust_point_for_property is called freely manipulate values
of current_buffer and PT, so perhaps the solution is add there a
condition that selected_window's buffer and current_buffer are equal,
such that in the case in point adjust_point_for_property is not called
at all.

To answer your question directly, it should be possible to compute the
(Lisp integer) value of point in the selected window as either

    make_number (marker_position (XWINDOW (selected_window)->pointm))
or
    Fwindow_point (selected_window)
or
    make_number (XBUFFER (XWINDOW (selected_window)->contents)->pt)

bug#20734: 25.0.50; "Args out of range" with help-window-select t

[Nicolas Richard]

Eli Zaretskii <eliz@gnu.org> writes:
>> I understand that
>>   get_char_property_and_overlay
>>       (make_number (PT), Qdisplay, selected_window,
>>        &overlay))
>> should not refer to PT but to "PT in selected_window", but I don't know
>> how to do that.
>
> Are you saying that adjust_point_for_property is called with the
> current buffer different from what's recorded in selected_window's
> buffer?

Yes, that's what I was guessing anyway.

>          If so, please show the evidence: what is in current_buffer
> and what is recorded as the selected window's buffer at that point.

Let me try that. What I did :
1. run under gdb
2. follow the recipe up to the point where I type "C-h f if", but don't
   hit RET
3. then go to gdb window
4. hit C-z to put a breakpoint in adjust_point_for_property
5. continue
6. go back to emacs and hit RET

Now back to gdb :
(gdb) p XBUFFER(XWINDOW(selected_window)->contents)->name_
$1 = 143880388
(gdb) xpr
Lisp_String
$2 = (struct Lisp_String *) 0x89370c0
"*Help*"
(gdb) p current_buffer->name_
$3 = 139646148
(gdb) xpr
Lisp_String
$4 = (struct Lisp_String *) 0x852d4c0
"*Messages*"
(gdb) p PT
$5 = 10067
(gdb) p Fwindow_point (selected_window)
$6 = 6

> And please note that the conditions in the 'if' clause that determine
> whether adjust_point_for_property is called freely manipulate values
> of current_buffer and PT, so perhaps the solution is add there a
> condition that selected_window's buffer and current_buffer are equal,
> such that in the case in point adjust_point_for_property is not called
> at all.

In command_loop_1 ?

I can more or less follow the code step by step, but I don't really
understand it. The only thing I'm now almost 100% confident is that in
this call (found in adjust_point_for_property) :
  get_char_property_and_overlay
      (make_number (PT), Qdisplay, selected_window,
       &overlay))
PT refers to current_buffer, which is different from the buffer in
selected_window in the situation I encountered.

> To answer your question directly, it should be possible to compute the
> (Lisp integer) value of point in the selected window as either

Thanks. This works for me, but is it TRT ?

Commit 53fb39a08a6d18b67ea6073966874fa700d9f97f
References: yf-master
Author:     Nicolas Richard <youngfrog@members.fsf.org>
AuthorDate: Fri Jun 5 11:18:19 2015 +0200
Commit:     Nicolas Richard <youngfrog@members.fsf.org>
CommitDate: Fri Jun 5 11:20:08 2015 +0200

    * src/keyboard.c (adjust_point_for_property): Use selected window (Bug#20590)

1 parent commit, 6 merged branches, 1 containing branch
 Parent     | f10a924 (todo-show): Throw an error if buffer is empty but was modified.
 Merged     | emacs-24 master run-2015-05-29_19-09 running yf-fix-query-replace-with-NULs yf-master
 Containing | yf-master
 Follows    | emacs-24.5-rc3-fixed (4919)

1 file changed, 4 insertions(+), 2 deletions(-)
 src/keyboard.c | 6 ++++--

modified   src/keyboard.c
@@ -1702,8 +1702,10 @@ adjust_point_for_property (ptrdiff_t last_pt, bool modified)
       if (check_display
 	  && PT > BEGV && PT < ZV
 	  && !NILP (val = get_char_property_and_overlay
-		              (make_number (PT), Qdisplay, selected_window,
-			       &overlay))
+		              (make_number
+                               (marker_position
+                                (XWINDOW (selected_window)->pointm)),
+                               Qdisplay, selected_window, &overlay))
 	  && display_prop_intangible_p (val, overlay, PT, PT_BYTE)
 	  && (!OVERLAYP (overlay)
 	      ? get_property_and_range (PT, Qdisplay, &val, &beg, &end, Qnil)

-- 
Nico.

bug#20734: 25.0.50; "Args out of range" with help-window-select t

[Eli Zaretskii]

> From: Nicolas Richard <youngfrog@members.fsf.org>
> Cc: 20734@debbugs.gnu.org
> Date: Fri, 05 Jun 2015 11:25:35 +0200
> 
> 1. run under gdb
> 2. follow the recipe up to the point where I type "C-h f if", but don't
>    hit RET
> 3. then go to gdb window
> 4. hit C-z to put a breakpoint in adjust_point_for_property
> 5. continue
> 6. go back to emacs and hit RET
> 
> Now back to gdb :
> (gdb) p XBUFFER(XWINDOW(selected_window)->contents)->name_
> $1 = 143880388
> (gdb) xpr
> Lisp_String
> $2 = (struct Lisp_String *) 0x89370c0
> "*Help*"

You could also do this, which is slightly easier:

 (gdb) p XWINDOW(selected_window)->contents
 (gdb) xtype # make sure it's a buffer
 (gdb) xbuffer

> > And please note that the conditions in the 'if' clause that determine
> > whether adjust_point_for_property is called freely manipulate values
> > of current_buffer and PT, so perhaps the solution is add there a
> > condition that selected_window's buffer and current_buffer are equal,
> > such that in the case in point adjust_point_for_property is not called
> > at all.
> 
> In command_loop_1 ?

Yes.

> I can more or less follow the code step by step, but I don't really
> understand it. The only thing I'm now almost 100% confident is that in
> this call (found in adjust_point_for_property) :
>   get_char_property_and_overlay
>       (make_number (PT), Qdisplay, selected_window,
>        &overlay))
> PT refers to current_buffer, which is different from the buffer in
> selected_window in the situation I encountered.

Yes, that's the bug.  But the real problem is that we've switched
windows temporarily, and we shouldn't move point in the current buffer
at all when we do that.

> > To answer your question directly, it should be possible to compute the
> > (Lisp integer) value of point in the selected window as either
> 
> Thanks. This works for me, but is it TRT ?

No, I think we shouldn't call adjust_point_for_property in this case
at all.  Can you see if adding

  && EQ (XWINDOW (selected_window)->contents, current_buffer)

to the condition that guards the call to adjust_point_for_property
fixes the problem?  I.e., instead of this (after the label 'finalize'):

      if (current_buffer == prev_buffer
	  && last_point_position != PT
	  && NILP (Vdisable_point_adjustment)
	  && NILP (Vglobal_disable_point_adjustment))

the condition should read

      if (current_buffer == prev_buffer
	  && EQ (XWINDOW (selected_window)->contents, current_buffer)
	  && last_point_position != PT
	  && NILP (Vdisable_point_adjustment)
	  && NILP (Vglobal_disable_point_adjustment))

IOW, if the selected window's buffer is not the current buffer, we
shouldn't be adjusting point in the current buffer at all.

Thanks.

bug#20734: 25.0.50; "Args out of range" with help-window-select t

[Nicolas Richard]

Eli Zaretskii <eliz@gnu.org> writes:
> Yes, that's the bug.  But the real problem is that we've switched
> windows temporarily, and we shouldn't move point in the current buffer
> at all when we do that.

Ok. I think I understand now.  What about adding an assertion to make
sure current_buffer is shown in selected_window at that point?

>     I think we shouldn't call adjust_point_for_property in this case
> at all.  Can you see if adding
>
>   && EQ (XWINDOW (selected_window)->contents, current_buffer)
>
> to the condition that guards the call to adjust_point_for_property
> fixes the problem?

Yes it does, but I wrapped the lhs into XBUFFER :

  && EQ (XBUFFER (XWINDOW (selected_window)->contents), current_buffer)

otherwise it complains:

--8<---------------cut here---------------start------------->8---
In file included from keyboard.c:26:0:
keyboard.c: In function ‘command_loop_1’:
lisp.h:339:34: warning: comparison between pointer and integer [enabled by default]
 #define lisp_h_EQ(x, y) (XLI (x) == XLI (y))
                                  ^
lisp.h:388:19: note: in expansion of macro ‘lisp_h_EQ’
 # define EQ(x, y) lisp_h_EQ (x, y)
                   ^
keyboard.c:1621:7: note: in expansion of macro ‘EQ’
    && EQ (XWINDOW (selected_window)->contents, current_buffer)
       ^
--8<---------------cut here---------------end--------------->8---

-- 
Nico.

bug#20734: 25.0.50; "Args out of range" with help-window-select t

[Eli Zaretskii]

> From: Nicolas Richard <youngfrog@members.fsf.org>
> Cc: 20734@debbugs.gnu.org
> Date: Fri, 05 Jun 2015 21:48:40 +0200
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> > Yes, that's the bug.  But the real problem is that we've switched
> > windows temporarily, and we shouldn't move point in the current buffer
> > at all when we do that.
> 
> Ok. I think I understand now.  What about adding an assertion to make
> sure current_buffer is shown in selected_window at that point?

That'd be fine with me.

> >     I think we shouldn't call adjust_point_for_property in this case
> > at all.  Can you see if adding
> >
> >   && EQ (XWINDOW (selected_window)->contents, current_buffer)
> >
> > to the condition that guards the call to adjust_point_for_property
> > fixes the problem?
> 
> Yes it does, but I wrapped the lhs into XBUFFER :
> 
>   && EQ (XBUFFER (XWINDOW (selected_window)->contents), current_buffer)

Sorry, my bad.  But in that case, you don't need EQ, just use == to
compare 2 C pointers.

bug#20734: 25.0.50; "Args out of range" with help-window-select t

[Nicolas Richard]

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

>> Ok. I think I understand now.  What about adding an assertion to make
>> sure current_buffer is shown in selected_window at that point?
>
> That'd be fine with me.

I can suggest :

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Add-assertion-in-adjust_point_for_property.patch --]
[-- Type: text/x-diff, Size: 971 bytes --]

From 52cd3204743442fc6d8fc281ed3d9b0b0e6a88e4 Mon Sep 17 00:00:00 2001
From: Nicolas Richard <youngfrog@members.fsf.org>
Date: Sat, 6 Jun 2015 10:05:07 +0200
Subject: [PATCH] Add assertion in adjust_point_for_property

* src/keyboard.c (adjust_point_for_property): Add eassert for
current buffer being shown in selected window.
---
 src/keyboard.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/src/keyboard.c b/src/keyboard.c
index bedd10b..23f7ce7 100644
--- a/src/keyboard.c
+++ b/src/keyboard.c
@@ -1687,6 +1687,8 @@ adjust_point_for_property (ptrdiff_t last_pt, bool modified)
   bool check_composition = ! modified, check_display = 1, check_invisible = 1;
   ptrdiff_t orig_pt = PT;
 
+  eassert (XBUFFER (XWINDOW (selected_window)->contents) == current_buffer);
+
   /* FIXME: cycling is probably not necessary because these properties
      can't be usefully combined anyway.  */
   while (check_composition || check_display || check_invisible)
-- 
1.9.1


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


>>   && EQ (XBUFFER (XWINDOW (selected_window)->contents), current_buffer)

>                     in that case, you don't need EQ, just use == to
> compare 2 C pointers.

Ok, I pushed that solution as d31cd49 and I mark the bug as done. Thanks
for your guidance.

-- 
Nico.

bug#20734: 25.0.50; "Args out of range" with help-window-select t

[Eli Zaretskii]

> From: Nicolas Richard <youngfrog@members.fsf.org>
> Cc: 20734-done@debbugs.gnu.org
> Date: Sat, 06 Jun 2015 10:44:26 +0200
> 
> +  eassert (XBUFFER (XWINDOW (selected_window)->contents) == current_buffer);

Fine with me, please push.

Thanks.

> Ok, I pushed that solution as d31cd49 and I mark the bug as done. Thanks
> for your guidance.

Great, thanks.