bug#73838: 31.0.50; Problems in note_mouse_highlight if -nw

bug#73838: 31.0.50; Problems in note_mouse_highlight if -nw

[Gerd Möllmann]

This is with master fdac10b216f7b47e2eea129d2a96807a0c2055f3 on
macOS, built with ASAN.

  $ /Users/gerd/emacs/savannah/master/configure --cache-file /var/folders/1d/k_6t25f94sl83szqbf8gpkrh0000gn/T//config.cache.master --without-tree-sitter --with-native-compilation=no CC=clang 'LDFLAGS=-fsanitize=address -fno-omit-frame-pointer' 'CFLAGS=-Wgnu-imaginary-constant -Wunused-result -g -fno-omit-frame-pointer -g -O0 -fsanitize=address -fno-omit-frame-pointer'

Recipe:

- emacs -nw -q
- M-x xterm-mouse-mode RET
- M-x make TAB
- Move the move over the completion candidates

=> ASAN abort in note_mouse_highlight, xdisp.c:36108

The line number may vary. Looking at that in the debugger, I see

	default:
	  /* This should not happen.  */
	  if (cursor != FRAME_OUTPUT_DATA (f)->nontext_cursor)
	    cursor = FRAME_OUTPUT_DATA (f)->nontext_cursor;

nsterm.h defines FRAME_OUTPUT_DATA(f) as

  #define FRAME_OUTPUT_DATA(f) ((f)->output_data.ns)

and since we are not in a GUI frame, this is no good. Analogous defines
are in xterm.h etc., so the problem is not limited to macOS.

(I built with ASAN because I observe weird things with mouse stuff in
conjunction with tty child frames, and wanted to check what's up with
that in master.)

bug#73838: 31.0.50; Problems in note_mouse_highlight if -nw

[Gerd Möllmann]

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

Gerd Möllmann <gerd.moellmann@gmail.com> writes:

> This is with master fdac10b216f7b47e2eea129d2a96807a0c2055f3 on
> macOS, built with ASAN.
>
>   $ /Users/gerd/emacs/savannah/master/configure --cache-file /var/folders/1d/k_6t25f94sl83szqbf8gpkrh0000gn/T//config.cache.master --without-tree-sitter --with-native-compilation=no CC=clang 'LDFLAGS=-fsanitize=address -fno-omit-frame-pointer' 'CFLAGS=-Wgnu-imaginary-constant -Wunused-result -g -fno-omit-frame-pointer -g -O0 -fsanitize=address -fno-omit-frame-pointer'
>
> Recipe:
>
> - emacs -nw -q
> - M-x xterm-mouse-mode RET
> - M-x make TAB
> - Move the move over the completion candidates
>
> => ASAN abort in note_mouse_highlight, xdisp.c:36108
>
> The line number may vary. Looking at that in the debugger, I see
>
> 	default:
> 	  /* This should not happen.  */
> 	  if (cursor != FRAME_OUTPUT_DATA (f)->nontext_cursor)
> 	    cursor = FRAME_OUTPUT_DATA (f)->nontext_cursor;
>
> nsterm.h defines FRAME_OUTPUT_DATA(f) as
>
>   #define FRAME_OUTPUT_DATA(f) ((f)->output_data.ns)
>
> and since we are not in a GUI frame, this is no good. Analogous defines
> are in xterm.h etc., so the problem is not limited to macOS.
>
> (I built with ASAN because I observe weird things with mouse stuff in
> conjunction with tty child frames, and wanted to check what's up with
> that in master.)

I'd like to propose a change like in the attached patch (from my tty
child frames branch). It fixes the case I mentioned above.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: FRAME_OUTPUT_DATA --]
[-- Type: text/x-patch, Size: 4758 bytes --]

diff --git a/src/nsterm.h b/src/nsterm.h
index 256a233558e..0343064fe5b 100644
--- a/src/nsterm.h
+++ b/src/nsterm.h
@@ -1020,10 +1020,18 @@ #define KEY_NS_SHOW_PREFS              ((1<<28)|(0<<16)|14)
   int unused;
 };
 
+INLINE struct ns_output *
+frame_output_data (struct frame *f)
+{
+  eassert (FRAME_NS_P (f));
+  return f->output_data.ns;
+}
+
+/* xdisp.c checks for this macro being #defined.  */
+#define FRAME_OUTPUT_DATA(f) frame_output_data (f)
 
 /* This gives the ns_display_info structure for the display F is on.  */
 #define FRAME_DISPLAY_INFO(f) ((f)->output_data.ns->display_info)
-#define FRAME_OUTPUT_DATA(f) ((f)->output_data.ns)
 #define FRAME_NS_WINDOW(f) ((f)->output_data.ns->window_desc)
 #define FRAME_NATIVE_WINDOW(f) FRAME_NS_WINDOW (f)
 
diff --git a/src/xdisp.c b/src/xdisp.c
index 25184e8d186..8321702b760 100644
--- a/src/xdisp.c
+++ b/src/xdisp.c
@@ -36106,7 +36106,8 @@ note_mouse_highlight (struct frame *f, int x, int y)
 #ifdef HAVE_WINDOW_SYSTEM
   /* If the cursor is on the internal border of FRAME and FRAME's
      internal border is draggable, provide some visual feedback.  */
