Patchwork pr57457

login
register
mail settings
Submitter Iyer, Balaji V
Date June 4, 2013, 5:37 p.m.
Message ID <BF230D13CA30DD48930C31D4099330003A42ACA4@FMSMSX101.amr.corp.intel.com>
Download mbox | patch
Permalink /patch/248793/
State New
Headers show

Comments

Iyer, Balaji V - June 4, 2013, 5:37 p.m.
> -----Original Message-----
> From: Jeff Law [mailto:law@redhat.com]
> Sent: Monday, June 03, 2013 3:07 PM
> To: Iyer, Balaji V
> Cc: gcc-patches@gcc.gnu.org; Steve Ellcey
> Subject: Re: [PATCH] pr57457
> 
> On 05/31/2013 12:01 PM, Iyer, Balaji V wrote:
> >
> >
> >> -----Original Message----- From: gcc-patches-owner@gcc.gnu.org
> >> [mailto:gcc-patches- owner@gcc.gnu.org] On Behalf Of Jeff Law Sent:
> >> Friday, May 31, 2013 11:50 AM To: Iyer, Balaji V Cc:
> >> gcc-patches@gcc.gnu.org; Steve Ellcey Subject: Re: [PATCH] pr57457
> >>
> >> On 05/31/2013 07:54 AM, Iyer, Balaji V wrote:
> >>> Hello Everyone, This patch will fix a bug reported in PR57457.
> >>> One of the array notation
> >> function was not checking for NULL_TREE before accessing its fields.
> >> This patch should fix that issue. A test case is also added.
> >>>
> >>> Is this OK for trunk?
> >>>
> >>> Here are the ChangeLog Entries:
> >>>
> >>> gcc/c/ChangeLog 2013-05-31  Balaji V. Iyer <balaji.v.iyer@intel.com>
> >>>
> >>> * c-array-notation.c (is_cilkplus_reduce_builtin): Added a check for
> >>> NULL_TREE parameter input.
> >>>
> >>> gcc/testsuite/ChangeLog 2013-05-31  Balaji V. Iyer
> >>> <balaji.v.iyer@intel.com>
> >>>
> >>> PR c/57457 * c-c++-common/cilk-plus/AN/pr57457.c: New testcase.
> >> So what you need to do is explain how you got into this function with
> >> a NULL fndecl and why that's OK.
> >
> > Hi Jeff, I looked into it, and there is another function call called
> > inform_declaration, and that does exactly what I did (i.e. check for
> > NULL fundecl before accessing its fields). From what I can tell,
> > fundecl will be NULL_TREE if a function declaration is a function
> > pointer.
> The problematical calls are coming from convert_arguments which is a bit
> inconsistent in how it handles a null FUNDECL.  In some places it checks it
> direction in convert_arguments and avoids certain actions.  In other places the
> callee checks.
> 
> The code looks like it's screaming to be simplified.  Neither flag_cilkplus nor
> FUNDECL change inside the main loop in convert_arguments, so the first thing
> I'd ponder is pulling that condition out of the loop.  And after doing that we a
> similar condition being used to suppress warnings just after the loop.  I wonder if
> we could have something like
> 
> if (flag_enable_cilkplus
>      && fundecl
>      && is_cilkplus_reduction_builtin (fundecl))
>    return vec_safe_length (values);
> 
> before the loop, then eliminate the the test inside the loop and just after the
> loop.
> 
> That simplifies the code a bit and should fix this problem unless I'm missing
> something?

Yes, that does simplify the whole thing. Here is an updated ChangeLog and patch (with testcode) attached. So, is it Ok for trunk?

gcc/testsuite/ChangeLog
2013-06-04  Balaji V. Iyer  <balaji.v.iyer@intel.com>

       PR C/57457
        * c-c++-common/cilk-plus/AN/pr57457.c: New test.

gcc/c/ChangeLog
2013-06-04  Balaji V. Iyer  <balaji.v.iyer@intel.com>

        * c-typeck.c (convert_arguments): Moved checking of builtin cilkplus
        reduction functions outside the for-loop.  Also, added a check if the
        fundecl is non-NULL.

Thanks,

Balaji V. Iyer.

