From patchwork Fri Jul 29 20:55:31 2011 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Martin Jambor X-Patchwork-Id: 107453 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 2D0F6B6F64 for ; Sat, 30 Jul 2011 06:55:51 +1000 (EST) Received: (qmail 24032 invoked by alias); 29 Jul 2011 20:55:49 -0000 Received: (qmail 24023 invoked by uid 22791); 29 Jul 2011 20:55:48 -0000 X-SWARE-Spam-Status: No, hits=-3.3 required=5.0 tests=AWL, BAYES_00, TW_FN, TW_TM, TW_YP X-Spam-Check-By: sourceware.org Received: from cantor2.suse.de (HELO mx2.suse.de) (195.135.220.15) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Fri, 29 Jul 2011 20:55:33 +0000 Received: from relay1.suse.de (charybdis-ext.suse.de [195.135.221.2]) (using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits)) (No client certificate requested) by mx2.suse.de (Postfix) with ESMTP id AC45C8A908; Fri, 29 Jul 2011 22:55:31 +0200 (CEST) Date: Fri, 29 Jul 2011 22:55:31 +0200 From: Martin Jambor To: GCC Patches , Jan Hubicka Subject: Re: [PATCH, PR 49886] Prevent fnsplit from changing signature when there are type attributes Message-ID: <20110729205530.GA24476@virgil.arch.suse.de> Mail-Followup-To: GCC Patches , Jan Hubicka References: <20110728165205.GC26096@virgil.arch.suse.de> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20110728165205.GC26096@virgil.arch.suse.de> User-Agent: Mutt/1.5.21 (2010-09-15) 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 Hi, On Thu, Jul 28, 2011 at 06:52:05PM +0200, Martin Jambor wrote: > pass_split_functions is happy to split functions which have type > attributes but cannot update them if the new clone has in any way > different parameters than the original. This can lead to > miscompilations in cases like the testcase. > > This patch solves it by 1) making the inliner set the > can_change_signature flag to false for them because their signature > cannot be changed (this step is also necessary to make IPA-CP operate > on them and handle them correctly), and 2) make the splitting pass > keep all parameters if the flag is set. The second step might involve > inventing some default definitions if the parameters did not really > have any. > > I spoke about this with Honza and he claimed that the new function is > really an entirely different thing and that the parameters may > correspond only very loosely and thus the type attributes should be > cleared. I'm not sure I agree, but in any case I needed this to work > to allow me continue with promised IPA-CP polishing and so I decided > to do this because it was easier. (My own opinion is that the current > representation of parameter-describing function type attributes is > evil and will cause harm no matter hat we do.) > Actually, I'd like to commit the patch below which also clears can_change_signature for BUILT_IN_VA_START. It is not really necessary for this fix but fixes some problems in a followup patch and is also the correct thing to do because if we clone a function calling it and pass non-NULL for args_to_skip, the new clone would not have a stdarg_p type and fold_builtin_next_arg could error when dealing with it. Also bootstrapped and tested on x86_64-linux. OK for trunk? Thanks, Martin 2011-07-29 Martin Jambor PR middle-end/49886 * ipa-inline-analysis.c (compute_inline_parameters): Set can_change_signature of noes with typde attributes. * ipa-split.c (split_function): Do not skip any arguments if can_change_signature is set. * testsuite/gcc.c-torture/execute/pr49886.c: New testcase. Index: src/gcc/ipa-inline-analysis.c =================================================================== --- src.orig/gcc/ipa-inline-analysis.c +++ src/gcc/ipa-inline-analysis.c @@ -1658,18 +1658,28 @@ compute_inline_parameters (struct cgraph /* Can this function be inlined at all? */ info->inlinable = tree_inlinable_function_p (node->decl); - /* Inlinable functions always can change signature. */ - if (info->inlinable) - node->local.can_change_signature = true; + /* Type attributes can use parameter indices to describe them. */ + if (TYPE_ATTRIBUTES (TREE_TYPE (node->decl))) + node->local.can_change_signature = false; else { - /* Functions calling builtin_apply can not change signature. */ - for (e = node->callees; e; e = e->next_callee) - if (DECL_BUILT_IN (e->callee->decl) - && DECL_BUILT_IN_CLASS (e->callee->decl) == BUILT_IN_NORMAL - && DECL_FUNCTION_CODE (e->callee->decl) == BUILT_IN_APPLY_ARGS) - break; - node->local.can_change_signature = !e; + /* Otherwise, inlinable functions always can change signature. */ + if (info->inlinable) + node->local.can_change_signature = true; + else + { + /* Functions calling builtin_apply can not change signature. */ + for (e = node->callees; e; e = e->next_callee) + { + tree cdecl = e->callee->decl; + if (DECL_BUILT_IN (cdecl) + && DECL_BUILT_IN_CLASS (cdecl) == BUILT_IN_NORMAL + && (DECL_FUNCTION_CODE (cdecl) == BUILT_IN_APPLY_ARGS + || DECL_FUNCTION_CODE (cdecl) == BUILT_IN_VA_START)) + break; + } + node->local.can_change_signature = !e; + } } estimate_function_body_sizes (node, early); Index: src/gcc/ipa-split.c =================================================================== --- src.orig/gcc/ipa-split.c +++ src/gcc/ipa-split.c @@ -945,10 +945,10 @@ static void split_function (struct split_point *split_point) { VEC (tree, heap) *args_to_pass = NULL; - bitmap args_to_skip = BITMAP_ALLOC (NULL); + bitmap args_to_skip; tree parm; int num = 0; - struct cgraph_node *node; + struct cgraph_node *node, *cur_node = cgraph_get_node (current_function_decl); basic_block return_bb = find_return_bb (); basic_block call_bb; gimple_stmt_iterator gsi; @@ -968,17 +968,30 @@ split_function (struct split_point *spli dump_split_point (dump_file, split_point); } + if (cur_node->local.can_change_signature) + args_to_skip = BITMAP_ALLOC (NULL); + else + args_to_skip = NULL; + /* Collect the parameters of new function and args_to_skip bitmap. */ for (parm = DECL_ARGUMENTS (current_function_decl); parm; parm = DECL_CHAIN (parm), num++) - if (!is_gimple_reg (parm) - || !gimple_default_def (cfun, parm) - || !bitmap_bit_p (split_point->ssa_names_to_pass, - SSA_NAME_VERSION (gimple_default_def (cfun, parm)))) + if (args_to_skip + && (!is_gimple_reg (parm) + || !gimple_default_def (cfun, parm) + || !bitmap_bit_p (split_point->ssa_names_to_pass, + SSA_NAME_VERSION (gimple_default_def (cfun, + parm))))) bitmap_set_bit (args_to_skip, num); else { arg = gimple_default_def (cfun, parm); + if (!arg) + { + arg = make_ssa_name (parm, gimple_build_nop ()); + set_default_def (parm, arg); + } + if (TYPE_MAIN_VARIANT (DECL_ARG_TYPE (parm)) != TYPE_MAIN_VARIANT (TREE_TYPE (arg))) { @@ -1081,9 +1094,7 @@ split_function (struct split_point *spli /* Now create the actual clone. */ rebuild_cgraph_edges (); - node = cgraph_function_versioning (cgraph_get_node (current_function_decl), - NULL, NULL, - args_to_skip, + node = cgraph_function_versioning (cur_node, NULL, NULL, args_to_skip, split_point->split_bbs, split_point->entry_bb, "part"); /* For usual cloning it is enough to clear builtin only when signature @@ -1094,7 +1105,7 @@ split_function (struct split_point *spli DECL_BUILT_IN_CLASS (node->decl) = NOT_BUILT_IN; DECL_FUNCTION_CODE (node->decl) = (enum built_in_function) 0; } - cgraph_node_remove_callees (cgraph_get_node (current_function_decl)); + cgraph_node_remove_callees (cur_node); if (!split_part_return_p) TREE_THIS_VOLATILE (node->decl) = 1; if (dump_file) Index: src/gcc/testsuite/gcc.c-torture/execute/pr49886.c =================================================================== --- /dev/null +++ src/gcc/testsuite/gcc.c-torture/execute/pr49886.c @@ -0,0 +1,100 @@ +struct PMC { + unsigned flags; +}; + +typedef struct Pcc_cell +{ + struct PMC *p; + long bla; + long type; +} Pcc_cell; + +int gi; +int cond; + +extern void abort (); +extern void never_ever(int interp, struct PMC *pmc) + __attribute__((noinline,noclone)); + +void never_ever (int interp, struct PMC *pmc) +{ + abort (); +} + +static void mark_cell(int * interp, Pcc_cell *c) + __attribute__((__nonnull__(1))); + +static void +mark_cell(int * interp, Pcc_cell *c) +{ + if (!cond) + return; + + if (c && c->type == 4 && c->p + && !(c->p->flags & (1<<18))) + never_ever(gi + 1, c->p); + if (c && c->type == 4 && c->p + && !(c->p->flags & (1<<17))) + never_ever(gi + 2, c->p); + if (c && c->type == 4 && c->p + && !(c->p->flags & (1<<16))) + never_ever(gi + 3, c->p); + if (c && c->type == 4 && c->p + && !(c->p->flags & (1<<15))) + never_ever(gi + 4, c->p); + if (c && c->type == 4 && c->p + && !(c->p->flags & (1<<14))) + never_ever(gi + 5, c->p); + if (c && c->type == 4 && c->p + && !(c->p->flags & (1<<13))) + never_ever(gi + 6, c->p); + if (c && c->type == 4 && c->p + && !(c->p->flags & (1<<12))) + never_ever(gi + 7, c->p); + if (c && c->type == 4 && c->p + && !(c->p->flags & (1<<11))) + never_ever(gi + 8, c->p); + if (c && c->type == 4 && c->p + && !(c->p->flags & (1<<10))) + never_ever(gi + 9, c->p); +} + +static void +foo(int * interp, Pcc_cell *c) +{ + mark_cell(interp, c); +} + +static struct Pcc_cell * +__attribute__((noinline,noclone)) +getnull(void) +{ + return (struct Pcc_cell *) 0; +} + + +int main() +{ + int i; + + cond = 1; + for (i = 0; i < 100; i++) + foo (&gi, getnull ()); + return 0; +} + + +void +bar_1 (int * interp, Pcc_cell *c) +{ + c->bla += 1; + mark_cell(interp, c); +} + +void +bar_2 (int * interp, Pcc_cell *c) +{ + c->bla += 2; + mark_cell(interp, c); +} +