diff mbox

[1/3] Fix logic bug in Cilk Plus array expansion

Message ID alpine.DEB.2.20.9.1601142305000.23354@idea
State New
Headers show

Commit Message

Patrick Palka Jan. 15, 2016, 4:07 a.m. UTC 
On Thu, 14 Jan 2016, Jeff Law wrote:

> On 01/10/2016 08:55 PM, Patrick Palka wrote:
>> On Mon, Jan 4, 2016 at 1:35 PM, Jeff Law <law@redhat.com> wrote:
>>> On 01/02/2016 04:26 PM, Patrick Palka wrote:
>>>> 
>>>> On Sat, Jan 2, 2016 at 3:21 AM, Jakub Jelinek <jakub@redhat.com> wrote:
>>>>> 
>>>>> On Fri, Jan 01, 2016 at 10:06:34PM -0700, Jeff Law wrote:
>>>>>>> 
>>>>>>> gcc/cp/ChangeLog:
>>>>>>>
>>>>>>>       * cp-array-notation.c (cp_expand_cond_array_notations): Return
>>>>>>>       error_mark_node only if find_rank failed, not if it was
>>>>>>>       successful.
>>>>>> 
>>>>>> Can you use -fdump-tree-original in the testcase and verify there's no
>>>>>> <<<
>>>>>> error >>> expressions in the resulting dump file?
>>>>>> 
>>>>>> With that change, this is OK.
>>>>> 
>>>>> 
>>>>> I think the patch is incomplete.  Because, find_rank does not always 
>>>>> emit
>>>>> an error if it returns false, so we again have cases where we can get
>>>>> error_mark_node in the code without error being emitted.
>>>>>         else if (*rank != current_rank)
>>>>>           {
>>>>>             /* In this case, find rank is being recursed through a set 
>>>>> of
>>>>>                expression of the form A <OPERATION> B, where A and B 
>>>>> both
>>>>> have
>>>>>                array notations in them and the rank of A is not equal to
>>>>> rank of B.
>>>>>                A simple example of such case is the following: X[:] +
>>>>> Y[:][:] */
>>>>>             *rank = current_rank;
>>>>>             return false;
>>>>>           }
>>>>> and other spots.  E.g.
>>>>>                     if (prev_arg && EXPR_HAS_LOCATION (prev_arg))
>>>>>                       error_at (EXPR_LOCATION (prev_arg),
>>>>>                                 "rank mismatch between %qE and %qE",
>>>>> prev_arg,
>>>>>                                 TREE_OPERAND (expr, ii));
>>>>> looks very suspicious.
>>>> 
>>>> 
>>>> Hmm, good point. Here's a contrived test case that causes find_rank to
>>>> return false without emitting an error message thus we again end up
>>>> with an error_mark_node in the gimplifier:
>>>> 
>>>> /* { dg-do compile } */
>>>> /* { dg-options "-fcilkplus" } */
>>>> 
>>>> void foo() {}
>>>> 
>>>> #define ALEN 1024
>>>> 
>>>> int main(int argc, char* argv[])
>>>> {
>>>>     typedef void (*f) (void *);
>>>>     f b[ALEN], c[ALEN][ALEN];
>>>>     (b[:]) ((void *)c[:][:]);
>>>>     _Cilk_spawn foo();
>>>>     return 0;
>>>> }
>>>> 
>>>> But this patch was intended to only fix the testsuite fallout that
>>>> patch 3 would have otherwise caused, and not to e.g. fix all the bugs
>>>> with find_rank.
>>>> 
>>>> (BTW patch 3 also makes this test case trigger an ICE, instead of
>>>> being silently miscompiled.)
>>> 
>>> Can you please include this test (xfailed) when you commit patch #1.  I
>>> think you want the test to scan for error_mark_node in the gimplified 
>>> dump.
>> 
>> There are four subdirectories under
>> gcc/testsuite/c-c++-common/cilk-plus -- AN, CK, PS and SE. Into which
>> directory should this new xfailed test go?
> These are for array notation, so AN seems best.
>
> Jeff
>

Thanks, this is what I'm going to commit some time tomorrow (assuming no
objections).

I have written the new xfail'd test case to expect that a
"rank mismatch" error ought to have been issued, which I hope is the
correct expectation.

gcc/cp/ChangeLog:

 	* cp-array-notation.c (cp_expand_cond_array_notations): Return
 	error_mark_node only if find_rank failed, not if it was
 	successful.

