Re: [PATCH] avoid assuming gimple_call_alloc_size argument is a call (PR 99489)

[Martin Sebor <msebor@gmail.com>]

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