Re: [PATCH] ipa: Avoid constructing aggregate jump functions with huge offsets (PR 109303)

[Martin Jambor <mjambor@suse.cz>]

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