Re: [committed] Fix UB in gfc_trans_omp_clauses (PR fortran/93162)

[Tobias Burnus]

On 1/8/20 9:22 AM, Jakub Jelinek wrote:

With mixed REF_COMPONENT and REF_ARRAY, one can have var(:), or var2%comp(:)
or var3(:)%comp, or var3%comp(:)%comp2 etc.
Technically, one can also have var3(4)%comp(:)%comp2(1) – with one 
nonelement/AR_FULL reference and two element references. (At least as 
long 'comp2' is neither a pointer or an allocatable.)

and so the question is
what exactly we want to handle in the first if and what in the other cases.


My impression was back in October that – except for the additional 
component ref – that if the last reference is an array, it can be 
handled fine with the existing code AR_FULL code as a lot of the 
complications was already handled when converting gfc_expr to a tree. – 
But I didn't explore this back then as I stashed the project.



And something else should handle the other references, and that one needs to
walk the ref chain and handle each REF_ARRAY or REF_COMPONENT in there.


I am not sure whether one needs to handle each REF_COMPONENT and 
REF_ARRAY here. Talking of OpenACC 2.6 and OpenMP 4.5, one can largely 
ignore the components references before either the last one or before 
the first non-element array access – and for element access after the 
last component, the current code seems to map "comp(:)" when specifying 
"comp(:)%comp2", which I think it is fine.


* * *

