This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: PATCH: Bug fix for counter overflow and arithmetic exception


Hi,

    Yes, I tested with bootstrap and regtest passes. The overflow was
seen in the routines modified in tree-inline.c due to count_scale
being defined as an int. The division by 0 was seen in value-prof.c.
When adding the check to avoid division by 0, I also changed the type
of the variable prob to be gcov_type to be extra safe.

Best,
    Vinodha

On Fri, Jun 6, 2008 at 3:08 AM, Jan Hubicka <hubicka@ucw.cz> wrote:
>> On Fri, Jun 6, 2008 at 12:39 AM, Vinodha Ramasamy <vinodha@google.com> wrote:
>> > Description:
>> > Variables to hold edge counts were incorrectly defined as int type, resulting
>> > in counter overflows. Fixed the type of such variables to be gcov_type. Also
>
> Those variables all hold probability that is supposed to be normalized
> in range 0...REG_BR_PROB_BASE (that is count <= all).  Where do you see
> the overflow?
>
> Honza
>> > added code to check if a counter value is 0, when the counter is used as the
>> > denominator in a divide operation to avoid arithmetic exception.
>> >
>> > Changelog:
>> > 2008-06-05 Vinodha Ramasamy  <vinodha@google.com>
>> >
>> >       * value_prob.c (tree_divmod_fixed_value_transform): Use gcov_type
>> >       for prob.  Add check for all > 0.
>>
>> A little bit too "explicit" IMHO.  I'd just say "Use gcov_type.  Avoid
>> division by zero."  likewise below.
>>
>> >       (tree_mod_pow2_value_transform, tree_ic_transform): Likewise.
>> >       (tree_stringops_transform): Likewise.
>> >       (tree_mod_subtract_transform): Use gcov_type for prob1 and prob2.
>> >       Add check for all > 0.
>> >       * tree-inline.c (copy_bb, copy_edges_for_bb): Corrected type of
>> >       count_scale to gcov_type.
>> >       (initialize_cfun): Corrected type of count_scale and frequency_scale
>> >       to gcov_type.
>> >
>> > Index: gcc/value-prof.c
>> > ==============================
>> > =====================================
>> > --- gcc/value-prof.c    (revision 136174)
>> > +++ gcc/value-prof.c    (working copy)
>> > @@ -609,7 +609,7 @@ tree_divmod_fixed_value_transform (tree
>> >   enum tree_code code;
>> >   gcov_type val, count, all;
>> >   tree modify, op, op1, op2, result, value, tree_val;
>> > -  int prob;
>> > +  gcov_type prob;
>> >
>> >   modify = stmt;
>> >   if (TREE_CODE (stmt) == RETURN_EXPR
>> > @@ -650,7 +650,10 @@ tree_divmod_fixed_value_transform (tree
>> >     return false;
>> >
>> >   /* Compute probability of taking the optimal path.  */
>> > -  prob = (count * REG_BR_PROB_BASE + all / 2) / all;
>> > +  if (all > 0)
>> > +    prob = (count * REG_BR_PROB_BASE + all / 2) / all;
>> > +  else
>> > +    prob = 0;
>> >
>> >   tree_val = build_int_cst_wide (get_gcov_type (),
>> >                                 (unsigned HOST_WIDE_INT) val,
>> > @@ -769,7 +772,7 @@ tree_mod_pow2_value_transform (tree stmt
>> >   enum tree_code code;
>> >   gcov_type count, wrong_values, all;
>> >   tree modify, op, op1, op2, result, value;
>> > -  int prob;
>> > +  gcov_type prob;
>> >
>> >   modify = stmt;
>> >   if (TREE_CODE (stmt) == RETURN_EXPR
>> > @@ -816,7 +819,10 @@ tree_mod_pow2_value_transform (tree stmt
>> >   if (check_counter (stmt, "pow2", all, bb_for_stmt (stmt)->count))
>> >     return false;
>> >
>> > -  prob = (count * REG_BR_PROB_BASE + all / 2) / all;
>> > +  if (all > 0)
>> > +    prob = (count * REG_BR_PROB_BASE + all / 2) / all;
>> > +  else
>> > +    prob = 0;
>> >
>> >   result = tree_mod_pow2 (stmt, op, op1, op2, prob, count, all);
>> >
>> > @@ -948,7 +954,7 @@ tree_mod_subtract_transform (tree stmt)
>> >   enum tree_code code;
>> >   gcov_type count, wrong_values, all;
>> >   tree modify, op, op1, op2, result, value;
>> > -  int prob1, prob2;
>> > +  gcov_type prob1, prob2;
>> >   unsigned int i, steps;
>> >   gcov_type count1, count2;
>> >
>> > @@ -1015,8 +1021,15 @@ tree_mod_subtract_transform (tree stmt)
>> >     }
>> >
>> >   /* Compute probability of taking the optimal path(s).  */
>> > -  prob1 = (count1 * REG_BR_PROB_BASE + all / 2) / all;
>> > -  prob2 = (count2 * REG_BR_PROB_BASE + all / 2) / all;
>> > +  if (all > 0)
>> > +    {
>> > +      prob1 = (count1 * REG_BR_PROB_BASE + all / 2) / all;
>> > +      prob2 = (count2 * REG_BR_PROB_BASE + all / 2) / all;
>> > +    }
>> > +  else
>> > +    {
>> > +      prob1 = prob2 = 0;
>> > +    }
>>
>> no braces needed around the last statement.
>>
>> You didn't say how you tested this patch, thus - ok with the above
>> changes if the patch passes boostrap and regtest.
>>
>> Thanks,
>> Richard.
>


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]