diff mbox series

use all same precision in wide_int arguments (PR 93986)

Message ID 8bc7e8b7-c6c4-5ab7-8995-54093ef96676@gmail.com
State New
Headers show
Series use all same precision in wide_int arguments (PR 93986) | expand

Commit Message

Martin Sebor March 2, 2020, 11:04 p.m. UTC 
The wide_int APIs expect operands to have the same precision and
abort when they don't.  This is especially insidious in code where
the operands normally do have the same precision but where mixed
precision arguments can come up as a result of unusual combinations
optimization options.  That is also what precipitated pr93986.

The attached patch adjusts the code to extend all wide_int operands
to the same precision to avoid the ICE.

Besides the usual bootstrap/testing I also compiled all string tests
in gcc.dg with the same options as in the test case in pr93986 in
an effort to weed out any lingering bugs like it (found none).

Martin

Comments

Richard Biener March 3, 2020, 9:42 a.m. UTC | #1 
On Tue, Mar 3, 2020 at 12:04 AM Martin Sebor <msebor@gmail.com> wrote:
>
> The wide_int APIs expect operands to have the same precision and
> abort when they don't.  This is especially insidious in code where
> the operands normally do have the same precision but where mixed
> precision arguments can come up as a result of unusual combinations
> optimization options.  That is also what precipitated pr93986.

If you want sth like (signed) arbitrary precision arithmetic then you can
use widest_int instead.  Or, since you're working with offsets, offset_int
is another good choice.

> The attached patch adjusts the code to extend all wide_int operands
> to the same precision to avoid the ICE.
>
> Besides the usual bootstrap/testing I also compiled all string tests
> in gcc.dg with the same options as in the test case in pr93986 in
> an effort to weed out any lingering bugs like it (found none).
>
> Martin
Martin Sebor March 3, 2020, 3:39 p.m. UTC | #2 
On 3/3/20 2:42 AM, Richard Biener wrote:
> On Tue, Mar 3, 2020 at 12:04 AM Martin Sebor <msebor@gmail.com> wrote:
>>
>> The wide_int APIs expect operands to have the same precision and
>> abort when they don't.  This is especially insidious in code where
>> the operands normally do have the same precision but where mixed
>> precision arguments can come up as a result of unusual combinations
>> optimization options.  That is also what precipitated pr93986.
> 
> If you want sth like (signed) arbitrary precision arithmetic then you can
> use widest_int instead.  Or, since you're working with offsets, offset_int
> is another good choice.

Yes, I would much prefer not to have to do all this myself (and
risk getting it wrong).  Unfortunately, the APIs that obtain
the ranges all use wide_int, so I'd have to convert them one way
or the other.  I could change some of the APIs but not all of
them (e.g., get_range_info).

Martin


> 
>> The attached patch adjusts the code to extend all wide_int operands
>> to the same precision to avoid the ICE.
>>
>> Besides the usual bootstrap/testing I also compiled all string tests
>> in gcc.dg with the same options as in the test case in pr93986 in
>> an effort to weed out any lingering bugs like it (found none).
>>
>> Martin
Richard Biener March 3, 2020, 6:50 p.m. UTC | #3 
On March 3, 2020 4:39:34 PM GMT+01:00, Martin Sebor <msebor@gmail.com> wrote:
>On 3/3/20 2:42 AM, Richard Biener wrote:
>> On Tue, Mar 3, 2020 at 12:04 AM Martin Sebor <msebor@gmail.com>
>wrote:
>>>
>>> The wide_int APIs expect operands to have the same precision and
>>> abort when they don't.  This is especially insidious in code where
>>> the operands normally do have the same precision but where mixed
>>> precision arguments can come up as a result of unusual combinations
>>> optimization options.  That is also what precipitated pr93986.
>> 
>> If you want sth like (signed) arbitrary precision arithmetic then you
>can
>> use widest_int instead.  Or, since you're working with offsets,
>offset_int
>> is another good choice.
>
>Yes, I would much prefer not to have to do all this myself (and
>risk getting it wrong).  Unfortunately, the APIs that obtain
>the ranges all use wide_int, so I'd have to convert them one way
>or the other.  I could change some of the APIs but not all of
>them (e.g., get_range_info).