gcc/testsuite/ChangeLog:

 	* c-c++-common/cilk-plus/AN/an-if.c: Check that the original
 	dump does not contain an error_mark_node.
 	* c-c++-common/cilk-plus/CK/pr60469.c: Likewise.
 	* c-c++-common/cilk-plus/AN/fn_ptr-2.c: New xfail'd test.
---
  gcc/cp/cp-array-notation.c                         |  4 ++--
  gcc/testsuite/c-c++-common/cilk-plus/AN/an-if.c    |  5 ++++-
  gcc/testsuite/c-c++-common/cilk-plus/AN/fn_ptr-2.c | 14 ++++++++++++++
  gcc/testsuite/c-c++-common/cilk-plus/CK/pr60469.c  |  5 ++++-
  4 files changed, 24 insertions(+), 4 deletions(-)
  create mode 100644 gcc/testsuite/c-c++-common/cilk-plus/AN/fn_ptr-2.c
diff mbox

Patch

diff --git a/gcc/cp/cp-array-notation.c b/gcc/cp/cp-array-notation.c
index f7a4598..4687ced 100644
--- a/gcc/cp/cp-array-notation.c
+++ b/gcc/cp/cp-array-notation.c
@@ -807,8 +807,8 @@  cp_expand_cond_array_notations (tree orig_stmt)
        if (!find_rank (EXPR_LOCATION (cond), cond, cond, true, &cond_rank)
  	  || !find_rank (EXPR_LOCATION (yes_expr), yes_expr, yes_expr, true,
  			 &yes_rank)
-	  || find_rank (EXPR_LOCATION (no_expr), no_expr, no_expr, true,
-			&no_rank))
+	  || !find_rank (EXPR_LOCATION (no_expr), no_expr, no_expr, true,
+			 &no_rank))
  	return error_mark_node;
        /* If the condition has a zero rank, then handle array notations in body
  	 separately.  */
diff --git a/gcc/testsuite/c-c++-common/cilk-plus/AN/an-if.c b/gcc/testsuite/c-c++-common/cilk-plus/AN/an-if.c
index 4bf85b5..4ac46ab 100644
--- a/gcc/testsuite/c-c++-common/cilk-plus/AN/an-if.c
+++ b/gcc/testsuite/c-c++-common/cilk-plus/AN/an-if.c
@@ -1,5 +1,5 @@ 
  /* { dg-do run } */
-/* { dg-options "-fcilkplus" } */
+/* { dg-options "-fcilkplus -fdump-tree-original" } */

  #if HAVE_IO
  #include <stdio.h>
@@ -46,3 +46,6 @@  int main() {
      }
      return 0;
  }
+
+/* The C++ FE once emitted a bogus error_mark_node for this test case.  */
+/* { dg-final { scan-tree-dump-not "<<< error >>>" "original" } } */
diff --git a/gcc/testsuite/c-c++-common/cilk-plus/AN/fn_ptr-2.c b/gcc/testsuite/c-c++-common/cilk-plus/AN/fn_ptr-2.c
new file mode 100644
index 0000000..4e1990f
--- /dev/null
+++ b/gcc/testsuite/c-c++-common/cilk-plus/AN/fn_ptr-2.c
@@ -0,0 +1,14 @@ 
+/* { dg-do compile } */
+/* { dg-options "-fcilkplus" } */
+
+typedef void (*f) (void *);
+f b[1024];
+void *c[1024][1024];
+
+int
+main (void)
+{
+  (b[:]) (c[:][:]); /* { dg-error "rank mismatch" "" { xfail *-*-* } } */
+  return 0;
+}
+
diff --git a/gcc/testsuite/c-c++-common/cilk-plus/CK/pr60469.c b/gcc/testsuite/c-c++-common/cilk-plus/CK/pr60469.c
index ca0cf7f..670df17 100644
--- a/gcc/testsuite/c-c++-common/cilk-plus/CK/pr60469.c
+++ b/gcc/testsuite/c-c++-common/cilk-plus/CK/pr60469.c
@@ -1,6 +1,6 @@ 
  /* PR middle-end/60469 */
  /* { dg-do compile } */
-/* { dg-options "-fcilkplus" } */
+/* { dg-options "-fcilkplus -fdump-tree-original" } */

  void foo() {}

@@ -13,3 +13,6 @@  int main(int argc, char* argv[])
    _Cilk_spawn foo();
    return 0;
  }
+
+/* The C++ FE once emitted a bogus error_mark_node for this test case.  */
+/* { dg-final { scan-tree-dump-not "<<< error >>>" "original" } } */