Re: [PATCH v2] rs6000: Stackoverflow in optimized code on PPC [PR100799]

[Jakub Jelinek <jakub@redhat.com>]

On Wed, Apr 03, 2024 at 05:02:40PM +0800, Kewen.Lin wrote:
> on 2024/4/3 16:35, Jakub Jelinek wrote:
> > On Wed, Apr 03, 2024 at 01:18:54PM +0800, Kewen.Lin wrote:
> >>> I'd prefer not to remove DECL_ARGUMENTS chains, they are valid arguments that just some
> >>> invalid code doesn't pass.  By removing them you basically always create an
> >>> invalid case, this time in the other direction, valid caller passes more
> >>> arguments than the callee (invalidly) expects.
> >>
> >> Thanks for the comments, do you mean it can affect the arguments validation when there
> >> is explicit function declaration with interface?  Then can we strip them when we are
> >> going to expand them (like checking currently_expanding_function_start)?
> > 
> > I'd prefer not stripping them at all; they are clearly marked as perhaps not
> > passed in buggy programs (the DECL_HIDDEN_STRING_LENGTH argument) and
> > removing them implies the decl is a throw away, that after expansion
> 
> Yes, IMHO it's safe as they are unused.

But they are still passed in the usual case.

> > nothing will actually look at it anymore.  I believe that is the case of
> > function bodies, we expand them into RTL and throw away the GIMPLE, and
> > after final clear the bodies, but it is not the case of the FUNCTION_DECL
> > or its DECL_ARGUMENTs etc.  E.g. GIMPLE optimizations or expansion of
> > callers could be looking at those as well.
> 
> At expand time GIMPLE optimizations should already finish, so it should be
> safe to strip them at that time?

No.
The IPA/post IPA behavior is that IPA optimizations are performed and then
cgraph finalizes one function at a time, going there from modifications
needed from IPA passes, post IPA GIMPLE optimizations, expansion to RTL,
RTL optimizations, emitting assembly, throwing away the body, then picking
another function and repeating that etc.
So, when one function makes it to expansion, if you modify its
DECL_ARGUMENTS etc., all the post IPA GIMPLE optimization passes of other
functions might still see such changes.

>  It would surprise me if expansions of
> callers will look at callee's information, it's more like what should be
> done in IPA analysis instead?

Depends on what exactly it is.  E.g. C non-prototyped functions have
just DECL_ARGUMENTS to check how many arguments the call should have vs.
what is actually passed.

> No, it's not what I was looking for.  Peter's comments made me feel it's not
> good to have assembly at O0 like:
> 
>         std %r3,112(%r31)
>         std %r4,120(%r31)
>         std %r5,128(%r31)
>         std %r6,136(%r31)
>         std %r7,144(%r31)
>         std %r8,152(%r31)
>         std %r9,160(%r31)
>         std %r10,168(%r31)
>         std %r11,176(%r31) // this mislead people that we pass 9th arg via r11,
>                            // it would be nice not to have it.
> 
> so I was thinking if there is some way to get rid of it.

You want to optimize at -O0?  Don't.
That will screw up debugging.  The function does have that argument, it
should show up in debug info; it should show up also at -O2 in debug info
etc.  If you remove chains from DECL_ARGUMENTS, because we have early dwarf
these days, DW_TAG_formal_parameter nodes should have been already created,
but it would mean that DW_AT_location for those arguments likely isn't
filled.  Now, for -O2 it might be the case that the argument has useful
location only at the start of the function, could have
DW_OP_entry_value(%r11) afterwards, but at -O0 it better should have some
stack slot into which the argument is saved and DW_AT_location should be
that stack slot.  All that should change with the workaround is that if the
stack slot would be normally in the argument save area in the caller's
frame, if such argument save area can't be counted on, then it needs to be
saved in some other stack slot, like arguments are saved to when there are
only <= 8 arguments.

Now, sure, if IPA optimizes a call because it can prove it sees a callee and
all its callers and can modify them all, it can optimize away passing of
that argument but 1) it isn't done at -O0/-Og 2) it ensures debug info
reflects the case that the argument isn't passed and arranges for the
callers to provide DW_OP_GNU_parameter_ref/call site info value 3) it
doesn't modify the FUNCTION_DECL itself or its DECL_ARGUMENTS, but makes
a clone of the FUNCTION_DECL and modifies the clones DECL_ARGUMENTS

	Jakub