-  if (FRAME_INTERNAL_BORDER_WIDTH (f) > 0
+  if (FRAME_WINDOW_P (f)
+      && FRAME_INTERNAL_BORDER_WIDTH (f) > 0
       && !NILP (get_frame_param (f, Qdrag_internal_border)))
     {
       enum internal_border_part part = frame_internal_border_part (f, x, y);
@@ -36170,7 +36171,8 @@ note_mouse_highlight (struct frame *f, int x, int y)
      buffer.  */
   if (EQ (window, f->menu_bar_window))
     {
-      cursor = FRAME_OUTPUT_DATA (f)->nontext_cursor;
+      if (FRAME_WINDOW_P (f))
+	cursor = FRAME_OUTPUT_DATA (f)->nontext_cursor;
       goto set_cursor;
     }
 #endif
@@ -36181,10 +36183,13 @@ note_mouse_highlight (struct frame *f, int x, int y)
   if (EQ (window, f->tab_bar_window))
     {
       note_tab_bar_highlight (f, x, y);
-      if (tab_bar__dragging_in_progress)
-	  cursor = FRAME_OUTPUT_DATA (f)->hand_cursor;
-      else
-	cursor = FRAME_OUTPUT_DATA (f)->nontext_cursor;
+      if (FRAME_WINDOW_P (f))
+	{
+	  if (tab_bar__dragging_in_progress)
+	    cursor = FRAME_OUTPUT_DATA (f)->hand_cursor;
+	  else
+	    cursor = FRAME_OUTPUT_DATA (f)->nontext_cursor;
+	}
       goto set_cursor;
     }
   else
@@ -36203,7 +36208,8 @@ note_mouse_highlight (struct frame *f, int x, int y)
   if (EQ (window, f->tool_bar_window))
     {
       note_tool_bar_highlight (f, x, y);
-      cursor = FRAME_OUTPUT_DATA (f)->nontext_cursor;
+      if (FRAME_WINDOW_P (f))
+	cursor = FRAME_OUTPUT_DATA (f)->nontext_cursor;
       goto set_cursor;
     }
 #endif
@@ -36217,7 +36223,8 @@ note_mouse_highlight (struct frame *f, int x, int y)
 #ifdef HAVE_WINDOW_SYSTEM
       if (part == ON_LEFT_MARGIN || part == ON_RIGHT_MARGIN)
 	{
-	  cursor = FRAME_OUTPUT_DATA (f)->nontext_cursor;
+	  if (FRAME_WINDOW_P (f))
+	    cursor = FRAME_OUTPUT_DATA (f)->nontext_cursor;
 	  /* Show non-text cursor (Bug#16647).  */
 	  goto set_cursor;
 	}
@@ -36229,13 +36236,15 @@ note_mouse_highlight (struct frame *f, int x, int y)
 #ifdef HAVE_WINDOW_SYSTEM
   if (part == ON_VERTICAL_BORDER)
     {
-      cursor = FRAME_OUTPUT_DATA (f)->horizontal_drag_cursor;
+      if (FRAME_WINDOW_P (f))
+	cursor = FRAME_OUTPUT_DATA (f)->horizontal_drag_cursor;
       help_echo_string = build_string ("drag-mouse-1: resize");
       goto set_cursor;
     }
   else if (part == ON_RIGHT_DIVIDER)
     {
-      cursor = FRAME_OUTPUT_DATA (f)->horizontal_drag_cursor;
+      if (FRAME_WINDOW_P (f))
+	cursor = FRAME_OUTPUT_DATA (f)->horizontal_drag_cursor;
       help_echo_string = build_string ("drag-mouse-1: resize");
       goto set_cursor;
     }
@@ -36244,7 +36253,8 @@ note_mouse_highlight (struct frame *f, int x, int y)
 	|| minibuf_level
 	|| NILP (Vresize_mini_windows))
       {
-	cursor = FRAME_OUTPUT_DATA (f)->vertical_drag_cursor;
+	if (FRAME_WINDOW_P (f))
+	  cursor = FRAME_OUTPUT_DATA (f)->vertical_drag_cursor;
 	help_echo_string = build_string ("drag-mouse-1: resize");
 	goto set_cursor;
       }
@@ -36252,13 +36262,17 @@ note_mouse_highlight (struct frame *f, int x, int y)
       cursor = FRAME_OUTPUT_DATA (f)->nontext_cursor;
   else if (part == ON_LEFT_FRINGE || part == ON_RIGHT_FRINGE)
     {
-      cursor = FRAME_OUTPUT_DATA (f)->nontext_cursor;
+      if (FRAME_WINDOW_P (f))
+	cursor = FRAME_OUTPUT_DATA (f)->nontext_cursor;
       note_fringe_highlight (f, window, x, y, part);
     }
   else if (part == ON_VERTICAL_SCROLL_BAR
 	   || part == ON_HORIZONTAL_SCROLL_BAR)
-    cursor = FRAME_OUTPUT_DATA (f)->nontext_cursor;
-  else
+    {
+      if (FRAME_WINDOW_P (f))
+	cursor = FRAME_OUTPUT_DATA (f)->nontext_cursor;
+    }
+  else if (FRAME_WINDOW_P (f))
     cursor = FRAME_OUTPUT_DATA (f)->text_cursor;
 #endif
 

bug#73838: 31.0.50; Problems in note_mouse_highlight if -nw

[Eli Zaretskii]

> From: Gerd Möllmann <gerd.moellmann@gmail.com>
> Date: Wed, 16 Oct 2024 12:47:13 +0200
> 
> This is with master fdac10b216f7b47e2eea129d2a96807a0c2055f3 on
> macOS, built with ASAN.
> 
>   $ /Users/gerd/emacs/savannah/master/configure --cache-file /var/folders/1d/k_6t25f94sl83szqbf8gpkrh0000gn/T//config.cache.master --without-tree-sitter --with-native-compilation=no CC=clang 'LDFLAGS=-fsanitize=address -fno-omit-frame-pointer' 'CFLAGS=-Wgnu-imaginary-constant -Wunused-result -g -fno-omit-frame-pointer -g -O0 -fsanitize=address -fno-omit-frame-pointer'
> 
> Recipe:
> 
> - emacs -nw -q
> - M-x xterm-mouse-mode RET
> - M-x make TAB
> - Move the move over the completion candidates
> 
> => ASAN abort in note_mouse_highlight, xdisp.c:36108
> 
> The line number may vary. Looking at that in the debugger, I see
> 
> 	default:
> 	  /* This should not happen.  */
> 	  if (cursor != FRAME_OUTPUT_DATA (f)->nontext_cursor)
> 	    cursor = FRAME_OUTPUT_DATA (f)->nontext_cursor;
> 
> nsterm.h defines FRAME_OUTPUT_DATA(f) as
> 
>   #define FRAME_OUTPUT_DATA(f) ((f)->output_data.ns)
> 
> and since we are not in a GUI frame, this is no good. Analogous defines
> are in xterm.h etc., so the problem is not limited to macOS.

How come you got to that code on a TTY frame, when the condition for
it is

   if (FRAME_INTERNAL_BORDER_WIDTH (f) > 0
      && !NILP (get_frame_param (f, Qdrag_internal_border)))

FRAME_INTERNAL_BORDER_WIDTH is supposed to be zero on TTY frames.  Why
isn't it?

bug#73838: 31.0.50; Problems in note_mouse_highlight if -nw

[Eli Zaretskii]

> From: Gerd Möllmann <gerd.moellmann@gmail.com>
> Date: Wed, 16 Oct 2024 16:19:53 +0200
> 
> I'd like to propose a change like in the attached patch (from my tty
> child frames branch). It fixes the case I mentioned above.

I'd like to understand why these changes are needed.  Could you please
elaborate on each and every change?

