Patchwork pr57457

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

Comments

Iyer, Balaji V - June 5, 2013, 3:18 p.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.

Hi Jeff,
	This email is going to be a bit long, so I apologize for it ahead of time. 

	Here is the diff of c-typeck.c (to show that I have removed the unwanted if statement)

=============================================================================


Here is the testcode that I have written that should trigger the "too few arguments to function error" with the appropriate note to say where the function declaration is.

----------------------------------
/* { dg-do compile } */
/* { dg-options "-fcilkplus" } */

/* Test-case contains no array notation but is compiled with -fcilkplus.
   It will still print the too few arguments func, thereby saying the
   if-statement after the for-loop to check for !flag_enable_cilkplus ||
   !is_cilkplus_reduce_function (fundecl) is not valid is always taken.  */

int func (int, int); /* { dg-message "declared here" } */

int main (void)
{
  int a = 5, b = 2;
  return func (a); /* { dg-error "too few arguments to function" } */
}
--------------------------------

Here is the output when I run this:

bviyer@lakshmi2a:/export/users/gcc-svn/b-trunk-gcc/gcc> ../../install_dir/trunk-install/bin/gcc -fcilkplus test.c
test.c: In function âmainâ:
test.c:14:3: error: too few arguments to function âfuncâ
   return func (a); /* { dg-error "too few arguments to function" } */
   ^
test.c:9:5: note: declared here
 int func (int, int); /* { dg-message "declared here" } */
     ^



The whole patch with testcode  is attached, so is this Ok for trunk?

Here are the ChangeLog entries:
gcc/c/ChangeLog
2013-06-05  Balaji V. Iyer  <balaji.v.iyer@intel.com>

        * c-typeck.c (convert_arguments): Moved checking of builtin cilkplus
        reduction functions outside the for-loop.  Added a check if the fundecl
        is non-NULL.  Finally, removed an unwanted if-statement, and made the
        body unconditional

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

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

Thanks,

Balaji V. Iyer.




> 
> jeff
Index: gcc/c/ChangeLog
===================================================================
--- gcc/c/ChangeLog	(revision 199672)
+++ gcc/c/ChangeLog	(working copy)
@@ -1,3 +1,9 @@
+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.
+
 2013-06-03  Balaji V. Iyer  <balaji.v.iyer@intel.com>
 
 	* c-typeck.c (c_finish_if_stmt): Added a check to see if the rank of the
Index: gcc/c/c-typeck.c
===================================================================
--- gcc/c/c-typeck.c	(revision 199672)
+++ gcc/c/c-typeck.c	(working copy)
@@ -2942,6 +2942,8 @@
 	  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 @@
       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)
@@ -3207,16 +3198,10 @@
 
   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;
-	}
+      error_at (input_location,
+		"too few arguments to function %qE", function);
+      inform_declaration (fundecl);
+      return -1;
     }
 
   return error_args ? -1 : (int) parmnum;
Index: gcc/testsuite/ChangeLog
===================================================================
--- gcc/testsuite/ChangeLog	(revision 199672)
+++ gcc/testsuite/ChangeLog	(working copy)
@@ -4,6 +4,11 @@
 
 2013-06-04  Balaji V. Iyer  <balaji.v.iyer@intel.com>
 
+	PR C/57457
+	* c-c++-common/cilk-plus/AN/pr57457.c: New test.
+	
+2013-06-04  Balaji V. Iyer  <balaji.v.iyer@intel.com>
+
 	* c-c++-common/cilk-plus/AN/array_test1.c (main): Replaced argc, argv
 	parameters with void.
 	(main2): Removed argc parameter.
