* [PATCH] ipa: Avoid constructing aggregate jump functions with huge offsets (PR 109303) @ 2023-03-31 8:45 Martin Jambor 2023-03-31 8:57 ` Richard Biener 0 siblings, 1 reply; 4+ messages in thread From: Martin Jambor @ 2023-03-31 8:45 UTC (permalink / raw) To: GCC Patches; +Cc: Jan Hubicka Hi, we are in the process of changing data structures holding information about constants passed by reference and in aggregates to use unsigned int offsets rather than HOST_WIDE_INT (which was selected simply because that is what fell out of get_ref_base_and_extent at that time) in order to conserve memory, especially at WPA time. PR 109303 testcase discovers that we do not properly check that we only create jump functions with offsets (plus sizes) that fit into the smaller type. This patch adds the necessary check. Bootstrapped and tested on x86_64-linux. OK for master? Thanks, Martin gcc/ChangeLog: 2023-03-30 Martin Jambor <mjambor@suse.cz> PR ipa/109303 * ipa-prop.cc (determine_known_aggregate_parts): Check that the offset + size will be representable in unsigned int. gcc/testsuite/ChangeLog: 2023-03-30 Jakub Jelinek <jakub@redhat.com> Martin Jambor <mjambor@suse.cz> PR ipa/109303 * gcc.dg/pr109303.c: New test. --- gcc/ipa-prop.cc | 4 +++- gcc/testsuite/gcc.dg/pr109303.c | 24 ++++++++++++++++++++++++ 2 files changed, 27 insertions(+), 1 deletion(-) create mode 100644 gcc/testsuite/gcc.dg/pr109303.c diff --git a/gcc/ipa-prop.cc b/gcc/ipa-prop.cc index de45dbccf16..9ffd49b590c 100644 --- a/gcc/ipa-prop.cc +++ b/gcc/ipa-prop.cc @@ -2086,7 +2086,9 @@ determine_known_aggregate_parts (struct ipa_func_body_info *fbi, whether its value is clobbered any other dominating one. */ if ((content->value.pass_through.formal_id >= 0 || content->value.pass_through.operand) - && !clobber_by_agg_contents_list_p (all_list, content)) + && !clobber_by_agg_contents_list_p (all_list, content) + && (content->offset + content->size - arg_offset + <= (HOST_WIDE_INT) UINT_MAX * BITS_PER_UNIT)) { struct ipa_known_agg_contents_list *copy = XALLOCA (struct ipa_known_agg_contents_list); diff --git a/gcc/testsuite/gcc.dg/pr109303.c b/gcc/testsuite/gcc.dg/pr109303.c new file mode 100644 index 00000000000..f91535991c7 --- /dev/null +++ b/gcc/testsuite/gcc.dg/pr109303.c @@ -0,0 +1,24 @@ +/* PR ipa/109303 */ +/* { dg-do compile { target lp64 } } */ +/* { dg-options "-O2" } */ + +struct __attribute__((packed)) A { char c1; short a1[__INT_MAX__]; }; +struct __attribute__((packed)) B { char c2; short a2[100]; }; +struct S { struct A p1; struct B p2[4]; }; +void bar (short int); + +static void +foo (struct S *q) +{ + for (int i = 0; i < q->p1.c1; i++) + for (int j = 0; j < q->p2[i].c2; j++) + bar (q->p2[i].a2[j]); +} + +int +main () +{ + struct S q = {}; + q.p2[0].c2 = q.p2[1].c2 = 3; + foo (&q); +} -- 2.40.0 ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] ipa: Avoid constructing aggregate jump functions with huge offsets (PR 109303) 2023-03-31 8:45 [PATCH] ipa: Avoid constructing aggregate jump functions with huge offsets (PR 109303) Martin Jambor @ 2023-03-31 8:57 ` Richard Biener 2023-03-31 9:18 ` Martin Jambor 0 siblings, 1 reply; 4+ messages in thread From: Richard Biener @ 2023-03-31 8:57 UTC (permalink / raw) To: Martin Jambor; +Cc: GCC Patches, Jan Hubicka On Fri, Mar 31, 2023 at 10:46 AM Martin Jambor <mjambor@suse.cz> wrote: > > Hi, > > we are in the process of changing data structures holding information > about constants passed by reference and in aggregates to use unsigned > int offsets rather than HOST_WIDE_INT (which was selected simply > because that is what fell out of get_ref_base_and_extent at that time) > in order to conserve memory, especially at WPA time. > > PR 109303 testcase discovers that we do not properly check that we > only create jump functions with offsets (plus sizes) that fit into the > smaller type. This patch adds the necessary check. > > Bootstrapped and tested on x86_64-linux. OK for master? > > Thanks, > > Martin > > > gcc/ChangeLog: > > 2023-03-30 Martin Jambor <mjambor@suse.cz> > > PR ipa/109303 > * ipa-prop.cc (determine_known_aggregate_parts): Check that the > offset + size will be representable in unsigned int. > > gcc/testsuite/ChangeLog: > > 2023-03-30 Jakub Jelinek <jakub@redhat.com> > Martin Jambor <mjambor@suse.cz> > > PR ipa/109303 > * gcc.dg/pr109303.c: New test. > --- > gcc/ipa-prop.cc | 4 +++- > gcc/testsuite/gcc.dg/pr109303.c | 24 ++++++++++++++++++++++++ > 2 files changed, 27 insertions(+), 1 deletion(-) > create mode 100644 gcc/testsuite/gcc.dg/pr109303.c > > diff --git a/gcc/ipa-prop.cc b/gcc/ipa-prop.cc > index de45dbccf16..9ffd49b590c 100644 > --- a/gcc/ipa-prop.cc > +++ b/gcc/ipa-prop.cc > @@ -2086,7 +2086,9 @@ determine_known_aggregate_parts (struct ipa_func_body_info *fbi, > whether its value is clobbered any other dominating one. */ > if ((content->value.pass_through.formal_id >= 0 > || content->value.pass_through.operand) > - && !clobber_by_agg_contents_list_p (all_list, content)) > + && !clobber_by_agg_contents_list_p (all_list, content) > + && (content->offset + content->size - arg_offset > + <= (HOST_WIDE_INT) UINT_MAX * BITS_PER_UNIT)) > { it does seem a bit misplaced since after the if we add the same 'content' to another list anyway. Wouldn't a more obvious place be where we end up truncating this sum? > struct ipa_known_agg_contents_list *copy > = XALLOCA (struct ipa_known_agg_contents_list); > diff --git a/gcc/testsuite/gcc.dg/pr109303.c b/gcc/testsuite/gcc.dg/pr109303.c > new file mode 100644 > index 00000000000..f91535991c7 > --- /dev/null > +++ b/gcc/testsuite/gcc.dg/pr109303.c > @@ -0,0 +1,24 @@ > +/* PR ipa/109303 */ > +/* { dg-do compile { target lp64 } } */ > +/* { dg-options "-O2" } */ > + > +struct __attribute__((packed)) A { char c1; short a1[__INT_MAX__]; }; > +struct __attribute__((packed)) B { char c2; short a2[100]; }; > +struct S { struct A p1; struct B p2[4]; }; > +void bar (short int); > + > +static void > +foo (struct S *q) > +{ > + for (int i = 0; i < q->p1.c1; i++) > + for (int j = 0; j < q->p2[i].c2; j++) > + bar (q->p2[i].a2[j]); > +} > + > +int > +main () > +{ > + struct S q = {}; > + q.p2[0].c2 = q.p2[1].c2 = 3; > + foo (&q); > +} > -- > 2.40.0 > ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] ipa: Avoid constructing aggregate jump functions with huge offsets (PR 109303) 2023-03-31 8:57 ` Richard Biener @ 2023-03-31 9:18 ` Martin Jambor 2023-03-31 9:24 ` Richard Biener 0 siblings, 1 reply; 4+ messages in thread From: Martin Jambor @ 2023-03-31 9:18 UTC (permalink / raw) To: Richard Biener; +Cc: GCC Patches, Jan Hubicka Hi, On Fri, Mar 31 2023, Richard Biener wrote: > On Fri, Mar 31, 2023 at 10:46 AM Martin Jambor <mjambor@suse.cz> wrote: >> >> Hi, >> >> we are in the process of changing data structures holding information >> about constants passed by reference and in aggregates to use unsigned >> int offsets rather than HOST_WIDE_INT (which was selected simply >> because that is what fell out of get_ref_base_and_extent at that time) >> in order to conserve memory, especially at WPA time. >> >> PR 109303 testcase discovers that we do not properly check that we >> only create jump functions with offsets (plus sizes) that fit into the >> smaller type. This patch adds the necessary check. >> >> Bootstrapped and tested on x86_64-linux. OK for master? >> >> Thanks, >> >> Martin >> >> >> gcc/ChangeLog: >> >> 2023-03-30 Martin Jambor <mjambor@suse.cz> >> >> PR ipa/109303 >> * ipa-prop.cc (determine_known_aggregate_parts): Check that the >> offset + size will be representable in unsigned int. >> >> gcc/testsuite/ChangeLog: >> >> 2023-03-30 Jakub Jelinek <jakub@redhat.com> >> Martin Jambor <mjambor@suse.cz> >> >> PR ipa/109303 >> * gcc.dg/pr109303.c: New test. >> --- >> gcc/ipa-prop.cc | 4 +++- >> gcc/testsuite/gcc.dg/pr109303.c | 24 ++++++++++++++++++++++++ >> 2 files changed, 27 insertions(+), 1 deletion(-) >> create mode 100644 gcc/testsuite/gcc.dg/pr109303.c >> >> diff --git a/gcc/ipa-prop.cc b/gcc/ipa-prop.cc >> index de45dbccf16..9ffd49b590c 100644 >> --- a/gcc/ipa-prop.cc >> +++ b/gcc/ipa-prop.cc >> @@ -2086,7 +2086,9 @@ determine_known_aggregate_parts (struct ipa_func_body_info *fbi, >> whether its value is clobbered any other dominating one. */ >> if ((content->value.pass_through.formal_id >= 0 >> || content->value.pass_through.operand) >> - && !clobber_by_agg_contents_list_p (all_list, content)) >> + && !clobber_by_agg_contents_list_p (all_list, content) >> + && (content->offset + content->size - arg_offset >> + <= (HOST_WIDE_INT) UINT_MAX * BITS_PER_UNIT)) >> { > > it does seem a bit misplaced since after the if we add the same > 'content' to another > list anyway. The other list is a clobber list, as we crawl backwards from the call statement searching for stores, we also look whether we have already encountered a store of something else to an overlapping area. In theory we could have a store to a smaller data type, where the offset + size would still fit unsigned int, be followed by a larger store, which would not. We want the large store to end up in the clobber list so that the smaller one does not. This is the place where we also calculate the size of the final heap-allocated vector, so that is why eventually I put it there. > Wouldn't a more obvious place be where we end up truncating this sum? My reasoning was that since we know we would not be able to use it, it makes sense to discard the data before we stream it from compilation to WPA. Also, when we shorten the offset type also in ipa_agg_jf_item (which is what I want to do next), this is where the check eventually needs to be. Thanks, Martin ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] ipa: Avoid constructing aggregate jump functions with huge offsets (PR 109303) 2023-03-31 9:18 ` Martin Jambor @ 2023-03-31 9:24 ` Richard Biener 0 siblings, 0 replies; 4+ messages in thread From: Richard Biener @ 2023-03-31 9:24 UTC (permalink / raw) To: Martin Jambor; +Cc: GCC Patches, Jan Hubicka On Fri, Mar 31, 2023 at 11:18 AM Martin Jambor <mjambor@suse.cz> wrote: > > Hi, > > On Fri, Mar 31 2023, Richard Biener wrote: > > On Fri, Mar 31, 2023 at 10:46 AM Martin Jambor <mjambor@suse.cz> wrote: > >> > >> Hi, > >> > >> we are in the process of changing data structures holding information > >> about constants passed by reference and in aggregates to use unsigned > >> int offsets rather than HOST_WIDE_INT (which was selected simply > >> because that is what fell out of get_ref_base_and_extent at that time) > >> in order to conserve memory, especially at WPA time. > >> > >> PR 109303 testcase discovers that we do not properly check that we > >> only create jump functions with offsets (plus sizes) that fit into the > >> smaller type. This patch adds the necessary check. > >> > >> Bootstrapped and tested on x86_64-linux. OK for master? > >> > >> Thanks, > >> > >> Martin > >> > >> > >> gcc/ChangeLog: > >> > >> 2023-03-30 Martin Jambor <mjambor@suse.cz> > >> > >> PR ipa/109303 > >> * ipa-prop.cc (determine_known_aggregate_parts): Check that the > >> offset + size will be representable in unsigned int. > >> > >> gcc/testsuite/ChangeLog: > >> > >> 2023-03-30 Jakub Jelinek <jakub@redhat.com> > >> Martin Jambor <mjambor@suse.cz> > >> > >> PR ipa/109303 > >> * gcc.dg/pr109303.c: New test. > >> --- > >> gcc/ipa-prop.cc | 4 +++- > >> gcc/testsuite/gcc.dg/pr109303.c | 24 ++++++++++++++++++++++++ > >> 2 files changed, 27 insertions(+), 1 deletion(-) > >> create mode 100644 gcc/testsuite/gcc.dg/pr109303.c > >> > >> diff --git a/gcc/ipa-prop.cc b/gcc/ipa-prop.cc > >> index de45dbccf16..9ffd49b590c 100644 > >> --- a/gcc/ipa-prop.cc > >> +++ b/gcc/ipa-prop.cc > >> @@ -2086,7 +2086,9 @@ determine_known_aggregate_parts (struct ipa_func_body_info *fbi, > >> whether its value is clobbered any other dominating one. */ > >> if ((content->value.pass_through.formal_id >= 0 > >> || content->value.pass_through.operand) > >> - && !clobber_by_agg_contents_list_p (all_list, content)) > >> + && !clobber_by_agg_contents_list_p (all_list, content) > >> + && (content->offset + content->size - arg_offset > >> + <= (HOST_WIDE_INT) UINT_MAX * BITS_PER_UNIT)) > >> { > > > > it does seem a bit misplaced since after the if we add the same > > 'content' to another > > list anyway. > > The other list is a clobber list, as we crawl backwards from the call > statement searching for stores, we also look whether we have already > encountered a store of something else to an overlapping area. In theory > we could have a store to a smaller data type, where the offset + size > would still fit unsigned int, be followed by a larger store, which would > not. We want the large store to end up in the clobber list so that the > smaller one does not. > > This is the place where we also calculate the size of the final > heap-allocated vector, so that is why eventually I put it there. > > > Wouldn't a more obvious place be where we end up truncating this sum? > > My reasoning was that since we know we would not be able to use it, it > makes sense to discard the data before we stream it from compilation to > WPA. > > Also, when we shorten the offset type also in ipa_agg_jf_item (which is > what I want to do next), this is where the check eventually needs to > be. OK, maybe add the above as comment then. OK with that change. Richard. > Thanks, > > Martin ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2023-03-31 9:24 UTC | newest] Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2023-03-31 8:45 [PATCH] ipa: Avoid constructing aggregate jump functions with huge offsets (PR 109303) Martin Jambor 2023-03-31 8:57 ` Richard Biener 2023-03-31 9:18 ` Martin Jambor 2023-03-31 9:24 ` Richard Biener
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).