> 
> 
> jeff
>
Jeff Law - June 4, 2013, 6:07 p.m.
On 06/04/2013 11:37 AM, Iyer, Balaji V wrote:
>
> Yes, that does simplify the whole thing. Here is an updated ChangeLog
> and patch (with testcode) attached. So, is it Ok for trunk?
>
> gcc/testsuite/ChangeLog 2013-06-04  Balaji V. Iyer
> <balaji.v.iyer@intel.com>
>
> PR C/57457 * c-c++-common/cilk-plus/AN/pr57457.c: New test.
>
> gcc/c/ChangeLog 2013-06-04  Balaji V. Iyer
> <balaji.v.iyer@intel.com>
>
> * c-typeck.c (convert_arguments): Moved checking of builtin cilkplus
> reduction functions outside the for-loop.  Also, added a check if
> the fundecl is non-NULL.
OK.  This looks good.  Please install.

As a follow-up, don't we need to do something with the test after the 
loop as well:

   if (typetail != 0 && TREE_VALUE (typetail) != void_type_node)
     {
       /* If array notation is used and Cilk Plus is enabled, then we do not
          worry about this error now.  We will handle them in a later 
place.  */
       if (!flag_enable_cilkplus
           || !is_cilkplus_reduce_builtin (fundecl))
         {
           error_at (input_location,
                     "too few arguments to function %qE", function);
           inform_declaration (fundecl);
           return -1;
         }
     }

ISTM we need to check for a NULL FUNDECL here too.

jeff
Iyer, Balaji V - June 4, 2013, 6:58 p.m.
> -----Original Message-----
> From: Jeff Law [mailto:law@redhat.com]
> Sent: Tuesday, June 04, 2013 2:07 PM
> To: Iyer, Balaji V
> Cc: gcc-patches@gcc.gnu.org; Steve Ellcey
> Subject: Re: [PATCH] pr57457
> 
> On 06/04/2013 11:37 AM, Iyer, Balaji V wrote:
> >
> > Yes, that does simplify the whole thing. Here is an updated ChangeLog
> > and patch (with testcode) attached. So, is it Ok for trunk?
> >
> > gcc/testsuite/ChangeLog 2013-06-04  Balaji V. Iyer
> > <balaji.v.iyer@intel.com>
> >
> > PR C/57457 * c-c++-common/cilk-plus/AN/pr57457.c: New test.
> >
> > gcc/c/ChangeLog 2013-06-04  Balaji V. Iyer <balaji.v.iyer@intel.com>
> >
> > * c-typeck.c (convert_arguments): Moved checking of builtin cilkplus
> > reduction functions outside the for-loop.  Also, added a check if the
> > fundecl is non-NULL.
> OK.  This looks good.  Please install.
> 
> As a follow-up, don't we need to do something with the test after the loop as
> well:
> 
>    if (typetail != 0 && TREE_VALUE (typetail) != void_type_node)
>      {
>        /* If array notation is used and Cilk Plus is enabled, then we do not
>           worry about this error now.  We will handle them in a later place.  */
>        if (!flag_enable_cilkplus
>            || !is_cilkplus_reduce_builtin (fundecl))
>          {
>            error_at (input_location,
>                      "too few arguments to function %qE", function);
>            inform_declaration (fundecl);
>            return -1;
>          }
>      }
> 
> ISTM we need to check for a NULL FUNDECL here too.

Actually, you can eliminate the entire if-statement (i.e. remove if-statement and make the body unconditional). This is because, if flag_enable_cilkplus is true and is_cilkplus_reduce_builtin (fundecl) is true, then it would have returned vec_safe_length(values) and will not even get to this point in the first place. So, this is technically equivalent to if (1). So, can I remove that and check it in also? It passes all my regression tests.

Thanks,

Balaji V. Iyer.

> 
> jeff
Jeff Law - June 5, 2013, 3:48 a.m.
On 06/04/13 12:58, Iyer, Balaji V wrote:
>
>
> Actually, you can eliminate the entire if-statement (i.e. remove
> if-statement and make the body unconditional). This is because, if
> flag_enable_cilkplus is true and is_cilkplus_reduce_builtin (fundecl)
> is true, then it would have returned vec_safe_length(values) and will
> not even get to this point in the first place. So, this is
> technically equivalent to if (1). So, can I remove that and check it
> in also? It passes all my regression tests.
I originally thought it could be eliminated as well, but after further 
reflection I couldn't convince myself it'd do the right thing for the 
case when flag_enable_cilkplus is true but is_cilkplus_reduce_builtin 
was false.

Note that triggering that code my be nontrivial, AFAICT we're 
suppressing a diagnostic.  So you're going to need to write invalid code 
to get into that condition at the bottom of the loop at all.

