From patchwork Fri Jan 15 04:07:02 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Patrick Palka X-Patchwork-Id: 567911 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from sourceware.org (server1.sourceware.org [209.132.180.131]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 95535140BF7 for ; Fri, 15 Jan 2016 15:07:19 +1100 (AEDT) Authentication-Results: ozlabs.org; dkim=pass (1024-bit key; unprotected) header.d=gcc.gnu.org header.i=@gcc.gnu.org header.b=urRzvc4D; dkim-atps=neutral DomainKey-Signature: a=rsa-sha1; c=nofws; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender:from :date:to:cc:subject:in-reply-to:message-id:references :mime-version:content-type; q=dns; s=default; b=GW06vxMZJTTw1O7g FNsXBjkaXlz9KGIUXRM5SeztkM9ABzsXGLZ/8e0owLVMiHuol4TlNO2ZbI7TaDhW AUN0kQnlaPgFxDkIlZacYleGgsd0Vv8hzWEsdnQmqjduGuK3BG0t8pR/Q1Cj1cyN ynjnqvtQ1F+nFeJF4NYEYPPuNJc= DKIM-Signature: v=1; a=rsa-sha1; c=relaxed; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender:from :date:to:cc:subject:in-reply-to:message-id:references :mime-version:content-type; s=default; bh=kUW/ymz7PPaIvprOEYartx q+laQ=; b=urRzvc4D/0rD3SpuZuQCFE5oyypFxyi+xG8kCjIOufQ1f4p3WHmxHr 9EpYB9FalLxdNhshdXNqjWjCXGMM5omKIOAXaD6eB6Fn2CuMnLgoVbAto5CeL2Li giE51DQbndiA4d7RD1p2C4zNKTzzM1QUAjD6Vp0IqgvdI/6JGaHEQ= Received: (qmail 48765 invoked by alias); 15 Jan 2016 04:07:12 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Delivered-To: mailing list gcc-patches@gcc.gnu.org Received: (qmail 48752 invoked by uid 89); 15 Jan 2016 04:07:11 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.6 required=5.0 tests=BAYES_00, RCVD_IN_DNSWL_LOW autolearn=ham version=3.3.2 spammy=114 X-HELO: mail-qg0-f65.google.com Received: from mail-qg0-f65.google.com (HELO mail-qg0-f65.google.com) (209.85.192.65) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-GCM-SHA256 encrypted) ESMTPS; Fri, 15 Jan 2016 04:07:10 +0000 Received: by mail-qg0-f65.google.com with SMTP id b35so48831857qge.2 for ; Thu, 14 Jan 2016 20:07:09 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:from:date:to:cc:subject:in-reply-to:message-id :references:user-agent:mime-version:content-type; bh=8eDv66lRnN1D3NKoiA2l+1Lbk6KA0QAACDFChwIH25k=; b=ecNirgchx37v+B8H232NqHyB7oZweV1XTjeYWP3C2zglrzdxGbQ8DmRcV4SB/Gz8AH tdcdkTnbJk9pD0lM/7iX/3uzPmRc0F43kLJtT7cfwsYWg0VWxu2xzxcAUrus4EdOOmc/ gBgGj6KFW06M947CNRpXwAKHjm786sqR/Yoeb37+QhkEvrcbNRfIe4GGqkKs3AotKv+q LDkS+HqL8NFH72cw5m987U/s9ZcynsBnOYAFMPBFqhAh5d1phhFDkwYZzABXNsqGQf17 vNzpdbAK/RR47zSR2nqnC9PLVw+9HQ8Z1QFR15QbBt51ULAJncVoPrFkrdGt/TVTGL5L 6Bpw== X-Gm-Message-State: ALoCoQma/Z0PizEhEkAV7ptdEXQoYZOKoXExdw+ciCpKHPC8xlvU5EkCnXudG8xg3tU3TH2bmnG12EOr0e/BTKv61yhluJOTfw== X-Received: by 10.140.195.136 with SMTP id q130mr11203790qha.45.1452830827923; Thu, 14 Jan 2016 20:07:07 -0800 (PST) Received: from [192.168.1.130] (ool-4353abbc.dyn.optonline.net. [67.83.171.188]) by smtp.gmail.com with ESMTPSA id y199sm3797238qhb.4.2016.01.14.20.07.07 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Thu, 14 Jan 2016 20:07:07 -0800 (PST) From: Patrick Palka X-Google-Original-From: Patrick Palka Date: Thu, 14 Jan 2016 23:07:02 -0500 (EST) To: Jeff Law cc: Patrick Palka , Jakub Jelinek , GCC Patches , Jason Merrill Subject: Re: [PATCH 1/3] Fix logic bug in Cilk Plus array expansion In-Reply-To: <56980DCE.1080100@redhat.com> Message-ID: References: <1451576417-8710-1-git-send-email-patrick@parcs.ath.cx> <56875ADA.6090805@redhat.com> <20160102082141.GD18720@tucnak.redhat.com> <568ABB77.5000409@redhat.com> <56980DCE.1080100@redhat.com> User-Agent: Alpine 2.20.9 (DEB 106 2015-09-22) MIME-Version: 1.0 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 wrote: >>> On 01/02/2016 04:26 PM, Patrick Palka wrote: >>>> >>>> On Sat, Jan 2, 2016 at 3:21 AM, Jakub Jelinek 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 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 --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 @@ -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" } } */