| Message ID | BF230D13CA30DD48930C31D4099330003A429D70@FMSMSX101.amr.corp.intel.com |
|---|---|
| State | New |
| Headers | show |
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
> -----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
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
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
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); +}
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.