You can convert wide_int to both offset and widest int. 

Richard. 

>Martin
>
>
>> 
>>> The attached patch adjusts the code to extend all wide_int operands
>>> to the same precision to avoid the ICE.
>>>
>>> Besides the usual bootstrap/testing I also compiled all string tests
>>> in gcc.dg with the same options as in the test case in pr93986 in
>>> an effort to weed out any lingering bugs like it (found none).
>>>
>>> Martin
Martin Sebor March 4, 2020, 12:25 a.m. UTC | #4 
On 3/3/20 11:50 AM, Richard Biener wrote:
> On March 3, 2020 4:39:34 PM GMT+01:00, Martin Sebor <msebor@gmail.com> wrote:
>> On 3/3/20 2:42 AM, Richard Biener wrote:
>>> On Tue, Mar 3, 2020 at 12:04 AM Martin Sebor <msebor@gmail.com>
>> wrote:
>>>>
>>>> The wide_int APIs expect operands to have the same precision and
>>>> abort when they don't.  This is especially insidious in code where
>>>> the operands normally do have the same precision but where mixed
>>>> precision arguments can come up as a result of unusual combinations
>>>> optimization options.  That is also what precipitated pr93986.
>>>
>>> If you want sth like (signed) arbitrary precision arithmetic then you
>> can
>>> use widest_int instead.  Or, since you're working with offsets,
>> offset_int
>>> is another good choice.
>>
>> Yes, I would much prefer not to have to do all this myself (and
>> risk getting it wrong).  Unfortunately, the APIs that obtain
>> the ranges all use wide_int, so I'd have to convert them one way
>> or the other.  I could change some of the APIs but not all of
>> them (e.g., get_range_info).
> 
> You can convert wide_int to both offset and widest int.

Yes, I realize that.  But it seems like six of one vs half a dozen
of the other.  Either way some variables need converting.  I don't
really have a preference for either approach.  I just copied
the solution I already used in gimple_call_alloc_size, and I chose
that one there because the get_range calls take wide_int, and
because gimple_call_alloc_size's caller (the function I'm changing
now) also uses wide_int.

Everything could be changed to widest_int instead but I'm not sure
it would make sense (e.g., get_range_info).  And unless everything
is changed, the APIs that interoperate need to convert between one
another.

I went ahead and rewrote the patch to use widest_int.  It let me get
rid of some conversions but it introduced others.  Most of them look
pretty much the same between the two approaches but there are more
of them with widest_int because of the extra variables.  The updated
patch is also about one and half times longer.

Is one approach significantly better than the other or were you just
pointing out another way of doing it?

Either way, please choose one and approve.

Thanks
Martin

