sync up new type indices for body adjustments

The logic in fill_vector_of_new_param_types may skip some parameters
when pushing into m_new_types, but common_initialization doesn't take
that into account, and may end up attempting to access the vector past
its end when IPA_PARAM_OP_(NEW|SPLIT) operands appear after skipped
_COPY ones.

This patch adjusts the consumer logic to match the indexing in the
producer.  It came up in libstdc++-v3's testsuite, in
std/ranges/adaptors/filter.cc, but only with wrappers introduced by a pass
I'm working on.  The _NEW parameters were reference-typed replacements
for some by-value ones in my function-wrapping logic, and other IPA
transformations cause plenty of unused/irrelevant arguments to be
dropped for certain calls.

Regstrapped on x86_64-linux-gnu.  Ok to install?


for  gcc/ChangeLog

	* ipa-param-manipulation.c
	(ipa_param_body_adjustments::common_initialization): Adjust
	m_new_types indexing to match producer.
---
 gcc/ipa-param-manipulation.c |   10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

Hi,

On Tue, Jul 13 2021, Alexandre Oliva wrote:
> The logic in fill_vector_of_new_param_types may skip some parameters
> when pushing into m_new_types, but common_initialization doesn't take
> that into account, and may end up attempting to access the vector past
> its end when IPA_PARAM_OP_(NEW|SPLIT) operands appear after skipped
> _COPY ones.
>
> This patch adjusts the consumer logic to match the indexing in the
> producer.  It came up in libstdc++-v3's testsuite, in
> std/ranges/adaptors/filter.cc, but only with wrappers introduced by a pass
> I'm working on.  The _NEW parameters were reference-typed replacements
> for some by-value ones in my function-wrapping logic, and other IPA
> transformations cause plenty of unused/irrelevant arguments to be
> dropped for certain calls.
>
> Regstrapped on x86_64-linux-gnu.  Ok to install?

I agree that there is a mismatch but I do not like proposed change.
Indeed fill_vector_of_new_param_types can skip some parameters when they
appear to be copies of something that is not there - but that bail-out
is not supposed to ever happen when fill_vector_of_new_param_types is
called from ipa_param_body_adjustments.

It is there for cases where one compilation unit has a completely bogus
idea of a type defined in another compilation unit and so creates a bad
type for gimple_call_fntype, but then LTO decides to do something with
the parameters and modifying the gimple_call_fntype becomes a
garbage-in-garbage-out operation (but avoids ICE).

That should never happen when you actually have a body and so presumably
the type and parameters match.  So I would first check how come that you
request IPA_PARAM_OP_COPY of something that does not seem to have a
corresponding type but there is a DECL, otherwise the code would have
ICEd when attempting to "carry over" that.

If you believe that what you're doing is correct (but check twice), I
would prefer the following change (only mildly tested) that is a
clean-up and should help you with your problem too.

Thanks,

Martin


