Re: [patch, fortran] Fix PR 92004, restore Lapack compilation

[Thomas Koenig-6]

Am 06.10.19 um 17:26 schrieb Thomas Koenig:
> This
> also restores Lapack compilation without warning.

Well, up to an error in the testing routines, at least.

TESTING/LIN/sdrvls.f has

       REAL, ALLOCATABLE :: WORK (:)
...

       REAL               RESULT( NTESTS ), WQ

and calls

                               CALL SGELS( TRANS, M, N, NRHS, A, LDA,
      $                                    B, LDB, WQ, -1, INFO )

[...]

                               CALL SGELS( TRANS, M, N, NRHS, A, LDA, B,
      $                                    LDB, WORK, LWORK, INFO )

so that one really is illegal and should be flagged.

[Thomas Koenig-6]

Hi Tobias,

> function ("o" missing); I think it is not clause 14 but paragraph 14.

Fixed. (That one was easy :-)

>> +   warning for this can be suppressed later.  */
>> +
>> +bool
>> +maybe_dummy_array_arg (gfc_expr *e)
>> +{
>> +  gfc_symbol *s;
>> +
>> +  if (e->rank > 0)
>> +    return false;
>> +
>> +  if (e->ts.type == BT_CHARACTER && e->ts.kind == 1)
>> +    return true;
>> +
>> +  if (e->expr_type != EXPR_VARIABLE)
>> +    return false;
>
> What about PARAMETER? :-)

Good catch.

I found that, by the time the code is reached, an element of a
parameter array is already simplified; so I added a flag during
constructor expansion.

>
>> +  s = e->symtree->n.sym;
>> +  if (s->as == NULL)
>> +    return false;
>
> This looks wrong. You also want to permit dt%array(1) – but not
> dt(1)%scalar

Fixed.

>> +  if (s->ts.type == BT_CLASS || s->as->type == AS_ASSUMED_SHAPE
>> +      || s->attr.pointer)
>> +    return false;
>
> dt%foo – again, "foo" can be an allocatable of polymorphic type or a
> pointer, but at least, it cannot be of assumed shape.

Really? The paragraph reads

# 14 If the actual argument is a noncoindexed scalar, the corresponding
# dummy argument shall be scalar unless
# * the actual argument is default character, of type character with the
#   C character kind (18.2.2), or is an element or substring of an
#   element of an array that is not an assumed-shape, pointer, or
#   polymorphic array,

(The last two points do not apply here because they are invalid without
explicit interface).  Unless I have my negatives wrong, the code is
correct (but I have been getting standardese wrong before).

Anyway, here's an update of the patch. OK, or is there still something
missing?  Or how should I interpret that paragraph? :-)

Regards

        Thomas

[Tobias Burnus-3]

Hi Thomas,

On 10/10/19 12:23 AM, Thomas Koenig wrote:
>>> +  if (s->ts.type == BT_CLASS || s->as->type == AS_ASSUMED_SHAPE
>>> +      || s->attr.pointer)
>>> +    return false;
>>
>> dt%foo – again, "foo" can be an allocatable of polymorphic type or a
>> pointer, but at least, it cannot be of assumed shape.
>
> Really? The paragraph reads […]

What I meant is assumed-shape implies dummy argument. Hence,
"s->as->type" is a good check.

Whereas for deferred-shape, one had to take care of "dt%allocatable_arg"
– thus, the s->attr.pointer and the s->ts.type check aren't good.

Technical background for those requirements: pointers and assumed-shape
arrays can have strides, but if one passes a scalar to an array dummy
argument, one wants to be reasonably sure that the memory is contiguous.

(Actually, one could permit assumed-shape or pointer with contiguous
argument. But as one doesn't want to encourage this abuse. The reason
for permitting character(kind=1) is to call C "char*" functions without
using ["H", "e", "l", "l", "o", null] instead of "Hello" + null].)


> Anyway, here's an update of the patch. OK, or is there still something
> missing?

It would be nice to have a ChangeLog item (not as diff).


> + /* Set if an interface to a procedure could actually be to an array
> + although the actual argument is scalar. */
> + unsigned maybe_array:1;

Actually, I find this sentence hard to parse. Maybe:
"Set if the dummy argument of a procedure could be an array despite
being called with a scalar actual argument."

