[PATCH] openmp, fortran: Check that event handles passed to detach clauses are not arrays [PR104131]

[PATCH] openmp, fortran: Check that event handles passed to detach clauses are not arrays [PR104131]

[Kwok Cheung Yeung]

[-- Attachment #1: Type: text/plain, Size: 451 bytes --]

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

[-- Attachment #2: 0001-openmp-fortran-Check-that-event-handles-passed-to-de.patch --]
[-- Type: text/plain, Size: 4057 bytes --]

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

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\\\)" }
-- 
2.25.1

Re: [PATCH] openmp, fortran: Check that event handles passed to detach clauses are not arrays [PR104131]

[Jakub Jelinek]

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

Re: [PATCH] openmp, fortran: Check that event handles passed to detach clauses are not arrays [PR104131]

[Kwok Cheung Yeung]

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

Re: [PATCH] openmp, fortran: Check that event handles passed to detach clauses are not arrays [PR104131]

[Mikael Morin]

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.

Re: [PATCH] openmp, fortran: Check that event handles passed to detach clauses are not arrays [PR104131]

[Jakub Jelinek]

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

Re: [PATCH] openmp, fortran: Check that event handles passed to detach clauses are not arrays [PR104131]

[Mikael Morin]

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.

Re: [PATCH] openmp, fortran: Check that event handles passed to detach clauses are not arrays [PR104131]

[Jakub Jelinek]

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

Re: [PATCH] openmp, fortran: Check that event handles passed to detach clauses are not arrays [PR104131]

[Kwok Cheung Yeung]

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

Re: [PATCH] openmp, fortran: Check that event handles passed to detach clauses are not arrays [PR104131]

[Mikael Morin]

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?

Re: [PATCH] openmp, fortran: Check that event handles passed to detach clauses are not arrays [PR104131]

[Mikael Morin]

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?

Re: [PATCH] openmp, fortran: Check that event handles passed to detach clauses are not arrays [PR104131]

[Jakub Jelinek]

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

Re: [PATCH] openmp, fortran: Check that event handles passed to detach clauses are not arrays [PR104131]

[Mikael Morin]

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.

Re: [PATCH] openmp, fortran: Check that event handles passed to detach clauses are not arrays [PR104131]

[Tobias Burnus]

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

Re: [PATCH] openmp, fortran: Check that event handles passed to detach clauses are not arrays [PR104131]

[Jakub Jelinek]

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

Re: [PATCH] openmp, fortran: Check that event handles passed to detach clauses are not arrays [PR104131]

[Tobias Burnus]

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

Re: [PATCH] openmp, fortran: Check that event handles passed to detach clauses are not arrays [PR104131]

[Mikael Morin]

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.

[PATCH][v2] openmp, fortran: Check that the type of an event handle in a detach clause is suitable [PR104131]

[Kwok Cheung Yeung]

[-- Attachment #1: Type: text/plain, Size: 1640 bytes --]

Hello

I have updated the patch to catch array elements and structure 
components as additional checks, in addition to checking that the 
variable is a scalar.

The check has been moved to the end of resolve_omp_clauses as it is more 
appropriate there. This gets rid of the additional 'Unexpected !$OMP END 
TASK statement' error, since the type error is now caught after the 
matching phase.

Coarrays (with the testcases in pr104131-2.f90) can be dealt with in a 
separate patch. Is this part okay for trunk?

Thanks

Kwok

On 01/03/2022 3:37 pm, Mikael Morin wrote:
> 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.

[-- Attachment #2: 0001-openmp-fortran-Check-that-the-type-of-an-event-handl.patch --]
[-- Type: text/plain, Size: 5382 bytes --]

From 3ed6eb1e38ad2a25c6eca18f9ff4d05d3f227db3 Mon Sep 17 00:00:00 2001
From: Kwok Cheung Yeung <kcy@codesourcery.com>
Date: Wed, 2 Mar 2022 17:09:45 +0000
Subject: [PATCH] openmp, fortran: Check that the type of an event handle in a
 detach clause is suitable [PR104131]

This rejects variables that are array types, array elements or derived type
members when used as the event handle inside a detach clause (in accordance
with the OpenMP specification).  This would previously lead to an ICE.

2022-03-02  Kwok Cheung Yeung  <kcy@codesourcery.com>

gcc/fortran/

	PR fortran/104131
	* openmp.cc (gfc_match_omp_detach): Move check for type of event
	handle to...
	(resolve_omp_clauses) ...here.  Also check that the event handle is
	not an array, or an array access or structure element access.

gcc/testsuite/

	PR fortran/104131
	* gfortran.dg/gomp/pr104131.f90: New.
	* gfortran.dg/gomp/task-detach-1.f90: Update expected error message.
---
 gcc/fortran/openmp.cc                         | 34 +++++++++++++------
 gcc/testsuite/gfortran.dg/gomp/pr104131.f90   | 26 ++++++++++++++
 .../gfortran.dg/gomp/task-detach-1.f90        |  4 +--
 3 files changed, 51 insertions(+), 13 deletions(-)
 create mode 100644 gcc/testsuite/gfortran.dg/gomp/pr104131.f90

diff --git a/gcc/fortran/openmp.cc b/gcc/fortran/openmp.cc
index 19142c4d8d0..16cd03a3d67 100644
--- a/gcc/fortran/openmp.cc
+++ b/gcc/fortran/openmp.cc
@@ -531,14 +531,6 @@ 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)
-    {
-      gfc_error ("%qs at %L should be of type "
-		 "integer(kind=omp_event_handle_kind)",
-		 (*expr)->symtree->n.sym->name, &(*expr)->where);
-      return MATCH_ERROR;
-    }
-
   if (gfc_match_char (')') != MATCH_YES)
     goto syntax_error;
 
@@ -7581,9 +7573,29 @@ resolve_omp_clauses (gfc_code *code, gfc_omp_clauses *omp_clauses,
 	gfc_error ("%s must contain at least one MAP clause at %L",
 		   p, &code->loc);
     }
-  if (!openacc && omp_clauses->mergeable && omp_clauses->detach)
-    gfc_error ("%<DETACH%> clause at %L must not be used together with "
-	       "%<MERGEABLE%> clause", &omp_clauses->detach->where);
+
+  if (!openacc && omp_clauses->detach)
+    {
+      if (!gfc_resolve_expr (omp_clauses->detach)
+	  || omp_clauses->detach->ts.type != BT_INTEGER
+	  || omp_clauses->detach->ts.kind != gfc_c_intptr_kind
+	  || omp_clauses->detach->rank != 0)
+	gfc_error ("%qs at %L should be a scalar of type "
+		   "integer(kind=omp_event_handle_kind)",
+		   omp_clauses->detach->symtree->n.sym->name,
+		   &omp_clauses->detach->where);
+      else if (omp_clauses->detach->symtree->n.sym->attr.dimension > 0)
+	gfc_error ("The event handle at %L must not be an array element",
+		   &omp_clauses->detach->where);
+      else if (omp_clauses->detach->symtree->n.sym->ts.type == BT_DERIVED
+	       || omp_clauses->detach->symtree->n.sym->ts.type == BT_CLASS)
+	gfc_error ("The event handle at %L must not be part of "
+		   "a derived type or class", &omp_clauses->detach->where);
+
+      if (omp_clauses->mergeable)
+	gfc_error ("%<DETACH%> clause at %L must not be used together with "
+		   "%<MERGEABLE%> clause", &omp_clauses->detach->where);
+    }
 }
 
 
diff --git a/gcc/testsuite/gfortran.dg/gomp/pr104131.f90 b/gcc/testsuite/gfortran.dg/gomp/pr104131.f90
new file mode 100644
index 00000000000..472d19dd753
--- /dev/null
+++ b/gcc/testsuite/gfortran.dg/gomp/pr104131.f90
@@ -0,0 +1,26 @@
+! { dg-do compile }
+! { dg-options "-fopenmp" }
+
+program p
+  use iso_c_binding, only: c_intptr_t
+  implicit none
+  integer, parameter :: omp_event_handle_kind = c_intptr_t
+
+  type dt
+    integer(omp_event_handle_kind) :: f
+  end type
+  integer(omp_event_handle_kind) :: x(1)
+  type(dt) :: y
+
+  !$omp task detach(x) ! { dg-error "'x' at \\\(1\\\) should be a scalar of type integer\\\(kind=omp_event_handle_kind\\\)" }
+  !$omp end task
+
+  !$omp task detach(x(1)) ! { dg-error "The event handle at \\\(1\\\) must not be an array element" }
+  !$omp end task
+
+  !$omp task detach(y) ! { dg-error "'y' at \\\(1\\\) should be a scalar of type integer\\\(kind=omp_event_handle_kind\\\)" }
+  !$omp end task
+
+  !$omp task detach(y%f) ! { dg-error "The event handle at \\\(1\\\) must not be part of a derived type or class" }
+  !$omp end task
+end program
diff --git a/gcc/testsuite/gfortran.dg/gomp/task-detach-1.f90 b/gcc/testsuite/gfortran.dg/gomp/task-detach-1.f90
index 020be13a8b6..2e77aea0549 100644
--- a/gcc/testsuite/gfortran.dg/gomp/task-detach-1.f90
+++ b/gcc/testsuite/gfortran.dg/gomp/task-detach-1.f90
@@ -18,8 +18,8 @@ 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 end task ! { dg-error "Unexpected !\\\$OMP END TASK statement at \\\(1\\\)" }
+  !$omp task detach(z) ! { dg-error "'z' at \\\(1\\\) should be a scalar of type integer\\\(kind=omp_event_handle_kind\\\)" }
+  !$omp end task
   
   !$omp task detach (x) firstprivate (x) ! { dg-error "DETACH event handle 'x' in FIRSTPRIVATE clause at \\\(1\\\)" }
   !$omp end task
-- 
2.25.1

Re: [PATCH][v2] openmp, fortran: Check that the type of an event handle in a detach clause is suitable [PR104131]

[Jakub Jelinek]

On Wed, Mar 02, 2022 at 05:22:21PM +0000, Kwok Cheung Yeung wrote:
> I have updated the patch to catch array elements and structure components as
> additional checks, in addition to checking that the variable is a scalar.
> 
> The check has been moved to the end of resolve_omp_clauses as it is more
> appropriate there. This gets rid of the additional 'Unexpected !$OMP END
> TASK statement' error, since the type error is now caught after the matching
> phase.
> 
> Coarrays (with the testcases in pr104131-2.f90) can be dealt with in a
> separate patch. Is this part okay for trunk?

LGTM, thanks.
Yes, coarrays is something that we need to deal with in all the clauses,
not just this one, so before we claim OpenMP 5.0 support we need to go
through it fully.

	Jakub