>    if (EQ (window, f->menu_bar_window))
>      {
> -      cursor = FRAME_OUTPUT_DATA (f)->nontext_cursor;
> +      if (FRAME_WINDOW_P (f))
> +	cursor = FRAME_OUTPUT_DATA (f)->nontext_cursor;

Can a TTY frame have a menu-bar window?  AFAIK, a menu bar is just a
frame glyph row on TTY frames, it is not a window.

>    if (EQ (window, f->tab_bar_window))
>      {
>        note_tab_bar_highlight (f, x, y);
> -      if (tab_bar__dragging_in_progress)
> -	  cursor = FRAME_OUTPUT_DATA (f)->hand_cursor;
> -      else
> -	cursor = FRAME_OUTPUT_DATA (f)->nontext_cursor;
> +      if (FRAME_WINDOW_P (f))
> +	{
> +	  if (tab_bar__dragging_in_progress)
> +	    cursor = FRAME_OUTPUT_DATA (f)->hand_cursor;
> +	  else
> +	    cursor = FRAME_OUTPUT_DATA (f)->nontext_cursor;
> +	}

Same question here about tab-bar window.

>    if (EQ (window, f->tool_bar_window))
>      {
>        note_tool_bar_highlight (f, x, y);
> -      cursor = FRAME_OUTPUT_DATA (f)->nontext_cursor;
> +      if (FRAME_WINDOW_P (f))
> +	cursor = FRAME_OUTPUT_DATA (f)->nontext_cursor;
>        goto set_cursor;

Same question here about tool-bar window.

>  #ifdef HAVE_WINDOW_SYSTEM
>        if (part == ON_LEFT_MARGIN || part == ON_RIGHT_MARGIN)
>  	{
> -	  cursor = FRAME_OUTPUT_DATA (f)->nontext_cursor;
> +	  if (FRAME_WINDOW_P (f))
> +	    cursor = FRAME_OUTPUT_DATA (f)->nontext_cursor;

This part I could understand.

>  #ifdef HAVE_WINDOW_SYSTEM
>    if (part == ON_VERTICAL_BORDER)
>      {
> -      cursor = FRAME_OUTPUT_DATA (f)->horizontal_drag_cursor;
> +      if (FRAME_WINDOW_P (f))
> +	cursor = FRAME_OUTPUT_DATA (f)->horizontal_drag_cursor;

Do we have vertical borders on TTY frames?  If yes, this is also
understood.

>    else if (part == ON_RIGHT_DIVIDER)
>      {
> -      cursor = FRAME_OUTPUT_DATA (f)->horizontal_drag_cursor;
> +      if (FRAME_WINDOW_P (f))
> +	cursor = FRAME_OUTPUT_DATA (f)->horizontal_drag_cursor;
>        help_echo_string = build_string ("drag-mouse-1: resize");
>        goto set_cursor;

Do we have right dividers on TTY frames?

> @@ -36244,7 +36253,8 @@ note_mouse_highlight (struct frame *f, int x, int y)
>  	|| minibuf_level
>  	|| NILP (Vresize_mini_windows))
>        {
> -	cursor = FRAME_OUTPUT_DATA (f)->vertical_drag_cursor;
> +	if (FRAME_WINDOW_P (f))
> +	  cursor = FRAME_OUTPUT_DATA (f)->vertical_drag_cursor;
>  	help_echo_string = build_string ("drag-mouse-1: resize");

Same question about bottom dividers as above about right dividers.

>    else if (part == ON_LEFT_FRINGE || part == ON_RIGHT_FRINGE)
>      {
> -      cursor = FRAME_OUTPUT_DATA (f)->nontext_cursor;
> +      if (FRAME_WINDOW_P (f))
> +	cursor = FRAME_OUTPUT_DATA (f)->nontext_cursor;

How can fringes happen on a TTY frame?

>    else if (part == ON_VERTICAL_SCROLL_BAR
>  	   || part == ON_HORIZONTAL_SCROLL_BAR)
> -    cursor = FRAME_OUTPUT_DATA (f)->nontext_cursor;
> -  else
> +    {
> +      if (FRAME_WINDOW_P (f))
> +	cursor = FRAME_OUTPUT_DATA (f)->nontext_cursor;
> +    }
> +  else if (FRAME_WINDOW_P (f))
>      cursor = FRAME_OUTPUT_DATA (f)->text_cursor;
>  #endif

How can scroll bars happen on TTY frames?

bug#73838: 31.0.50; Problems in note_mouse_highlight if -nw

[Gerd Möllmann]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Gerd Möllmann <gerd.moellmann@gmail.com>
>> Date: Wed, 16 Oct 2024 12:47:13 +0200
>> 
>> This is with master fdac10b216f7b47e2eea129d2a96807a0c2055f3 on
>> macOS, built with ASAN.
>> 
>>   $ /Users/gerd/emacs/savannah/master/configure --cache-file /var/folders/1d/k_6t25f94sl83szqbf8gpkrh0000gn/T//config.cache.master --without-tree-sitter --with-native-compilation=no CC=clang 'LDFLAGS=-fsanitize=address -fno-omit-frame-pointer' 'CFLAGS=-Wgnu-imaginary-constant -Wunused-result -g -fno-omit-frame-pointer -g -O0 -fsanitize=address -fno-omit-frame-pointer'
>> 
>> Recipe:
>> 
>> - emacs -nw -q
>> - M-x xterm-mouse-mode RET
>> - M-x make TAB
>> - Move the move over the completion candidates
>> 
>> => ASAN abort in note_mouse_highlight, xdisp.c:36108
>> 
>> The line number may vary. Looking at that in the debugger, I see
>> 
>> 	default:
>> 	  /* This should not happen.  */
>> 	  if (cursor != FRAME_OUTPUT_DATA (f)->nontext_cursor)
>> 	    cursor = FRAME_OUTPUT_DATA (f)->nontext_cursor;
>> 
>> nsterm.h defines FRAME_OUTPUT_DATA(f) as
>> 
>>   #define FRAME_OUTPUT_DATA(f) ((f)->output_data.ns)
>> 
>> and since we are not in a GUI frame, this is no good. Analogous defines
>> are in xterm.h etc., so the problem is not limited to macOS.
>
> How come you got to that code on a TTY frame, when the condition for
> it is
>
>    if (FRAME_INTERNAL_BORDER_WIDTH (f) > 0
>       && !NILP (get_frame_param (f, Qdrag_internal_border)))
>
> FRAME_INTERNAL_BORDER_WIDTH is supposed to be zero on TTY frames.  Why
> isn't it?

I'm not sure but I meanwhile get the impression that ASAN on Xcode 16
doesn't report correct line numbers anymore. Probably the problem was
somewhere else.

bug#73838: 31.0.50; Problems in note_mouse_highlight if -nw

[Gerd Möllmann]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Gerd Möllmann <gerd.moellmann@gmail.com>
>> Date: Wed, 16 Oct 2024 16:19:53 +0200
>> 
>> I'd like to propose a change like in the attached patch (from my tty
>> child frames branch). It fixes the case I mentioned above.
>
> I'd like to understand why these changes are needed.  Could you please
> elaborate on each and every change?
>
>>    if (EQ (window, f->menu_bar_window))
>>      {
>> -      cursor = FRAME_OUTPUT_DATA (f)->nontext_cursor;
>> +      if (FRAME_WINDOW_P (f))
>> +	cursor = FRAME_OUTPUT_DATA (f)->nontext_cursor;
>
> Can a TTY frame have a menu-bar window?  AFAIK, a menu bar is just a
> frame glyph row on TTY frames, it is not a window.
>
>>    if (EQ (window, f->tab_bar_window))
>>      {
>>        note_tab_bar_highlight (f, x, y);
>> -      if (tab_bar__dragging_in_progress)
>> -	  cursor = FRAME_OUTPUT_DATA (f)->hand_cursor;
>> -      else
>> -	cursor = FRAME_OUTPUT_DATA (f)->nontext_cursor;
>> +      if (FRAME_WINDOW_P (f))
>> +	{
>> +	  if (tab_bar__dragging_in_progress)
>> +	    cursor = FRAME_OUTPUT_DATA (f)->hand_cursor;
>> +	  else
>> +	    cursor = FRAME_OUTPUT_DATA (f)->nontext_cursor;
>> +	}
>
> Same question here about tab-bar window.
>
>>    if (EQ (window, f->tool_bar_window))
>>      {
>>        note_tool_bar_highlight (f, x, y);
>> -      cursor = FRAME_OUTPUT_DATA (f)->nontext_cursor;
>> +      if (FRAME_WINDOW_P (f))
>> +	cursor = FRAME_OUTPUT_DATA (f)->nontext_cursor;
>>        goto set_cursor;
>
> Same question here about tool-bar window.
>
>>  #ifdef HAVE_WINDOW_SYSTEM
>>        if (part == ON_LEFT_MARGIN || part == ON_RIGHT_MARGIN)
>>  	{
>> -	  cursor = FRAME_OUTPUT_DATA (f)->nontext_cursor;
>> +	  if (FRAME_WINDOW_P (f))
>> +	    cursor = FRAME_OUTPUT_DATA (f)->nontext_cursor;
>
> This part I could understand.
>
>>  #ifdef HAVE_WINDOW_SYSTEM
>>    if (part == ON_VERTICAL_BORDER)
>>      {
>> -      cursor = FRAME_OUTPUT_DATA (f)->horizontal_drag_cursor;
>> +      if (FRAME_WINDOW_P (f))
>> +	cursor = FRAME_OUTPUT_DATA (f)->horizontal_drag_cursor;
>
> Do we have vertical borders on TTY frames?  If yes, this is also
> understood.
>
>>    else if (part == ON_RIGHT_DIVIDER)
>>      {
>> -      cursor = FRAME_OUTPUT_DATA (f)->horizontal_drag_cursor;
>> +      if (FRAME_WINDOW_P (f))
>> +	cursor = FRAME_OUTPUT_DATA (f)->horizontal_drag_cursor;
>>        help_echo_string = build_string ("drag-mouse-1: resize");
>>        goto set_cursor;
>
> Do we have right dividers on TTY frames?
>
>> @@ -36244,7 +36253,8 @@ note_mouse_highlight (struct frame *f, int x, int y)
>>  	|| minibuf_level
>>  	|| NILP (Vresize_mini_windows))
>>        {
>> -	cursor = FRAME_OUTPUT_DATA (f)->vertical_drag_cursor;
>> +	if (FRAME_WINDOW_P (f))
>> +	  cursor = FRAME_OUTPUT_DATA (f)->vertical_drag_cursor;
>>  	help_echo_string = build_string ("drag-mouse-1: resize");
>
> Same question about bottom dividers as above about right dividers.
>
>>    else if (part == ON_LEFT_FRINGE || part == ON_RIGHT_FRINGE)
>>      {
>> -      cursor = FRAME_OUTPUT_DATA (f)->nontext_cursor;
>> +      if (FRAME_WINDOW_P (f))
>> +	cursor = FRAME_OUTPUT_DATA (f)->nontext_cursor;
>
> How can fringes happen on a TTY frame?
>
>>    else if (part == ON_VERTICAL_SCROLL_BAR
>>  	   || part == ON_HORIZONTAL_SCROLL_BAR)
>> -    cursor = FRAME_OUTPUT_DATA (f)->nontext_cursor;
>> -  else
>> +    {
>> +      if (FRAME_WINDOW_P (f))
>> +	cursor = FRAME_OUTPUT_DATA (f)->nontext_cursor;
>> +    }
>> +  else if (FRAME_WINDOW_P (f))
>>      cursor = FRAME_OUTPUT_DATA (f)->text_cursor;
>>  #endif
>
> How can scroll bars happen on TTY frames?

I'll try to answer this a bit more broadly: I'd like the code to be more
resilient, and not make implicit assumptions about the absence of of
features on ttys, which when false, leads to such hard to detect errors.

Consider the internal border case. In master, FRAME_INTERNAL_BORDER
happens to return 0 in master. But that might change, if I ever get
setting internal borders to work for tty child frames, to draw the
border there.

bug#73838: 31.0.50; Problems in note_mouse_highlight if -nw

[Eli Zaretskii]

> From: Gerd Möllmann <gerd.moellmann@gmail.com>
> Cc: 73838@debbugs.gnu.org
> Date: Wed, 16 Oct 2024 18:56:53 +0200
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > How can scroll bars happen on TTY frames?
> 
> I'll try to answer this a bit more broadly: I'd like the code to be more
> resilient, and not make implicit assumptions about the absence of of
> features on ttys, which when false, leads to such hard to detect errors.
> 
> Consider the internal border case. In master, FRAME_INTERNAL_BORDER
> happens to return 0 in master. But that might change, if I ever get
> setting internal borders to work for tty child frames, to draw the
> border there.

But then we need to audit a lot more than just these bits.  E.g., who
will guarantee that FRAME_WINDOW_P is always zero on TTY frames?

More seriously, I think we should prefer functional tests to
declarative tests.  Which means not to assume that TTY frames can
never have some display feature.  Thus, IMO your suggestion is in a
sense a step back, because it assumes that TTY frames can never have
these decorations and can never have different cursor types.  So my
suggestion would be to do the opposite: understand why
FRAME_OUTPUT_DATA segfaults when dereferenced on TTY frames, and fix
that so that it doesn't.

bug#73838: 31.0.50; Problems in note_mouse_highlight if -nw

[Gerd Möllmann]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Gerd Möllmann <gerd.moellmann@gmail.com>
>> Cc: 73838@debbugs.gnu.org
>> Date: Wed, 16 Oct 2024 18:56:53 +0200
>> 
>> Eli Zaretskii <eliz@gnu.org> writes:
>> 
>> > How can scroll bars happen on TTY frames?
>> 
>> I'll try to answer this a bit more broadly: I'd like the code to be more
>> resilient, and not make implicit assumptions about the absence of of
>> features on ttys, which when false, leads to such hard to detect errors.
>> 
>> Consider the internal border case. In master, FRAME_INTERNAL_BORDER
>> happens to return 0 in master. But that might change, if I ever get
>> setting internal borders to work for tty child frames, to draw the
>> border there.
>
> But then we need to audit a lot more than just these bits.  E.g., who
> will guarantee that FRAME_WINDOW_P is always zero on TTY frames?

Quite unlikely, it's the only way we know what is in the output_data
union :-). Anyway.

>
> More seriously, I think we should prefer functional tests to
> declarative tests.  Which means not to assume that TTY frames can
> never have some display feature.  

That would be good. But if I may add that from my experience with the
child frames -- we're far from it.

> Thus, IMO your suggestion is in a sense a step back, because it
> assumes that TTY frames can never have these decorations and can never
> have different cursor types. So my suggestion would be to do the
> opposite: understand why FRAME_OUTPUT_DATA segfaults when dereferenced
> on TTY frames, and fix that so that it doesn't.

But the current situation is that we follow from the presence of an
internal border that it's a window system frame. We're using
FRAME_OUTPUT_DATA. If that would segv it would be a good thing. It
doesn't do that, it just silently accesses some unrelated memory (in my
case this is equivalent to casting the actual output_data contents to
(struct ns_output *) regardless of what it actually is.

I've just dragged the FRAME_WINDOW_P out of this stuff because the
whole if-statement is concerned with cursor = ... using FRAME_OUTPUT_DATA.

bug#73838: 31.0.50; Problems in note_mouse_highlight if -nw

[Eli Zaretskii]

> From: Gerd Möllmann <gerd.moellmann@gmail.com>
> Cc: 73838@debbugs.gnu.org
> Date: Wed, 16 Oct 2024 21:03:15 +0200
> 
> > Thus, IMO your suggestion is in a sense a step back, because it
> > assumes that TTY frames can never have these decorations and can never
> > have different cursor types. So my suggestion would be to do the
> > opposite: understand why FRAME_OUTPUT_DATA segfaults when dereferenced
> > on TTY frames, and fix that so that it doesn't.
> 
> But the current situation is that we follow from the presence of an
> internal border that it's a window system frame. We're using
> FRAME_OUTPUT_DATA. If that would segv it would be a good thing. It
> doesn't do that, it just silently accesses some unrelated memory (in my
> case this is equivalent to casting the actual output_data contents to
> (struct ns_output *) regardless of what it actually is.
> 
> I've just dragged the FRAME_WINDOW_P out of this stuff because the
> whole if-statement is concerned with cursor = ... using FRAME_OUTPUT_DATA.

My suggestion is to extend 'struct tty_display_info' so that
FRAME_OUTPUT_DATA on TTY frames will not access unrelated memory, when
these macros/inline functions are called.  Alternatively, we could have
the macros/functions (FRAME_INTERNAL_BORDER etc.) test for TTY frame
and DTRT.

bug#73838: 31.0.50; Problems in note_mouse_highlight if -nw

[Gerd Möllmann]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Gerd Möllmann <gerd.moellmann@gmail.com>
>> Cc: 73838@debbugs.gnu.org
>> Date: Wed, 16 Oct 2024 21:03:15 +0200
>> 
>> > Thus, IMO your suggestion is in a sense a step back, because it
>> > assumes that TTY frames can never have these decorations and can never
>> > have different cursor types. So my suggestion would be to do the
>> > opposite: understand why FRAME_OUTPUT_DATA segfaults when dereferenced
>> > on TTY frames, and fix that so that it doesn't.
>> 
>> But the current situation is that we follow from the presence of an
>> internal border that it's a window system frame. We're using
>> FRAME_OUTPUT_DATA. If that would segv it would be a good thing. It
>> doesn't do that, it just silently accesses some unrelated memory (in my
>> case this is equivalent to casting the actual output_data contents to
>> (struct ns_output *) regardless of what it actually is.
>> 
>> I've just dragged the FRAME_WINDOW_P out of this stuff because the
>> whole if-statement is concerned with cursor = ... using FRAME_OUTPUT_DATA.
>
> My suggestion is to extend 'struct tty_display_info' so that
> FRAME_OUTPUT_DATA on TTY frames will not access unrelated memory, when
> these macros/inline functions are called.  Alternatively, we could have
> the macros/functions (FRAME_INTERNAL_BORDER etc.) test for TTY frame
> and DTRT.

FRAME_OUTPUT_DATA is meaningful only for window system frames. Each
window system's "term" header defines it. For example

  xterm.h:
  #define FRAME_X_OUTPUT(f) ((f)->output_data.x)
  #define FRAME_OUTPUT_DATA(f) FRAME_X_OUTPUT (f)

  nsterm.h:
  #define FRAME_OUTPUT_DATA(f) ((f)->output_data.ns)

and so on. So using FRAME_OUTPUT_DATA is per se only valid if
FRAME_WINDOW_P. Which is equivalent to FRAME_NS_P in my case, or
whatever someone uses.

  #ifdef HAVE_X_WINDOWS
  #define FRAME_WINDOW_P(f) FRAME_X_P (f)
  #endif
  #ifdef HAVE_NS
  #define FRAME_WINDOW_P(f) FRAME_NS_P(f)
  #endif
  #ifndef FRAME_WINDOW_P
  #define FRAME_WINDOW_P(f) ((void) (f), false)
  #endif

I think changing that would be a major surgery. It's probably easier to
add checks like I did in the patch to FRAME_OUTPUT_DATA if the frame in
questioin is indeed a window system frame. It can be decided at run-time
only anyway.

The other idea is, IIUC, is to make code using FRAME_OUTPUT_DATA like
this one

  if (FRAME_INTERNAL_BORDER_WIDTH (f) > 0
      && !NILP (get_frame_param (f, Qdrag_internal_border)))
    {
      enum internal_border_part part = frame_internal_border_part (f, x, y);

      switch (part)
	{
	case INTERNAL_BORDER_NONE:
	  if (cursor != FRAME_OUTPUT_DATA (f)->nontext_cursor)
	    /* Reset cursor.  */
	    cursor = FRAME_OUTPUT_DATA (f)->nontext_cursor;

work by making sure that their if-conditions can't be true, if there
any. In the above case by making FRAME_INTERNAL_BORDER_WIDTH return 0 if the
frame is not FRAME_WINDOW_P. In other cases like this one

  if (EQ (window, f->tool_bar_window))
    {
      note_tool_bar_highlight (f, x, y);
      cursor = FRAME_OUTPUT_DATA (f)->nontext_cursor;

or here

  if (part == ON_MODE_LINE || part == ON_HEADER_LINE || part == ON_TAB_LINE
      || part == ON_LEFT_MARGIN || part == ON_RIGHT_MARGIN)
    {
      note_mode_line_or_margin_highlight (window, x, y, part);

#ifdef HAVE_WINDOW_SYSTEM
      if (part == ON_LEFT_MARGIN || part == ON_RIGHT_MARGIN)
	{
	  cursor = FRAME_OUTPUT_DATA (f)->nontext_cursor;
	  /* Show non-text cursor (Bug#16647).  */
	  goto set_cursor;
	}
      else
#endif
	return;
    }

by doing something else.

I have to admit that I don't like that. I don't understand what is wrong
to check FRAME_WINDOW_P before using something (using FRAME_OUTPUT_DATA)
that requires FRAME_WINDOW_P to be valid.

bug#73838: 31.0.50; Problems in note_mouse_highlight if -nw

[Eli Zaretskii]

> From: Gerd Möllmann <gerd.moellmann@gmail.com>
> Cc: 73838@debbugs.gnu.org
> Date: Thu, 17 Oct 2024 07:07:44 +0200
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > My suggestion is to extend 'struct tty_display_info' so that
> > FRAME_OUTPUT_DATA on TTY frames will not access unrelated memory, when
> > these macros/inline functions are called.  Alternatively, we could have
> > the macros/functions (FRAME_INTERNAL_BORDER etc.) test for TTY frame
> > and DTRT.
> 
> FRAME_OUTPUT_DATA is meaningful only for window system frames. Each
> window system's "term" header defines it. For example
> 
>   xterm.h:
>   #define FRAME_X_OUTPUT(f) ((f)->output_data.x)
>   #define FRAME_OUTPUT_DATA(f) FRAME_X_OUTPUT (f)
> 
>   nsterm.h:
>   #define FRAME_OUTPUT_DATA(f) ((f)->output_data.ns)
> 
> and so on. So using FRAME_OUTPUT_DATA is per se only valid if
> FRAME_WINDOW_P. Which is equivalent to FRAME_NS_P in my case, or
> whatever someone uses.
> 
>   #ifdef HAVE_X_WINDOWS
>   #define FRAME_WINDOW_P(f) FRAME_X_P (f)
>   #endif
>   #ifdef HAVE_NS
>   #define FRAME_WINDOW_P(f) FRAME_NS_P(f)
>   #endif
>   #ifndef FRAME_WINDOW_P
>   #define FRAME_WINDOW_P(f) ((void) (f), false)
>   #endif

My suggestion is to change the last part.  output_data is defined like
so:

  union output_data
  {
    struct tty_output *tty;		/* From termchar.h.  */
    struct x_output *x;			/* From xterm.h.  */
    struct w32_output *w32;		/* From w32term.h.  */
    struct ns_output *ns;		/* From nsterm.h.  */
    struct pgtk_output *pgtk;		/* From pgtkterm.h. */
    struct haiku_output *haiku;		/* From haikuterm.h. */
    struct android_output *android;	/* From androidterm.h.  */
  }
  output_data;

So it exists in TTY frames as well, and we could have bitfields in it
that correspond to the window-system's versions of output_data to
specify internal-border and other decorations.  We just need to make
sure these bitfields are zero as long as TTY frames don't support
those features.  Then we could avoid the FRAME_WINDOW_P tests which
you propose to add, and still have valid and future-proof code.

I thought you were proposing a future-proof change that will avoid the
need to dig into these macros and change them if and when TTY frames
gain more functionality.  If that's not what you are suggesting, I
wonder what is wrong with the current code that we need to make
changes which are basically half-solutions.  If the problem is access
to unrelated memory, that is easy to fix by just adding enough slack
to tty_output definition, for example.  Adding tests slows down
redisplay, albeit by a very small amount in each case (but these
slow-downs add up, since we use these macros all over the place).

> I think changing that would be a major surgery. It's probably easier to
> add checks like I did in the patch to FRAME_OUTPUT_DATA if the frame in
> questioin is indeed a window system frame. It can be decided at run-time
> only anyway.

It might be easier, but if we are going to make changes, why not do it
the right way to begin with?  After all, if what's problematic in the
current code is that valgrind or ASAN complain, we could simply regard
that as false positives, since there are no problems in production
builds.

> The other idea is, IIUC, is to make code using FRAME_OUTPUT_DATA like
> this one
> 
>   if (FRAME_INTERNAL_BORDER_WIDTH (f) > 0
>       && !NILP (get_frame_param (f, Qdrag_internal_border)))
>     {
>       enum internal_border_part part = frame_internal_border_part (f, x, y);
> 
>       switch (part)
> 	{
> 	case INTERNAL_BORDER_NONE:
> 	  if (cursor != FRAME_OUTPUT_DATA (f)->nontext_cursor)
> 	    /* Reset cursor.  */
> 	    cursor = FRAME_OUTPUT_DATA (f)->nontext_cursor;
> 
> work by making sure that their if-conditions can't be true, if there
> any. In the above case by making FRAME_INTERNAL_BORDER_WIDTH return 0 if the
> frame is not FRAME_WINDOW_P. In other cases like this one
> 
>   if (EQ (window, f->tool_bar_window))
>     {
>       note_tool_bar_highlight (f, x, y);
>       cursor = FRAME_OUTPUT_DATA (f)->nontext_cursor;
> 
> or here
> 
>   if (part == ON_MODE_LINE || part == ON_HEADER_LINE || part == ON_TAB_LINE
>       || part == ON_LEFT_MARGIN || part == ON_RIGHT_MARGIN)
>     {
>       note_mode_line_or_margin_highlight (window, x, y, part);
> 
> #ifdef HAVE_WINDOW_SYSTEM
>       if (part == ON_LEFT_MARGIN || part == ON_RIGHT_MARGIN)
> 	{
> 	  cursor = FRAME_OUTPUT_DATA (f)->nontext_cursor;
> 	  /* Show non-text cursor (Bug#16647).  */
> 	  goto set_cursor;
> 	}
>       else
> #endif
> 	return;
>     }
> 
> by doing something else.
> 
> I have to admit that I don't like that. I don't understand what is wrong
> to check FRAME_WINDOW_P before using something (using FRAME_OUTPUT_DATA)
> that requires FRAME_WINDOW_P to be valid.

Let me try explaining once again what I think is wrong with testing
FRAME_WINDOW_P in each of those cases.  Imagine that someone develops
a feature whereby TTY frames could have scroll bars or fringes.  With
the method you propose, we'd then need to change all the places where
the code accesses scroll bars or fringes, and remove the
FRAME_WINDOW_P and similar tests.  If some of these tests are
forgotten and not removed/rewritten, we'd have a subtle bug.  By
contrast, the way I propose it, whereby tty_output will have extended
to have the corresponding fields, all we'd need is to set those fields
to some non-zero values, and the rest will "just work".

IOW, my proposal is to make the tests be based on supported
functionalities rather than on some attribute of a frame which _today_
is associated with the lack of such functionalities, for the same
reasons we prefer testing functionalities with HAVE_SOMETHING to
testing version numbers or OS-specific symbols.  Because the
correlation between frame types and available functionalities can
change in the future, and then adjusting the code to such changes is
usually a tedious and error-prone job.  We had ample examples of that
when TTY frames learned to display colors, then again when they
learned to show menus and dialogs.  We should learn from those
examples.

bug#73838: 31.0.50; Problems in note_mouse_highlight if -nw

[Gerd Möllmann]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Gerd Möllmann <gerd.moellmann@gmail.com>
>> Cc: 73838@debbugs.gnu.org
>> Date: Thu, 17 Oct 2024 07:07:44 +0200
>> 
>> Eli Zaretskii <eliz@gnu.org> writes:
>> 
>> > My suggestion is to extend 'struct tty_display_info' so that
>> > FRAME_OUTPUT_DATA on TTY frames will not access unrelated memory, when
>> > these macros/inline functions are called.  Alternatively, we could have
>> > the macros/functions (FRAME_INTERNAL_BORDER etc.) test for TTY frame
>> > and DTRT.
>> 
>> FRAME_OUTPUT_DATA is meaningful only for window system frames. Each
>> window system's "term" header defines it. For example
>> 
>>   xterm.h:
>>   #define FRAME_X_OUTPUT(f) ((f)->output_data.x)
>>   #define FRAME_OUTPUT_DATA(f) FRAME_X_OUTPUT (f)
>> 
>>   nsterm.h:
>>   #define FRAME_OUTPUT_DATA(f) ((f)->output_data.ns)
>> 
>> and so on. So using FRAME_OUTPUT_DATA is per se only valid if
>> FRAME_WINDOW_P. Which is equivalent to FRAME_NS_P in my case, or
>> whatever someone uses.
>> 
>>   #ifdef HAVE_X_WINDOWS
>>   #define FRAME_WINDOW_P(f) FRAME_X_P (f)
>>   #endif
>>   #ifdef HAVE_NS
>>   #define FRAME_WINDOW_P(f) FRAME_NS_P(f)
>>   #endif
>>   #ifndef FRAME_WINDOW_P
>>   #define FRAME_WINDOW_P(f) ((void) (f), false)
>>   #endif
>
> My suggestion is to change the last part.  output_data is defined like
> so:
>
>   union output_data
>   {
>     struct tty_output *tty;		/* From termchar.h.  */
>     struct x_output *x;			/* From xterm.h.  */
>     struct w32_output *w32;		/* From w32term.h.  */
>     struct ns_output *ns;		/* From nsterm.h.  */
>     struct pgtk_output *pgtk;		/* From pgtkterm.h. */
>     struct haiku_output *haiku;		/* From haikuterm.h. */
>     struct android_output *android;	/* From androidterm.h.  */
>   }
>   output_data;
>
> So it exists in TTY frames as well, and we could have bitfields in it
> that correspond to the window-system's versions of output_data to
> specify internal-border and other decorations.  We just need to make
> sure these bitfields are zero as long as TTY frames don't support
> those features.  Then we could avoid the FRAME_WINDOW_P tests which
> you propose to add, and still have valid and future-proof code.

Ok, I think I understand that. That requires some "base class" design so
to speak, which all the xy_output types "inherit" from, right? We don't
have that ATM, at least not for NS and X. I agree that would be nice.
But it's a lot of work, and probably involves changing code for all
platforms. Which I can't.

> I thought you were proposing a future-proof change that will avoid the
> need to dig into these macros and change them if and when TTY frames
> gain more functionality.

Well how can I put it. I have actually two problems, let my try to
explain :-).

The immediate problem I'm facing is with tty child frames and
xterm-mouse: I'm opening a buffer selection child frame (consult-buffer)
and choose a candidate with a mouse click. The candidates are
mouse-highlighted. Result is eventually an endless loop in
process_mark_stack in the non-MPS GC. (Not using the mouse works just
fine.) Enabling chekcing to the max shhows nothing, so I build with
ASAN. And I'm getting stuck early because ASAN shows the invalid access
via FRAME_OUTPUT_DATA this bug is about.

My second, more general, problem is that the different types of frames
are handled so differently in our code, and that it's so difficult to
get things to work. I think I've spent at least 90% of the time I spent
with child frames with that. The internal-border stuff is an example. I
tried to add that for tty frames, for the borders, and it was such a
mess (even behaving differently if HVE_WINDOW_SYSTEM or not, i.e.
configuring --without-ns or --with-ns) that I git reset --hard in a rage
in the end :-). I'd be quite interested to improve that situation, but
I'd rather get so far that I can tackle my immediate problem.

> If that's not what you are suggesting, I
> wonder what is wrong with the current code that we need to make
> changes which are basically half-solutions.  If the problem is access
> to unrelated memory, that is easy to fix by just adding enough slack
> to tty_output definition, for example.

You mean by making sizeof (struct tty_output) == sizeof (ns_output) in
my case, and let it go? Bloodcurdlingly horrible! I don't even want to
think about it.

> Adding tests slows down
> redisplay, albeit by a very small amount in each case (but these
> slow-downs add up, since we use these macros all over the place).
>
>> I think changing that would be a major surgery. It's probably easier to
>> add checks like I did in the patch to FRAME_OUTPUT_DATA if the frame in
>> questioin is indeed a window system frame. It can be decided at run-time
>> only anyway.
>
> It might be easier, but if we are going to make changes, why not do it
> the right way to begin with?  After all, if what's problematic in the
> current code is that valgrind or ASAN complain, we could simply regard
> that as false positives, since there are no problems in production
> builds.

A "false positive" that provents me from using ASAN to find a real
problem.

>
>> The other idea is, IIUC, is to make code using FRAME_OUTPUT_DATA like
>> this one
>> 
...
>> I have to admit that I don't like that. I don't understand what is wrong
>> to check FRAME_WINDOW_P before using something (using FRAME_OUTPUT_DATA)
>> that requires FRAME_WINDOW_P to be valid.
>
> Let me try explaining once again what I think is wrong with testing
> FRAME_WINDOW_P in each of those cases.  Imagine that someone develops
> a feature whereby TTY frames could have scroll bars or fringes.  With
> the method you propose, we'd then need to change all the places where
> the code accesses scroll bars or fringes, and remove the
> FRAME_WINDOW_P and similar tests.  If some of these tests are
> forgotten and not removed/rewritten, we'd have a subtle bug.  By
> contrast, the way I propose it, whereby tty_output will have extended
> to have the corresponding fields, all we'd need is to set those fields
> to some non-zero values, and the rest will "just work".
>
> IOW, my proposal is to make the tests be based on supported
> functionalities rather than on some attribute of a frame which _today_
> is associated with the lack of such functionalities, for the same
> reasons we prefer testing functionalities with HAVE_SOMETHING to
> testing version numbers or OS-specific symbols.  Because the
> correlation between frame types and available functionalities can
> change in the future, and then adjusting the code to such changes is
> usually a tedious and error-prone job.  We had ample examples of that
> when TTY frames learned to display colors, then again when they
> learned to show menus and dialogs.  We should learn from those
> examples.

I'm think I understand that. And I can understand that you are not
interested unless a "grander" solution is available, maybe something
like the "base class" approach I tried to describe above. So be it.

I think you can as well close this bug as wontfix. It's unlikely that
I'll work in the direction you would like to see in the forseeable
future.

bug#73838: 31.0.50; Problems in note_mouse_highlight if -nw

[Eli Zaretskii]

> From: Gerd Möllmann <gerd.moellmann@gmail.com>
> Cc: 73838@debbugs.gnu.org
> Date: Thu, 17 Oct 2024 09:03:13 +0200
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > If that's not what you are suggesting, I
> > wonder what is wrong with the current code that we need to make
> > changes which are basically half-solutions.  If the problem is access
> > to unrelated memory, that is easy to fix by just adding enough slack
> > to tty_output definition, for example.
> 
> You mean by making sizeof (struct tty_output) == sizeof (ns_output) in
> my case, and let it go? Bloodcurdlingly horrible! I don't even want to
> think about it.

Why is it a problem to add some dummy buffer to a struct? what's so
horrible about that?  The reason is to let ASAN build run without
false positives.

Alternatively, cannot you tell ASAN in some way that this code is
okay?

> I'm think I understand that. And I can understand that you are not
> interested unless a "grander" solution is available, maybe something
> like the "base class" approach I tried to describe above. So be it.
> 
> I think you can as well close this bug as wontfix. It's unlikely that
> I'll work in the direction you would like to see in the forseeable
> future.

Understood.

bug#73838: 31.0.50; Problems in note_mouse_highlight if -nw

[Eli Zaretskii]

> From: Gerd Möllmann <gerd.moellmann@gmail.com>
> Cc: 73838@debbugs.gnu.org
> Date: Thu, 17 Oct 2024 09:03:13 +0200
> 
> The immediate problem I'm facing is with tty child frames and
> xterm-mouse: I'm opening a buffer selection child frame (consult-buffer)
> and choose a candidate with a mouse click. The candidates are
> mouse-highlighted. Result is eventually an endless loop in
> process_mark_stack in the non-MPS GC. (Not using the mouse works just
> fine.)

If you can show the details of that, i.e. step through the loop one
time until it gets to the same point, maybe someone could have an
idea.

bug#73838: 31.0.50; Problems in note_mouse_highlight if -nw

[Gerd Möllmann]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Gerd Möllmann <gerd.moellmann@gmail.com>
>> Cc: 73838@debbugs.gnu.org
>> Date: Thu, 17 Oct 2024 09:03:13 +0200
>> 
>> Eli Zaretskii <eliz@gnu.org> writes:
>> 
>> > If that's not what you are suggesting, I
>> > wonder what is wrong with the current code that we need to make
>> > changes which are basically half-solutions.  If the problem is access
>> > to unrelated memory, that is easy to fix by just adding enough slack
>> > to tty_output definition, for example.
>> 
>> You mean by making sizeof (struct tty_output) == sizeof (ns_output) in
>> my case, and let it go? Bloodcurdlingly horrible! I don't even want to
>> think about it.
>
> Why is it a problem to add some dummy buffer to a struct? what's so
> horrible about that?  The reason is to let ASAN build run without
> false positives.

Well, for me it's not a false positive. ASAN is absolutely right about
it says. We are accessing memory that's not there because we are
accessing something of the wrong type. Making the memory the memory
appear doesn't change the fact that we are still accessing something as
the wrong type. I find that horrible, sorry :-).

But it doesn't matter. I have what I posted in my branch, so it doesn't
prevent me personally from proceeding.

> Alternatively, cannot you tell ASAN in some way that this code is
> okay?

The docs say clang supports an ottribute for that 

  Disabling Instrumentation with __attribute__((no_sanitize("address")))

Haven'tever used that, though, so I don't know if it works.

bug#73838: 31.0.50; Problems in note_mouse_highlight if -nw

[Gerd Möllmann]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Gerd Möllmann <gerd.moellmann@gmail.com>
>> Cc: 73838@debbugs.gnu.org
>> Date: Thu, 17 Oct 2024 09:03:13 +0200
>> 
>> The immediate problem I'm facing is with tty child frames and
>> xterm-mouse: I'm opening a buffer selection child frame (consult-buffer)
>> and choose a candidate with a mouse click. The candidates are
>> mouse-highlighted. Result is eventually an endless loop in
>> process_mark_stack in the non-MPS GC. (Not using the mouse works just
>> fine.)
>
> If you can show the details of that, i.e. step through the loop one
> time until it gets to the same point, maybe someone could have an
> idea.

It's probably something pretty strange:

I built with ASAN, no MPS but my workaround for the hightlighting, and
get an error: GC marks char-code-property-alist (staticpro), and hits a
char-table object that is somehow broken. pdumper-object-p says yes for
it, but when checking the mark bit of that char-table with
pdumper_marked_p_impl, ASAN complains about an access outside of the
bitset being used for the pdumper mark bits.

(Not sure if that's already the loop I see without ASAN.)

Didn't get further than that today. LLDB decided to crash as well at
some point.

I'm a bit out of ideas how to find out what's up with that char-table.
Maybe I'll wait a bit until I have an idea how I could find that out.

bug#73838: 31.0.50; Problems in note_mouse_highlight if -nw

[Gerd Möllmann]

Gerd Möllmann <gerd.moellmann@gmail.com> writes:

> Eli Zaretskii <eliz@gnu.org> writes:
>
>>> From: Gerd Möllmann <gerd.moellmann@gmail.com>
>>> Cc: 73838@debbugs.gnu.org
>>> Date: Thu, 17 Oct 2024 09:03:13 +0200
>>> 
>>> The immediate problem I'm facing is with tty child frames and
>>> xterm-mouse: I'm opening a buffer selection child frame (consult-buffer)
>>> and choose a candidate with a mouse click. The candidates are
>>> mouse-highlighted. Result is eventually an endless loop in
>>> process_mark_stack in the non-MPS GC. (Not using the mouse works just
>>> fine.)
>>
>> If you can show the details of that, i.e. step through the loop one
>> time until it gets to the same point, maybe someone could have an
>> idea.
>
> It's probably something pretty strange:
>
> I built with ASAN, no MPS but my workaround for the hightlighting, and
> get an error: GC marks char-code-property-alist (staticpro), and hits a
> char-table object that is somehow broken. pdumper-object-p says yes for
> it, but when checking the mark bit of that char-table with
> pdumper_marked_p_impl, ASAN complains about an access outside of the
> bitset being used for the pdumper mark bits.
>
> (Not sure if that's already the loop I see without ASAN.)
>
> Didn't get further than that today. LLDB decided to crash as well at
> some point.
>
> I'm a bit out of ideas how to find out what's up with that char-table.
> Maybe I'll wait a bit until I have an idea how I could find that out.

FWIW, this is also reproducible in scratch/igc by building it without
MPS and with ASAN.

'../src/emacs' -batch --no-site-file --no-site-lisp --eval "(setq load-prefer-newer t byte-compile-warnings 'all)" --eval "(setq org--inhibit-version-check t)"  -f batch-byte-compile gnus/spam.el
emacs(33650,0x1f4c0f240) malloc: nano zone abandoned due to inability to reserve vm space.
=================================================================
ERROR: AddressSanitizer: heap-buffer-overflow on address 0x00010408f87c at pc 0x000100bc9e54 bp 0x00016f8d11b0 sp 0x00016f8d11a8
READ of size 4 at 0x00010408f87c thread T0
    #0 0x100bc9e50 in dump_bitset_bit_set_p pdumper.c:5401
    #1 0x100bca4ac in pdumper_marked_p_impl pdumper.c:5604
    #2 0x100bb6d70 in pdumper_marked_p pdumper.h:239
    #3 0x100bb239c in vector_marked_p alloc.c:4436
    #4 0x100bb0fec in process_mark_stack alloc.c:7555
    #5 0x100baf538 in mark_object alloc.c:7785
    #6 0x100bbc0e4 in mark_char_table alloc.c:7234
    #7 0x100bb1348 in process_mark_stack alloc.c:7621
    #8 0x100baf538 in mark_object alloc.c:7785
    #9 0x100bae71c in mark_object_root_visitor alloc.c:6682
    #10 0x100bac47c in visit_static_gc_roots alloc.c:6672
    #11 0x100bad420 in garbage_collect alloc.c:6895
...
0x00010408f87c is located 452 bytes after 372408-byte region [0x000104034800,0x00010408f6b8)
[1m[0m[1m[35mallocated by thread T0 here:[1m[0m
    #0 0x1032a0fd0 in calloc+0x9c (libclang_rt.asan_osx_dynamic.dylib:arm64e+0x54fd0)
    #1 0x100bcc2b8 in dump_bitsets_init pdumper.c:5376
    #2 0x100bcb79c in pdumper_load pdumper.c:6106
    #3 0x1009c10b0 in load_pdump emacs.c:980
    #4 0x1009bbe50 in main emacs.c:1436
    #5 0x19014c270  (<unknown module>)

bug#73838: 31.0.50; Problems in note_mouse_highlight if -nw

[Gerd Möllmann]

Gerd Möllmann <gerd.moellmann@gmail.com> writes:

> FWIW, this is also reproducible in scratch/igc by building it without
> MPS and with ASAN.

And configuring --with-ns. Does not happen for me --without-ns, which
made my bisect try flop.