Patchwork pr57457

login
register
mail settings
Submitter Iyer, Balaji V
Date May 31, 2013, 1:54 p.m.
Message ID <BF230D13CA30DD48930C31D4099330003A429D70@FMSMSX101.amr.corp.intel.com>
Download mbox | patch
Permalink /patch/247962/
State New
Headers show

Comments

Iyer, Balaji V - May 31, 2013, 1:54 p.m.
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.

Thanks,

Balaji V. Iyer.
Jeff Law - May 31, 2013, 3:50 p.m.
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.

ie, it's easy to sprinkle tests for NULL pointers in the sources to 
change behaviour, but it's more important to look at why we're getting a 
NULL pointer at any particular point and decide if it's valid or not.

You've probably already done the analysis, you just need to make sure to 
include it in the patch submission.  That way the reviewer can easily 
see the change is correct and the analysis is preserved for future 
reference.


Jeff
Iyer, Balaji V - May 31, 2013, 6:01 p.m.
> -----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.

Thanks,

Balaji V. Iyer.

> 
> ie, it's easy to sprinkle tests for NULL pointers in the sources to change
> behaviour, but it's more important to look at why we're getting a NULL pointer
> at any particular point and decide if it's valid or not.
> 
> You've probably already done the analysis, you just need to make sure to include
> it in the patch submission.  That way the reviewer can easily see the change is
> correct and the analysis is preserved for future reference.


> 
> 
> Jeff
Iyer, Balaji V - May 31, 2013, 9:56 p.m.
HI Jeff et al.,
	Forgot to ask in my previous email... Is this Ok for trunk?

-Balaji V. Iyer.

> -----Original Message-----
> From: Iyer, Balaji V
> Sent: Friday, May 31, 2013 2:02 PM
> To: 'Jeff Law'
> Cc: gcc-patches@gcc.gnu.org; Steve Ellcey
> Subject: RE: [PATCH] pr57457
> 
> 
> 
> > -----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.
> 
> Thanks,
> 
> Balaji V. Iyer.
> 
> >
> > ie, it's easy to sprinkle tests for NULL pointers in the sources to change
> > behaviour, but it's more important to look at why we're getting a NULL pointer
> > at any particular point and decide if it's valid or not.
> >
> > You've probably already done the analysis, you just need to make sure to
> include
> > it in the patch submission.  That way the reviewer can easily see the change is
> > correct and the analysis is preserved for future reference.
> 
> 
> >
> >
> > Jeff
Jeff Law - June 3, 2013, 7:06 p.m.
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?


jeff

Patch

diff --git a/gcc/c/ChangeLog b/gcc/c/ChangeLog
index 828d7c8..e376276 100755
Binary files a/gcc/c/ChangeLog and b/gcc/c/ChangeLog differ
diff --git a/gcc/c/c-array-notation.c b/gcc/c/c-array-notation.c
index 681e111..8d43fd9 100755
--- a/gcc/c/c-array-notation.c
+++ b/gcc/c/c-array-notation.c
@@ -140,6 +140,8 @@  length_mismatch_in_expr_p (location_t loc, tree **list, size_t x, size_t y)
 enum built_in_function
 is_cilkplus_reduce_builtin (tree fndecl)
 {
+  if (!fndecl)
+    return BUILT_IN_NONE;
   if (TREE_CODE (fndecl) == ADDR_EXPR)
     fndecl = TREE_OPERAND (fndecl, 0);
 
diff --git a/gcc/testsuite/ChangeLog b/gcc/testsuite/ChangeLog
index 20bf8b5..e1339db 100755
Binary files a/gcc/testsuite/ChangeLog and b/gcc/testsuite/ChangeLog differ
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);
+}