diff mbox series

ipa: Avoid constructing aggregate jump functions with huge offsets (PR 109303)

Message ID ri6pm8p48cj.fsf@suse.cz
State New
Headers show
Series ipa: Avoid constructing aggregate jump functions with huge offsets (PR 109303) | expand

Commit Message

Martin Jambor March 31, 2023, 8:45 a.m. UTC 
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

Comments

Richard Biener March 31, 2023, 8:57 a.m. UTC | #1 
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
>
Martin Jambor March 31, 2023, 9:18 a.m. UTC | #2 
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
Richard Biener March 31, 2023, 9:24 a.m. UTC | #3 
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
diff mbox series

Patch

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);
+}