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

[Eli Zaretskii <eliz@gnu.org>]

> 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.