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

["Gerd Möllmann" <gerd.moellmann@gmail.com>]

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.