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: [wide-int 2/8] Fix ubsan internal-fn.c handling


On Wed, Apr 23, 2014 at 3:29 PM, Richard Sandiford
<rdsandiford@googlemail.com> wrote:
> Richard Sandiford <rdsandiford@googlemail.com> writes:
>> This code was mixing hprec and hprec*2 wide_ints.  The simplest fix
>> seemed to be to introduce a function that gives the minimum precision
>> necessary to represent a function, which also means that no temporary
>> wide_ints are needed.
>>
>> Other places might be able to use this too, but I'd like to look at
>> that after the merge.
>>
>> The patch series fixed a regression in c-c++-common/ubsan/overflow-2.c
>> and I assume it's due to this change.
>>
>> Tested on x86_64-linux-gnu.  OK to install?
>
> Richard B. expressed doubts about this on IRC, so for a bit more detail:
>
> The comparisons we're doing are on the range of an SSA name.
> There are three ways that these ranges could be stored in the
> range_info_def:
>
> (1) as INTEGER_CSTs.  This was felt to be unacceptable because it would
>     create too many garbage constants.
>
> (2) as widest_ints.  This too was unacceptable because it would bloat
>     the range_info_def.
>
> (3) as a form of wide_int in which the HWIs are allocated as a trailing
>     part of the containing structure.  This means that range_info_defs
>     for 64-bit types only have 3 HWIs (smaller than now).
>
> We went for (3).
>
> Having decided to store the ranges like wide_ints, the question then is:
> what about the get/set_range_info interface?  Two obvious options are:
>
> (a) present the ranges as wide_ints.
>
> (b) present the ranges as widest_ints, converting in and out as necessary.
>
> (a) is more efficient and seems to fit well with the pre-ubsan callers,
> so that's what was chosen.
>
> In the patch we have two wide_ints that have the same precision as the
> SSA name.  The values we were creating via wi::min_value and wi::max_value
> instead had half that precision.  This is the same kind of mismatch as
> you'd get comparing HImode and SImode in RTL, say.
>
> We could fix the bug by using something like:
>
>                   if (wi::les_p (arg0_max, wi::mask (hprec, false, prec))
>                       && wi::les_p (wi::mask (hprec, true, prec), arg0_min))
>
> etc.  Or we could extend the wide_ints to widest_ints so that "precision
> doesn't matter" when doing the comparisons.  But both those options
> involve temporaries and seem unnecessarily complicated.  All we're
> really asking here is: what is the minimum precision needed to represent
> this constant?  That's something that could be generally useful
> (e.g. when checking whether a value fits a type) so the patch adds
> a corresponding wi:: function.

Ah, that makes sense now ;)

Thus the patch is ok.

Thanks,
Richard.

> Thanks,
> Richard
>
>
>>
>> Thanks,
>> Richard
>>
>>
>> Index: gcc/internal-fn.c
>> ===================================================================
>> --- gcc/internal-fn.c 2014-04-22 20:31:10.516895118 +0100
>> +++ gcc/internal-fn.c 2014-04-22 20:31:25.842005530 +0100
>> @@ -478,7 +478,7 @@ ubsan_expand_si_overflow_mul_check (gimp
>>         rtx do_overflow = gen_label_rtx ();
>>         rtx hipart_different = gen_label_rtx ();
>>
>> -       int hprec = GET_MODE_PRECISION (hmode);
>> +       unsigned int hprec = GET_MODE_PRECISION (hmode);
>>         rtx hipart0 = expand_shift (RSHIFT_EXPR, mode, op0, hprec,
>>                                     NULL_RTX, 0);
>>         hipart0 = gen_lowpart (hmode, hipart0);
>> @@ -513,12 +513,11 @@ ubsan_expand_si_overflow_mul_check (gimp
>>             wide_int arg0_min, arg0_max;
>>             if (get_range_info (arg0, &arg0_min, &arg0_max) == VR_RANGE)
>>               {
>> -               if (wi::les_p (arg0_max, wi::max_value (hprec, SIGNED))
>> -                   && wi::les_p (wi::min_value (hprec, SIGNED), arg0_min))
>> +               unsigned int mprec0 = wi::min_precision (arg0_min, SIGNED);
>> +               unsigned int mprec1 = wi::min_precision (arg0_max, SIGNED);
>> +               if (mprec0 <= hprec && mprec1 <= hprec)
>>                   op0_small_p = true;
>> -               else if (wi::les_p (arg0_max, wi::max_value (hprec, UNSIGNED))
>> -                        && wi::les_p (~wi::max_value (hprec, UNSIGNED),
>> -                                      arg0_min))
>> +               else if (mprec0 <= hprec + 1 && mprec1 <= hprec + 1)
>>                   op0_medium_p = true;
>>                 if (!wi::neg_p (arg0_min, TYPE_SIGN (TREE_TYPE (arg0))))
>>                   op0_sign = 0;
>> @@ -531,12 +530,11 @@ ubsan_expand_si_overflow_mul_check (gimp
>>             wide_int arg1_min, arg1_max;
>>             if (get_range_info (arg1, &arg1_min, &arg1_max) == VR_RANGE)
>>               {
>> -               if (wi::les_p (arg1_max, wi::max_value (hprec, SIGNED))
>> -                   && wi::les_p (wi::min_value (hprec, SIGNED), arg1_min))
>> +               unsigned int mprec0 = wi::min_precision (arg1_min, SIGNED);
>> +               unsigned int mprec1 = wi::min_precision (arg1_max, SIGNED);
>> +               if (mprec0 <= hprec && mprec1 <= hprec)
>>                   op1_small_p = true;
>> -               else if (wi::les_p (arg1_max, wi::max_value (hprec, UNSIGNED))
>> -                        && wi::les_p (~wi::max_value (hprec, UNSIGNED),
>> -                                      arg1_min))
>> +               else if (mprec0 <= hprec + 1 && mprec1 <= hprec + 1)
>>                   op1_medium_p = true;
>>                 if (!wi::neg_p (arg1_min, TYPE_SIGN (TREE_TYPE (arg1))))
>>                   op1_sign = 0;
>> Index: gcc/wide-int.h
>> ===================================================================
>> --- gcc/wide-int.h    2014-04-22 20:31:10.516895118 +0100
>> +++ gcc/wide-int.h    2014-04-22 20:31:25.842005530 +0100
>> @@ -562,6 +562,9 @@ #define SHIFT_FUNCTION \
>>
>>    template <typename T>
>>    unsigned HOST_WIDE_INT extract_uhwi (const T &, unsigned int, unsigned int);
>> +
>> +  template <typename T>
>> +  unsigned int min_precision (const T &, signop);
>>  }
>>
>>  namespace wi
>> @@ -2995,6 +2998,17 @@ wi::extract_uhwi (const T &x, unsigned i
>>    return zext_hwi (res, width);
>>  }
>>
>> +/* Return the minimum precision needed to store X with sign SGN.  */
>> +template <typename T>
>> +inline unsigned int
>> +wi::min_precision (const T &x, signop sgn)
>> +{
>> +  if (sgn == SIGNED)
>> +    return wi::get_precision (x) - clrsb (x);
>> +  else
>> +    return wi::get_precision (x) - clz (x);
>> +}
>> +
>>  template<typename T>
>>  void
>>  gt_ggc_mx (generic_wide_int <T> *)


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