> 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
