Re: [RFA:] Fix frame-pointer-clobbering in builtins.c:expand_builtin_setjmp_receiver

[Eric Botcazou <ebotcazou at adacore dot com>]

> But, in the builtins.c:expand_builtin_setjmp_receiver, the
> frame-pointer is *clobbered* for a mysterious and fuddy reason:
> 
>       /* This might change the hard frame pointer in ways that aren't
> 	 apparent to early optimization passes, so force a clobber.  */
>       emit_clobber (hard_frame_pointer_rtx);
> 
> That comment might have been true eons ago, but these days clobbering
> the frame-pointer means that its value is void and any restoring insns
> emitted before the clobber are deleted *because* of the clobber.  For
> example, in built-in-setjmp.c at -O2.  Before .191r.ud_dce (i.e. in
> the .190r.init-regs dump):
> 
> (code_label/s 47 115 48 3 3 "" [2 uses])
> (note 48 47 49 3 [bb 3] NOTE_INSN_BASIC_BLOCK)
> (insn 49 48 168 3 (use (reg/f:DI 253 $253))
> /tmp/mmiximp2/gcc/gcc/testsuite/gcc.c-torture/execute/built-in-setjmp.c:21
> -1 (nil))
> (insn 168 49 51 3 (set (reg/f:DI 253 $253)
>         (plus:DI (reg/f:DI 253 $253)
>             (const_int 24 [0x18])))
> /tmp/mmiximp2/gcc/gcc/testsuite/gcc.c-torture/execute/built-in-setjmp.c:21
> -1 (nil))
> (insn 51 168 52 3 (clobber (reg/f:DI 253 $253))
> /tmp/mmiximp2/gcc/gcc/testsuite/gcc.c-torture/execute/built-in-setjmp.c:21
> -1 (nil))
> (insn 52 51 54 3 (parallel [
>             (unspec_volatile [
>                     (reg/f:DI 253 $253)
>                 ] 1)
>             (clobber (scratch:DI))
>             (clobber (reg:DI 259 rJ))
>         ])
> /tmp/mmiximp2/gcc/gcc/testsuite/gcc.c-torture/execute/built-in-setjmp.c:21
> 63 {*nonlocal_goto_receiver_expanded} (expr_list:REG_UNUSED (reg:DI 259 rJ)
>         (nil)))
> (insn 54 52 136 3 (asm_input/v ("") (null):0)
> /tmp/mmiximp2/gcc/gcc/testsuite/gcc.c-torture/execute/built-in-setjmp.c:21
> -1 (nil))
> 
> After:
> 
> (code_label/s 47 115 48 3 3 "" [2 uses])
> (note 48 47 49 3 [bb 3] NOTE_INSN_BASIC_BLOCK)
> (insn 49 48 51 3 (use (reg/f:DI 253 $253))
> /tmp/mmiximp2/gcc/gcc/testsuite/gcc.c-torture/execute/built-in-setjmp.c:21
> -1 (nil))
> (insn 51 49 52 3 (clobber (reg/f:DI 253 $253))
> /tmp/mmiximp2/gcc/gcc/testsuite/gcc.c-torture/execute/built-in-setjmp.c:21
> -1 (nil))
> (insn 52 51 54 3 (parallel [
>             (unspec_volatile [
>                     (reg/f:DI 253 $253)
>                 ] 1)
>             (clobber (scratch:DI))
>             (clobber (reg:DI 259 rJ))
>         ])
> /tmp/mmiximp2/gcc/gcc/testsuite/gcc.c-torture/execute/built-in-setjmp.c:21
> 63 {*nonlocal_goto_receiver_expanded} (expr_list:REG_UNUSED (reg:DI 259 rJ)
>         (nil)))
> (insn 54 52 136 3 (asm_input/v ("") (null):0)
> /tmp/mmiximp2/gcc/gcc/testsuite/gcc.c-torture/execute/built-in-setjmp.c:21
> -1 (nil))
> 
> Note that insn 168 deleted, which seems a logical optimization.  The
> bug is to emit the clobber, not that the restoring insn is removed.

Had that worked in the past for MMIX?  If so, what changed recently?

> While grepping around for other emitters of nonlocal_goto_receiver I
> noticed that builtins.c:expand_builtin_setjmp_receiver is identical to
> stmt.c:expand_nl_goto_receiver save for two things: the frame-pointer
> clobbering(!) and that expand_builtin_setjmp_receiver instead prefers to
> emit setjmp_receiver.  I don't see how the frame-pointer-clobbering
> would be needed as part of emitting setjmp_receiver.

Indeed, the discrepancy seems to have no firm basis.

> I suggest eliminating the bug and one copy of the apparently bug-prone
> code.  I kept the function in builtins.c for obvious reasons (if not
> obvious, consider the name: expand *builtin* setjmp_receiver) with the
> setjmp-ness expressed through the label parameter, which is non-NULL
> for pre-existing calls.  Note also the fixed clobber-comment,
> obviously incorrect in the stmt.c almost-copy, and at least on the
> wrong line in expand_builtin_setjmp_receiver.

Agreed.  However, I'd suggest rescuing the comment for the ELIMINABLE_REGS 
block from expand_nl_goto_receiver as it still sounds valid to me.

> 	* stmt.c (expand_nl_goto_receiver): Remove almost-copy of
> 	expand_builtin_setjmp_receiver.
> 	(expand_label): Adjust, call expand_builtin_setjmp_receiver
> 	with NULL for the label parameter.
> 	* builtins.c (expand_builtin_setjmp_receiver): Don't clobber
> 	the frame-pointer.  Adjust comments.
> 	[HAVE_builtin_setjmp_receiver]: Emit builtin_setjmp_receiver
> 	only if LABEL is non-NULL.

I cannot formally approve, but this looks good to me modulo:

> Index: gcc/builtins.c
> ===================================================================
> --- gcc/builtins.c	(revision 192353)
> +++ gcc/builtins.c	(working copy)
> @@ -885,14 +885,16 @@ expand_builtin_setjmp_setup (rtx buf_add
>  }
> 
>  /* Construct the trailing part of a __builtin_setjmp call.  This is
> -   also called directly by the SJLJ exception handling code.  */
> +   also called directly by the SJLJ exception handling code.
> +   If RECEIVER_LABEL is NULL, instead the port-specific parts of a
> +   nonlocal goto handler are emitted.  */

The "port-specific parts" wording is a bit confusing I think.  I'd just write:

  If RECEIVER_LABEL is NULL, instead contruct a nonlocal goto handler.

-- 
Eric Botcazou