avoid assuming gimple_call_alloc_size argument is a call (PR 99489)

The gimple_call_alloc_size() function is documented to "return null
when STMT is not a call to a valid allocation function" but the code
assumes STMT is a call statement, causing the function to ICE when
it isn't.

The attached patch changes the function to fulfill its contract and
return null also when STMT isn't a call.  The fix seems obvious to
me but I'll wait some time before committing it in case it's not
to someone else.

Martin

On Tue, Mar 09, 2021 at 03:07:38PM -0700, Martin Sebor via Gcc-patches wrote:
> The gimple_call_alloc_size() function is documented to "return null
> when STMT is not a call to a valid allocation function" but the code
> assumes STMT is a call statement, causing the function to ICE when
> it isn't.
> 
> The attached patch changes the function to fulfill its contract and
> return null also when STMT isn't a call.  The fix seems obvious to
> me but I'll wait some time before committing it in case it's not
> to someone else.

I think the name of the function suggests that it should be called on calls,
not random stmts.  Currently the function has 3 callers, two of them
already verify is_gimple_call before calling it and only one doesn't,
and the stmt will never be NULL.
So I'd say it would be better to remove the if (!stmt) return NULL_TREE;
from the start of the function and add is_gimple_call (stmt) &&
in tree-ssa-strlen.c.

	Jakub

On Fri, 2021-03-12 at 14:52 +0100, Jakub Jelinek via Gcc-patches wrote:
> On Tue, Mar 09, 2021 at 03:07:38PM -0700, Martin Sebor via Gcc-
> patches wrote:
> > The gimple_call_alloc_size() function is documented to "return null
> > when STMT is not a call to a valid allocation function" but the
> > code
> > assumes STMT is a call statement, causing the function to ICE when
> > it isn't.
> > 
> > The attached patch changes the function to fulfill its contract and
> > return null also when STMT isn't a call.  The fix seems obvious to
> > me but I'll wait some time before committing it in case it's not
> > to someone else.
> 
> I think the name of the function suggests that it should be called on
> calls,
> not random stmts.  Currently the function has 3 callers, two of them
> already verify is_gimple_call before calling it and only one doesn't,
> and the stmt will never be NULL.
> So I'd say it would be better to remove the if (!stmt) return
> NULL_TREE;
> from the start of the function and add is_gimple_call (stmt) &&
> in tree-ssa-strlen.c.

Maybe even make it convert it to taking a "const gcall *", so those

  if (is_gimple_call (stmt))
    {
       ...
       if (gimple_call_alloc_size (stmt, ...))
         {
         }
    }

become:

  if (const gcall *call = dyn_cast <const gcall *> (stmt))
    {
       ...
       if (gimple_call_alloc_size (call, ...))
         {
         }

    }

so that the compiler can enforce this requirement via the type system?

Hope this is constructive
Dave

On Fri, Mar 12, 2021 at 09:04:33AM -0500, David Malcolm wrote:
> > from the start of the function and add is_gimple_call (stmt) &&
> > in tree-ssa-strlen.c.
> 
> Maybe even make it convert it to taking a "const gcall *", so those
> 
>   if (is_gimple_call (stmt))
>     {
>        ...
>        if (gimple_call_alloc_size (stmt, ...))
>          {
>          }
>     }
> 
> become:
> 
>   if (const gcall *call = dyn_cast <const gcall *> (stmt))
>     {
>        ...
>        if (gimple_call_alloc_size (call, ...))
>          {
>          }
> 
>     }

I'm not a big fan of that, to me it means too much typing/clutter
and think that runtime checking we have is sufficient, but could live with
that.

	Jakub

On 3/12/21 6:52 AM, Jakub Jelinek wrote:
> On Tue, Mar 09, 2021 at 03:07:38PM -0700, Martin Sebor via Gcc-patches wrote:
>> The gimple_call_alloc_size() function is documented to "return null
>> when STMT is not a call to a valid allocation function" but the code
>> assumes STMT is a call statement, causing the function to ICE when
>> it isn't.
>>
>> The attached patch changes the function to fulfill its contract and
>> return null also when STMT isn't a call.  The fix seems obvious to
>> me but I'll wait some time before committing it in case it's not
>> to someone else.
> 
> I think the name of the function suggests that it should be called on calls,
> not random stmts.

I wrote the function so I should know how I expected it to be used.
I can't say I remember for sure but I imagine I would have declared
the argument gcall* rather than gimple* if I had intended it to be
called with only gcall statements.

> Currently the function has 3 callers, two of them
> already verify is_gimple_call before calling it and only one doesn't,
> and the stmt will never be NULL.
> So I'd say it would be better to remove the if (!stmt) return NULL_TREE;
> from the start of the function and add is_gimple_call (stmt) &&
> in tree-ssa-strlen.c.

My preference is to make code more robust, not less, so that if another
caller is introduced that doesn't check the argument it doesn't cause
another ICE.

An alternative might be to change the function to take a gcall* as
some of the gimple_call_xxx() APIs do that expect to be called only
with  GIMPLE call statements, like gimple_call_fn(), but that would
force the caller to both do the checking and the conversion from
gimple* to gcall*.  That also seems less preferable to me.

A better variant of the above that would be in line with the GIMPLE
API design is to also introduce a gimple* overload/wrapper around
gcall* form of the function and convert its gimple* argument to
gcall* via a GIMPLE_CHECK<gcall*>()) cast.  I'd like to ultimately
move the function into gimple.{h,c} so that might be something to
consider then.  But making such a change now would introduce more
churn than is necessary to fix the regression.

I've committed the patch as is for now and will plan to revisit
the overload idea in stage 1.

Martin