diff --git a/gcc/ipa-param-manipulation.c b/gcc/ipa-param-manipulation.c
index 26b02d7aa95..c5285b7cdf3 100644
--- a/gcc/ipa-param-manipulation.c
+++ b/gcc/ipa-param-manipulation.c
@@ -1067,14 +1067,15 @@ ipa_param_body_adjustments::common_initialization (tree old_fndecl,
   auto_vec<tree,16> otypes;
   if (TYPE_ARG_TYPES (TREE_TYPE (old_fndecl)) != NULL_TREE)
     push_function_arg_types (&otypes, TREE_TYPE (old_fndecl));
-  else
+  if (m_oparms.length () != otypes.length ())
     {
-      auto_vec<tree,16> oparms;
-      push_function_arg_decls (&oparms, old_fndecl);
-      unsigned ocount = oparms.length ();
+      /* Parameter type information is not available or there is a mismatch
+        between it and the real parameters (probably K&R code or weird LTO
+        mismatched decls).  */
+      unsigned ocount = m_oparms.length ();
       otypes.reserve_exact (ocount);
       for (unsigned i = 0; i < ocount; i++)
-       otypes.quick_push (TREE_TYPE (oparms[i]));
+       otypes.quick_push (TREE_TYPE (m_oparms[i]));
     }
   fill_vector_of_new_param_types (&m_new_types, &otypes, m_adj_params, true);

On Jul 19, 2021, Martin Jambor <mjambor@suse.cz> wrote:

> So I would first check how come that you request IPA_PARAM_OP_COPY of
> something that does not seem to have a corresponding type but there is
> a DECL

The corresponding type is there all right, it was just stored in a
different vector entry, because some IPA optimization, applied after my
copying-and-wrapping pass, dropped several of the parms that came before
a NEW parms added by my pass.

This caused the types of the retained NEW parms to be pushed into lower
indices in the type array, but then accessed as if all of the dropped
parms were still there.  That can't be right.

I was actually lucky that enough parms were dropped as to make the
vector access out of range, flagged by checking.  If that wasn't the
case, we might have silently accessed an unrelated parm type.


Does this scenario make sense to you?

I can try to get you some code for a custom pass to trigger the problem
if you'd like to look more closely.

> If you believe that what you're doing is correct

I don't really know that it is.  IIRC back when I ran into this problem,
the logic to change some of the parameters in the wrapped function to
reference types was using NEW parameters.  Now I'm using COPY, save for
actual NEW parms, and changing the type of the clone after
create_version_clone_with_body.

Now, what puzzles me is why we even care about that parm mapping
afterwards.  The clone is created and materialized very early on, before
any preexisting ipa transformations, and there were not any edges
modified to use this clone.  As far as I'm concerned, it should be
entirely independent from the function it was cloned from, and it makes
no sense to me for IPA transformations applied to this clone to even
care what the function it was originally cloned from was: the clone is
already fully materialized, so argument back-mappings might as well stop
at it.

But I can't say I understand why it does that.  I haven't looked very
much into its internals, I'm mostly just trying to use
create_version_clone_with_body to clone a function, make some changes to
it, and turn the original function into a wrapper.

I'm not actually introducing IPA deferred transformations, and this is
all done before any relevant IPA transformations.  I can't even say I'm
using IPA proper, the reason I made it an IPA pass was because that has
enabled multiple passes over functions, which was convenient for some
purposes.  Then, I ended up iterating over aliases and undefined
functions, and relying on the call graph instead of iterating over
gimple bodies for some purposes, so now it *has* to be an IPA pass, but
not a typical one in that it doesn't queue up IPA transformations to be
applied at a later materialization.

Hi,

On Wed, Jul 21 2021, Alexandre Oliva wrote:
> On Jul 19, 2021, Martin Jambor <mjambor@suse.cz> wrote:
>
>> So I would first check how come that you request IPA_PARAM_OP_COPY of
>> something that does not seem to have a corresponding type but there is
>> a DECL
>
> The corresponding type is there all right, it was just stored in a
> different vector entry, because some IPA optimization, applied after my
> copying-and-wrapping pass, dropped several of the parms that came before
> a NEW parms added by my pass.
>
> This caused the types of the retained NEW parms to be pushed into lower
> indices in the type array, but then accessed as if all of the dropped
> parms were still there.  That can't be right.
>
> I was actually lucky that enough parms were dropped as to make the
> vector access out of range, flagged by checking.  If that wasn't the
> case, we might have silently accessed an unrelated parm type.
>
>
> Does this scenario make sense to you?
>
> I can try to get you some code for a custom pass to trigger the problem
> if you'd like to look more closely.
>
>> If you believe that what you're doing is correct
>
> I don't really know that it is.  IIRC back when I ran into this problem,
> the logic to change some of the parameters in the wrapped function to
> reference types was using NEW parameters.  Now I'm using COPY, save for
> actual NEW parms, and changing the type of the clone after
> create_version_clone_with_body.
>
> Now, what puzzles me is why we even care about that parm mapping
> afterwards.  The clone is created and materialized very early on, before
> any preexisting ipa transformations, and there were not any edges
> modified to use this clone.  As far as I'm concerned, it should be
> entirely independent from the function it was cloned from, and it makes
> no sense to me for IPA transformations applied to this clone to even
> care what the function it was originally cloned from was: the clone is
> already fully materialized, so argument back-mappings might as well stop
> at it.
>
> But I can't say I understand why it does that.  I haven't looked very
> much into its internals, I'm mostly just trying to use
> create_version_clone_with_body to clone a function, make some changes to
> it, and turn the original function into a wrapper.
>
> I'm not actually introducing IPA deferred transformations, and this is
> all done before any relevant IPA transformations.  I can't even say I'm
> using IPA proper, the reason I made it an IPA pass was because that has
> enabled multiple passes over functions, which was convenient for some
> purposes.  Then, I ended up iterating over aliases and undefined
> functions, and relying on the call graph instead of iterating over
> gimple bodies for some purposes, so now it *has* to be an IPA pass, but
> not a typical one in that it doesn't queue up IPA transformations to be
> applied at a later materialization.

So if I understand correctly, you clone during early tree optimizations
(or early-small-IPA passes) or even earlier, and yet somehow these
confuse clone materialization when it applies IPA modifications to
parameters.  I agree that should not be happening.

I cannot see how this can happen.  IPA-split and omp-simd also use
create_version_clone_with_body with parameter modifications and do not
cause this problem (and I have seen many interactions between ipa-split
and later IPA passes when debugging various issues).  Having said that,
these passes either act on fairly simple functions and/or do not do
sophisticated parameter modifications, so I would not be bugs when doing
them.

I am interested in making the infrastructure work for you, but at the
moment I unfortunately do not have an idea what the problem you are
facing might be.

Martin