_[Otherwise, one had to map each element separately or add more mapping 
support for some strided access patterns. – Depending on the element 
size [sizeof(comp(1)%comp2)] vs. stride [loc(comp(2))–loc(comp(1)] 
ratio, copying it as block or per element makes more sense. (Using 
A(::n,::m)%a it might make even sense to combine one stride as 
contiguous block and loop over the other.]_


In any case, that's something to study for Stage 1, whether first for 
OpenMP 4.5 or already preparing it for OpenMP 5, is the other big question.


For the latter, I am not sure how to handle it best. (I wonder how it is 
implemented for coarrays as one has there the same issue, especially 
with user-defined reduction functions and polymorphic variables, it can 
get complicated. – I did have an idea back then, but have not 
implemented it. I wonder whether someone else has tackled it in the 
meanwhile.) — I also have not checked whether OpenACC 2.7 or OpenACC 3.0 
or OpenMP 5.1/TR8 will add additional complications, which should be 
taken into account (at least design wise). [Nor whether new coarray 
issues pop(ped) up, which should be taken care of at the same time.]


To handle polymorphic types with allocatable components (which 
automatically get copied): I fear one cannot avoid adding a function to 
the v(irtual)tab(le). For user-defined reduction, I was thinking of 
passing a function pointer to the library, which then calls the function 
and provides a function pointer to a library function – that way, one 
can recursively pass calls between the library (which knows how to 
transfer the data) and the vtable functions in the code (which know the 
data structure).


Cheers,

Tobias

Re: [committed] Fix UB in gfc_trans_omp_clauses (PR fortran/93162)

[Jakub Jelinek]

On Wed, Jan 08, 2020 at 09:14:10AM +0100, Tobias Burnus wrote:
> Especially in light of the OpenMP 4.5's structure element mapping (for C/C++
> since GCC 7, for Fortran still unsupported), I had preferred some
> consolidation like taking the last reference in that check instead of just
> checking that the first reference is a whole array. Namely:
> 
> --- a/gcc/fortran/trans-openmp.c
> +++ b/gcc/fortran/trans-openmp.c
> @@ -2498,3 +2498,6 @@ gfc_trans_omp_clauses (stmtblock_t *block, 
> gfc_omp_clauses *clauses,
> -   if (n->expr == NULL
> -   || (n->expr->ref->type == REF_ARRAY
> -   && n->expr->ref->u.ar.type == AR_FULL))
> +   gfc_array_ref *array_ref = NULL;
> +   if (n->expr)
> + for (gfc_ref *ref = n->expr->ref; ref; ref = ref->next)
> +   if (ref->type == REF_ARRAY)
> + array_ref = >u.ar;
> +   if (array_ref && array_ref->type == AR_FULL)
> 
> I still believe believe that's a good/better approach.

With mixed REF_COMPONENT and REF_ARRAY, one can have var(:), or var2%comp(:)
or var3(:)%comp, or var3%comp(:)%comp2 etc. and so the question is
what exactly we want to handle in the first if and what in the other cases.
If it should handle what it was originally meant to, i.e. just whole
variables and treating full arrays therefore the same, that would be instead
a loop through the refs like you have that would for REF_ARRAY with
u.ar.type == AR_FULL just look through it and continue, but for anything
else would break and then check if we got NULL ref.
And something else should handle the other references, and that one needs to
walk the ref chain and handle each REF_ARRAY or REF_COMPONENT in there.

Jakub

Re: [committed] Fix UB in gfc_trans_omp_clauses (PR fortran/93162)

[Tobias Burnus]

Hi Thomas,

I had a quick look at the patch – I thought that it is only a band aid 
and should be handled more properly, but after having a closer look, I 
believe the latter is more work than I expected and Jakub's patch makes 
perfectly sense for Stage 3/4. (Hence, I also did not reply.)


On 1/7/20 6:33 PM, Thomas Schwinge wrote:
Do I understand correctly that this relates to the r279631 "Fortran 
polymorphic class-type support for OpenACC" changes?
I think it relates to the deep-copy change three commits / few minutes 
earlier: "OpenACC 2.6 deep copy: Fortran front-end parts" which 
permitted components in r279628.
Julian and/or Tobias, will you please review that at some point (in 
context of the OpenACC usage/support recently introduced), and add 
test cases etc. (... but need not be done right now.)


The bug only triggers if there is a reference of some kind, which is 
(obviously) not an array reference and where accessing "ar.type" in the 
union ref->-u will hit a condition which either evaluates to AR_FULL by 
accident or which reads invalid memory (unlikely as all union members 
are similar or size + alignment). — Or as with PR 93162 when building 
with UBSAN.


Well, you have asked for a test case: as the PR shows, 
testsuite/gfortran.dg/goacc/derived-types-3.f90 is a test case, you just 
need to bootstrap GCC with UBSAN enabled :-P


Especially in light of the OpenMP 4.5's structure element mapping (for 
C/C++ since GCC 7, for Fortran still unsupported), I had preferred some 
consolidation like taking the last reference in that check instead of 
just checking that the first reference is a whole array. Namely:


--- a/gcc/fortran/trans-openmp.c
+++ b/gcc/fortran/trans-openmp.c
@@ -2498,3 +2498,6 @@ gfc_trans_omp_clauses (stmtblock_t *block, 
gfc_omp_clauses *clauses,
- if (n->expr == NULL
- || (n->expr->ref->type == REF_ARRAY
- && n->expr->ref->u.ar.type == AR_FULL))
+ gfc_array_ref *array_ref = NULL;
+ if (n->expr)
+   for (gfc_ref *ref = n->expr->ref; ref; ref = ref->next)
+ if (ref->type == REF_ARRAY)
+   array_ref = >u.ar;
+ if (array_ref && array_ref->type == AR_FULL)

I still believe believe that's a good/better approach.

BUT: This needs some additional changes if one wants to handle components
– and the committed OpenACC code handles arrays at in other conditional blocks.
This can be disentangled, but that not during Stage 3 and just before Stage 4.

Tobias

PS: The attached patch – I think from October (r277442, i.e. before a lot of
changes went in) – is a draft implementation for OpenMP's 4.5 structure element
mapping. I think it does handled all required, but it also commented-out
nearly all checks. (Fortunately, OpenMP 4.5 does not permit automatic deep
copy of allocatable components. OpenMP 5 does, also for polymorphic components;
it just doesn't support "stacks"/recursive types, i.e. derived types with
allocatable components of the same type.)

diff --git a/gcc/fortran/openmp.c b/gcc/fortran/openmp.c
index 5c91fcdfd31..1644383bcbf 100644
--- a/gcc/fortran/openmp.c
+++ b/gcc/fortran/openmp.c
@@ -259,7 +259,9 @@ gfc_match_omp_variable_list (const char *str, gfc_omp_namelist **list,
 	case MATCH_YES:
 	  gfc_expr *expr;
 	  expr = NULL;
-	  if (allow_sections && gfc_peek_ascii_char () == '(')
+	  if (allow_sections
+	  && (gfc_peek_ascii_char () == '('
+		 || gfc_peek_ascii_char () == '%'))
 	{
 	  gfc_current_locus = cur_loc;
 	  m = gfc_match_variable (, 0);
@@ -4451,11 +4453,12 @@ resolve_omp_clauses (gfc_code *code, gfc_omp_clauses *omp_clauses,
 		if (!gfc_resolve_expr (n->expr)
 			|| n->expr->expr_type != EXPR_VARIABLE
 			|| n->expr->ref == NULL
-			|| n->expr->ref->next
-			|| n->expr->ref->type != REF_ARRAY)
+/*			|| n->expr->ref->next
+			|| n->expr->ref->type != REF_ARRAY*/)
 		  gfc_error ("%qs in %s clause at %L is not a proper "
  "array section", n->sym->name, name,
  >where);
+#if 0
 		else if (n->expr->ref->u.ar.codimen)
 		  gfc_error ("Coarrays not supported in %s clause at %L",
  name, >where);
@@ -4493,6 +4496,7 @@ resolve_omp_clauses (gfc_code *code, gfc_omp_clauses *omp_clauses,
 			  break;
 			}
 		  }
+#endif
 		  }
 		else if (openacc)
 		  {
diff --git a/gcc/fortran/trans-openmp.c b/gcc/fortran/trans-openmp.c
index dad11a24430..6ce2c6ce635 100644
--- a/gcc/fortran/trans-openmp.c
+++ b/gcc/fortran/trans-openmp.c
@@ -2167,12 +2167,54 @@ gfc_trans_omp_clauses (stmtblock_t *block, gfc_omp_clauses *clauses,
 	  tree node2 = NULL_TREE;
 	  tree node3 = NULL_TREE;
 	  tree node4 = NULL_TREE;
-	  tree decl = gfc_trans_omp_variable (n->sym, false);
+	  tree node5 = NULL_TREE;
+	  tree decl = NULL_TREE;
+	  gfc_array_ref *array_ref = NULL;
+	  bool compref = false;
+	  if (n->expr)
+		{
+		  // FIXME: Can be simplified if "dt(1)%comp" is invalid
+	

Re: [committed] Fix UB in gfc_trans_omp_clauses (PR fortran/93162)

[Thomas Schwinge]

Hi!

On 2020-01-07T08:20:44+0100, Jakub Jelinek  wrote:
> When I wrote the code, for map clause the arguments couldn't contain any
> REF_COMPONENT (nor REF_UNQUIRY nor REF_SUBSTRING) and therefore it was
> ok (although unclean) to just look at u.ar.type

Jakub, thanks for fixing this.

> but now that REF_COMPONENT
> can appear there (so far for OpenACC only, although OpenMP 5.0 also allows
> it), that is no longer the case.

Do I understand correctly that this relates to the r279631 "Fortran
polymorphic class-type support for OpenACC" changes?

> Not really sure if the code doesn't need
> further changes (how will e.g. var%comp(:) be handled in the mapping
> clauses?), but this conditional is clearly wrong.

Julian and/or Tobias, will you please review that at some point (in
context of the OpenACC usage/support recently introduced), and add test
cases etc.  (... but need not be done right now.)


Grüße
 Thomas


> 2020-01-07  Jakub Jelinek  
>
>   PR fortran/93162
>   * trans-openmp.c (gfc_trans_omp_clauses): Check for REF_ARRAY type
>   before testing u.ar.type == AR_FULL.
>
> --- gcc/fortran/trans-openmp.c.jj 2020-01-04 00:14:28.511400132 +0100
> +++ gcc/fortran/trans-openmp.c2020-01-06 17:01:10.538816323 +0100
> @@ -2495,7 +2495,9 @@ gfc_trans_omp_clauses (stmtblock_t *bloc
> tree decl = gfc_trans_omp_variable (n->sym, false);
> if (DECL_P (decl))
>   TREE_ADDRESSABLE (decl) = 1;
> -   if (n->expr == NULL || n->expr->ref->u.ar.type == AR_FULL)
> +   if (n->expr == NULL
> +   || (n->expr->ref->type == REF_ARRAY
> +   && n->expr->ref->u.ar.type == AR_FULL))
>   {
> tree present = gfc_omp_check_optional_argument (decl, true);
> if (n->sym->ts.type == BT_CLASS)


signature.asc
Description: PGP signature

[committed] Fix UB in gfc_trans_omp_clauses (PR fortran/93162)

[Jakub Jelinek]

Hi!

When I wrote the code, for map clause the arguments couldn't contain any
REF_COMPONENT (nor REF_UNQUIRY nor REF_SUBSTRING) and therefore it was
ok (although unclean) to just look at u.ar.type, but now that REF_COMPONENT
can appear there (so far for OpenACC only, although OpenMP 5.0 also allows
it), that is no longer the case.  Not really sure if the code doesn't need
further changes (how will e.g. var%comp(:) be handled in the mapping
clauses?), but this conditional is clearly wrong.

Bootstrapped/regtested on x86_64-linux and i686-linux, committed to trunk.

2020-01-07  Jakub Jelinek  

PR fortran/93162
* trans-openmp.c (gfc_trans_omp_clauses): Check for REF_ARRAY type
before testing u.ar.type == AR_FULL.

--- gcc/fortran/trans-openmp.c.jj   2020-01-04 00:14:28.511400132 +0100
+++ gcc/fortran/trans-openmp.c  2020-01-06 17:01:10.538816323 +0100
@@ -2495,7 +2495,9 @@ gfc_trans_omp_clauses (stmtblock_t *bloc
  tree decl = gfc_trans_omp_variable (n->sym, false);
  if (DECL_P (decl))
TREE_ADDRESSABLE (decl) = 1;
- if (n->expr == NULL || n->expr->ref->u.ar.type == AR_FULL)
+ if (n->expr == NULL
+ || (n->expr->ref->type == REF_ARRAY
+ && n->expr->ref->u.ar.type == AR_FULL))
{
  tree present = gfc_omp_check_optional_argument (decl, true);
  if (n->sym->ts.type == BT_CLASS)

Jakub