jeff
Iyer, Balaji V - June 5, 2013, 3:50 a.m.
> -----Original Message-----
> From: Jeff Law [mailto:law@redhat.com]
> Sent: Tuesday, June 04, 2013 11:49 PM
> To: Iyer, Balaji V
> Cc: gcc-patches@gcc.gnu.org; Steve Ellcey
> Subject: Re: [PATCH] pr57457
> 
> On 06/04/13 12:58, Iyer, Balaji V wrote:
> >
> >
> > Actually, you can eliminate the entire if-statement (i.e. remove
> > if-statement and make the body unconditional). This is because, if
> > flag_enable_cilkplus is true and is_cilkplus_reduce_builtin (fundecl)
> > is true, then it would have returned vec_safe_length(values) and will
> > not even get to this point in the first place. So, this is technically
> > equivalent to if (1). So, can I remove that and check it in also? It
> > passes all my regression tests.
> I originally thought it could be eliminated as well, but after further reflection I
> couldn't convince myself it'd do the right thing for the case when
> flag_enable_cilkplus is true but is_cilkplus_reduce_builtin was false.
> 
> Note that triggering that code my be nontrivial, AFAICT we're suppressing a
> diagnostic.  So you're going to need to write invalid code to get into that
> condition at the bottom of the loop at all.

OK, I will create one tomorrow morning.

Thanks,

Balaji V. Iyer.

> 
> jeff

Patch

diff --git a/gcc/c/c-typeck.c b/gcc/c/c-typeck.c
index e5e1455..91ce67a 100644
--- a/gcc/c/c-typeck.c
+++ b/gcc/c/c-typeck.c
@@ -2942,6 +2942,8 @@  convert_arguments (tree typelist, vec<tree, va_gc> *values,
 	  break;
 	}
     }
+  if (flag_enable_cilkplus && fundecl && is_cilkplus_reduce_builtin (fundecl))
+    return vec_safe_length (values);
 
   /* Scan the given expressions and types, producing individual
      converted arguments.  */
@@ -2959,17 +2961,6 @@  convert_arguments (tree typelist, vec<tree, va_gc> *values,
       bool npc;
       tree parmval;
 
-      // FIXME: I assume this code is here to handle the overloaded
-      // behavior of the __sec_reduce* builtins, and avoid giving
-      // argument mismatch warnings/errors.  We should probably handle
-      // this with the resolve_overloaded_builtin infrastructure.
-      /* If the function call is a builtin function call, then we do not
-	 worry about it since we break them up into its equivalent later and
-	 we do the appropriate checks there.  */
-      if (flag_enable_cilkplus
-	  && is_cilkplus_reduce_builtin (fundecl))
-	continue;
-      
       if (type == void_type_node)
 	{
 	  if (selector)
diff --git a/gcc/testsuite/c-c++-common/cilk-plus/AN/pr57457.c b/gcc/testsuite/c-c++-common/cilk-plus/AN/pr57457.c
new file mode 100644
index 0000000..68a1fd8
--- /dev/null
+++ b/gcc/testsuite/c-c++-common/cilk-plus/AN/pr57457.c
@@ -0,0 +1,39 @@ 
+/* { dg-do compile } */
+/* { dg-options "-fcilkplus" } */
+
+/* This test has no array notation components in it and thus should compile
+   fine without crashing.  */
+
+typedef unsigned int size_t;
+typedef int (*__compar_fn_t) (const void *, const void *);
+extern void *bsearch (const void *__key, const void *__base,
+		      size_t __nmemb, size_t __size, __compar_fn_t
+		      __compar)
+  __attribute__ ((__nonnull__ (1, 2, 5))) ;
+extern __inline __attribute__ ((__gnu_inline__)) void *
+bsearch (const void *__key, const void *__base, size_t __nmemb, size_t
+	 __size,
+	 __compar_fn_t __compar)
+{
+  size_t __l, __u, __idx;
+  const void *__p;
+  int __comparison;
+  __l = 0;
+  __u = __nmemb;
+  while (__l < __u)
+    {
+      __idx = (__l + __u) / 2;
+      __p = (void *) (((const char *) __base) +
+		      (__idx * __size));
+      __comparison = (*__compar) (__key,
+				  __p);
+      if (__comparison < 0)
+	__u = __idx;
+      else if (__comparison > 0)
+	__l = __idx + 1;
+      else
+	return (void *)
+	  __p;
+    }
+  return ((void *)0);
+}