Subject: Re: bug#76559: 31.0.50; [-O3 + PGTK] Crash when 'copying as kill'/'killing word'

From: Pip Cet <[email-masked]>
Date: Sat, 01 Mar 2025 11:31:55 +0000

"Paul Eggert" <[email-masked]> writes:

> On 2025-02-27 16:12, Po Lu wrote:
>>>      kbd_buffer_store_buffered_event ((union buffered_input_event *) event,
>>> 				     hold_quit);
>>>    }
>>>
>>> That cast is invalid and the resulting code need not work as one might
>>> expect with a circa 1978 C compiler.
>> The cast is valid.  It is accessing a union buffered_input_event from
>> the resultant pointer that is not.
>>
>> "... an aggregate or union type that includes one of the aforementioned
>> types among its members (including, recursively, a member of a
>> subaggregate or contained union), or ..."
>
> True, the later accessing is undefined as per C23 §6.5.1 ¶7. But the
> patch you proposed still does that sort of accessing, as the code still
> accesses event->kind when event is a union pointer not a struct pointer.
> (Also, the patched code doesn't copy the struct input_event on platforms
> where ! (HAVE_X11 || HAVE_PGTK); I assume that wasn't intended.)

If patches like this one are necessary:

@@ -3807,7 +3812,20 @@ kbd_buffer_store_buffered_event (union buffered_input_event *event,
   union buffered_input_event *next_slot = next_kbd_event (kbd_store_ptr);
   if (kbd_fetch_ptr != next_slot)
     {
-      *kbd_store_ptr = *event;
+      switch (kind)
+	{
+#if defined HAVE_X11 || defined HAVE_PGTK
+	case SELECTION_REQUEST_EVENT: case SELECTION_CLEAR_EVENT:
+	  struct selection_input_event const *sie = event;
+	  kbd_store_ptr->sie = *sie;
+	  break;
+#endif
+	default:
+	  struct input_event const *ie = event;
+	  kbd_store_ptr->ie = *ie;
+	  break;
+	}
+
       kbd_store_ptr = next_slot;
 #ifdef subprocesses
       if (kbd_buffer_nr_stored () > KBD_BUFFER_SIZE / 2

essentially all code dealing with X events needs to be rewritten, since
they use the same approach.  My vague memory is that C always made an
exception for unions like XEvent or our union buffered_input_event,
where the first element of all union members is identical and used
to decide which of the union members is "fully" active.

Looking at the -fdump-tree-sra-details output, I still think it's weird
that no whole-structure access for "ev" is being recorded for evq_flush
(and its inlined functions):

access { base = (8089)'ev', offset = 0, size = 512, expr = ev.ie, type = struct input_event, reverse = 0, grp_read = 1, grp_write = 1, grp_assignment_read = 1, grp_assignment_write = 1, grp_scalar_read = 0, grp_scalar_write = 0, grp_total_scalarization = 1, grp_hint = 0, grp_covered = 1, grp_unscalarizable_region = 0, grp_unscalarized_data = 0, grp_same_access_path = 0, grp_partial_lhs = 0, grp_to_be_replaced = 0, grp_to_be_debug_replaced = 0}

is the only recorded such access unless I comment out

	      //*hold_quit = event->ie;

access { base = (8089)'ev', offset = 0, size = 512, expr = ev, type = union buffered_input_event, reverse = 0, grp_read = 1, grp_write = 1, grp_assignment_read = 1, grp_assignment_write = 1, grp_scalar_read = 0, grp_scalar_write = 0, grp_total_scalarization = 0, grp_hint = 0, grp_covered = 0, grp_unscalarizable_region = 0, grp_unscalarized_data = 1, grp_same_access_path = 1, grp_partial_lhs = 0, grp_to_be_replaced = 0, grp_to_be_debug_replaced = 0}

So maybe it is time to ask the GCC folks for help here, even though we
do not have a reduced reproducer for the problem (which may well be on
our end) yet.)

Pip