> 
> Richard.
> 
>> Martin
>>
>>
>>>
>>>> The attached patch adjusts the code to extend all wide_int operands
>>>> to the same precision to avoid the ICE.
>>>>
>>>> Besides the usual bootstrap/testing I also compiled all string tests
>>>> in gcc.dg with the same options as in the test case in pr93986 in
>>>> an effort to weed out any lingering bugs like it (found none).
>>>>
>>>> Martin
>
Richard Biener March 4, 2020, 1:41 p.m. UTC | #5 
On Wed, Mar 4, 2020 at 1:26 AM Martin Sebor <msebor@gmail.com> wrote:
>
> On 3/3/20 11:50 AM, Richard Biener wrote:
> > On March 3, 2020 4:39:34 PM GMT+01:00, Martin Sebor <msebor@gmail.com> wrote:
> >> On 3/3/20 2:42 AM, Richard Biener wrote:
> >>> On Tue, Mar 3, 2020 at 12:04 AM Martin Sebor <msebor@gmail.com>
> >> wrote:
> >>>>
> >>>> The wide_int APIs expect operands to have the same precision and
> >>>> abort when they don't.  This is especially insidious in code where
> >>>> the operands normally do have the same precision but where mixed
> >>>> precision arguments can come up as a result of unusual combinations
> >>>> optimization options.  That is also what precipitated pr93986.
> >>>
> >>> If you want sth like (signed) arbitrary precision arithmetic then you
> >> can
> >>> use widest_int instead.  Or, since you're working with offsets,
> >> offset_int
> >>> is another good choice.
> >>
> >> Yes, I would much prefer not to have to do all this myself (and
> >> risk getting it wrong).  Unfortunately, the APIs that obtain
> >> the ranges all use wide_int, so I'd have to convert them one way
> >> or the other.  I could change some of the APIs but not all of
> >> them (e.g., get_range_info).
> >
> > You can convert wide_int to both offset and widest int.
>
> Yes, I realize that.  But it seems like six of one vs half a dozen
> of the other.  Either way some variables need converting.  I don't
> really have a preference for either approach.  I just copied
> the solution I already used in gimple_call_alloc_size, and I chose
> that one there because the get_range calls take wide_int, and
> because gimple_call_alloc_size's caller (the function I'm changing
> now) also uses wide_int.
>
> Everything could be changed to widest_int instead but I'm not sure
> it would make sense (e.g., get_range_info).  And unless everything
> is changed, the APIs that interoperate need to convert between one
> another.
>
> I went ahead and rewrote the patch to use widest_int.  It let me get
> rid of some conversions but it introduced others.  Most of them look
> pretty much the same between the two approaches but there are more
> of them with widest_int because of the extra variables.  The updated
> patch is also about one and half times longer.
>
> Is one approach significantly better than the other or were you just
> pointing out another way of doing it?

I was just pointing out other ways of doing it since you weren't happy.

> Either way, please choose one and approve.

The widest_int one is OK, it looks slightly better to me (no explicit
precisions).

Thanks,
Richard.

> Thanks
> Martin
>
> >
> > Richard.
> >
> >> Martin
> >>
> >>
> >>>
> >>>> The attached patch adjusts the code to extend all wide_int operands
> >>>> to the same precision to avoid the ICE.
> >>>>
> >>>> Besides the usual bootstrap/testing I also compiled all string tests
> >>>> in gcc.dg with the same options as in the test case in pr93986 in
> >>>> an effort to weed out any lingering bugs like it (found none).
> >>>>
> >>>> Martin
> >
>
diff mbox series

Patch

PR tree-optimization/93986 - ICE on mixed-precision wide_int arguments

gcc/testsuite/ChangeLog:

	PR tree-optimization/93986
	* gcc.dg/pr93986.c: New test.

gcc/ChangeLog:

	PR tree-optimization/93986
	* tree-ssa-strlen.c (maybe_warn_overflow): Convert all wide_int
	operands to the same precision to avoid ICEs.

