Re: [PATCH v2 2/2] fortran: Fix specification expression error with dummy procedures [PR111781]

[Mikael Morin <morin-mikael@orange.fr>]

Le 17/03/2024 à 21:57, Harald Anlauf a écrit :
> Hi Mikael,
> 
> thanks for the patch!
> 
> Regarding the first part of the patch, I think that fixing bad testcases
> can be done at any time.  Retaining identified, broken testcases means
> that one may hit bogus regressions, hindering progress.
> 
> The second part of the patch looks at first glance fine to me.  And as
> the patch changes less than its size suggests, in particular due to
> code refactoring, I don't see a reason to postpone it to stage 1.
> 
What worries me a bit is the changes of gfc_current_ns.
It's the kind of change that has broad impact and can uncover weird 
behaviors.

> (On the contrary, deferring it to stage 1 might make future backports
> from 15 for later patches on top of that code harder.
> This is my opinion, and others might see this differently.)
> 
To be honest, I posted it now in the hope that someone would be willing 
to push it before stage 1 so that I can get rid of it sooner.

> On 3/17/24 17:57, Mikael Morin wrote:
>>     * expr.cc (check_restricted): Remove the case where symbol is dummy
>>     and declared in the current ns.  Use gfc_get_spec_ns to get the
>>     right namespace.
> 
> Looking at the original and new code, I don't fully understand
> that part of the commit message: the changed check comes into play
> when the symbol is *not* in_common, ..., a dummy, ...
> So technically, we didn't access the (now removed) formal_arg_flag
> here in those cases.
> Or am I missing something?
> 
Oh, do you really read all of my prose?
I wrote it from vague memories of debugging the problem weeks ago, 
without thinking very much about it.

Actually, there are two dummy arguments here.
There is the one we are checking, and its bounds should be a 
specification expression, so could possibly be another dummy variable 
(the "sym" variable in this context).
The condition was allowing variables declared in the same scope to 
appear in specification expression of dummy arguments.

I will try to rephrase the sentence, as it sounds more clear (and 
sensible) as expressed here.

>> diff --git a/gcc/fortran/expr.cc b/gcc/fortran/expr.cc
>> index 82a642b01f7..0852bc5f493 100644
>> --- a/gcc/fortran/expr.cc
>> +++ b/gcc/fortran/expr.cc
>> @@ -3509,19 +3509,13 @@ check_restricted (gfc_expr *e)
>>         if (!check_references (e->ref, &check_restricted))
>>       break;
>>
>> -      /* gfc_is_formal_arg broadcasts that a formal argument list is 
>> being
>> -     processed in resolve.cc(resolve_formal_arglist).  This is done so
>> -     that host associated dummy array indices are accepted (PR23446).
>> -     This mechanism also does the same for the specification expressions
>> -     of array-valued functions.  */
>>         if (e->error
>>           || sym->attr.in_common
>>           || sym->attr.use_assoc
>>           || sym->attr.dummy
>>           || sym->attr.implied_index
>>           || sym->attr.flavor == FL_PARAMETER
>> -        || is_parent_of_current_ns (sym->ns)
>> -        || (gfc_is_formal_arg () && (sym->ns == gfc_current_ns)))
>> +        || is_parent_of_current_ns (gfc_get_spec_ns (sym)))
>>       {
>>         t = true;
>>         break;
> 
> 
>> diff --git a/gcc/testsuite/gfortran.dg/spec_expr_8.f90 
>> b/gcc/testsuite/gfortran.dg/spec_expr_8.f90
>> new file mode 100644
>> index 00000000000..5885810d421
>> --- /dev/null
>> +++ b/gcc/testsuite/gfortran.dg/spec_expr_8.f90
>> @@ -0,0 +1,24 @@
>> +! { dg-do compile }
>> +!
>> +! PR fortran/111781
>> +! We used to reject the example below because the dummy procedure g was
>> +! setting the current namespace without properly restoring it, which 
>> broke
>> +! the specification expression check for the dimension of A later on.
>> +!
>> +! Contributed by Markus Vikhamar-Sandberg  
>> <rasmus.vikhamar-sandberg@uit.no>
> 
> Is the reporter's first name Markus or Rasmus?
> 
Rasmus.
Reviews are sometimes helpful, more often than not.
I don't know where the Markus name comes from.

Thanks for the review.