From patchwork Tue Oct 16 00:29:06 2012 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Hans-Peter Nilsson X-Patchwork-Id: 191686 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from sourceware.org (server1.sourceware.org [209.132.180.131]) by ozlabs.org (Postfix) with SMTP id B5A882C0089 for ; Tue, 16 Oct 2012 11:29:20 +1100 (EST) Comment: DKIM? See http://www.dkim.org DKIM-Signature: v=1; a=rsa-sha1; c=relaxed/relaxed; d=gcc.gnu.org; s=default; x=1350952161; h=Comment: DomainKey-Signature:Received:Received:Received:Received:Received: Date:From:To:cc:Subject:In-Reply-To:Message-ID:References: User-Agent:MIME-Version:Content-Type:Mailing-List:Precedence: List-Id:List-Unsubscribe:List-Archive:List-Post:List-Help:Sender: Delivered-To; bh=3vmDXglTWx+O6JGy6nsm4IkoqXc=; b=hQFKDkkXX+eg1qt eF7CStOn8t1ZetEaggDdx5iv6/jn4uHpH5hH9I0OMI6SBahXMBiad9BOn6Gng4ZU ZgfOHOBqGlhaaiMg50N3IS/zix+ALOfgPplFPo2nsWgddrHaqkOTsDq0/cWUcKyr GBZtcl9pepq504MaN6t0txXBCaLQ= Comment: DomainKeys? See http://antispam.yahoo.com/domainkeys DomainKey-Signature: a=rsa-sha1; q=dns; c=nofws; s=default; d=gcc.gnu.org; h=Received:Received:X-SWARE-Spam-Status:X-Spam-Check-By:Received:Received:Received:Date:From:To:cc:Subject:In-Reply-To:Message-ID:References:User-Agent:MIME-Version:Content-Type:X-IsSubscribed:Mailing-List:Precedence:List-Id:List-Unsubscribe:List-Archive:List-Post:List-Help:Sender:Delivered-To; b=bbNxm7F1pxQ6yWP+k5fNtsZKb3HbyAn4060S0knjwsaTL4cp121I4Op5rG6yU8 I5ZPbrxC31l5tBg9jm+RyzXafeDGUK7O6qTLiA36f2d9HqFpn91o1RfW4kcc6vy1 5yGbfq6xUhLZ7vYTOa3hO5P4xtZmgW35g6TS4WJ7JOe1w=; Received: (qmail 30455 invoked by alias); 16 Oct 2012 00:29:15 -0000 Received: (qmail 30446 invoked by uid 22791); 16 Oct 2012 00:29:14 -0000 X-SWARE-Spam-Status: No, hits=-2.8 required=5.0 tests=AWL, BAYES_00, KHOP_THREADED, TW_SV, TW_TJ X-Spam-Check-By: sourceware.org Received: from dair.pair.com (HELO dair.pair.com) (209.68.1.49) by sourceware.org (qpsmtpd/0.43rc1) with SMTP; Tue, 16 Oct 2012 00:29:07 +0000 Received: (qmail 44777 invoked by uid 20157); 16 Oct 2012 00:29:06 -0000 Received: from localhost (sendmail-bs@127.0.0.1) by localhost with SMTP; 16 Oct 2012 00:29:06 -0000 Date: Mon, 15 Oct 2012 20:29:06 -0400 (EDT) From: Hans-Peter Nilsson To: Eric Botcazou cc: gcc-patches@gcc.gnu.org Subject: Re: [RFA:] Fix frame-pointer-clobbering in builtins.c:expand_builtin_setjmp_receiver In-Reply-To: <3114930.R4qRiBo81x@polaris> Message-ID: References: <3114930.R4qRiBo81x@polaris> User-Agent: Alpine 2.00 (BSF 1167 2008-08-23) MIME-Version: 1.0 X-IsSubscribed: yes Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Delivered-To: mailing list gcc-patches@gcc.gnu.org On Fri, 12 Oct 2012, Eric Botcazou wrote: > > (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)) ... > > 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? Yes, for svn revision 106027 (20051030) 4.1.0-era (!) where the test must have passed, as gcc.c-torture/execute/built-in-setjmp.c is at least four years older than that. > If so, what changed recently? By "these days" I didn't mean "recent", just not "eons ago". :) I see in a gcc-test-results posting from Mike Stein (whom I'd like to thank for test-results posting over the years), matching FAILs for svn revision 126095 (20070628) 4.3.0-era . Sorry, I have nothing in between those reports, my bad. Though I see no point narrowing down the failing revision further here IMO; as mentioned the bug is not that the restoring insn is removed. > 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. Oops, my bad; I see I removed all the good comments. Fixed. > > * 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: > > + 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. Sure. Thanks for the review. Updated patch below. As nothing was changed from the previous post but comments as per the review (mostly moving / reviving, fixing one grammo), already covered by the changelog quoted above, the previous testing is still valid. Ok for trunk, approvers? brgds, H-P Index: gcc/builtins.c =================================================================== --- gcc/builtins.c (revision 192353) +++ gcc/builtins.c (working copy) @@ -885,14 +885,15 @@ 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 contruct a nonlocal goto handler. */ void expand_builtin_setjmp_receiver (rtx receiver_label ATTRIBUTE_UNUSED) { rtx chain; - /* Clobber the FP when we get here, so we have to make sure it's + /* Mark the FP as used when we get here, so we have to make sure it's marked as used by this function. */ emit_use (hard_frame_pointer_rtx); @@ -907,17 +908,28 @@ expand_builtin_setjmp_receiver (rtx rece #ifdef HAVE_nonlocal_goto if (! HAVE_nonlocal_goto) #endif - { - emit_move_insn (virtual_stack_vars_rtx, hard_frame_pointer_rtx); - /* 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); - } + /* First adjust our frame pointer to its actual value. It was + previously set to the start of the virtual area corresponding to + the stacked variables when we branched here and now needs to be + adjusted to the actual hardware fp value. + + Assignments to virtual registers are converted by + instantiate_virtual_regs into the corresponding assignment + to the underlying register (fp in this case) that makes + the original assignment true. + So the following insn will actually be decrementing fp by + STARTING_FRAME_OFFSET. */ + emit_move_insn (virtual_stack_vars_rtx, hard_frame_pointer_rtx); #if !HARD_FRAME_POINTER_IS_ARG_POINTER if (fixed_regs[ARG_POINTER_REGNUM]) { #ifdef ELIMINABLE_REGS + /* If the argument pointer can be eliminated in favor of the + frame pointer, we don't need to restore it. We assume here + that if such an elimination is present, it can always be used. + This is the case on all known machines; if we don't make this + assumption, we do unnecessary saving on many machines. */ size_t i; static const struct elims {const int from, to;} elim_regs[] = ELIMINABLE_REGS; @@ -938,7 +950,7 @@ expand_builtin_setjmp_receiver (rtx rece #endif #ifdef HAVE_builtin_setjmp_receiver - if (HAVE_builtin_setjmp_receiver) + if (receiver_label != NULL && HAVE_builtin_setjmp_receiver) emit_insn (gen_builtin_setjmp_receiver (receiver_label)); else #endif Index: gcc/stmt.c =================================================================== --- gcc/stmt.c (revision 192353) +++ gcc/stmt.c (working copy) @@ -102,7 +102,6 @@ typedef struct case_node *case_node_ptr; static int n_occurrences (int, const char *); static bool tree_conflicts_with_clobbers_p (tree, HARD_REG_SET *); -static void expand_nl_goto_receiver (void); static bool check_operand_nalternatives (tree, tree); static bool check_unique_operand_names (tree, tree, tree); static char *resolve_operand_name_1 (char *, tree, tree, tree); @@ -196,7 +195,7 @@ expand_label (tree label) if (DECL_NONLOCAL (label)) { - expand_nl_goto_receiver (); + expand_builtin_setjmp_receiver (NULL); nonlocal_goto_handler_labels = gen_rtx_EXPR_LIST (VOIDmode, label_r, nonlocal_goto_handler_labels); @@ -1552,77 +1551,6 @@ expand_return (tree retval) } } -/* Emit code to restore vital registers at the beginning of a nonlocal goto - handler. */ -static void -expand_nl_goto_receiver (void) -{ - rtx chain; - - /* Clobber the FP when we get here, so we have to make sure it's - marked as used by this function. */ - emit_use (hard_frame_pointer_rtx); - - /* Mark the static chain as clobbered here so life information - doesn't get messed up for it. */ - chain = targetm.calls.static_chain (current_function_decl, true); - if (chain && REG_P (chain)) - emit_clobber (chain); - -#ifdef HAVE_nonlocal_goto - if (! HAVE_nonlocal_goto) -#endif - /* First adjust our frame pointer to its actual value. It was - previously set to the start of the virtual area corresponding to - the stacked variables when we branched here and now needs to be - adjusted to the actual hardware fp value. - - Assignments are to virtual registers are converted by - instantiate_virtual_regs into the corresponding assignment - to the underlying register (fp in this case) that makes - the original assignment true. - So the following insn will actually be - decrementing fp by STARTING_FRAME_OFFSET. */ - emit_move_insn (virtual_stack_vars_rtx, hard_frame_pointer_rtx); - -#if !HARD_FRAME_POINTER_IS_ARG_POINTER - if (fixed_regs[ARG_POINTER_REGNUM]) - { -#ifdef ELIMINABLE_REGS - /* If the argument pointer can be eliminated in favor of the - frame pointer, we don't need to restore it. We assume here - that if such an elimination is present, it can always be used. - This is the case on all known machines; if we don't make this - assumption, we do unnecessary saving on many machines. */ - static const struct elims {const int from, to;} elim_regs[] = ELIMINABLE_REGS; - size_t i; - - for (i = 0; i < ARRAY_SIZE (elim_regs); i++) - if (elim_regs[i].from == ARG_POINTER_REGNUM - && elim_regs[i].to == HARD_FRAME_POINTER_REGNUM) - break; - - if (i == ARRAY_SIZE (elim_regs)) -#endif - { - /* Now restore our arg pointer from the address at which it - was saved in our stack frame. */ - emit_move_insn (crtl->args.internal_arg_pointer, - copy_to_reg (get_arg_pointer_save_area ())); - } - } -#endif - -#ifdef HAVE_nonlocal_goto_receiver - if (HAVE_nonlocal_goto_receiver) - emit_insn (gen_nonlocal_goto_receiver ()); -#endif - - /* We must not allow the code we just generated to be reordered by - scheduling. Specifically, the update of the frame pointer must - happen immediately, not later. */ - emit_insn (gen_blockage ()); -} /* Emit code to save the current value of stack. */ rtx