[PATCH] Fortran: Fixes and additional tests for shape/ubound/size [PR94070]

[Tobias Burnus]

Hi Sandra,

On 20.10.21 22:03, Sandra Loosemore wrote:
> The one that was most concerning was an ICE when calling the SHAPE
> intrinsic with an assumed-rank class type argument ... In this case,
> SHAPE was calling a library function and trying to copy the array
> contents to a temporary, which is really stupid because SHAPE only
> needs to look at the descriptor and not the array contents.  I thought
> we could handle this inline the same as UBOUND and LBOUND, by
> extending gfc_trans_intrinsic_bound, and avoid the library function
> entirely.
>
> Then, I found some other existing problems in
> gfc_trans_intrinsic_bound; the conditional it was building to test for
> the extent-zero special cases for LBOUND and UBOUND was completely
> wrong, and the compile-time test for the assumed-rank/assumed-size
> case was wrong too.  So I ended up rewriting large parts of that
> function.
>
> I also fixed a bug in the SIZE intrinsic where it was not taking the
> class types into account.  (SIZE is already being handled inline in a
> separate place, otherwise I might've merged it into
> gfc_trans_intrinsic_bound as well.)

Thanks for your efforts!

LGTM with the changelog path fix, without the gfc_tree_array_size
attribute change and with the indentation fix.

Namely:

>     gcc/testsuite/gfortran.dg/
>         PR fortran/94070
>
>         * c-interop/shape-bindc.f90: New test.
The ChangeLog file is in testsuite/ not in testsuite/gfortran.dg/.

> @@ -8054,9 +8060,18 @@ gfc_tree_array_size (stmtblock_t *block, tree desc, gfc_expr *expr, tree dim)
>         return GFC_TYPE_ARRAY_SIZE (TREE_TYPE (desc));
>       }
>     tree size, tmp, rank = NULL_TREE, cond = NULL_TREE;
> -  symbol_attribute attr = gfc_expr_attr (expr);
> +  symbol_attribute attr;
>     gfc_array_spec *as = gfc_get_full_arrayspec_from_expr (expr);
>     gcc_assert (GFC_DESCRIPTOR_TYPE_P (TREE_TYPE (desc)));
> +
> +  if (expr->ts.type == BT_CLASS)
> +    {
> +      attr = CLASS_DATA (expr->symtree->n.sym)->attr;
> +      attr.pointer = attr.class_pointer;
> +    }
> +  else
> +    attr = gfc_expr_attr (expr);

Short version: Can you undo this change and verify that it still
fails? – Because I think that it works now due to the patch mentioned below.

At least it did pass here and an assert only pointed to testcases,
where I expected an issue.

  * * *

Long version:
I stumbled over this while reading the patch as I think it is wrong in the
general case. However, I belief it is fine for the particular use
(only allocatable + pointer with assumed-rank arrays). It does mishandle
"nonclass%class_comp" – but that is inaccessible if 'nonclass' is
assumed-rank (and the attributes are only used in that case).

Nonetheless, I fail to see when this fails - because I think that gfc_expr_attr
should yield the proper result (since Tue Oct 12, my eb92cd57a1ebe7cd7589bdbec34d9ae337752ead)

I think before before that patch, the problem was that expr was not 'var'
but 'var%_data' and gfc_expr then returned the attributes for gfc_expr,
which always had attr.pointer == 1 as the true 'pointer' attribute is in
attr.class_pointer and not in attr.pointer.

My bet is that you modified this before doing the "git pull" which pulled
in my patch above ...

For testing, I did turn your change into an assert and it only failed for
class_48.f90 and pr93792.f90. Those expose the issue I was concerned about:

(gdb) p gfc_debug_expr(expr)
test4:one % a % _data(FULL)

(gdb) p gfc_debug_expr(expr)
copy:self % x % _data(FULL)

(As said: nonissue in this case, but still feels wrong – and as the
workaround is longer be needed, it can also be removed.)
> --- a/gcc/fortran/trans-intrinsic.c
> +++ b/gcc/fortran/trans-intrinsic.c
> ...
> +       /* Descriptors for assumed-size arrays have ubound = -1
> +          in the last dimension.  */
> +       cond1 = fold_build2_loc (input_location, EQ_EXPR,
> +         logical_type_node, ubound, minus_one);
Here, indentation goes wrong.

Thanks,

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