Patchwork [GOOGLE] More strict checking for call args

login
register
mail settings
Submitter Dehao Chen
Date June 7, 2013, 4:39 a.m.
Message ID <CAO2gOZXkaogJftpdDJMocQR2saNbKuMvbnT6+RuANnGnbSKQZQ@mail.gmail.com>
Download mbox | patch
Permalink /patch/249596/
State New
Headers show

Comments

Dehao Chen - June 7, 2013, 4:39 a.m.
I've prepared a patch check for args for indirect call value profiling.

Testing on-going. Is it ok for trunk if testing is good?

Thanks,
Dehao

gcc/ChangeLog:
2013-06-06  Dehao Chen  <dehao@google.com>

        * tree-flow.h (gimple_check_call_matching_types): Add new argument.
        * gimple-low.c (gimple_check_call_matching_types): Likewise.
        (gimple_check_call_args): Likewise.
        * value-prof.c (check_ic_target): Likewise.
        * ipa-inline.c (early_inliner): Likewise.
        * ipa-prop.c (update_indirect_edges_after_inlining): Likewise.
        * cgraph.c (cgraph_create_edge_1): Likewise.
        (cgraph_make_edge_direct): Likewise.



On Thu, Jun 6, 2013 at 9:14 AM, Xinliang David Li <davidxl@google.com> wrote:
> gimple_check_call_matching_types is called by check_ic_target. Instead
> of moving the check out of this guard function, may be enhancing the
> interface to allow it to guard with different strictness?
>
> David
>
> On Thu, Jun 6, 2013 at 8:10 AM, Dehao Chen <dehao@google.com> wrote:
>> Hi, Martin,
>>
>> Yes, your patch can fix my case. Thanks a lot for the fix.
>>
>> With the fix, value profiling will still promote the wrong indirect
>> call target. Though it will not be inlining, but it results in an
>> additional check. How about in check_ic_target, after calling
>> gimple_check_call_matching_types, we also check if number of args
>> match number of params in target->symbol.decl?
>>
>> Thanks,
>> Dehao
>>
>>
>> On Thu, Jun 6, 2013 at 7:11 AM, Martin Jambor <mjambor@suse.cz> wrote:
>>>
>>> Hi,
>>>
>>> On Tue, Jun 04, 2013 at 05:19:02PM -0700, Dehao Chen wrote:
>>> > attached is a testcase that would cause problem when source has changed:
>>> >
>>> > $ g++ test.cc -O2 -fprofile-generate -DOLD
>>> > $ ./a.out
>>> > $ g++ test.cc -O2 -fprofile-use
>>> > test.cc:34:1: internal compiler error: in operator[], at vec.h:815
>>> >  }
>>> >  ^
>>> > 0x512740 vec<tree_node*, va_heap, vl_embed>::operator[](unsigned int)
>>> > ../../gcc/vec.h:815
>>> > 0x512740 vec<tree_node*, va_heap, vl_ptr>::operator[](unsigned int)
>>> > ../../gcc/vec.h:1244
>>> > 0xf24464 vec<tree_node*, va_heap, vl_embed>::operator[](unsigned int)
>>> > ../../gcc/vec.h:815
>>> > 0xf24464 vec<tree_node*, va_heap, vl_ptr>::operator[](unsigned int)
>>> > ../../gcc/vec.h:1244
>>> > 0xf24464 ipa_get_indirect_edge_target_1
>>> > ../../gcc/ipa-cp.c:1535
>>> > 0x971b9a estimate_edge_devirt_benefit
>>> > ../../gcc/ipa-inline-analysis.c:2757
>>>
>>> Hm, this seems rather like an omission in ipa_get_indirect_edge_target_1.
>>> Since it is called also from inlining, we can have parameter count
>>> mismatches... and in fact in non-virtual paths of that function we do
>>> check that we don't.  Because all callers have to pass known_vals
>>> describing all formal parameters of the inline tree root, we should
>>> apply the fix below (I've only just started running a bootstrap and
>>> testsuite on x86_64, though).
>>>
>>> OTOH, while I understand that FDO can change inlining sufficiently so
>>> that this error occurs, IMHO this should not be caused by outdated
>>> profiles but there is somewhere a parameter mismatch in the source.
>>>
>>> Dehao, can you please check that this patch helps?
>>>
>>> Richi, if it does and the patch passes bootstrap and tests, is it OK
>>> for trunk and 4.8 branch?
>>>
>>> Thanks and sorry for the trouble,
>>>
>>> Martin
>>>
>>>
>>> 2013-06-06  Martin Jambor  <mjambor@suse.cz>
>>>
>>>         * ipa-cp.c (ipa_get_indirect_edge_target_1): Check that param_index is
>>>         within bounds at the beginning of the function.
>>>
>>> Index: src/gcc/ipa-cp.c
>>> ===================================================================
>>> --- src.orig/gcc/ipa-cp.c
>>> +++ src/gcc/ipa-cp.c
>>> @@ -1481,7 +1481,8 @@ ipa_get_indirect_edge_target_1 (struct c
>>>    tree otr_type;
>>>    tree t;
>>>
>>> -  if (param_index == -1)
>>> +  if (param_index == -1
>>> +      || known_vals.length () <= (unsigned int) param_index)
>>>      return NULL_TREE;
>>>
>>>    if (!ie->indirect_info->polymorphic)
>>> @@ -1516,8 +1517,7 @@ ipa_get_indirect_edge_target_1 (struct c
>>>             t = NULL;
>>>         }
>>>        else
>>> -       t = (known_vals.length () > (unsigned int) param_index
>>> -            ? known_vals[param_index] : NULL);
>>> +       t = NULL;
>>>
>>>        if (t &&
>>>           TREE_CODE (t) == ADDR_EXPR
Xinliang David Li - June 7, 2013, 5:20 a.m.
This one should be submitted and discussed in trunk.

thanks,

David

On Thu, Jun 6, 2013 at 9:39 PM, Dehao Chen <dehao@google.com> wrote:
> I've prepared a patch check for args for indirect call value profiling.
>
> Testing on-going. Is it ok for trunk if testing is good?
>
> Thanks,
> Dehao
>
> gcc/ChangeLog:
> 2013-06-06  Dehao Chen  <dehao@google.com>
>
>         * tree-flow.h (gimple_check_call_matching_types): Add new argument.
>         * gimple-low.c (gimple_check_call_matching_types): Likewise.
>         (gimple_check_call_args): Likewise.
>         * value-prof.c (check_ic_target): Likewise.
>         * ipa-inline.c (early_inliner): Likewise.
>         * ipa-prop.c (update_indirect_edges_after_inlining): Likewise.
>         * cgraph.c (cgraph_create_edge_1): Likewise.
>         (cgraph_make_edge_direct): Likewise.
>
>
>
> On Thu, Jun 6, 2013 at 9:14 AM, Xinliang David Li <davidxl@google.com> wrote:
>> gimple_check_call_matching_types is called by check_ic_target. Instead
>> of moving the check out of this guard function, may be enhancing the
>> interface to allow it to guard with different strictness?
>>
>> David
>>
>> On Thu, Jun 6, 2013 at 8:10 AM, Dehao Chen <dehao@google.com> wrote:
>>> Hi, Martin,
>>>
>>> Yes, your patch can fix my case. Thanks a lot for the fix.
>>>
>>> With the fix, value profiling will still promote the wrong indirect
>>> call target. Though it will not be inlining, but it results in an
>>> additional check. How about in check_ic_target, after calling
>>> gimple_check_call_matching_types, we also check if number of args
>>> match number of params in target->symbol.decl?
>>>
>>> Thanks,
>>> Dehao
>>>
>>>
>>> On Thu, Jun 6, 2013 at 7:11 AM, Martin Jambor <mjambor@suse.cz> wrote:
>>>>
>>>> Hi,
>>>>
>>>> On Tue, Jun 04, 2013 at 05:19:02PM -0700, Dehao Chen wrote:
>>>> > attached is a testcase that would cause problem when source has changed:
>>>> >
>>>> > $ g++ test.cc -O2 -fprofile-generate -DOLD
>>>> > $ ./a.out
>>>> > $ g++ test.cc -O2 -fprofile-use
>>>> > test.cc:34:1: internal compiler error: in operator[], at vec.h:815
>>>> >  }
>>>> >  ^
>>>> > 0x512740 vec<tree_node*, va_heap, vl_embed>::operator[](unsigned int)
>>>> > ../../gcc/vec.h:815
>>>> > 0x512740 vec<tree_node*, va_heap, vl_ptr>::operator[](unsigned int)
>>>> > ../../gcc/vec.h:1244
>>>> > 0xf24464 vec<tree_node*, va_heap, vl_embed>::operator[](unsigned int)
>>>> > ../../gcc/vec.h:815
>>>> > 0xf24464 vec<tree_node*, va_heap, vl_ptr>::operator[](unsigned int)
>>>> > ../../gcc/vec.h:1244
>>>> > 0xf24464 ipa_get_indirect_edge_target_1
>>>> > ../../gcc/ipa-cp.c:1535
>>>> > 0x971b9a estimate_edge_devirt_benefit
>>>> > ../../gcc/ipa-inline-analysis.c:2757
>>>>
>>>> Hm, this seems rather like an omission in ipa_get_indirect_edge_target_1.
>>>> Since it is called also from inlining, we can have parameter count
>>>> mismatches... and in fact in non-virtual paths of that function we do
>>>> check that we don't.  Because all callers have to pass known_vals
>>>> describing all formal parameters of the inline tree root, we should
>>>> apply the fix below (I've only just started running a bootstrap and
>>>> testsuite on x86_64, though).
>>>>
>>>> OTOH, while I understand that FDO can change inlining sufficiently so
>>>> that this error occurs, IMHO this should not be caused by outdated
>>>> profiles but there is somewhere a parameter mismatch in the source.
>>>>
>>>> Dehao, can you please check that this patch helps?
>>>>
>>>> Richi, if it does and the patch passes bootstrap and tests, is it OK
>>>> for trunk and 4.8 branch?
>>>>
>>>> Thanks and sorry for the trouble,
>>>>
>>>> Martin
>>>>
>>>>
>>>> 2013-06-06  Martin Jambor  <mjambor@suse.cz>
>>>>
>>>>         * ipa-cp.c (ipa_get_indirect_edge_target_1): Check that param_index is
>>>>         within bounds at the beginning of the function.
>>>>
>>>> Index: src/gcc/ipa-cp.c
>>>> ===================================================================
>>>> --- src.orig/gcc/ipa-cp.c
>>>> +++ src/gcc/ipa-cp.c
>>>> @@ -1481,7 +1481,8 @@ ipa_get_indirect_edge_target_1 (struct c
>>>>    tree otr_type;
>>>>    tree t;
>>>>
>>>> -  if (param_index == -1)
>>>> +  if (param_index == -1
>>>> +      || known_vals.length () <= (unsigned int) param_index)
>>>>      return NULL_TREE;
>>>>
>>>>    if (!ie->indirect_info->polymorphic)
>>>> @@ -1516,8 +1517,7 @@ ipa_get_indirect_edge_target_1 (struct c
>>>>             t = NULL;
>>>>         }
>>>>        else
>>>> -       t = (known_vals.length () > (unsigned int) param_index
>>>> -            ? known_vals[param_index] : NULL);
>>>> +       t = NULL;
>>>>
>>>>        if (t &&
>>>>           TREE_CODE (t) == ADDR_EXPR

Patch

Index: gcc/gimple-low.c
===================================================================
--- gcc/gimple-low.c	(revision 199780)
+++ gcc/gimple-low.c	(working copy)
@@ -204,7 +204,7 @@  struct gimple_opt_pass pass_lower_cf =
    return false.  */
 
 static bool
-gimple_check_call_args (gimple stmt, tree fndecl)
+gimple_check_call_args (gimple stmt, tree fndecl, bool args_count_match)
 {
   tree parms, p;
   unsigned int i, nargs;
@@ -243,6 +243,8 @@  static bool
 		  && !fold_convertible_p (DECL_ARG_TYPE (p), arg)))
             return false;
 	}
+      if (args_count_match && p)
+	return false;
     }
   else if (parms)
     {
@@ -271,11 +273,13 @@  static bool
 }
 
 /* Verify if the type of the argument and lhs of CALL_STMT matches
-   that of the function declaration CALLEE.
+   that of the function declaration CALLEE. If ARGS_COUNT_MATCH is
+   true, the arg count needs to be the same.
    If we cannot verify this or there is a mismatch, return false.  */
 
 bool
