[PR,87347] Prevent segfaults if TYPE_ARG_TYPES is NULL

Hi,

the warning for suspicious calls of abs-like functions segfaults if a
user declared their own parameter-less-ish variant of abs like in the
testcase below.  Fixed by looking whether there is any TYPE_ARG_TYPES
before trying to compare the actual argument with it.

Bootstrapped and tested on x86_64-linux and aarch64-linux, the same on
i686-linux is pending.

OK for trunk?

Thanks,

Martin


2018-09-24  Martin Jambor  <mjambor@suse.cz>

	PR c/87347
	c/
	* c-parser.c (warn_for_abs): Bail out if TYPE_ARG_TYPES is NULL.

	testsuite/
	* gcc.dg/pr87347.c: New test.
---
 gcc/c/c-parser.c               | 7 ++++---
 gcc/testsuite/gcc.dg/pr87347.c | 6 ++++++
 2 files changed, 10 insertions(+), 3 deletions(-)
 create mode 100644 gcc/testsuite/gcc.dg/pr87347.c

On Mon, Sep 24, 2018 at 08:40:13PM +0200, Martin Jambor wrote:
> Hi,
> 
> the warning for suspicious calls of abs-like functions segfaults if a
> user declared their own parameter-less-ish variant of abs like in the
> testcase below.  Fixed by looking whether there is any TYPE_ARG_TYPES
> before trying to compare the actual argument with it.
> 
> Bootstrapped and tested on x86_64-linux and aarch64-linux, the same on
> i686-linux is pending.
> 
> OK for trunk?

Isn't that bail out too early?
I mean most of the warnings that are emitted by the function don't really
need TYPE_ARG_TYPES, only the last one does, so can't you just bail out
before the last warning?

Also, the function comment has "gracely", did you mean "gracefully"?

	Jakub

Hi,

On Tue, Sep 25 2018, Jakub Jelinek wrote:
> On Mon, Sep 24, 2018 at 08:40:13PM +0200, Martin Jambor wrote:
>> Hi,
>> 
>> the warning for suspicious calls of abs-like functions segfaults if a
>> user declared their own parameter-less-ish variant of abs like in the
>> testcase below.  Fixed by looking whether there is any TYPE_ARG_TYPES
>> before trying to compare the actual argument with it.
>> 
>> Bootstrapped and tested on x86_64-linux and aarch64-linux, the same on
>> i686-linux is pending.
>> 
>> OK for trunk?
>
> Isn't that bail out too early?
> I mean most of the warnings that are emitted by the function don't really
> need TYPE_ARG_TYPES, only the last one does, so can't you just bail out
> before the last warning?

my reasoning was that if the function is not what I expect it to be, it
is better not to touch it.  On the other hand, I have no problems moving
this test lower as done in the patch below.

> Also, the function comment has "gracely", did you mean "gracefully"?

Of course I did, thanks for spotting that.

Bootstrapped and tested on x86_64-linux and aarch64-linux.  OK for
trunk?

Thanks,

Martin


2018-09-25  Martin Jambor  <mjambor@suse.cz>

	PR c/87347
	c/
	* c-parser.c (warn_for_abs): Bail out if TYPE_ARG_TYPES is NULL.  Fix
        the function comment.

	testsuite/
	* gcc.dg/pr87347.c: New test.
---
 gcc/c/c-parser.c               | 7 +++++--
 gcc/testsuite/gcc.dg/pr87347.c | 6 ++++++
 2 files changed, 11 insertions(+), 2 deletions(-)
 create mode 100644 gcc/testsuite/gcc.dg/pr87347.c

diff --git a/gcc/c/c-parser.c b/gcc/c/c-parser.c
index 1766a256633..1f173fc10e2 100644
--- a/gcc/c/c-parser.c
+++ b/gcc/c/c-parser.c
@@ -9102,8 +9102,8 @@ sizeof_ptr_memacc_comptypes (tree type1, tree type2)
 }
 
 /* Warn for patterns where abs-like function appears to be used incorrectly,
-   gracely ignore any non-abs-like function.  The warning location should be
-   LOC.  FNDECL is the declaration of called function, it must be a
+   gracefully ignore any non-abs-like function.  The warning location should
+   be LOC.  FNDECL is the declaration of called function, it must be a
    BUILT_IN_NORMAL function.  ARG is the first and only argument of the
    call.  */
 
@@ -9223,6 +9223,9 @@ warn_for_abs (location_t loc, tree fndecl, tree arg)
       return;
     }
 
+  if (!TYPE_ARG_TYPES (TREE_TYPE (fndecl)))
+    return;
+
   tree ftype = TREE_VALUE (TYPE_ARG_TYPES (TREE_TYPE (fndecl)));
   if (TREE_CODE (atype) == COMPLEX_TYPE)
     {
diff --git a/gcc/testsuite/gcc.dg/pr87347.c b/gcc/testsuite/gcc.dg/pr87347.c
new file mode 100644
index 00000000000..d0bdf2a9fec
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/pr87347.c
@@ -0,0 +1,6 @@
+/* {dg-do compile} */
+/* { dg-options "-Wabsolute-value" } */
+
+int a;
+int abs();
+void b() { abs(a); }

On Tue, Sep 25, 2018 at 05:47:49PM +0200, Martin Jambor wrote:
> > Isn't that bail out too early?
> > I mean most of the warnings that are emitted by the function don't really
> > need TYPE_ARG_TYPES, only the last one does, so can't you just bail out
> > before the last warning?
> 
> my reasoning was that if the function is not what I expect it to be, it
> is better not to touch it.  On the other hand, I have no problems moving
> this test lower as done in the patch below.

I guess the question is if we then treat it as a builtin or don't.
Anyway, I'd like to defer that decision to the C FE maintainers.

> > Also, the function comment has "gracely", did you mean "gracefully"?
> 
> Of course I did, thanks for spotting that.
> 
> Bootstrapped and tested on x86_64-linux and aarch64-linux.  OK for
> trunk?

	Jakub

On Tue, 25 Sep 2018, Jakub Jelinek wrote:

> On Tue, Sep 25, 2018 at 05:47:49PM +0200, Martin Jambor wrote:
> > > Isn't that bail out too early?
> > > I mean most of the warnings that are emitted by the function don't really
> > > need TYPE_ARG_TYPES, only the last one does, so can't you just bail out
> > > before the last warning?
> > 
> > my reasoning was that if the function is not what I expect it to be, it
> > is better not to touch it.  On the other hand, I have no problems moving
> > this test lower as done in the patch below.
> 
> I guess the question is if we then treat it as a builtin or don't.
> Anyway, I'd like to defer that decision to the C FE maintainers.

This patch is OK.  (If there's something odd about the argument meaning 
it ends up not being handled as a built-in, warn_for_abs will have 
returned early anyway.)