This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


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


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]