diff --git a/gcc/testsuite/gcc.dg/pr93986.c b/gcc/testsuite/gcc.dg/pr93986.c
new file mode 100644
index 00000000000..bdbc192a01d
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/pr93986.c
@@ -0,0 +1,16 @@ 
+/* PR tree-optimization/93986 - ICE in decompose, at wide-int.h:984
+   { dg-do compile }
+   { dg-options "-O1 -foptimize-strlen -ftree-slp-vectorize" } */
+
+int dd (void);
+
+void ya (int cm)
+{
+  char s2[cm];
+
+  s2[cm-12] = s2[cm-11] = s2[cm-10] = s2[cm-9]
+    = s2[cm-8] = s2[cm-7] = s2[cm-6] = s2[cm-5] = ' ';
+
+  if (dd ())
+    __builtin_exit (0);
+}
diff --git a/gcc/tree-ssa-strlen.c b/gcc/tree-ssa-strlen.c
index b76b54efbd8..136a72700d9 100644
--- a/gcc/tree-ssa-strlen.c
+++ b/gcc/tree-ssa-strlen.c
@@ -1924,11 +1924,22 @@  maybe_warn_overflow (gimple *stmt, tree len,
   if (TREE_NO_WARNING (dest))
     return;
 
+  /* Use maximum precision to avoid overflow in the addition below.
+     Make sure all operands have the same precision to keep wide_int
+     from ICE'ing.  */
+  const int prec = ADDR_MAX_PRECISION;
+  /* Convenience constants.  */
+  const wide_int diff_min
+    = wi::to_wide (TYPE_MIN_VALUE (ptrdiff_type_node), prec);
+  const wide_int diff_max
+    = wi::to_wide (TYPE_MAX_VALUE (ptrdiff_type_node), prec);
+  const wide_int size_max
+    = wi::to_wide (TYPE_MAX_VALUE (size_type_node), prec);
+
   /* The offset into the destination object computed below and not
      reflected in DESTSIZE.  */
   wide_int offrng[2];
-  const int off_prec = TYPE_PRECISION (ptrdiff_type_node);
-  offrng[0] = offrng[1] = wi::zero (off_prec);
+  offrng[0] = offrng[1] = wi::zero (prec);
 
   if (!si)
     {
@@ -1943,13 +1954,14 @@  maybe_warn_overflow (gimple *stmt, tree len,
 	  ref = TREE_OPERAND (ref, 0);
 	  if (get_range (off, offrng, rvals))
 	    {
-	      offrng[0] = offrng[0].from (offrng[0], off_prec, SIGNED);
-	      offrng[1] = offrng[1].from (offrng[1], off_prec, SIGNED);
+	      /* Convert offsets to the expected precision.  */
+	      offrng[0] = wide_int::from (offrng[0], prec, SIGNED);
+	      offrng[1] = wide_int::from (offrng[1], prec, SIGNED);
 	    }
 	  else
 	    {
-	      offrng[0] = wi::to_wide (TYPE_MIN_VALUE (ptrdiff_type_node));
-	      offrng[1] = wi::to_wide (TYPE_MAX_VALUE (ptrdiff_type_node));
+	      offrng[0] = diff_min;
+	      offrng[1] = diff_max;
 	    }
 	}
 
@@ -1960,13 +1972,13 @@  maybe_warn_overflow (gimple *stmt, tree len,
 	  wide_int memoffrng[2];
 	  if (get_range (mem_off, memoffrng, rvals))
 	    {
-	      offrng[0] += memoffrng[0];
-	      offrng[1] += memoffrng[1];
+	      offrng[0] += wide_int::from (memoffrng[0], prec, SIGNED);
+	      offrng[1] += wide_int::from (memoffrng[1], prec, SIGNED);
 	    }
 	  else
 	    {
-	      offrng[0] = wi::to_wide (TYPE_MIN_VALUE (ptrdiff_type_node));
-	      offrng[1] = wi::to_wide (TYPE_MAX_VALUE (ptrdiff_type_node));
+	      offrng[0] = diff_min;
+	      offrng[1] = diff_max;
 	    }
 	}
 
@@ -1974,8 +1986,8 @@  maybe_warn_overflow (gimple *stmt, tree len,
       if (int idx = get_stridx (ref, stroffrng, rvals))
 	{
 	  si = get_strinfo (idx);
-	  offrng[0] += stroffrng[0];
-	  offrng[1] += stroffrng[1];
+	  offrng[0] += wide_int::from (stroffrng[0], prec, SIGNED);
+	  offrng[1] += wide_int::from (stroffrng[1], prec, SIGNED);
 	}
     }
 
@@ -1995,7 +2007,6 @@  maybe_warn_overflow (gimple *stmt, tree len,
   /* Compute the range of sizes of the destination object.  The range
      is constant for declared objects but may be a range for allocated
      objects.  */
-  const int siz_prec = TYPE_PRECISION (size_type_node);
   wide_int sizrng[2];
   if (si)
     {
@@ -2003,7 +2014,7 @@  maybe_warn_overflow (gimple *stmt, tree len,
       alloc_call = si->alloc;
     }
   else
-    offrng[0] = offrng[1] = wi::zero (off_prec);
+    offrng[0] = offrng[1] = wi::zero (prec);
 
   if (!destsize)
     {
@@ -2014,7 +2025,7 @@  maybe_warn_overflow (gimple *stmt, tree len,
 	{
 	  /* Remember OFF but clear OFFRNG that may have been set above.  */
 	  destoff = off;
-	  offrng[0] = offrng[1] = wi::zero (off_prec);
+	  offrng[0] = offrng[1] = wi::zero (prec);
 
 	  if (destdecl && TREE_CODE (destdecl) == SSA_NAME)
 	    {
@@ -2030,20 +2041,20 @@  maybe_warn_overflow (gimple *stmt, tree len,
 		 so that overflow in allocated objects whose size depends
 		 on the strlen of the source can still be diagnosed
 		 below.  */
-	      sizrng[0] = wi::zero (siz_prec);
-	      sizrng[1] = wi::to_wide (TYPE_MAX_VALUE (sizetype));
+	      sizrng[0] = wi::zero (prec);
+	      sizrng[1] = size_max;
 	    }
 	}
     }
 
   if (!destsize)
     {
-      sizrng[0] = wi::zero (siz_prec);
-      sizrng[1] = wi::to_wide (TYPE_MAX_VALUE (sizetype));
+      sizrng[0] = wi::zero (prec);
+      sizrng[1] = size_max;
     };
 
-  sizrng[0] = sizrng[0].from (sizrng[0], siz_prec, UNSIGNED);
-  sizrng[1] = sizrng[1].from (sizrng[1], siz_prec, UNSIGNED);
+  sizrng[0] = wide_int::from (sizrng[0], prec, UNSIGNED);
+  sizrng[1] = wide_int::from (sizrng[1], prec, UNSIGNED);
 
   /* Return early if the DESTSIZE size expression is the same as LEN
      and the offset into the destination is zero.  This might happen
@@ -2056,6 +2067,9 @@  maybe_warn_overflow (gimple *stmt, tree len,
   if (!get_range (len, lenrng, rvals))
     return;
 
+  lenrng[0] = wide_int::from (lenrng[0], prec, SIGNED);
+  lenrng[1] = wide_int::from (lenrng[1], prec, SIGNED);
+
   if (plus_one)
     {
       lenrng[0] += 1;
@@ -2259,8 +2273,7 @@  maybe_warn_overflow (gimple *stmt, tree len,
       else if (sizrng[0] == 0)
 	{
 	  /* Avoid printing impossible sizes.  */
-	  if (wi::ltu_p (sizrng[1],
-			 wi::to_wide (TYPE_MAX_VALUE (ptrdiff_type_node)) - 2))
+	  if (wi::ltu_p (sizrng[1], diff_max - 2))
 	    inform (gimple_location (alloc_call),
 		    "at offset %s to an object with size at most %wu "
 		    "declared here",
@@ -2284,8 +2297,7 @@  maybe_warn_overflow (gimple *stmt, tree len,
   else if (sizrng[0] == 0)
     {
       /* Avoid printing impossible sizes.  */
-      if (wi::ltu_p (sizrng[1],
-		     wi::to_wide (TYPE_MAX_VALUE (ptrdiff_type_node)) - 2))
+      if (wi::ltu_p (sizrng[1], diff_max - 2))
 	inform (gimple_location (alloc_call),
 		"at offset %s to an object with size at most %wu allocated "
 		"by %qD here",