Index: gcc/testsuite/c-c++-common/cilk-plus/AN/pr57457-2.c
===================================================================
--- gcc/testsuite/c-c++-common/cilk-plus/AN/pr57457-2.c	(revision 0)
+++ gcc/testsuite/c-c++-common/cilk-plus/AN/pr57457-2.c	(revision 0)
@@ -0,0 +1,15 @@
+/* { dg-do compile } */
+/* { dg-options "-fcilkplus" } */
+
+/* Test-case contains no array notation but is compiled with -fcilkplus.  
+   It will still print the too few arguments func, thereby saying the
+   if-statement after the for-loop to check for !flag_enable_cilkplus ||
+   !is_cilkplus_reduce_function (fundecl) is not valid is always taken.  */
+
+int func (int, int); /* { dg-message "declared here" } */
+
+int main (void)
+{
+  int a = 5, b = 2;
+  return func (a); /* { dg-error "too few arguments to function" } */
+}
Index: gcc/testsuite/c-c++-common/cilk-plus/AN/pr57457.c
===================================================================
--- gcc/testsuite/c-c++-common/cilk-plus/AN/pr57457.c	(revision 0)
+++ gcc/testsuite/c-c++-common/cilk-plus/AN/pr57457.c	(revision 0)
@@ -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);
+}
Jeff Law - June 5, 2013, 7:40 p.m.
On 06/05/13 09:18, Iyer, Balaji V wrote:
>
>
>> -----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.
>
> Hi Jeff,
> 	This email is going to be a bit long, so I apologize for it ahead of time.
>
> 	Here is the diff of c-typeck.c (to show that I have removed the unwanted if statement)
>
> Index: gcc/c/c-typeck.c
> ===================================================================
> --- gcc/c/c-typeck.c    (revision 199672)
> +++ gcc/c/c-typeck.c    (working copy)
> @@ -2942,6 +2942,8 @@
>            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 @@
>         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)
> @@ -3207,16 +3198,10 @@
>
>     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;
> -       }
> +      error_at (input_location,
> +               "too few arguments to function %qE", function);
> +      inform_declaration (fundecl);
> +      return -1;
>       }
>
>     return error_args ? -1 : (int) parmnum;
> =============================================================================
>
>
> Here is the testcode that I have written that should trigger the "too few arguments to function error" with the appropriate note to say where the function declaration is.
>
> ----------------------------------
> /* { dg-do compile } */
> /* { dg-options "-fcilkplus" } */
>
> /* Test-case contains no array notation but is compiled with -fcilkplus.
>     It will still print the too few arguments func, thereby saying the
>     if-statement after the for-loop to check for !flag_enable_cilkplus ||
>     !is_cilkplus_reduce_function (fundecl) is not valid is always taken.  */
>
> int func (int, int); /* { dg-message "declared here" } */
>
> int main (void)
> {
>    int a = 5, b = 2;
>    return func (a); /* { dg-error "too few arguments to function" } */
> }
> --------------------------------
>
> Here is the output when I run this:
>
> bviyer@lakshmi2a:/export/users/gcc-svn/b-trunk-gcc/gcc> ../../install_dir/trunk-install/bin/gcc -fcilkplus test.c
> test.c: In function âmainâ:
> test.c:14:3: error: too few arguments to function âfuncâ
>     return func (a); /* { dg-error "too few arguments to function" } */
>     ^
> test.c:9:5: note: declared here
>   int func (int, int); /* { dg-message "declared here" } */
>       ^
>
>
>
> The whole patch with testcode  is attached, so is this Ok for trunk?
>
> Here are the ChangeLog entries:
> gcc/c/ChangeLog
> 2013-06-05  Balaji V. Iyer  <balaji.v.iyer@intel.com>
>
>          * c-typeck.c (convert_arguments): Moved checking of builtin cilkplus
>          reduction functions outside the for-loop.  Added a check if the fundecl
>          is non-NULL.  Finally, removed an unwanted if-statement, and made the
>          body unconditional
>
> gcc/testsuite/ChangeLog
> 2013-06-05  Balaji V. Iyer  <balaji.v.iyer@intel.com>
>
>          PR C/57457
>          * c-c++-common/cilk-plus/AN/pr57457.c: New test.
>          * c-c++-common/cilk-plus/AN/pr57457-2.c: Likewise.
This is fine.  Please install.  Thanks!

jeff

Patch

Index: gcc/c/c-typeck.c
===================================================================
--- gcc/c/c-typeck.c    (revision 199672)
+++ gcc/c/c-typeck.c    (working copy)
@@ -2942,6 +2942,8 @@ 
          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 @@ 
       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)
@@ -3207,16 +3198,10 @@ 

   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;
-       }
+      error_at (input_location,
+               "too few arguments to function %qE", function);
+      inform_declaration (fundecl);
+      return -1;
     }

   return error_args ? -1 : (int) parmnum;