This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [PATCH 1/3] Fix logic bug in Cilk Plus array expansion
- From: Patrick Palka <patrick at parcs dot ath dot cx>
- To: Jeff Law <law at redhat dot com>
- Cc: Jakub Jelinek <jakub at redhat dot com>, GCC Patches <gcc-patches at gcc dot gnu dot org>, Jason Merrill <jason at redhat dot com>
- Date: Sun, 10 Jan 2016 22:55:06 -0500
- Subject: Re: [PATCH 1/3] Fix logic bug in Cilk Plus array expansion
- Authentication-results: sourceware.org; auth=none
- References: <1451576417-8710-1-git-send-email-patrick at parcs dot ath dot cx> <56875ADA dot 6090805 at redhat dot com> <20160102082141 dot GD18720 at tucnak dot redhat dot com> <CA+C-WL8f0XmphVHHpxUaUjpUf7jsfRd-ZbQkW4zEVtwtnbF+YQ at mail dot gmail dot com> <568ABB77 dot 5000409 at redhat dot com>
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?
>
> Jeff
>