openmp, fortran: Check that event handles passed to detach clauses are not arrays [PR104131]
Message ID | b9291725-ceb8-3038-d47c-b67dfe221ba6@codesourcery.com |
---|---|
State | New |
Headers |
Return-Path: <gcc-patches-bounces+patchwork=sourceware.org@gcc.gnu.org> X-Original-To: patchwork@sourceware.org Delivered-To: patchwork@sourceware.org Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 302263857C47 for <patchwork@sourceware.org>; Mon, 28 Feb 2022 14:02:50 +0000 (GMT) X-Original-To: gcc-patches@gcc.gnu.org Delivered-To: gcc-patches@gcc.gnu.org Received: from esa1.mentor.iphmx.com (esa1.mentor.iphmx.com [68.232.129.153]) by sourceware.org (Postfix) with ESMTPS id A1F1A3858C83; Mon, 28 Feb 2022 14:02:21 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org A1F1A3858C83 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=codesourcery.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=mentor.com X-IronPort-AV: E=Sophos;i="5.90,142,1643702400"; d="scan'208,223";a="75068294" Received: from orw-gwy-02-in.mentorg.com ([192.94.38.167]) by esa1.mentor.iphmx.com with ESMTP; 28 Feb 2022 06:02:03 -0800 IronPort-SDR: N5F/5lvIjiVkHXmlfHoEVpmISoO5ZGVmUrrdKWswMqXMyMPc70WXK3QUGfcmlvF6bfbCkBozZf Mctwes89AlEgB2FgwpeYEQ38R/Y0jVseifXNG21DPKL7tGeSZai9x+v/a0VSJfHn6dcfkvQPH5 B8Ab5OFshvXeFVjnUdmLUPFUlHzMGArNZO0UmAfSdJRzbutpYOT732qCtZGW4l4PaePqgM0rDQ PsuW6gpNIcNHIMItdcI82Z8Mm03INM80CsPxnS8IjZlC3ZigLfXpGnI6qRQztHGMqBd74cH/DL ogE= Content-Type: multipart/mixed; boundary="------------PYEwxKd6jgENmPB6kssc2kHF" Message-ID: <b9291725-ceb8-3038-d47c-b67dfe221ba6@codesourcery.com> Date: Mon, 28 Feb 2022 14:01:03 +0000 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101 Thunderbird/91.6.1 From: Kwok Cheung Yeung <kcy@codesourcery.com> Subject: [PATCH] openmp, fortran: Check that event handles passed to detach clauses are not arrays [PR104131] To: gcc-patches <gcc-patches@gcc.gnu.org>, fortran <fortran@gcc.gnu.org>, Jakub Jelinek <jakub@redhat.com>, <gscfq@t-online.de> X-Originating-IP: [137.202.0.90] X-ClientProxiedBy: SVR-IES-MBX-07.mgc.mentorg.com (139.181.222.7) To SVR-IES-MBX-04.mgc.mentorg.com (139.181.222.4) X-Spam-Status: No, score=-11.9 required=5.0 tests=BAYES_00, GIT_PATCH_0, HEADER_FROM_DIFFERENT_DOMAINS, KAM_DMARC_STATUS, SPF_HELO_PASS, SPF_PASS, TXREP, T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on server2.sourceware.org X-BeenThere: gcc-patches@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gcc-patches mailing list <gcc-patches.gcc.gnu.org> List-Unsubscribe: <https://gcc.gnu.org/mailman/options/gcc-patches>, <mailto:gcc-patches-request@gcc.gnu.org?subject=unsubscribe> List-Archive: <https://gcc.gnu.org/pipermail/gcc-patches/> List-Post: <mailto:gcc-patches@gcc.gnu.org> List-Help: <mailto:gcc-patches-request@gcc.gnu.org?subject=help> List-Subscribe: <https://gcc.gnu.org/mailman/listinfo/gcc-patches>, <mailto:gcc-patches-request@gcc.gnu.org?subject=subscribe> Errors-To: gcc-patches-bounces+patchwork=sourceware.org@gcc.gnu.org Sender: "Gcc-patches" <gcc-patches-bounces+patchwork=sourceware.org@gcc.gnu.org> |
Series |
openmp, fortran: Check that event handles passed to detach clauses are not arrays [PR104131]
|
|
Commit Message
Kwok Cheung Yeung
Feb. 28, 2022, 2:01 p.m. UTC
Hello
This patch addresses PR fortran/104131 on the GCC bug tracker, where an
ICE would occur if an array or co-array was passed as the event handle
in the detach clause of a task.
Since the event handle is supposed to be a scalar of type
omp_event_handle_kind, we can simply reject the event handle during
parsing if it is any type of array, thereby preventing the situation
leading to an ICE in the first place.
Okay for trunk?
Thanks
Kwok
From 8ed3b8bd793298f94bdefbdff32f91eaea1a9d70 Mon Sep 17 00:00:00 2001
From: Kwok Cheung Yeung <kcy@codesourcery.com>
Date: Mon, 28 Feb 2022 12:34:22 +0000
Subject: [PATCH] openmp, fortran: Check that event handles passed to detach
clauses are not arrays [PR104131]
2022-02-28 Kwok Cheung Yeung <kcy@codesourcery.com>
gcc/fortran/
PR fortran/104131
* openmp.cc (gfc_match_omp_detach): Check that the event handle is not
an array type.
gcc/testsuite/
PR fortran/104131
* gfortran.dg/gomp/pr104131.f90: New.
* gfortran.dg/gomp/pr104131-2.f90: New.
* gfortran.dg/gomp/task-detach-1.f90: Update expected error message.
---
gcc/fortran/openmp.cc | 5 +++--
gcc/testsuite/gfortran.dg/gomp/pr104131-2.f90 | 10 ++++++++++
gcc/testsuite/gfortran.dg/gomp/pr104131.f90 | 10 ++++++++++
gcc/testsuite/gfortran.dg/gomp/task-detach-1.f90 | 2 +-
4 files changed, 24 insertions(+), 3 deletions(-)
create mode 100644 gcc/testsuite/gfortran.dg/gomp/pr104131-2.f90
create mode 100644 gcc/testsuite/gfortran.dg/gomp/pr104131.f90
Comments
On Mon, Feb 28, 2022 at 02:01:03PM +0000, Kwok Cheung Yeung wrote: > gcc/fortran/ > > PR fortran/104131 > * openmp.cc (gfc_match_omp_detach): Check that the event handle is not > an array type. > > gcc/testsuite/ > > PR fortran/104131 > * gfortran.dg/gomp/pr104131.f90: New. > * gfortran.dg/gomp/pr104131-2.f90: New. > * gfortran.dg/gomp/task-detach-1.f90: Update expected error message. > --- > gcc/fortran/openmp.cc | 5 +++-- > gcc/testsuite/gfortran.dg/gomp/pr104131-2.f90 | 10 ++++++++++ > gcc/testsuite/gfortran.dg/gomp/pr104131.f90 | 10 ++++++++++ > gcc/testsuite/gfortran.dg/gomp/task-detach-1.f90 | 2 +- > 4 files changed, 24 insertions(+), 3 deletions(-) > create mode 100644 gcc/testsuite/gfortran.dg/gomp/pr104131-2.f90 > create mode 100644 gcc/testsuite/gfortran.dg/gomp/pr104131.f90 > > diff --git a/gcc/fortran/openmp.cc b/gcc/fortran/openmp.cc > index 19142c4d8d0..50a1c476009 100644 > --- a/gcc/fortran/openmp.cc > +++ b/gcc/fortran/openmp.cc > @@ -531,9 +531,10 @@ gfc_match_omp_detach (gfc_expr **expr) > if (gfc_match_variable (expr, 0) != MATCH_YES) > goto syntax_error; > > - if ((*expr)->ts.type != BT_INTEGER || (*expr)->ts.kind != gfc_c_intptr_kind) > + if ((*expr)->ts.type != BT_INTEGER || (*expr)->ts.kind != gfc_c_intptr_kind > + || (*expr)->symtree->n.sym->as) Don't we usually test instead || (*expr)->rank != 0 when testing for scalars? Jakub
On 28/02/2022 2:07 pm, Jakub Jelinek wrote: > On Mon, Feb 28, 2022 at 02:01:03PM +0000, Kwok Cheung Yeung wrote: >> diff --git a/gcc/fortran/openmp.cc b/gcc/fortran/openmp.cc >> index 19142c4d8d0..50a1c476009 100644 >> --- a/gcc/fortran/openmp.cc >> +++ b/gcc/fortran/openmp.cc >> @@ -531,9 +531,10 @@ gfc_match_omp_detach (gfc_expr **expr) >> if (gfc_match_variable (expr, 0) != MATCH_YES) >> goto syntax_error; >> >> - if ((*expr)->ts.type != BT_INTEGER || (*expr)->ts.kind != gfc_c_intptr_kind) >> + if ((*expr)->ts.type != BT_INTEGER || (*expr)->ts.kind != gfc_c_intptr_kind >> + || (*expr)->symtree->n.sym->as) > > Don't we usually test instead || (*expr)->rank != 0 when testing for > scalars? > > Jakub > If I run GCC in GDB on the pr104131.f90 testcase and inspect the expr, I get: 534 if ((*expr)->ts.type != BT_INTEGER || (*expr)->ts.kind != gfc_c_intptr_kind (gdb) p **expr $2 = {expr_type = EXPR_VARIABLE, ts = {type = BT_INTEGER, kind = 8, u = {derived = 0x0, cl = 0x0, pad = 0}, interface = 0x0, is_c_interop = 1, is_iso_c = 0, f90_type = BT_INTEGER, deferred = false, interop_kind = 0x2e3fb80}, rank = 0, shape = 0x0, symtree = 0x2e3ffe0, ref = 0x2e3e600, where = { ... So (*expr)->rank is 0 here even with an array. I'm not sure why - is rank updated later, or did we forget to call something on the event handle expression? Testing against n->sym->as for an array check has been used elsewhere in openmp.cc, to prevent reductions against arrays in OpenACC in resolve_omp_clauses. Kwok
Le 28/02/2022 à 15:27, Kwok Cheung Yeung a écrit : > On 28/02/2022 2:07 pm, Jakub Jelinek wrote: (...) >> Don't we usually test instead || (*expr)->rank != 0 when testing for >> scalars? >> (...) > > So (*expr)->rank is 0 here even with an array. I'm not sure why - is > rank updated later, or did we forget to call something on the event > handle expression? > > Testing against n->sym->as for an array check has been used elsewhere in > openmp.cc, to prevent reductions against arrays in OpenACC in > resolve_omp_clauses. > I can’t tell what openmp requires; it depends on your needs. Checking sym->as captures array variables which may include scalar expressions (arr(10) is a scalar expression even if arr is an array variable), while checking expr->rank only capture array expression, including scalar variable with array subcomponent (scal%array_comp(:) is an array expression, even if scal is a scalar variable). gfc_resolve_expr, through gfc_expression_rank takes care of properly setting expr->rank. If the check is done at resolution stage (somewhere in resolve_omp_clauses I guess?), the rank should be set. I hope it helps.
On Mon, Feb 28, 2022 at 04:54:24PM +0100, Mikael Morin wrote: > Le 28/02/2022 à 15:27, Kwok Cheung Yeung a écrit : > > On 28/02/2022 2:07 pm, Jakub Jelinek wrote: > (...) > > > Don't we usually test instead || (*expr)->rank != 0 when testing for > > > scalars? > > > > (...) > > > > So (*expr)->rank is 0 here even with an array. I'm not sure why - is > > rank updated later, or did we forget to call something on the event > > handle expression? > > > > Testing against n->sym->as for an array check has been used elsewhere in > > openmp.cc, to prevent reductions against arrays in OpenACC in > > resolve_omp_clauses. > > > I can’t tell what openmp requires; it depends on your needs. > > Checking sym->as captures array variables which may include scalar > expressions (arr(10) is a scalar expression even if arr is an array > variable), while checking expr->rank only capture array expression, > including scalar variable with array subcomponent (scal%array_comp(:) is an > array expression, even if scal is a scalar variable). > > gfc_resolve_expr, through gfc_expression_rank takes care of properly setting > expr->rank. > If the check is done at resolution stage (somewhere in resolve_omp_clauses I > guess?), the rank should be set. > > I hope it helps. It is true that the spots I saw in fortran/openmp.cc that test rank look like: if (!gfc_resolve_expr (el->expr) || el->expr->ts.type != BT_INTEGER || el->expr->rank != 0) etc., so probably !gfc_resolve_expr call is missing. Jakub
Le 28/02/2022 à 17:00, Jakub Jelinek a écrit : > On Mon, Feb 28, 2022 at 04:54:24PM +0100, Mikael Morin wrote: >> Le 28/02/2022 à 15:27, Kwok Cheung Yeung a écrit : >>> On 28/02/2022 2:07 pm, Jakub Jelinek wrote: >> (...) >>>> Don't we usually test instead || (*expr)->rank != 0 when testing for >>>> scalars? >>>> >> (...) >>> >>> So (*expr)->rank is 0 here even with an array. I'm not sure why - is >>> rank updated later, or did we forget to call something on the event >>> handle expression? >>> >>> Testing against n->sym->as for an array check has been used elsewhere in >>> openmp.cc, to prevent reductions against arrays in OpenACC in >>> resolve_omp_clauses. >>> >> I can’t tell what openmp requires; it depends on your needs. >> >> Checking sym->as captures array variables which may include scalar >> expressions (arr(10) is a scalar expression even if arr is an array >> variable), while checking expr->rank only capture array expression, >> including scalar variable with array subcomponent (scal%array_comp(:) is an >> array expression, even if scal is a scalar variable). >> >> gfc_resolve_expr, through gfc_expression_rank takes care of properly setting >> expr->rank. >> If the check is done at resolution stage (somewhere in resolve_omp_clauses I >> guess?), the rank should be set. >> >> I hope it helps. > > It is true that the spots I saw in fortran/openmp.cc that test rank look > like: > if (!gfc_resolve_expr (el->expr) > || el->expr->ts.type != BT_INTEGER || el->expr->rank != 0) > etc., so probably !gfc_resolve_expr call is missing. > As long as the expression is expected to not be a (contained) function call, I think it should work. In the general case non-syntaxic errors are preferably checked and reported later at resolution stage, where contained functions are known.
On Mon, Feb 28, 2022 at 06:33:15PM +0100, Mikael Morin wrote: > > It is true that the spots I saw in fortran/openmp.cc that test rank look > > like: > > if (!gfc_resolve_expr (el->expr) > > || el->expr->ts.type != BT_INTEGER || el->expr->rank != 0) > > etc., so probably !gfc_resolve_expr call is missing. > > > As long as the expression is expected to not be a (contained) function call, > I think it should work. > > In the general case non-syntaxic errors are preferably checked and reported > later at resolution stage, where contained functions are known. Oh, I've missed that it is done during parsing and not during resolution. That !gfc_resolve_expr call and the checking if it is BT_INTEGER etc. should be certainly moved to resolve_omp_clauses. Jakub
On 28/02/2022 5:37 pm, Jakub Jelinek wrote: > On Mon, Feb 28, 2022 at 06:33:15PM +0100, Mikael Morin wrote: >>> It is true that the spots I saw in fortran/openmp.cc that test rank look >>> like: >>> if (!gfc_resolve_expr (el->expr) >>> || el->expr->ts.type != BT_INTEGER || el->expr->rank != 0) >>> etc., so probably !gfc_resolve_expr call is missing. >>> >> As long as the expression is expected to not be a (contained) function call, >> I think it should work. >> >> In the general case non-syntaxic errors are preferably checked and reported >> later at resolution stage, where contained functions are known. > > Oh, I've missed that it is done during parsing and not during resolution. > That !gfc_resolve_expr call and the checking if it is BT_INTEGER etc. > should be certainly moved to resolve_omp_clauses. > Calling gfc_resolve_expr does not work to update the rank when called from gfc_match_omp_detach: (gdb) p *e->ref $3 = {type = REF_ARRAY, u = {ar = {type = AR_ELEMENT, dimen = 0, codimen = 1, in_allocate = false, team = 0x0, stat = 0x0, where = {nextc = 0x2e532d8, lb = 0x2e53260}, as = 0x2e04110, c_where = {{nextc = 0x0, lb = 0x0} <repeats 15 times>}, start = {0x0 <repeats 15 times>}, end = {0x0 <repeats 15 times>}, stride = {0x0 <repeats 15 times>}, dimen_type = {DIMEN_THIS_IMAGE, 0 <repeats 14 times>}}, c = {component = 0x2, sym = 0x1}, ss = {start = 0x2, end = 0x1, length = 0x0}, i = INQUIRY_KIND}, next = 0x0} In gfc_expression_rank, e->ref is non-NULL, so e->rank is not set from the symtree. It then iterates through the ref elements - ref->type == REF_ARRAY and ref->u.ar.type == AR_ELEMENT, so e->rank remains at 0. I'll move the check to resolve_omp_clauses and see if it works there. Thanks Kwok
Le 28/02/2022 à 19:38, Kwok Cheung Yeung a écrit : > > In gfc_expression_rank, e->ref is non-NULL, so e->rank is not set from > the symtree. It then iterates through the ref elements - ref->type == > REF_ARRAY and ref->u.ar.type == AR_ELEMENT, so e->rank remains at 0. > This is the expected behavior. > I'll move the check to resolve_omp_clauses and see if it works there. > It won’t work differently there. Looking at the testcases, the rank should be 1 for pr104131.f90 and 0 for pr104131-2.f90. A scalar coarray remains a scalar; its rank is 0. Maybe corank should be checked together with rank?
Le 28/02/2022 à 21:37, Mikael Morin a écrit :
> Maybe corank should be checked together with rank?
Lesson learned today: expressions don’t have a corank.
Does pr104131-2.f90 really need to be rejected?
On Mon, Feb 28, 2022 at 09:45:10PM +0100, Mikael Morin wrote: > Le 28/02/2022 à 21:37, Mikael Morin a écrit : > > Maybe corank should be checked together with rank? > > Lesson learned today: expressions don’t have a corank. > Does pr104131-2.f90 really need to be rejected? OpenMP 5.2 says that detach clause should be treated as if it appears on a firstprivate clause and for the privatization clauses says: "A private variable must not be coindexed or appear as an actual argument to a procedure where the corresponding dummy argument is a coarray." 5.1 had the same restriction. Jakub
Le 28/02/2022 à 22:37, Jakub Jelinek a écrit : > On Mon, Feb 28, 2022 at 09:45:10PM +0100, Mikael Morin wrote: >> Le 28/02/2022 à 21:37, Mikael Morin a écrit : >>> Maybe corank should be checked together with rank? >> >> Lesson learned today: expressions don’t have a corank. There is gfc_is_coindexed that can be used. >> Does pr104131-2.f90 really need to be rejected? > > OpenMP 5.2 says that detach clause should be treated as if it appears on a > firstprivate clause and for the privatization clauses says: > "A private variable must not be coindexed or appear as an actual argument to a procedure where > the corresponding dummy argument is a coarray." > 5.1 had the same restriction. > There is also: > A variable that is part of another variable (as an array element or a structure element) cannot > appear in a detach clause. which tells that the check should be on expr->ref instead of expr->sym->as or expr->rank. None of the above excerpts from the spec forbid pr104131.f90 or pr104131-2.f90 by the way.
On 28.02.22 22:37, Jakub Jelinek via Fortran wrote: > On Mon, Feb 28, 2022 at 09:45:10PM +0100, Mikael Morin wrote: >> Lesson learned today: expressions don’t have a corank. >> Does pr104131-2.f90 really need to be rejected? In my reading of the spec, pr104131-2.f90 is _valid_ (in newer OMP). At least that's implied by the spec as quoted by Jakub: > OpenMP 5.2 says that detach clause should be treated as if it appears on a > firstprivate clause and for the privatization clauses says: > "A private variable must not be coindexed or appear as an actual argument to a procedure where > the corresponding dummy argument is a coarray." > 5.1 had the same restriction. +++ b/gcc/testsuite/gfortran.dg/gomp/pr104131-2.f90 ... + integer (kind=omp_event_handle_kind) :: x[*] + !$omp task detach (x) Here, 'x' is a coarray – but refers to the local variable on this image. But the following is invalid as it is coindexed: !$omp task detach (x[3]) where x[3] means that the value from 'x' on image 3 should be used. The wording actually also permits array sections. But contrary to coarrays, (which are odd but otherwise fine), I think it does not really make sense to have arrays and array sections here. (Do we need/want to get this clarified/changed in spec?) But from the wording of the spec, also the first testcase seems to be valid. * * * >> A variable that is part of another variable (as an array element or a >> structure element) cannot appear in a detach clause. > which tells that the check should be on expr->ref instead of > expr->sym->as or expr->rank. I think looking at the "sym" is fine when matching the expression via gfc_match_omp_variable_list with allow_derived=false (default). As then there cannot be derived-type components. Additionally, expr->rank > 0 rules out arrays/array sections but permits array elements while sym->addr.dimension also rules out array elements. BTW: after resolving a variable, expr->ref always exists for arrays – either to select an element or array section or otherwise, there is an AR_FULL for a whole array. Tobias ----------------- Siemens Electronic Design Automation GmbH; Anschrift: Arnulfstraße 201, 80634 München; Gesellschaft mit beschränkter Haftung; Geschäftsführer: Thomas Heurung, Frank Thürauf; Sitz der Gesellschaft: München; Registergericht München, HRB 106955
On Tue, Mar 01, 2022 at 08:58:54AM +0100, Tobias Burnus wrote: > The wording actually also permits array sections. But contrary to coarrays, > (which are odd but otherwise fine), I think it does not really make sense > to have arrays and array sections here. > > (Do we need/want to get this clarified/changed in spec?) In 5.2 we have: "A variable that is part of another variable (as an array element or a structure element) cannot appear in a detach clause." and "An array section can appear only in clauses for which it is explicitly allowed." and C/C++ "A variable of <generic_name> OpenMP type is a variable of type omp_<generic_name>_t." and Fortran "A variable of <generic_name> OpenMP type is a scalar integer variable of kind omp_<generic_name>_kind." and "event-handle variable of event_handle type default" As there is no explicit allowing of array sections, array sections aren't allowed in detach, and it must be scalar integer. The question is if a non-coindexed coarray is a scalar integer variable or not. Jakub
First, thanks for looking into the standard. I think the information is too much spread through the standard. I keep searching for restrictions, find them at 5 places and miss another 5. With OpenMP 5.2, it became even worse. On 01.03.22 09:16, Jakub Jelinek wrote: > As there is no explicit allowing of array sections, array sections aren't > allowed in detach, and it must be scalar integer. > The question is if a non-coindexed coarray is a scalar integer variable or > not. I think the Fortran sense, yes. That only coindexed vars are disallowed also implies this in the OpenMP spec. In terms of the implementation, a local part of a coarray is really not much different from any other variable, except that all coarrays have to be either in static memory or allocatables/pointers (which are collectively allocated). For a non-descriptor variable like: program main integer, save :: a[*] call foo(a) contains subroutine foo(x) integer :: x[*] end end That's also the case for the internal representation: static void * restrict caf_token.2; static integer(kind=4) * restrict a; void foo (integer(kind=4) * restrict x, void * restrict caf_token.0, integer(kind=8) caf_offset.1) 'x' is a pretty normal variable (admittedly, internally now a pointer) which can be used like a noncoarray. (The pointer is there to permit to put the var into some special, remote accessible memory.) Tobias ----------------- Siemens Electronic Design Automation GmbH; Anschrift: Arnulfstraße 201, 80634 München; Gesellschaft mit beschränkter Haftung; Geschäftsführer: Thomas Heurung, Frank Thürauf; Sitz der Gesellschaft: München; Registergericht München, HRB 106955
So, if I try to sum up what has been gathered in this thread: - pr104131.f90 is invalid, as x is not scalar. Checks are better done in resolve_omp_clauses after a call to gfc_resolve_expr. Checking expr->sym->attr.dimension seems to cover more cases than expr->rank > 0. - pr104131-2.f90 is valid and should be accepted. - Some other cases should be rejected, including x[1] (coindexed variable), x(1) (array element), x%comp (structure component). Is that correct? Anything else? Regarding the expr->rank vs expr->sym->attr.dimension controversy, my take is that it should stick to the error message. Use expr->rank is the error is about scalar vs array, use expr->sym->attr.dimension if it’s about subobject-ness of an array variable. Coming back to the PR, the ICE backtraces for pr104131.f90 and pr104131-2.f90 are different and should probably be treated separatedly. I don’t know how difficult the bullet 2 above would be, but bullet 1 and 3 seem quite doable.
diff --git a/gcc/fortran/openmp.cc b/gcc/fortran/openmp.cc index 19142c4d8d0..50a1c476009 100644 --- a/gcc/fortran/openmp.cc +++ b/gcc/fortran/openmp.cc @@ -531,9 +531,10 @@ gfc_match_omp_detach (gfc_expr **expr) if (gfc_match_variable (expr, 0) != MATCH_YES) goto syntax_error; - if ((*expr)->ts.type != BT_INTEGER || (*expr)->ts.kind != gfc_c_intptr_kind) + if ((*expr)->ts.type != BT_INTEGER || (*expr)->ts.kind != gfc_c_intptr_kind + || (*expr)->symtree->n.sym->as) { - gfc_error ("%qs at %L should be of type " + gfc_error ("%qs at %L should be a scalar of type " "integer(kind=omp_event_handle_kind)", (*expr)->symtree->n.sym->name, &(*expr)->where); return MATCH_ERROR; diff --git a/gcc/testsuite/gfortran.dg/gomp/pr104131-2.f90 b/gcc/testsuite/gfortran.dg/gomp/pr104131-2.f90 new file mode 100644 index 00000000000..8d10367ba3b --- /dev/null +++ b/gcc/testsuite/gfortran.dg/gomp/pr104131-2.f90 @@ -0,0 +1,10 @@ +! { dg-do compile } +! { dg-options "-fopenmp -fcoarray=single" } + +program p + use iso_c_binding, only: c_intptr_t + integer, parameter :: omp_event_handle_kind = c_intptr_t + integer (kind=omp_event_handle_kind) :: x[*] + !$omp task detach (x) ! { dg-error "'x' at \\\(1\\\) should be a scalar of type integer\\\(kind=omp_event_handle_kind\\\)" } + !$omp end task ! { dg-error "Unexpected !\\\$OMP END TASK statement at \\\(1\\\)" } +end diff --git a/gcc/testsuite/gfortran.dg/gomp/pr104131.f90 b/gcc/testsuite/gfortran.dg/gomp/pr104131.f90 new file mode 100644 index 00000000000..70a2dedfd7f --- /dev/null +++ b/gcc/testsuite/gfortran.dg/gomp/pr104131.f90 @@ -0,0 +1,10 @@ +! { dg-do compile } +! { dg-options "-fopenmp" } + +program p + use iso_c_binding, only: c_intptr_t + integer, parameter :: omp_event_handle_kind = c_intptr_t + integer(omp_event_handle_kind) :: x(1) + !$omp task detach(x) ! { dg-error "'x' at \\\(1\\\) should be a scalar of type integer\\\(kind=omp_event_handle_kind\\\)" } + !$omp end task ! { dg-error "Unexpected !\\\$OMP END TASK statement at \\\(1\\\)" } +end diff --git a/gcc/testsuite/gfortran.dg/gomp/task-detach-1.f90 b/gcc/testsuite/gfortran.dg/gomp/task-detach-1.f90 index 020be13a8b6..b73db07b7c3 100644 --- a/gcc/testsuite/gfortran.dg/gomp/task-detach-1.f90 +++ b/gcc/testsuite/gfortran.dg/gomp/task-detach-1.f90 @@ -18,7 +18,7 @@ program task_detach_1 !$omp task detach(x) mergeable ! { dg-error "'DETACH' clause at \\\(1\\\) must not be used together with 'MERGEABLE' clause" } !$omp end task - !$omp task detach(z) ! { dg-error "'z' at \\\(1\\\) should be of type integer\\\(kind=omp_event_handle_kind\\\)" } + !$omp task detach(z) ! { dg-error "'z' at \\\(1\\\) should be a scalar of type integer\\\(kind=omp_event_handle_kind\\\)" } !$omp end task ! { dg-error "Unexpected !\\\$OMP END TASK statement at \\\(1\\\)" } !$omp task detach (x) firstprivate (x) ! { dg-error "DETACH event handle 'x' in FIRSTPRIVATE clause at \\\(1\\\)" }