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

From: Po Lu <[email-masked]>
Date: Sat, 01 Mar 2025 21:52:13 +0800

Pip Cet <[email-masked]> writes:

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

Correct.  But I think Paul's point is that the pointed-to object is not
a union, and in consequence that the exception does not apply.

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

GCC assumes that, being of the same size and having the same offset, the
access to `struct input_event' never aliases another access in the same
procedure with the same properties but a different type.  Which type
prevails is contingent on the order in which they are defined.  If an
incomplete declaration of `union buffered_input_event' is inserted
before `struct input_event', you will observe the access to `ev.ie'
disappearing in place of that to `ev' as a whole.

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

We do now.