-gimple_check_call_matching_types (gimple call_stmt, tree callee)
+gimple_check_call_matching_types (gimple call_stmt, tree callee,
+				  bool args_count_match)
 {
   tree lhs;
 
@@ -285,7 +289,7 @@  bool
        && !useless_type_conversion_p (TREE_TYPE (DECL_RESULT (callee)),
                                       TREE_TYPE (lhs))
        && !fold_convertible_p (TREE_TYPE (DECL_RESULT (callee)), lhs))
-      || !gimple_check_call_args (call_stmt, callee))
+      || !gimple_check_call_args (call_stmt, callee, args_count_match))
     return false;
   return true;
 }
Index: gcc/ipa-inline.c
===================================================================
--- gcc/ipa-inline.c	(revision 199780)
+++ gcc/ipa-inline.c	(working copy)
@@ -2054,8 +2054,8 @@  early_inliner (void)
 	      es->call_stmt_time
 		= estimate_num_insns (edge->call_stmt, &eni_time_weights);
 	      if (edge->callee->symbol.decl
-		  && !gimple_check_call_matching_types (edge->call_stmt,
-							edge->callee->symbol.decl))
+		  && !gimple_check_call_matching_types (
+		      edge->call_stmt, edge->callee->symbol.decl, false))
 		edge->call_stmt_cannot_inline_p = true;
 	    }
 	  timevar_pop (TV_INTEGRATION);
