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 |
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
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
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
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 >
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 > > >
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",