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

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

[Martin Sebor]

[-- Attachment #1: Type: text/plain, Size: 440 bytes --]

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

[-- Attachment #2: gcc-99489.diff --]
[-- Type: text/x-patch, Size: 1919 bytes --]

PR tree-optimization/99489 - ICE calling strncat after strcat

gcc/ChangeLog:

	PR tree-optimization/99489
	* builtins.c (gimple_call_alloc_size): Fail gracefully when argument
	is not a call statement.

gcc/testsuite/ChangeLog:

	PR tree-optimization/99489
	* gcc.dg/Wstringop-truncation-9.c: New test.

diff --git a/gcc/builtins.c b/gcc/builtins.c
index 41e336c071c..196dda3fa5e 100644
--- a/gcc/builtins.c
+++ b/gcc/builtins.c
@@ -4924,7 +4924,7 @@ tree
 gimple_call_alloc_size (gimple *stmt, wide_int rng1[2] /* = NULL */,
 			range_query * /* = NULL */)
 {
-  if (!stmt)
+  if (!stmt || !is_gimple_call (stmt))
     return NULL_TREE;
 
   tree allocfntype;
diff --git a/gcc/testsuite/gcc.dg/Wstringop-truncation-9.c b/gcc/testsuite/gcc.dg/Wstringop-truncation-9.c
new file mode 100644
index 00000000000..63614809da4
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/Wstringop-truncation-9.c
@@ -0,0 +1,41 @@
+/* PR tree-optimization/99489 - ICE calling strncat after strncat
+   { dg-do compile }
+   { dg-options "-O2 -Wall" } */
+
+// Important -- see pr82429.
+char *stpcpy (char *, const char *);
+
+void fchar (char *d, char c, char *s)
+{
+  __builtin_strcat (d, s);
+  __builtin_strncat (d, &c, 1);
+}
+
+void fcstchar (char *d, char *s)
+{
+  __builtin_strcat (d, s);
+
+  const char c = 'x';
+  __builtin_strncat (d, &c, 1);     // { dg-warning "-Wstringop-truncation" }
+}
+
+void fstr (char *d, char *s)
+{
+  __builtin_strcat (d, s);
+  __builtin_strncat (d, s, 1);
+}
+
+void farr (char *d, char *s)
+{
+  __builtin_strcat (d, s);
+
+  char a[] = "x";
+  __builtin_strncat (d, a, 1);      // { dg-warning "-Wstringop-truncation" }
+}
+
+void flit (char *d, char *s)
+{
+  __builtin_strcat (d, s);
+  __builtin_strncat (d, "x", 1);    // { dg-warning "-Wstringop-truncation" "pr?????" { xfail *-*-*} }
+                                    // { dg-warning "-Wstringop-overflow" "actual" { target *-*-*} .-1 }
+}

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

[Jakub Jelinek]

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

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

[David Malcolm]

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

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

[Jakub Jelinek]

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

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

[Martin Sebor]

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