Index: gcc/cgraph.c
===================================================================
--- gcc/cgraph.c	(revision 199780)
+++ gcc/cgraph.c	(working copy)
@@ -816,7 +816,8 @@  cgraph_create_edge_1 (struct cgraph_node *caller,
   pop_cfun ();
   if (call_stmt
       && callee && callee->symbol.decl
-      && !gimple_check_call_matching_types (call_stmt, callee->symbol.decl))
+      && !gimple_check_call_matching_types (call_stmt, callee->symbol.decl,
+					    false))
     edge->call_stmt_cannot_inline_p = true;
   else
     edge->call_stmt_cannot_inline_p = false;
@@ -1016,7 +1017,8 @@  cgraph_make_edge_direct (struct cgraph_edge *edge,
 
   if (edge->call_stmt)
     edge->call_stmt_cannot_inline_p
-      = !gimple_check_call_matching_types (edge->call_stmt, callee->symbol.decl);
+      = !gimple_check_call_matching_types (edge->call_stmt, callee->symbol.decl,
+					   false);
 
   /* We need to re-determine the inlining status of the edge.  */
   initialize_inline_failed (edge);
Index: gcc/tree-flow.h
===================================================================
--- gcc/tree-flow.h	(revision 199780)
+++ gcc/tree-flow.h	(working copy)
@@ -464,7 +464,7 @@  extern void record_vars_into (tree, tree);
 extern void record_vars (tree);
 extern bool gimple_seq_may_fallthru (gimple_seq);
 extern bool gimple_stmt_may_fallthru (gimple);
-extern bool gimple_check_call_matching_types (gimple, tree);
+extern bool gimple_check_call_matching_types (gimple, tree, bool);
 
 
 /* In tree-ssa.c  */
Index: gcc/value-prof.c
===================================================================
--- gcc/value-prof.c	(revision 199780)
+++ gcc/value-prof.c	(working copy)
@@ -1231,7 +1231,7 @@  static bool
 check_ic_target (gimple call_stmt, struct cgraph_node *target)
 {
    location_t locus;
-   if (gimple_check_call_matching_types (call_stmt, target->symbol.decl))
+   if (gimple_check_call_matching_types (call_stmt, target->symbol.decl, true))
      return true;
 
    locus =  gimple_location (call_stmt);
Index: gcc/ipa-prop.c
===================================================================
--- gcc/ipa-prop.c	(revision 199780)
+++ gcc/ipa-prop.c	(working copy)
@@ -2468,8 +2468,9 @@  update_indirect_edges_after_inlining (struct cgrap
 	  new_direct_edge->indirect_inlining_edge = 1;
 	  if (new_direct_edge->call_stmt)
 	    new_direct_edge->call_stmt_cannot_inline_p
-	      = !gimple_check_call_matching_types (new_direct_edge->call_stmt,
-						   new_direct_edge->callee->symbol.decl);
+	      = !gimple_check_call_matching_types (
+		  new_direct_edge->call_stmt,
+		  new_direct_edge->callee->symbol.decl, false);
 	  if (new_edges)
 	    {
 	      new_edges->safe_push (new_direct_edge);