Or something along this line.


> +/* Under certain conditions, a scalar actual argument can be passed
> +   to an array dummy argument - see F2018, 15.5.2.4, clause 14.  This
> +   functin returns true for these conditions so that an error or
Old patch? Still "functin".

> +   warning for this can be suppressed later.  */
> +
> +bool
> +maybe_dummy_array_arg (gfc_expr *e)
> +{
> +  gfc_symbol *s;
> +  gfc_ref *ref;
> +  bool last_array_ref;
> +
> +  if (e->rank > 0)
> +    return false;

Maybe add a comment "/* Return false as for arrays, the rank always
needs to be checked. */" or something like that. Otherweise,
"maybe_dummy_array_arg" + description above the function cause one to
stumble over this.


> +  s = e->symtree->n.sym;
> +  if (s->as == NULL)
> +    return false;

Again, assume  "call foo(dt%array(1))" – I think that's fine but
rejected by this check as "dt" is a scalar and only "dt%array" is an
array. – You have have to keep that array spec and then look the the
last component reference and see at its array spec.

> +  if (s->ts.type == BT_CLASS || s->as->type == AS_ASSUMED_SHAPE
> +      || s->attr.pointer)
> +    return false;

Similarly, "class%int_array(1)" is fine – I think you need "e->ts.type"
instead of "s".

For s->attr.pointer, likewise "ptr%int_array(1)" is fine, hence,
"gfc_expr_attr (e).pointer" or something like that is needed.

And for the "s->as->type", the following should be valid:

type t
integer :: ia(100)
end type t

type(t), allocatable :: x(:)
allocate(x(1))
call foo(x(1)%ia(5), 100-5)

But while x is assumed-shape

> +  last_array_ref = false;
> +
> +  for (ref=e->ref; ref; ref=ref->next)
> +    last_array_ref = ref->type == REF_ARRAY;

This rejects too much - you can also have a substring reference at the
end – and then the arrayness still matters.

character(type=4, len=5) :: str(50)

    call foo(str(1))  ! This makes sense
    call foo(str(1)(3:4))  ! Technically valid, but feels odd

> argument_checking_24.f90
>

I also would prefer to have some more test coverage.

For instance:

type(tt), pointer :: tt_var2
allocate(tt_var2)
call s2(tt_var2%x(1)) ! Valid

subroutine foo3(x)
type(tt) :: tt_var2(:)
call s1(tt_var2%x(1)) ! Valid

call s4(dt%array_var%scalar) ! Invalid


Actually, I wonder whether you code as any effects on strings as at
least the test for "Element of assumed-shaped or pointer array passed to
array dummy argument" permits any string and not only
default-kind/c_char strings. – I am pretty sure that some C-binding test
case already checks that those are accepted.


Cheers,

Tobias

[Thomas Koenig-6]

OK, so here's the update. There was a problem with uninitialized
variables, which for some reason was not detected on compilation.

OK for trunk?

2019-10-13  Thomas Koenig  <[hidden email]>

        PR fortran/92004
        * array.c (expand_constructor): Set from_constructor on
        expression.
        * gfortran.h (gfc_symbol): Add maybe_array.
        (gfc_expr): Add from_constructor.
        * interface.c (maybe_dummy_array_arg): New function.
        (compare_parameter): If the formal argument is generated from a
        call, check the conditions where an array element could be
        passed to an array.  Adjust error message for assumed-shape
        or pointer array.  Use correct language for assumed shaped arrays.
        (gfc_get_formal_from_actual_arglist): Set maybe_array on the
        symbol if the actual argument is an array element fulfilling
        the conditions of 15.5.2.4.

2019-10-13  Thomas Koenig  <[hidden email]>

        PR fortran/92004
        * gfortran.dg/argument_checking_24.f90: New test.
        * gfortran.dg/abstract_type_6.f90: Add error message.
        * gfortran.dg/argument_checking_11.f90: Correct wording
        in error message.
        * gfortran.dg/argumeent_checking_13.f90: Likewise.
        * gfortran.dg/interface_40.f90: Add error message.

[Thomas Koenig-6]

Committed, with that nitch, r276972.

> OK with a minor nit. — Thanks for the patch.

Thanks a lot for the review!

Regards

        Thomas