Re: [PATCH] PR fortrsn/101564 - ICE in resolve_allocate_deallocate, at fortran/resolve.c:8169

[Tobias Burnus <tobias@codesourcery.com>]

Hi Harald,

On 26.07.21 23:55, Harald Anlauf wrote:
> I've updated this for ALLOCATE/DEALLOCATE and STAT/ERRMSG, see
> attached patch.  This required updating the error messages of
> two existing files in the testsuite.
Thanks.
>> Also affected: Some I/O items, a bunch of other stat=%v and
>> errmsg=%v.
> We should rather open a separate PR on auditing the related uses
> of gfc_match.

I concur – I just wanted to quickly check how many %v are there –
besides %v, there are also direct calls to gfc_match_variable.

Can you open a PR?

>>>>          if ((stat->ts.type != BT_INTEGER
>>>>               && !(stat->ref && (stat->ref->type == REF_ARRAY
>>>>                                  || stat->ref->type == REF_COMPONENT)))
>>>>              || stat->rank > 0)
>>>>            gfc_error ("Stat-variable at %L must be a scalar INTEGER "
>>>>                       "variable", &stat->where);
>>>> I mean the ts.type != BT_INTEGER and stat->rank != 0 is clear,
>>>> but what's the reason for the refs?
>>> Well, that needs to be answered by Steve (see commit 3759634).
>>>
>>> (https://gcc.gnu.org/g:3759634f3208cbc1226bec19d22cbff989a287c3 (svn
>>> r145331))
>>>
>>> The reason for the ref checks is unclear and seem to be wrong. The added
>>> testcases also only use 'x' (real) and n or i (integer) as input, i.e.
>>> they do not exercise this. I did not look for the patch email for reasoning.
> Well, there is some text in the standard that I added in front of
> the for loops, and this code is now exercised in the new testcase.

The loops are clear – but the
  '!stat->ref || (...ref->type != ARRAY || ref->type != COMPONENT))'
is still not clear to me.

   * * *

Can you add the (working) test:
   allocate (A, stat=y%stat%kind)  ! { dg-error "cannot be a constant" }
   deallocate (A, stat=y%stat%kind)  ! { dg-error "cannot be a constant" }
to your testcase gcc/testsuite/gfortran.dg/allocate_stat_3.f90 ?


And also the following one, which does not get diagnosed and, hence,
later gives an ICE during gimplification.

   type tc
     character (len=:), allocatable :: str
   end type tc
...
   type(tc) :: z
...
   allocate(character(len=13) :: z%str)
   allocate (A, stat=z%str%len)
   deallocate (A, stat=z%str%len)

To fix it, I think the solution is to do the following:
* In gfc_check_vardef_context, handle also REF_INQUIRY; in the
     for (ref = e->ref; ref && check_intentin; ref = ref->next)
   loop, I think there should be a
     if (ref->type == REF_INQUIRY)
       {
         if (context)
          gfc_error ("Type parameter inquiry for %qs in "
                     "variable definition context (%s) at %L",
                     name, context, &e->where);
         return false;
       }
(untested)

I assume (but have not tested it) that will give
two error messages for:
   allocate (A, errmsg=z%str%len)
   deallocate (A, errmsg=z%str%len)
one for the new type-param-inquiry check and one for
   != BT_CHARACTER
if you want to prevent the double error, consider to
replace
    gfc_check_vardef_context (...);
by
    if (!gfc_check_vardef_context (...))
      goto done_errmsg;

> Regtested on x86_64-pc-linux-gnu.  OK?
LGTM - except for the two testcase additions proposed above
and fixing the ICE. If you are happy with my changes and they
work, feel free add them and commit without further review.
In either case, I have the feeling we are nearly there. :-)

Thanks for the patch and the review-modification-review-... patience!

Tobias

> Fortran: ICE in resolve_allocate_deallocate for invalid STAT argument
>
> gcc/fortran/ChangeLog:
>
>       PR fortran/101564
>       * match.c (gfc_match): Fix comment for %v code.
>       (gfc_match_allocate, gfc_match_deallocate): Replace use of %v code
>       by %e in gfc_match to allow for function references as STAT and
>       ERRMSG arguments.
>       * resolve.c (resolve_allocate_deallocate): Avoid NULL pointer
>       dereferences and shortcut for bad STAT and ERRMSG argument to
>       (DE)ALLOCATE.
>
> gcc/testsuite/ChangeLog:
>
>       PR fortran/101564
>       * gfortran.dg/allocate_stat_3.f90: New test.
>       * gfortran.dg/allocate_stat.f90: Adjust error messages.
>       * gfortran.dg/implicit_11.f90: Adjust error messages.
-----------------
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