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

  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
  

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.