This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
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.
>