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

["Richard Guenther" <richard dot guenther at gmail dot com>]

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
> 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.